diff --git a/docs/PLAN.md b/docs/PLAN.md index e7747cd..3a2db48 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -2451,15 +2451,20 @@ root `CMakeLists.txt`. **Does not own:** `export_plan` / `import_plan` (Ε-W2), - **Two version integers, not one.** `formatVersion` = what this writer emitted; `minReaderVersion` = the oldest reader that can read it safely. The reader's whole rule is `minReaderVersion <= kPackageFormatVersion`. An **additive** change (a new optional - manifest key, a new enum value with a defined degrade) bumps `formatVersion` only; a - **structural** change bumps both. The header carries the writer's semver + manifest key, a new `Sample` field with a defined absent-value) bumps `formatVersion` + only; a **structural** change bumps both. **Growing a persisted enum's vocabulary is + structural here, not additive** — `BankModel::deserialize` *rejects* an out-of-range + `SourceMode` or `Tier` rather than degrading it (`bank_model.cpp:232-239`, `:339-346`), + and every enum a package carries rides inside the nested `BankModel` blob, so a new + `SourceMode` or `Tier` value bumps both integers. The header carries the writer's semver (`version::stampVersion()`) alongside them, informational, so a refusal message can name what to install. - **The ladder is documented the way `origin_ledger.cpp:8-21` documents its own** — a header comment listing every shipped version and what changed, with the read-and-validate rule stated, not implied. - **Unknown manifest keys are skipped** (the `bank_book_json.cpp:182` behaviour), and - **unknown persisted enum integers degrade to their defined `Unknown` equivalent**, never to + **unknown persisted enum integers — the manifest's own, not `BankModel`'s nested ones, which + reject per the bullet above — degrade to their defined `Unknown` equivalent**, never to the numeric default and never to a parse failure (`core/wire/CLAUDE.md`'s `BakeStatus` rule, verbatim). Both are pinned by tests, not left to inheritance. - **The manifest nests `BankModel`'s own serialization verbatim**, exactly as @@ -2510,9 +2515,11 @@ parallel with the format it will carry. **Surface boundary — owns:** new `src/shell/package/package_io` (read a file's bytes, write bytes through temp + atomic rename, read one bank file, write one landed file, enumerate the -bank folder's existing names, and the rollback delete), the platform file-picker seam under -the `#ifdef _WIN32` / `#else swell/swell.h` split this codebase already uses -(`src/shell/panel/draw_kit.cpp:11-15`, `src/shell/persist/prune_fs.cpp:35-38`), the +bank folder's existing names, and the rollback delete), the file-picker seam for both verbs — +**one picker, REAPER's own, on every platform**, so this track carries **no** +`#ifdef _WIN32` / `#else swell/swell.h` split. That split is a real pattern in this codebase +(`src/shell/panel/draw_kit.cpp:11-15`, `src/shell/persist/prune_fs.cpp:35-38`); it is simply +not this track's shape, because REAPER owns the dialog. Also owns the directory's `CMakeLists.txt` and `CLAUDE.md`, and one appended `add_subdirectory` line in the root `CMakeLists.txt`. **Does not own:** `export_bank` / `import_bank` (Ε-W2), anything under `core/`, and — emphatically — `prune_fs`, which stays the deletion authority. @@ -2529,12 +2536,22 @@ root `CMakeLists.txt`. **Does not own:** `export_bank` / `import_bank` (Ε-W2), `src/shell/persist/prune_fs.cpp:5-11` and states the discriminator it satisfies — this call created the file, and no index ever referenced it — rather than restating the carve-out's text. Anything that does not satisfy that discriminator is not this function's business. -- **The two pickers are asymmetric, and the asymmetry is real.** Import uses REAPER's own - `GetUserFileNameForRead(char* filenameNeed4096, const char* title, const char* defext)` — - **verified**, `vendor/reaper-sdk/sdk/reaper_plugin_functions.h:3798`. There is **no save - picker in the REAPER API** — a sweep of that header for `FileNameFor|SaveFile|Browse` - returns only the read picker — so export uses Win32 `GetSaveFileNameW` on Windows and - SWELL's `BrowseForSaveFile` elsewhere (`vendor/WDL/WDL/swell/swell-functions.h:167`). +- **Both pickers are REAPER's own, and they are symmetric.** `GetUserFileName(int mode, + const char* caption, const char* initial_file_or_path, const char* extension_list, char* + fnOutNeedBig, int fnOutNeedBig_sz)` — **verified**, + `vendor/reaper-sdk/sdk/reaper_plugin_functions.h:3790`, documented at `:3788` — serves + both directions: `mode=0` ("choose a new file") is export's destination picker, `mode=1` + ("existing file") is import's source picker. `extension_list` takes the + `'ReaSampler banks|*.rsbank|All files|*.*'` form, and `initial_file_or_path` may be a bare + `'.rsbank'` to set the default extension. There is **no `#ifdef _WIN32` / SWELL split and + no wide-char round trip here** — REAPER owns the dialog on every platform, so no + `GetSaveFileNameW` and no `BrowseForSaveFile`. `GetUserFileNameForRead` is explicitly + "Superseded, see GetUserFileName" (`:3796`) and is not used. +- **No fallback path.** `src/app/main.cpp:15` defines `REAPERAPI_IMPLEMENT` *without* + `REAPERAPI_MINIMAL`, so the resolver walks the full table — `GetUserFileName` included + (`reaper_plugin_functions.h:9084`) — and `main.cpp:292-293` aborts the extension load if + any single function fails to resolve. No REAPER build that loads this extension can lack + `GetUserFileName`, which makes a fallback unreachable code. - **No REAPER project state is touched here.** No ext-state read or write, no undo block, no generation bump; those belong to the verbs in Ε-W2. @@ -2545,16 +2562,27 @@ root `CMakeLists.txt`. **Does not own:** `export_bank` / `import_bank` (Ε-W2), test asserts against a seam counter, not against a memory measurement. - The rollback deletes exactly the files it was given and nothing else, and is a no-op on a path it did not write. -- The Windows and SWELL picker paths both compile; the SWELL signature matches - `swell-functions.h:167` exactly. `[verify — DAW]` — neither picker is exercised in a live - REAPER session by this track. -- No file exceeds ~600 lines; the platform picker lives in its own TU, following the - `drag_out` / `drag_out_win` precedent. +- **There is exactly one picker call site and it is REAPER's.** Both verbs reach + `GetUserFileName` — export with `mode=0`, import with `mode=1` — and no symbol named + `GetSaveFileNameW`, `BrowseForSaveFile`, or `GetUserFileNameForRead` appears anywhere in + `src/shell/package/`, nor any platform `#ifdef` in the picker's TU. Greppable, so it stays + true. `[verify — DAW]` — the picker is not exercised in a live REAPER session by this track. +- No file exceeds ~600 lines; the picker lives in its own TU with its own header. **Not** for + the `drag_out` / `drag_out_win` reason — that precedent isolates a *Win32-only* TU, and + there is no platform split here — but because the picker is the only REAPER-facing part of + an otherwise REAPER-free, unit-tested module: folding it into a shared header would drag + `reaper_plugin_functions.h` into the testable seam's include graph. -**Open questions.** **[propose at review]** whether the file-picker seam is its own module or -part of `package_io` — the `drag_out_win` precedent argues its own TU; whether it also wants -its own header is a judgment call at the size it lands. **[verify — DAW]** the default -extension and filter strings each platform's picker actually accepts. +**Open questions.** **[propose at review]** where the `extension_list` and default-extension +strings live — this track's picker TU, or the Ε-W2 verbs that call it. They are user-facing +text, and the verbs own the rest of the user-facing text; the counter-argument is that they +are picker plumbing and only one picker exists. **[verify — DAW]** two things the header does +not answer: (1) whether the `mode=0` picker **appends** an extension when the user types a +bare name — `:3788` documents that `initial_file_or_path` may be `'.rsbank'` "to set the +default extension," but not that the dialog enforces it on return, so the verb may still have +to append `.rsbank` itself; (2) **dialog parenting** — `GetUserFileName`'s signature +(`:3790`) takes no owner window, where the abandoned Win32 `OPENFILENAME` path would have +passed `GetMainHwnd()`, so modality against the REAPER main window is unobserved. #### Ε-W1-T3 — `import-origin-kind` @@ -4321,7 +4349,7 @@ Phase Epsilon — The bank package (none of the seventeen; a direct reque T1 package-format .............. core/package: framing + TWO version ints [E-F1 RULED: proprietary RSBK. No ZIP, no zlib] T2 package-fs-shell ............ shell/package: atomic write, streaming, pickers - [no REAPER save-picker exists; SWELL/Win32 split] + [ONE picker: REAPER GetUserFileName, mode 0/1] T3 import-origin-kind .......... OriginKind::PackageImport = 5, append-only W2 The two verbs [2 tracks; disjointness CONDITIONAL — see below] T1 bank-export ................. export_plan + verb + action; project untouched diff --git a/docs/product/bank-package.md b/docs/product/bank-package.md index c565667..92c9ee4 100644 --- a/docs/product/bank-package.md +++ b/docs/product/bank-package.md @@ -188,12 +188,28 @@ So the package header carries two integers: - **`minReaderVersion`** — the oldest reader that can read this package *safely*. Bumped only when a change is **structural** (a field's meaning changes, a section is removed, framing changes); left alone when a change is **additive** (a new - optional manifest key, a new enum value with a defined degrade). + optional manifest key, a new `Sample` field with a defined absent-value — exactly + the four listed above). The reader's rule is one line: **read it iff `minReaderVersion <= kPackageFormatVersion`.** `formatVersion` is then only for the message text and the log. +**One change class that looks additive and is not: a new enum value.** +`BankModel::deserialize` *rejects* an out-of-range `SourceMode` or `Tier` rather than +degrading it (`bank_model.cpp:232-239`, `:339-346`), and every enum a package carries +rides inside the nested `BankModel` blob. So growing either vocabulary is +**structural** and bumps `minReaderVersion` too. This is wider than packages and +predates them: `BankModel::deserialize` is also the live project ext-state parser +(`bank_book_json.cpp:99`), so appending a `SourceMode` value already strands an older +build opening a newer project's `.rpp`. Phase Ε inherits that property; it did not +cause it, and changing it — degrade-to-`Unknown` at those two sites, the way +`BakeStatus` already does — is a change to the model layer, not a package concern. It +leaves the argument above untouched: the four fields that motivated the two-integer +design are *fields*, and `parseSample`'s `skipValue()` fallback +(`bank_model.cpp:370-372`), plus the manifest parsers' equivalent at each level, still +carries them forward. + This is a borrowed pattern, not an invention: Matroska's `EBMLVersion` / `EBMLReadVersion` pair, PDF's catalog `/Version` over the header version, and OOXML's `mc:Ignorable` markup-compatibility mechanism all separate "what I am" from "what you @@ -211,8 +227,9 @@ keys take their defined defaults, exactly as `Sample`'s additive fields already and exactly as `origin_ledger` lifts a v1 path-only blob into v2 records with kind `Unknown` and empty ids (`origin_ledger.cpp:14-16`). Unrecognized manifest keys are skipped, which is already how every parser in this repo behaves -(`bank_book_json.cpp:182`). Unrecognized enum integers degrade to their defined -`Unknown`-equivalent, never to the numeric default and never to a parse failure — +(`bank_book_json.cpp:182`). Unrecognized enum integers (the manifest's own — +`BankModel`'s nested ones reject) degrade to their defined `Unknown`-equivalent, +never to the numeric default and never to a parse failure — `bake_wire`'s rule verbatim (`src/core/wire/CLAUDE.md:83`: "an unrecognized value decodes as `Failed` rather than as the numeric default `Ok`"). **The user sees:** a normal import summary. Optionally a single console line naming @@ -617,16 +634,19 @@ responsibility seam, which is what the structural heuristic asks for. - `package_io` — read a package file to bytes, write bytes through temp + atomic rename, read a bank file's bytes, write a landed file, enumerate existing bank-folder names, and execute the rollback delete (citing the `prune_fs` carve-out). -- The platform file pickers, under the `#ifdef _WIN32` / `#else swell/swell.h` split - this codebase already uses (`src/shell/panel/draw_kit.cpp:11-15`, - `src/shell/persist/prune_fs.cpp:35-38`). **Verified:** the REAPER API offers a *read* - picker — `GetUserFileNameForRead(char* filenameNeed4096, const char* title, const - char* defext)`, `reaper_plugin_functions.h:3798` — and **no save picker at all** - (a sweep of the header for `FileNameFor|SaveFile|Browse` returns only that one - entry). Export's destination picker therefore comes from Win32 `GetSaveFileNameW` - on Windows and SWELL's `BrowseForSaveFile` elsewhere - (`vendor/WDL/WDL/swell/swell-functions.h:167`). This is a real asymmetry between - the two verbs and the spec should not paper over it. +- The file picker, which is REAPER's own on every platform — no `#ifdef _WIN32` / + `#else swell/swell.h` split, no Win32 `GetSaveFileNameW`, no wide-char round trip. + **Verified:** `GetUserFileName(int mode, const char* caption, const char* + initial_file_or_path, const char* extension_list, char* fnOutNeedBig, int + fnOutNeedBig_sz)` — `reaper_plugin_functions.h:3790`, documented at `:3788` — serves + both verbs symmetrically: `mode=0` chooses a new file (export's destination), + `mode=1` an existing one (import's source). `extension_list` takes the + `'ReaSampler banks|*.rsbank|All files|*.*'` form. `GetUserFileNameForRead` is + explicitly "Superseded, see GetUserFileName" (`:3796`) and is not used. No fallback + is needed: `src/app/main.cpp:15` defines `REAPERAPI_IMPLEMENT` without + `REAPERAPI_MINIMAL`, so the resolver walks the full table (`GetUserFileName` at + `:9084`), and `main.cpp:292-293` refuses to load the extension if any one function + fails to resolve — so no REAPER build that loads us can lack it. - `export_bank` / `import_bank` — the promptless verbs, mirroring `src/shell/bank_ops/`'s pattern exactly: take a `ReaSamplerSession&`, do the work, return an outcome, **no prompts and no message boxes**. The bindable action and the