daniel/reasampler
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs: correct two false claims in the Phase Ε spec

Browse Source 
REAPER's GetUserFileName serves both pickers — the no-save-picker finding was a
regex miss. A new enum value is structural, not additive: BankModel rejects an
out-of-range SourceMode/Tier rather than degrading.
This commit is contained in:
Daniel Harvey
2026-08-02 12:38:41 -04:00
parent 2f8e88880e
commit 2cb0b52884
2 changed files with 83 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/PLAN.md
+50 -22
View File
@@ -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
docs/product/bank-package.md
+33 -13
View File
@@ -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