From a20cb46d65b7225ee3bd810d63a3fdafe84351e6 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 26 Jul 2026 20:56:31 -0400 Subject: [PATCH] =?UTF-8?q?docs:=20spec=20Phase=20Q=20(Quality)=20?= =?UTF-8?q?=E2=80=94=20structural=20reorganization=20pillar?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Product framing (docs/product/code-organization.md), PLAN.md $Phase Q (Q-W1..Q-W6), CONTEXT.md $Phase Q. Gated on tree quiescence; zero-hot-path-cost acceptance bar. Namespace Q settled; Q-2..Q-6 carry recommendations. --- CONTEXT.md | 217 ++++++++++++++++ PLAN.md | 231 +++++++++++++++++ docs/product/code-organization.md | 403 ++++++++++++++++++++++++++++++ 3 files changed, 851 insertions(+) create mode 100644 docs/product/code-organization.md diff --git a/CONTEXT.md b/CONTEXT.md index 9e3051a..de61883 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -1527,3 +1527,220 @@ unchanged — a visual refresh is not a compat event. - **A visual refresh is not a compat event.** VST3 class UID, command-id strings, ext-state namespaces, and component-state contracts are unchanged by Phase L. - **Verify LICE/WDL/SWELL surfaces** against `vendor/WDL` before use. + +--- + +# Structural reorganization — reorg spec (Phase Q — Quality) + +> **New pillar, own lettered phase, and the LAST structural pillar.** Phase Q is a **pure +> structural refactor** of `src/` — more encapsulation, granular namespaces, `core/`/`shell/`/ +> `app/` subdirectories — against a stated quality bar (*"mtytel Vital is my code reference for +> quality"*), to bring the codebase "into the realm of something I can stand to look at." It +> **ships no feature and changes no behavior**: the test suite passing unchanged is the proof of +> correctness. Namespaced **`Q` (Quality)** — M/D/B/R/V/S/L taken; `Q` names the end (the quality +> bar), the reorg being the means. Product framing, the Vital-grounded target shape, the +> grep-verified SOLID audit (the evidence base), and the fork record (Q-1..Q-6): +> `docs/product/code-organization.md`. When a point lands, doc-keeper moves it to `COMPLETED.md`. + +## What it is + +A directory + namespace + file-split reorganization that makes ReaSampler's **already-real, +CMake-enforced** architecture **legible in the code's shape**. The pure/shell split exists (30 +pure static libs, each with its own test executable, none linking a REAPER SDK) but is invisible: +all 45 files sit in **one flat `src/`**, all 37 headers in **one flat `reasampler` namespace**, +four modules have grown into god-modules, and the JSON parser is copy-pasted across four models. +Phase Q gives the existing subsystem grouping — model / view / capture / audio / ui / reclaim / +version — a **structural home** (directories + namespaces), splits the four god-modules along +validated seams, and extracts the duplicated JSON into one pure module. Nothing about the +architecture *changes*; it becomes *visible*. This is why the phase can be zero-runtime-cost and +CTest-green throughout: the seams already exist in the link graph; Phase Q draws them where a +reader sees them. + +## The quality bar — Vital (read from its actual `src/` tree) + +Vital (`github.com/mtytel/vital`) groups its synth by **subsystem** — `common/` `synthesis/` +`interface/` `plugin/` — with `synthesis/` further subdivided by function (`synth_engine/ +modulators/ filters/ effects/ producers/ framework/ lookups/ utilities/`). The grouping principle +is **layered functional architecture**: the directory tree *is* the architecture diagram; you read +the subsystem map off the folders. ReaSampler adopts the *pattern* (directory = architecture), +adapted to its own most load-bearing invariant — the pure/shell split — as the top level (see +below). Vital is GPLv3; the borrowed artifact is the **structural pattern**, not code. + +## Settled decisions (Q-1 settled; Q-2..Q-6 recommended — see `docs/product/code-organization.md` §6) + +- **Q-1 — namespace letter. SETTLED: `Q` (Quality).** Point-id family `Q1..Qn`, wave prefixes + `Q-W1..Q-W6`. `O` (Organization) was set aside: the glyph reads ambiguously against zero in + point ids, and "Organization" undersells a phase measured against a *quality* bar. +- **Q-2 — JSON extraction in scope + first. REC: yes.** The 4× duplicated `Parser` is the largest + DRY+SRP violation and is entirely off the hot paths — the ideal safe, high-leverage opener + (Q-W1). +- **Q-3 — directory shape. REC: `core/`/`shell/`/`app/` top-split, subsystem dirs beneath.** Top + level by the pure/shell discipline (so the invariant is *structural*, not merely conventional), + subsystem grouping one level down. Preferred over pure-Vital subsystem-first because ReaSampler + has a pure/host split Vital lacks and that split is the invariant most worth protecting + structurally. +- **Q-4 — sub-namespace to match sub-directory. REC: both.** `reasampler::model`/`view`/`capture`/ + `audio`/`ui`/`reclaim`/`version`/`json`. Directory and namespace agree; a symbol's home is + unambiguous from either. +- **Q-5 — god-module split granularity. REC: to the audit's named seams, no finer.** Well-factored, + not atomized. +- **Q-6 — OCP registration-table. REC: in scope, last (most droppable if narrowing).** + +## The directory + namespace map (Q-3 / Q-4) + +Top-level by the pure/shell discipline; subsystem dirs beneath `core/`; namespaces mirror +directories. + +**`core/` (pure, no REAPER types, unit-tested — `reasampler::`):** +- `core/model/` (`::model`) — `bank_model`, `bank_book`, `owned_manifest`, `provenance` +- `core/view/` (`::view`) — `view_mode_model`, `view_tree`, `lane_keys`, `mode_switch` +- `core/capture/` (`::capture`) — `render_settings`, `batch_capture`, `tail_control`, + `capture_paths`, `wav_trim`, `insert_plan` +- `core/audio/` (`::audio`) — `peaks` +- `core/ui/` (`::ui`) — `theme`, `component_geometry`, `bank_grid`, `tab_strip`, `action_buttons`, + `prune_button` +- `core/reclaim/` (`::reclaim`) — `prune_reconcile` +- `core/version/` (`::version`) — `app_version` +- `core/json/` (`::json`) — **NEW** — extracted parser/serializer (replaces the 4 duplicate + `Parser`s) + +**`shell/` (REAPER-facing — subdir by subsystem, namespace as house style prefers):** +- `shell/capture/` — `capture`, `capture_realtime`, `provenance_shell`, `track_guid`, `item_read`; + **post-Q-W3** `capture_orchestrator`, `scope_resolve`, `realtime_lifecycle` +- `shell/panel/` — `draw_kit`; **post-Q-W2** `panel_render`, `panel_thumbnails`, + `panel_audition`, `panel_input`, `panel_bank_ops`, `panel_window` (from `bank_panel`) +- `shell/view/` — `view` +- `shell/persist/` — **post-Q-W5** `session`, `ext_state_io`, `prune_fs` (from `persist`) +- `shell/actions/` — `drag_out_win`; **post-Q-W4** `design_view_actions`, `bank_actions`, + `prune_action` (from `actions`) + +**`app/`:** `main.cpp` (post-Q-W3: API-pointer ownership + `ReaperPluginEntry` + dispatch only). + +**Collision sweep (before W1):** `Sample` (`::model`) vs `AudioSample` (`::audio` alias) vs the +unified `Parser` (`::json`) must not collide once flattened into granular namespaces; resolve by +subsystem home. + +## The god-module split seams (Q-5) + +Split each to the audit-validated seams, no finer: + +- **`bank_panel.cpp` (2424 LOC → `shell/panel/`, Q-W2):** `panel_render` (draw/paint) / + `panel_thumbnails` (compute+cache) / `panel_audition` (preview engine — **hot path, direct + call-through**) / `panel_input` (mouse/key/wheel + new-content detection) / `panel_bank_ops` + (bank CRUD — the single owner W4 dedupes against) / `panel_window` (lifecycle + OS + drag-out/drop-target). Split the fat `bank_panel.h` per seam (I). +- **`main.cpp` (1762 LOC → hoist to `shell/capture/`, Q-W3):** `capture_orchestrator` + (`RunCapture`/`captureAndIndexOne`/`renderOffline`/batch/recapture/realtime `Run*`) / + `scope_resolve` (range/razor/track resolution + provenance assembly inputs) / + `realtime_lifecycle` (state machine + globals + selection guards). `FxBypassGuard` moves out + but **stays stack RAII** (precision-critical). `main.cpp` → `app/`, reduced to pointers + entry + + dispatch. +- **`actions.cpp` (981 LOC → `shell/actions/`, Q-W4):** `design_view_actions` / + `bank_actions` / `prune_action` (`doBankPruneFolder` — the single file-deletion action). Dedupe + `promptText`/`mintBankId` + bank verbs against `panel_bank_ops`. +- **`persist.cpp` (766 LOC → `shell/persist/`, Q-W5):** `session` (lifecycle/poll + + `BeginLoadProjectState` reload hook) / `ext_state_io` (serialization bridge + GUID minting + + folder relocation) / **`prune_fs`** (prune scan + `deleteOrphanFile` via `SHFileOperationW` — + the isolated single file-deletion authority). + +## The JSON extraction (Q-2 / Q-W1) + +Extract one pure **`core/json`** (`::json`): parser (`parseString`/`parseInt`/`parseKey`/ +`skipValue` + escape) + serialize/emit helpers. Rewire `bank_model`, `bank_book`, +`view_mode_model`, `owned_manifest` onto it and **delete their four hand-rolled `Parser`s**. +Round-trip output must be **byte-identical** to before — this is a structural dedupe, not a format +change. Off all hot paths (serialization runs at save/load, never per frame) — safe to abstract +freely. + +## The OCP registration-table (Q-6 / Q-W6) + +Replace the ~350-line hand-written **non-table** action registration blocks (isolated in +`app/main.cpp` after Q-W3) with a **data-driven registration table**, so adding an action edits one +place, not four parallel `Register("command_id"/"gaccel"/"hookcommand")` sites. Unload +mirror-unregisters from the same table. Command-id + display strings stay **byte-identical** +(FOREVER-STABLE, per-channel — the Phase V V4 contract). Also split residual fat headers +(`capture.h`/`persist.h`) alongside their TU splits (I). + +## Performance guardrails (HARD CONSTRAINT — Daniel's non-negotiable) + +The reorg must cost **zero runtime.** The two hot paths must keep their exact call/inline shape; +the following are acceptance criteria on every point: + +- **`peaks` envelope compute** (`computeEnvelope` / `lastFrameAboveThreshold` over full PCM): **NO + virtual dispatch, NO `peaks` interface, NO added header→TU indirection.** `computeEnvelope` + stays a **free function on `const std::vector&`** so it inlines as today. Relocate + + namespace only; never wrap in an abstraction. +- **Audition / preview**: `panel_audition` may be its own TU (Q-W2), but the call stays a **direct + call-through, not virtual.** +- **Realtime-capture tick**: keep the idle fast-path a **single pointer test**; + `realtime_lifecycle` (Q-W3) must not change the tick's branch shape. +- **JSON extraction** is **off all hot paths** — safe to abstract freely (why Q-W1 is the safe + opener). +- **`FxBypassGuard`** runs per-capture, not per-frame — keep it **stack RAII** when it moves out + of `main.cpp` (Q-W3); never heap-allocate or virtualize it. + +**Net:** every recommended split falls on a cold path or preserves call/inline shape on the two +hot ones. *A split that would add a hot-path indirection is out of scope — rework it or drop it.* + +## The GATE (load-bearing — Phase Q is last) + +Phase Q is **gated on the tree being otherwise quiescent.** It does not begin until **Phase S has +merged to dev**, **Phase L (L2 + L3) has merged to dev**, any **D2 residuals** are closed, and +**M9** is landed-or-abandoned. *Why:* Phase Q touches **nearly every file in `src/`** (relocate, +re-namespace, split the four largest TUs); every large in-flight branch (Phase S on its worktree, +Phase L's `bank_panel`-touching L2/L3) is diffed against the *current flat layout*, so landing a +rename-and-relocate-everything reorg mid-flight forces every open branch through the worst conflict +class — a combinatorial re-resolution, not a linear one. Phase Q is *last* precisely because it +reshapes the ground every other pillar stands on: landing it early taxes every subsequent phase; +landing it last taxes nothing. Re-confirm quiescence against dev before Q-W1. + +## Wave sequencing (each independently landable, CTest-green at every step) + +Big-bang is rejected — the CMake per-module static-lib + per-module test-executable seams make a +file move + namespace change **mechanically verifiable** (`ctest --test-dir build` green or not, at +every commit), a property only an *incremental* reorg uses. Risk-ordered: + +- **Q-W1** — safe opener: `core/json` extract (delete 4 `Parser`s) + impose the directory/ + namespace layout on the 30 clean pure libs + clean shells (pure relocation, no logic change). + All later waves assume this layout. +- **Q-W2..Q-W5** — the four god-module splits, one per wave, risk-ordered (`bank_panel` → + `main.cpp` → `actions.cpp` → `persist.cpp`). Q-W4 depends on Q-W2 (`panel_bank_ops` dedupe + target); Q-W5 best after Q-W4 (`prune_action` → `prune_fs` routing); otherwise parallel-safe. +- **Q-W6** — OCP registration-table + residual fat-header (I) splits. Depends on Q-W3 (registration + code isolated first). Sequenced last; most droppable if narrowing. + +## Precision / invariant implications (what Phase Q does NOT change) + +- **The pure/shell split is strengthened, never dissolved.** Top-level `core/` vs `shell/` makes + it *structural*; no file crosses the boundary; no `core/` file gains a REAPER type; the CMake + per-module test executables stay green. +- **Every precision invariant holds.** Null test, bit-identical repeats, non-destructive capture, + exact bounds — none is code Phase Q rewrites. `FxBypassGuard` moves but stays stack RAII with + identical behavior. +- **Capture ≠ placement.** No hoisted `Run*` / `capture_orchestrator` path may gain an + `InsertMedia` call during the split. +- **Single file-deletion authority is concentrated, never spread.** `prune_fs` (Q-W5) is the one + module that deletes bytes; the reorg improves this invariant. +- **Relative-paths-only** in the persisted index — untouched. +- **On-the-wire/on-disk contract strings unchanged.** Command-id strings, action display names, + ext-state namespaces, VST3 class UID are byte-identical (per-channel, per Phase V V4). + Re-namespacing C++ symbols is orthogonal to these. +- **No behavior change.** Phase Q is a pure structural refactor; the test suite passing unchanged + is the proof of correctness. A point that changes observable behavior has exceeded its charter. + +## Non-goals / guardrails + +- **No feature, no behavior change.** If a point ships anything user-visible, it is out of scope. +- **No hot-path indirection.** No virtual dispatch or header→TU indirection on `peaks`/audition/ + realtime-tick — ever (a hard acceptance bar, not advice). +- **No design-level (D-letter SOLID) rework.** `main`/`bank_panel` depending on concrete capture + backends is a low-priority Dependency-Inversion concern — **out of scope** (a design change, not + a reorg). Phase Q reorganizes; it does not re-architect interfaces. +- **No `peaks` data-ownership change.** `peaks` forcing a whole-file `std::vector` copy on + the thumbnail path is noted but **not touched** — reworking it risks the hot path. +- **No big-bang commit.** Every wave is independently landable and CTest-green; reject a change set + that cannot be verified at each step. +- **Do not begin before the GATE.** Re-confirm the tree is quiescent (Phase S + L + D2 + M9 + merged/closed) before any Q point. +- **Verify** the CMake `src/` path updates and the SWELL/LICE surfaces still resolve after + relocation, as the existing build already requires. diff --git a/PLAN.md b/PLAN.md index cf749aa..9cb83bd 100644 --- a/PLAN.md +++ b/PLAN.md @@ -406,3 +406,234 @@ for Phase S to reach dev. Phase S feature work runs in parallel, ungated by Phas - **M11 button inventory (L2)** — L2's layout depends on the *actual* set of buttons M11 lands (action-trigger buttons + keybinding-help labels). Re-inventory against dev after M11 merges before finalizing the layout. + +--- + +# Phase Q — Quality (structural reorganization; zero-runtime-cost) + +> **New pillar, own lettered namespace, and — uniquely — the LAST structural pillar.** Phase +> Q is a **pure structural refactor**: it reorganizes `src/` into a healthier shape (more +> encapsulation, granular namespaces, `core/`/`shell/`/`app/` subdirectories) against a stated +> quality bar — *"mtytel Vital is my code reference for quality"* — to bring the codebase +> "into the realm of something I can stand to look at." It ships **no feature and changes no +> behavior**: the test suite passing unchanged is the proof of correctness. Namespaced **`Q` +> (Quality)** — M/D/B/R/V/S/L are all taken; `Q` names the *end* (the quality bar), the reorg +> being the *means*. Authoritative spec: **CONTEXT.md §Phase Q — structural reorganization +> (reorg spec)**. Product framing, the Vital-grounded target shape, the grep-verified SOLID +> audit that is the evidence base, and the settled/recommended fork record (Q-1..Q-6): +> `docs/product/code-organization.md`. When a point lands, doc-keeper moves it to +> `COMPLETED.md`. +> +> **THE GATE (load-bearing — state first).** Phase Q is **gated on the tree being otherwise +> quiescent.** It does not begin until **Phase S has merged to dev**, **Phase L (L2 + L3) has +> merged to dev**, any **D2 residuals** are closed, and **M9** is landed-or-abandoned. *Why:* +> Phase Q touches **nearly every file in `src/`** (relocate into subdirectories, re-namespace +> every header, split the four largest TUs). Every large in-flight branch (Phase S on its +> worktree, Phase L's `bank_panel`-touching L2/L3) is diffed against the *current flat layout*; +> landing a rename-and-relocate-everything reorg mid-flight forces every open branch through the +> worst conflict class (every hunk moved, every qualified reference changed) — a combinatorial +> re-resolution, not a linear one. Phase Q is *last* precisely because it reshapes the ground +> every other pillar stands on. Landing it early taxes every subsequent phase; landing it last +> taxes nothing. **Do not begin any Q point until the gate is satisfied.** +> +> **Settled (Q-1, this-doc):** the phase is **`Q` (Quality)**; point-id family `Q1..Qn`, wave +> prefixes `Q-W1..Q-W6`. **Recommended, Daniel's to call (Q-2..Q-6, see +> `docs/product/code-organization.md` §6):** Q-2 JSON extraction in scope + first (rec: yes); +> Q-3 directory shape `core/`/`shell/`/`app/` top-split with subsystem dirs beneath (rec: this +> over pure-Vital subsystem-first — it makes the pure/shell invariant *structural*); Q-4 +> sub-namespace to match sub-directory (rec: both); Q-5 split god-modules to the audit's named +> seams, no finer (rec: yes); Q-6 OCP registration-table as the final wave (rec: in, last). +> +> **HARD CONSTRAINT — performance (see CONTEXT.md §Phase Q, `docs/product/code-organization.md` +> §3).** The reorg must cost **zero runtime.** On the three hot paths — `peaks` envelope +> compute, audition/preview, the realtime-capture tick — **no added virtual dispatch, no +> header→TU indirection, no changed call/inline or branch shape.** `computeEnvelope` stays a +> free function on `const std::vector&`; audition split stays a direct call-through; +> the realtime idle tick stays a single pointer test; `FxBypassGuard` stays stack RAII. This is +> an acceptance criterion on every point: *a split that would add a hot-path indirection is out +> of scope — rework it or drop it.* +> +> **Every point is independently landable and CTest-green at EVERY step.** The CMake +> per-module static-lib + per-module test-executable seams already draw the module boundaries; +> a file move + namespace change is mechanically verifiable — `ctest --test-dir build` is green +> or it isn't. **Green-CTest-at-every-point is an acceptance criterion.** Big-bang is rejected; +> the reorg is risk-ordered waves (W1 safe opener → W2–W5 god-module splits → W6 OCP finish). + +## Q-W1 — safe opener: extract `core/json` + impose the directory/namespace layout on clean modules +**Goal:** The zero-god-module-risk opener. Two moves: (1) extract a pure **`core/json`** module +(parser + serializer) and **delete the four hand-rolled `Parser`s** in `bank_model` / +`bank_book` / `view_mode_model` / `owned_manifest` (the single largest DRY+SRP violation, and +entirely off the hot paths); (2) impose the settled `core/`/`shell/`/`app/` directory layout + +sub-namespaces (`reasampler::model`/`view`/`capture`/`audio`/`ui`/`reclaim`/`version`/`json`) on +the **30 clean pure libs + the clean shells that need no splitting** — pure relocation, no logic +change. Proves the wave discipline (relocate + encapsulate, CTest-green) before any god-module +surgery. CONTEXT.md §Phase Q (json extraction; directory + namespace map). +**Verify:** CTest green at every commit. The four duplicate `Parser`s are gone, replaced by one +`core/json` consumed by all four models; round-trip serialization is byte-identical to before +(no format change — a *structural* dedupe, not a behavior change). Every relocated clean module +compiles and its test executable passes unmoved. `Sample` (model) vs `AudioSample` (audio) vs +unified `Parser` (json) do not collide once sub-namespaced. No REAPER type crosses into any +`core/` file; the CMake pure/shell enforcement still holds. +**Depends on:** the GATE (tree quiescent). Nothing else in Phase Q. + +- [ ] Extract `core/json` (pure parser + serializer: parseString/parseInt/parseKey/skipValue + + escape, plus emit helpers); unify under `reasampler::json`; guard the `Parser` name against + cross-lib collision. Off all hot paths — safe to abstract freely. +- [ ] Rewire `bank_model`, `bank_book`, `view_mode_model`, `owned_manifest` onto `core/json`; + **delete the four duplicate `Parser`s.** Round-trip output byte-identical (dedupe, not + reformat). +- [ ] Relocate the 30 clean pure libs into `core/{model,view,capture,audio,ui,reclaim,version, + json}/` and the clean shells into `shell/{capture,panel,view,persist,actions}/`; move + `main.cpp` to `app/`. Update `CMakeLists.txt` `src/` paths only (no target-graph change). +- [ ] Apply sub-namespaces matching the directories on every relocated *clean* module (the + god-modules re-namespace their own new TUs as they split, W2–W5). Resolve `Sample`/ + `AudioSample`/`Parser` homes. +- [ ] Confirm CTest green + no hot-path change: `peaks`/audition/realtime-tick untouched by this + wave (pure relocation of clean modules; `peaks` stays a free function). + +## Q-W2 — split `bank_panel.cpp` (the biggest god-module, 2424 LOC) +**Goal:** Split the largest god-module (8+ responsibilities) along the audit's named seams: +`panel_render` / `panel_thumbnails` / `panel_audition` / `panel_input` / `panel_bank_ops` / +`panel_window`. Split the fat `bank_panel.h` alongside (Interface Segregation). **Preserve the +audition hot path as a direct call-through, never virtual.** `panel_bank_ops` becomes the single +home for the bank-CRUD verbs that W4 will dedupe `actions.cpp` against. CONTEXT.md §Phase Q +(bank_panel split seams; hot-path audition guardrail). See `docs/product/code-organization.md` +§2.1, §5. +**Verify:** CTest green at every commit. Each seam is its own TU under `shell/panel/`; the panel +draws, thumbnails, auditions, handles input, does bank ops, and manages its window exactly as +before (no behavior change — verify in DAW that the panel is visually and interactively +unchanged). Audition/preview call path stays a **direct call-through** (no virtual dispatch, no +added header→TU indirection on the preview path). The ~20-function public API is now segmented +across the split headers. +**Depends on:** Q-W1 (directory/namespace layout established). Independently landable. + +- [ ] Split rendering (`draw*`/`paint*`) → `panel_render`; thumbnail compute+cache → + `panel_thumbnails`. +- [ ] Split the audio audition/preview engine → `panel_audition` — **direct call-through, not + virtual; preview idle path unchanged.** +- [ ] Split input handling (mouse/key/wheel) + new-content detection → `panel_input`; window + lifecycle + OS drag-out/drop-target → `panel_window`. +- [ ] Extract bank-CRUD verbs → `panel_bank_ops` (the future single owner; W4 dedupes + `actions.cpp` against it). Split `bank_panel.h` into per-seam headers (I). +- [ ] Verify in DAW: panel unchanged; CTest green; no hot-path indirection added. + +## Q-W3 — split `main.cpp` (hoist orchestration; leave main = pointers + entry + dispatch) +**Goal:** Reduce `main.cpp` (1762 LOC) to its actual job — API pointers + `ReaperPluginEntry` + +dispatch (~the owns-pointers ~120 lines) — by hoisting: `capture_orchestrator` (`RunCapture` / +`captureAndIndexOne` / `renderOffline` / batch/recapture/realtime `Run*`), `scope_resolve` +(`resolveRange`/`resolveRazorRange`/`collectSelectedTracks` + provenance assembly inputs), and +`realtime_lifecycle` (the realtime-capture state machine + globals + selection guards). +**`FxBypassGuard` moves out but stays a stack RAII object (precision-critical); the realtime idle +tick stays a single pointer test.** CONTEXT.md §Phase Q (main split seams; FxBypassGuard + +realtime-tick guardrails). See `docs/product/code-organization.md` §2.1, §3. +**Verify:** CTest green at every commit. Capture (offline + realtime + batch + recapture) behaves +identically in DAW; the null test still nulls, bit-identical repeats still match (the precision +invariants `FxBypassGuard` protects are unchanged); capture ≠ placement holds (no hoisted `Run*` +path gains an `InsertMedia` call). The realtime idle fast-path is still a single pointer test. +`main.cpp` is now pointers + entry + dispatch only. +**Depends on:** Q-W1. Independent of Q-W2. + +- [ ] Hoist capture orchestration → `capture_orchestrator` (`shell/capture/`); keep + `FxBypassGuard` a **stack RAII** object as it moves (precision-invariant-critical). +- [ ] Hoist scope/source resolution + provenance assembly inputs → `scope_resolve`. +- [ ] Hoist the realtime-capture lifecycle state machine + globals + the two RAII selection + guards → `realtime_lifecycle`; **idle tick stays a single pointer test.** +- [ ] Leave `main.cpp` = API-pointer ownership + `ReaperPluginEntry` + dispatch; move to `app/`. +- [ ] Verify in DAW: null test nulls, bit-identical repeats match, capture≠placement holds; + CTest green; no realtime-tick branch-shape change. + +## Q-W4 — split `actions.cpp` + dedupe bank verbs against `panel_bank_ops` +**Goal:** Split the two unrelated command-id families in one TU (981 LOC) into +`design_view_actions` / `bank_actions` / `prune_action`, and **dedupe** `actions.cpp`'s own +`promptText`/`mintBankId` and bank verbs against the `panel_bank_ops` single-owner established in +Q-W2. `prune_action` keeps the `doBankPruneFolder` deletion authority contract intact (routes to +`persist`'s `prune_fs` after W5). CONTEXT.md §Phase Q (actions split seams; bank-verb dedupe). +See `docs/product/code-organization.md` §2.1, §2.4. +**Verify:** CTest green at every commit. Every action fires identically in DAW (Design View +family; multi-bank create/rename/reorder/delete/evacuate/activate/move/copy/remove; prune). The +bank-CRUD verbs have **one** implementation home (no `bank_panel`/`actions` duplication). Each +bank verb still wraps its mutation in one batched undo point; the prune action still writes no +ext state and opens no undo point. Command-id strings are **unchanged** (FOREVER-STABLE +contract — a reorg must not touch a shipped command id). +**Depends on:** Q-W2 (`panel_bank_ops` is the dedupe target). Independent of Q-W3. + +- [ ] Split → `design_view_actions` (toggle/activate/tag/untag/showBoth/moveItems), + `bank_actions` (bank CRUD family), `prune_action` (`doBankPruneFolder` — the single + file-deletion action). +- [ ] Dedupe `actions.cpp`'s `promptText`/`mintBankId` + bank verbs against `panel_bank_ops` + (one owner); do **not** change any command-id string. +- [ ] Verify in DAW: all action families fire unchanged; one bank op = one Ctrl-Z; prune still + no-undo/no-ext-state; CTest green. + +## Q-W5 — split `persist.cpp` (isolate the single file-deletion authority into `prune_fs`) +**Goal:** Split `persist.cpp` (766 LOC, 5 responsibilities) into `session` (lifecycle+poll, +`BeginLoadProjectState` reload hook), `ext_state_io` (the ext-state ↔ JSON serialization bridge + +GUID minting + folder relocation), and **`prune_fs`** (prune scanning + `deleteOrphanFile` via +`SHFileOperationW`). The split **concentrates** the byte-deleting authority into one obvious +module — it must never spread it. CONTEXT.md §Phase Q (persist split seams; deletion-authority +isolation). See `docs/product/code-organization.md` §2.1, §7. +**Verify:** CTest green at every commit. Session save/load/undo-reload, ext-state round-trip, +folder relocation, and prune deletion all behave identically in DAW. **File deletion lives in +exactly one module (`prune_fs`)** — the single-file-deletion-authority invariant is *improved* +(concentrated), never diluted. Relative-paths-only persistence is unchanged. +**Depends on:** Q-W1. Best after Q-W4 (so `prune_action` routes cleanly to `prune_fs`), but +independently landable. + +- [ ] Split → `session` (lifecycle/poll + `projectconfig` reload hook), `ext_state_io` + (serialization bridge + GUID minting + folder relocation). +- [ ] Isolate prune scanning + `deleteOrphanFile` (`SHFileOperationW`) → **`prune_fs`** — the + one file-deletion module; nothing else may delete bytes. +- [ ] Verify in DAW: save/load/undo-reload/relocation/prune unchanged; deletion authority is one + module; relative-paths-only holds; CTest green. + +## Q-W6 — OCP registration-table + residual fat-header (I) splits +**Goal:** Close the last SOLID wart: replace the ~350-line hand-written **non-table** action +registration blocks (now isolated in `app/main.cpp` after Q-W3) with a **registration table**, so +adding an action edits one place, not four parallel ones (OCP). Split any remaining fat headers +(`capture.h`/`persist.h`) not already resolved by their TU splits (I). CONTEXT.md §Phase Q (OCP +registration-table). See `docs/product/code-organization.md` §2.3, §6 (Q-6). +**Verify:** CTest green at every commit. Every action still registers, appears in the Actions +list, and fires via `hookcommand` exactly as before; command-id + display strings unchanged +(FOREVER-STABLE, per-channel); unload still mirror-unregisters everything. Adding a hypothetical +new action now touches the table only (demonstrated in review, not shipped). Remaining fat +headers are segmented. +**Depends on:** Q-W3 (registration code must be isolated first). Sequenced last; the most +droppable point if the phase needs narrowing (Q-6). + +- [ ] Convert the hand-written `Register("command_id"/"gaccel"/"hookcommand")` blocks to a + data-driven registration table; unload mirror-unregisters from the same table. +- [ ] Split residual fat headers (`capture.h`/`persist.h` and any other) alongside their TUs (I). +- [ ] Verify: all actions register/fire/unregister unchanged; command-id strings untouched; CTest + green. + +## Phase Q — sequencing +``` +GATE: Phase S + Phase L (L2+L3) + D2 residuals + M9 all merged/closed to dev (tree quiescent) + │ + ▼ +Q-W1 (safe opener: core/json extract + directory/namespace layout on clean modules) + ├─► Q-W2 (split bank_panel) ──► Q-W4 (split actions + dedupe bank verbs vs panel_bank_ops) + ├─► Q-W3 (split main.cpp; hoist orchestration) ──► Q-W6 (OCP registration-table + I splits) + └─► Q-W5 (split persist; isolate prune_fs) [best after Q-W4] +``` +W1 is the safe, high-leverage opener (all later waves assume the layout it establishes). The four +god-module splits (W2–W5) are risk-ordered and mostly parallel-safe; W4 depends on W2's +`panel_bank_ops`, W6 depends on W3's isolated registration code. Big-bang is rejected — every wave +is independently landable and CTest-green. + +## Phase Q — must-verify-before-build +- **Hot-path call/inline shape** — before landing each split, confirm no virtual dispatch and no + header→TU indirection was added on `peaks` envelope compute, audition/preview, or the realtime + tick. `computeEnvelope` stays a free function on `const std::vector&`; + audition stays a direct call-through; the idle tick stays a single pointer test. (CONTEXT.md + §Phase Q, `docs/product/code-organization.md` §3.) +- **Command-id + display strings are FOREVER-STABLE** — a reorg must not change a shipped + `command_id` string, action display name, ext-state namespace, or VST3 class UID. Re-namespacing + C++ symbols is orthogonal to these on-the-wire/on-disk contract strings; keep them byte-identical + (per-channel, per the Phase V V4 contract). (CONTEXT.md §Phase Q.) +- **Name-collision sweep on sub-namespacing** — `Sample` (model) vs `AudioSample` (audio) vs the + unified `Parser` (json), and any other cross-lib name that collides once flattened into + granular namespaces. Resolve by each symbol's new subsystem home before landing W1. (audit §2.4; + `docs/product/code-organization.md` §6 Q-4.) +- **The GATE** — do not begin any Q point until Phase S + Phase L (L2/L3) + D2 residuals + M9 are + merged/closed and the tree is quiescent. Re-confirm quiescence against dev before W1. diff --git a/docs/product/code-organization.md b/docs/product/code-organization.md new file mode 100644 index 0000000..33d73da --- /dev/null +++ b/docs/product/code-organization.md @@ -0,0 +1,403 @@ +# Code organization & structural quality — Phase Q framing + +The **why** behind a dedicated code-reorganization / structural-refactor pillar. This is +the framing and settled-decision record for **Phase Q (Quality)** — bringing the ReaSampler +`src/` tree into a healthier structure (more encapsulation, granular namespaces, `src/` +subdirectories) **without sacrificing runtime performance**, against a stated quality bar: + +> "**mtytel Vital is my code reference for quality.**" — Daniel +> +> Bring the codebase "**into the realm of something I can stand to look at.**" + +Its build roadmap lives in **PLAN.md §Phase Q** and its authoritative spec in +**CONTEXT.md §Phase Q**. This doc holds the *why* — the quality bar, the evidence base +(a grep-verified SOLID audit), the target directory/namespace shape grounded in the Vital +reference, and the numbered fork decisions. + +**Status:** framed by product-designer (2026-07-26). Forks Q-1 … Q-6 below are the decision +record; **Q-1 (namespace letter) is settled by this doc**; the remaining forks carry a +leading recommendation and are Daniel's to call. The SOLID audit that grounds every claim +is a **grep-verified** staff-engineer analysis of the actual `src/` tree, reproduced in §2. + +--- + +## 0. TL;DR + +- **This is not a feature phase — it is a quality phase.** The code works. The pure-core / + shell split is real and healthy (30 pure static libs, each with its own CTest executable, + the discipline CMake-enforces). What Phase Q fixes is that the *shape* of the code doesn't + yet read the way the architecture actually is: `src/` is one flat 45-file directory, all + 37 headers sit in one flat `reasampler` namespace, four modules have grown into + god-modules, and one utility (JSON parsing) is copy-pasted across four models. +- **The quality bar is Vital** (§1). Vital groups its ~1,000-file synth by *subsystem* + (`common/` `synthesis/` `interface/` `plugin/`) with nested functional sub-dirs. That is + the aspirational shape: the directory tree *is* the architecture diagram. ReaSampler's + subsystem groupings already exist — latent in the CMake link graph — but are invisible in + the code's shape. Phase Q makes them visible. +- **Performance is a hard constraint, not a nicety.** The reorg is directory + namespace + + file-split — it must cost **zero runtime**. The two hot paths (`peaks` envelope compute; + audition/preview; the realtime-capture tick) must keep their exact call/inline shape: **no + added virtual dispatch, no header→TU indirection on a hot path.** This is an explicit + acceptance criterion on every point, not a footnote (§3). +- **This phase is GATED on the tree being otherwise quiescent** (§4). A structural reorg that + lands while Phase S (on the phase-s worktree) and Phase L (L2/L3) are mid-flight would + create catastrophic merge conflicts — the reorg touches nearly every file, and every + in-flight branch is diffed against the *old* layout. Phase Q starts only when Phase S, + Phase L, and any D2/M9 residuals have merged to dev and the tree is quiet. Stated + prominently because getting the gate wrong is the one way this phase does real damage. +- **Every point is independently landable and CTest-green at every step** (§5). The CMake + targets already draw the module seams; a file move + namespace change keeps + `ctest --test-dir build` green at each point. Green-CTest-at-every-point is an acceptance + criterion, not an aspiration — it is *how* a reorg this broad stays safe. + +--- + +## 1. The quality bar — Vital, read from its actual structure + +Daniel named **Vital** (`github.com/mtytel/vital`, the spectral-warping wavetable synth) as +his code-quality reference. Read from the actual repository, not from reputation, here is +what makes Vital's structure the aspirational shape — and what maps cleanly onto ReaSampler. + +### 1.1 Vital groups by subsystem; the directory tree *is* the architecture + +Vital's `src/` is organized top-level by **subsystem concern**: + +| Vital `src/` dir | Contains | ReaSampler analogue | +|---|---|---| +| `common/` | shared utilities, data types used everywhere | the pure data/util core (`bank_model`, `app_version`, a new `json`) | +| `synthesis/` | the audio engine — nested further by concern | the capture/audio core (`capture`, `peaks`, `render_settings`, `wav_trim`) | +| `interface/` | all UI / editor code | the UI core + shells (`theme`, `component_geometry`, `bank_panel`, `draw_kit`) | +| `plugin/` | the plugin-wrapper / host-boundary layer | the REAPER-facing shells + `main.cpp` | +| `headless/` `standalone/` | build-shape entry points | (n/a in one extension binary; the Phase S VST3 is the second artifact) | + +And critically, `synthesis/` is **itself** subdivided by function — +`synth_engine/ modulators/ filters/ effects/ producers/ framework/ lookups/ utilities/`. +The grouping principle is **layered functional architecture**: core generation → processing +→ infrastructure → support, each a directory. You can read the subsystem map off the folder +tree without opening a file. *That* is "code you can stand to look at": the structure +teaches you the architecture. + +### 1.2 What ReaSampler has vs. what Vital has + +ReaSampler's architecture is **already** subsystem-layered — it is just invisible: + +- **The pure/shell split is real and CMake-enforced** — 30 pure static libraries, each with + its own test executable, none linking a REAPER SDK. This is the *load-bearing* discipline + and it is genuinely healthy (the SOLID audit confirms: "NO genuine core→REAPER leaks + found — the split is intact and CMake-enforced"). +- **But the *shape* hides it.** All 45 files sit in **one flat `src/` directory**; all 37 + headers sit in **one flat `reasampler` namespace**. The subsystem groupings — + model / view / capture / audio / ui / reclaim / version — exist only in the link graph + (`target_link_libraries` edges), never in the code you *read*. +- **Vital's lesson, applied:** ReaSampler doesn't need to *invent* an architecture — it needs + to make its existing one **legible**. The reorg is a change of *shape*, not of *substance*. + That is exactly why it can be zero-runtime-cost and CTest-green throughout: the seams + already exist; Phase Q draws them where a reader can see them. + +*(Note on the reference: Daniel wrote "mtytell"; the canonical repo is `mtytel/vital`. The +structural facts above are read from that repository's `src/` tree. Vital is GPLv3 — we +borrow its **structural pattern**, not its code.)* + +--- + +## 2. The evidence base — grep-verified SOLID audit (Single-Responsibility lens) + +The reorg is not driven by taste alone; it is grounded in a **grep-verified** staff-engineer +SOLID analysis of the actual `src/` tree (45 files / ~19,800 LOC). Reproduced here as the +decision-grade evidence; the PLAN points and CONTEXT spec cite back to it. + +### 2.1 The three structural problems that dominate + +1. **Four god-modules** — each carrying 4–8 distinct responsibilities: + - `bank_panel.cpp` (**2,424 LOC**) — 8+ responsibilities: rendering, thumbnail + compute+cache, audio audition engine, bank CRUD (*duplicates `actions.cpp`'s verbs*), + context menus, input handling, new-content detection, OS drag-out + drop-target, window + lifecycle + a ~20-function public API. + - `main.cpp` (**1,762 LOC**) — owns-the-API-pointers is only ~120 lines of it. Also + carries: full capture orchestration (`RunCapture` / `captureAndIndexOne` / + `renderOffline` / batch/recapture/realtime `Run*`), `FxBypassGuard` (100+ LOC, + precision-invariant-critical, buried), scope/source resolution, provenance assembly, + the realtime-capture lifecycle state machine + globals, two RAII selection guards, and + ~350 lines of hand-written registration boilerplate. + - `actions.cpp` (**981 LOC**) — two unrelated command-id families in one TU (Design View + family + multi-bank/prune family, the latter holding `doBankPruneFolder`, **the only + file-deletion authority**), plus its own `promptText`/`mintBankId` *duplicating* + `bank_panel`. + - `persist.cpp` (**766 LOC**) — 5 responsibilities: session lifecycle+poll, ext-state + serialization bridge, folder relocation, GUID minting, and prune scanning + filesystem + deletion (`deleteOrphanFile` via `SHFileOperationW`). + +2. **JSON serialization duplicated across four model modules.** `bank_model`, `bank_book`, + `view_mode_model`, and `owned_manifest` **each hand-roll their own `Parser`** + (parseString / parseInt / parseKey / skipValue + escape). This is the single largest + DRY + SRP violation and the **highest-leverage single fix** in the phase. + +3. **Zero namespace granularity.** All 37 headers live in one flat `reasampler` namespace; + `src/` is one flat directory. Subsystem groupings are latent in the CMake link graph but + invisible in code shape — the exact gap §1 identifies against Vital. + +### 2.2 Per-module Single-Responsibility verdicts (the audit's own words) + +- **Pure core is mostly clean.** `peaks`, `provenance`, `prune_reconcile`, and every geometry + mirror (`mode_switch` / `bank_grid` / `tab_strip` / `action_buttons` / + `component_geometry` / `prune_button`) are **single-responsibility exemplars** — leave them + alone, just relocate + namespace them. +- **Mixed (model + JSON):** `bank_model`, `bank_book`, `view_mode_model` (1,040 LOC — 4 + indices + planner + JSON), `owned_manifest`. The JSON extraction (Q-W1) resolves the mixed + half of each. +- **Shells split cleanly:** `capture` / `capture_realtime` (two backends behind one header); + `view` (derive-visibility + apply-to-REAPER). + +### 2.3 Other SOLID letters (secondary, in-scope where cheap) + +- **O (Open/Closed):** the capture-action *table* is good OCP; the ~350-line hand-written + **non-table** registration blocks in `main.cpp` are the opposite — every new action edits + 4 parallel places. A registration-table closes this (a candidate Q point, Q-6 dependent). +- **I (Interface Segregation):** `capture.h` / `persist.h` / `bank_panel.h` are **fat + headers** — split them alongside their TU splits. +- **L / D:** `ICaptureBackend`/`Offline`/`Realtime` is a clean Liskov story (no violation); + `main`/`bank_panel` depending on concrete backends is a low-priority D concern, explicitly + **out of scope** for Phase Q (it is a design change, not a reorg). + +### 2.4 Encapsulation gaps (beyond the four god-modules) + +- **JSON parser duplicated 4× → extract a pure `json` core** (the highest-leverage change). +- **Bank-CRUD verbs duplicated** in `bank_panel.cpp` *and* `actions.cpp` — dedupe to one + owner. +- `peaks` forces whole-file `std::vector` materialization on the thumbnail path (clean + API, but a data-ownership boundary forces a copy) — **noted, not scoped**: touching it + risks the hot path (§3), so it is a deliberate non-goal for Phase Q. + +--- + +## 3. Performance is a hard constraint (the guardrail, carried verbatim-in-spirit) + +Daniel's stated non-negotiable: reorganize **without sacrificing actual performance.** The +reorg is directory / namespace / file-split — it is **zero runtime cost IF** the following +hold. These are acceptance criteria on every relevant point, not advice: + +1. **Hot path — `peaks`** (`computeEnvelope` / `lastFrameAboveThreshold` over full PCM): + **NO virtual dispatch, NO `peaks` interface, NO added header→TU indirection.** Keep + `computeEnvelope` a **free function on `const std::vector&`** so it inlines exactly + as today. Relocating the file + namespacing it is fine; wrapping it in an abstraction is + forbidden. +2. **Hot path — audition / preview:** splitting `panel_audition` into its own TU is fine — + but the call must stay a **direct call-through, not virtual**. +3. **Hot path — realtime-capture tick:** keep the idle fast-path a **single pointer test.** + The realtime lifecycle may move to its own TU (`realtime_lifecycle`) but the tick's + branch shape must not change. +4. **JSON extraction is OFF all hot paths** — serialization runs at save/load, never per + frame. Safe to abstract freely (this is *why* Q-W1 is the safe opener). +5. **`FxBypassGuard` runs per-capture, not per-frame** — keep it a **stack RAII** object when + it moves out of `main.cpp`; do not heap-allocate or virtualize it. + +**Net (the audit's own conclusion):** every recommended split falls on a **cold path** or +**preserves call/inline shape** on the two hot ones. The acceptance bar for the phase: +*if a split would add an indirection on a path in the list above, it is out of scope — +rework the split to avoid it, or drop it.* + +--- + +## 4. The GATE — Phase Q starts only when the tree is quiescent (load-bearing) + +**Phase Q is gated on the completion of ALL other scheduled and in-flight work.** State this +prominently; it is the single most important sequencing fact in the phase. + +**Why the gate exists — merge-conflict blast radius.** Phase Q's whole nature is that it +touches **nearly every file in `src/`** (relocating into subdirectories, changing the +namespace of every header, splitting the four largest TUs). Meanwhile: + +- **Phase S** lives on the **phase-s worktree**, is **not on dev**, and is a large body of + work (a whole second VST3 build artifact + pure sampler core). Its branch is diffed against + the *current* flat layout. +- **Phase L** has **L2** (dock-panel layout redesign, itself gated after M11) and **L3** (VST + restyle, gated on Phase S) still to land — both touching `bank_panel` and the draw/UI + layer, exactly the files Phase Q's god-module split rewrites. +- **D2 residuals / M9** (deferred) could reactivate. + +A structural reorg landing while any of these is mid-flight would force every in-flight +branch through a **rename-and-relocate-everything** merge — the worst possible conflict +class (every hunk moved, every namespace-qualified reference changed). The cost is not linear; +it is a combinatorial re-resolution of every open branch against a moved tree. + +**The gate, stated as a rule:** Phase Q does not begin until **Phase S has merged to dev**, +**Phase L (L2 + L3) has merged to dev**, any **D2 residuals** are closed, and **M9** is either +landed or confirmed-abandoned — i.e. the tree is **quiescent**, with no large branch +outstanding. Phase Q is the *last* structural pillar precisely because it reshapes the ground +every other pillar stands on. Landing it early would tax every subsequent phase; landing it +last taxes nothing. + +*(Sequencing corollary: because the gate is "everything else first," Phase Q's own internal +sequencing —§5— is about risk-ordering the reorg, not about racing other phases.)* + +--- + +## 5. Sequencing shape — big-bang vs. incremental (feeds the waves) + +**Settled shape: incremental, risk-ordered waves, each independently landable and +CTest-green.** A big-bang "rename everything in one commit" is rejected — it defeats the one +safety property the CMake seams give us (green CTest at every step) and produces an +un-reviewable diff. Instead, the reorg is decomposed so each wave is a safe, reviewable, +individually-revertible step: + +- **W1 — the safe, high-leverage opener (zero-god-module-risk):** extract the pure `json` + core and delete the four duplicate `Parser`s, *plus* impose the `core/ shell/ app/` + directory layout and sub-namespaces on the modules that **don't** need splitting (the 30 + clean pure libs + the clean shells). This is pure relocation + one genuine + encapsulation win, no god-module surgery. Lowest risk, highest legibility payoff, done + first. +- **W2–W5 — the four god-module splits, one per wave**, ordered by risk (the audit's own + S-leverage ranking): `bank_panel` (W2, biggest), `main.cpp` (W3), `actions.cpp` (W4, + includes deduping bank verbs against `bank_panel`), `persist.cpp` (W5, isolates the + single file-deletion authority). Each god-module split is its own wave because each is a + large, independently-reviewable change with its own verification surface. +- **W6 — the OCP registration-table** (close the ~350-line hand-written registration blocks) + and any fat-header (I) splits not already resolved. Sequenced last because it depends on + the `main.cpp` split (W3) having already isolated the registration code. + +**Why incremental beats big-bang here, concretely:** the CMake per-module static-lib + per- +module test-executable structure means a file move + namespace change is *mechanically* +verifiable — `ctest` is green or it isn't, at every point. That property only pays off if the +reorg is *in points*. One giant commit throws the property away. Incremental is not just +safer; it is the only shape that uses the seams the architecture already gives us. + +--- + +## 6. Forks — the decision record + +**Fork Q-1 — namespace letter. SETTLED (this doc): `Q` (Quality).** + +- **Settled:** the phase is namespaced **`Q` (Quality)**. M / D / B / R / V / S / L are all + taken (Milestone / Design / Bank / Reclaim / Version / Sampler / Look-and-feel). +- **Considered and set aside:** `O` (Organization) — rejected on two grounds: (1) the glyph + `O` reads ambiguously against zero in point ids (`O1`, `O10`), a real legibility cost in a + phase *about* legibility; (2) "Organization" undersells the charter — this phase is + measured against a **quality bar** Daniel set (Vital), and the reorg is the *means*, not + the end. `Q` names the end. +- **Reasoning:** `Q` is unambiguous, unused, and reads sensibly ("Phase Q — Quality"). The + point-id family is `Q1 … Qn` with wave prefixes `Q-W1 … Q-W6` matching the house + wave-naming (cf. `D2-W1`, `ps-w6`, `pL-w2`). + +**Fork Q-2 — JSON extraction in scope? RECOMMEND: yes, and it is the W1 opener.** + +- **Recommendation:** **in scope, and first.** The 4× duplicated `Parser` is the single + largest DRY+SRP violation (§2.1), it is entirely **off the hot paths** (§3.4, safe to + abstract freely), and it is the highest-leverage single change in the audit. Extracting a + pure `core/json` module and deleting the four copies is the ideal low-risk opener — it + proves the wave discipline (relocate + encapsulate, CTest-green) before any god-module + surgery. +- **Alternative considered:** defer JSON to a later, separate cleanup. Rejected — it is the + cheapest, safest, highest-payoff move; deferring it wastes the opener slot on pure + relocation with no encapsulation win. +- **Watch (the audit flags it):** guard the `Parser` name-unification against cross-lib + collisions when the four copies merge into one; and mind `Sample` vs `AudioSample` when + sub-namespacing (§Q-5 collision note). + +**Fork Q-3 — directory naming: `core/ shell/ app/` vs a Vital-style subsystem-first tree. +RECOMMEND: `core/ shell/ app/` as the top split, subsystem dirs beneath `core/`.** + +- **Recommendation:** top-level by the **load-bearing discipline** (`core/` = pure/testable, + `shell/` = REAPER-facing, `app/` = the entry TU), then **subsystem dirs beneath** — the + audit's proposed map: `core/model/ core/view/ core/capture/ core/audio/ core/ui/ + core/reclaim/ core/version/ core/json/`, and `shell/capture/ shell/panel/ shell/view/ + shell/persist/ shell/actions/`, with `app/main.cpp`. +- **Why this over pure-Vital (subsystem-first, e.g. `model/ capture/ ui/` at top):** the + **pure/shell split is ReaSampler's most load-bearing invariant** — it is CMake-enforced and + it is the thing that keeps the core unit-testable outside the DAW. Making it the *top* level + of the directory tree makes the invariant **structurally visible and hard to violate** (a + file's directory tells you instantly whether it may touch a REAPER type). Vital has no + equivalent pure/host split to protect, so its subsystem-first tree is right *for Vital*; + `core/ shell/` is the ReaSampler-native reading of the same "directory = architecture" + lesson. Subsystem grouping still happens — one level down — so we get both readings. +- **Alternative:** flat subsystem-first (`model/ view/ capture/ audio/ ui/ …`) with the + pure/shell distinction living only in namespaces. Rejected: it demotes the most important + invariant from *structure* to *convention*, which is exactly the drift Phase Q exists to + reverse. + +**Fork Q-4 — sub-namespace, or sub-directory only? RECOMMEND: both — sub-namespace to match +the sub-directory.** + +- **Recommendation:** granular sub-namespaces mirroring the directories: + `reasampler::model`, `reasampler::view`, `reasampler::capture`, `reasampler::audio`, + `reasampler::ui`, `reasampler::reclaim`, `reasampler::version`, `reasampler::json`. Directory + and namespace agree, so a symbol's home is unambiguous from either. +- **Why:** namespaces are the *code-visible* half of the "make the architecture legible" goal + (directories are the filesystem half). Sub-dirs without sub-namespaces leaves every symbol + still in one flat `reasampler::` soup — half the win. Vital's own headers group by concern; + matching namespace-to-directory is the standard C++ reading of that. +- **Cost (name it):** sub-namespacing is the change with the **widest edit surface** — every + qualified reference across TUs updates. This is precisely why the GATE (§4) matters and why + W1 does the namespace move on the *clean* modules first (mechanical, no logic change), with + the god-module splits (W2–W5) namespacing their own new TUs as they land. +- **Collision watch (audit):** `Sample` (bank_model) vs `AudioSample` (peaks alias) and the + unified `Parser` must not collide once sub-namespaced — resolve by their new + `model::` / `audio::` / `json::` homes. + +**Fork Q-5 — how aggressive the god-module split? RECOMMEND: split to the audit's named seams, +no finer.** + +- **Recommendation:** split each god-module along the **exact seams the audit names** and + stop there: + - `bank_panel` → `panel_render` / `panel_thumbnails` / `panel_audition` / `panel_input` / + `panel_bank_ops` / `panel_window`. + - `main.cpp` → hoist `capture_orchestrator` / `scope_resolve` / `realtime_lifecycle`, + leaving `main` = API pointers + entry + dispatch. + - `actions.cpp` → `design_view_actions` / `bank_actions` / `prune_action`, deduping the + bank verbs against `bank_panel`'s `panel_bank_ops`. + - `persist.cpp` → `session` / `ext_state_io` / `prune_fs` (**isolating the single + file-deletion authority** into `prune_fs`). +- **Why stop there:** the seams are already validated by the audit and correspond to real + responsibilities. Splitting *finer* (one file per function) would trade a god-module for a + fragmentation problem — the opposite failure. "Something Daniel can stand to look at" is + well-factored, not atomized. +- **Alternative (lighter):** split only the two worst (`bank_panel`, `main.cpp`), leave + `actions`/`persist` as-is. Rejected: `persist`'s buried file-deletion authority and + `actions`'s duplicated bank verbs are real hazards worth resolving while the tree is open; + doing them now (behind the gate) is cheaper than a second reorg later. + +**Fork Q-6 — the OCP registration-table (W6): in scope or deferred? RECOMMEND: in scope, last.** + +- **Recommendation:** **in scope as the final wave.** The ~350-line hand-written registration + blocks are a genuine OCP wart (every new action edits 4 parallel places); a registration + table closes it. Sequenced **last** because it depends on the `main.cpp` split (W3) having + isolated the registration code — you can't table-ify code you haven't first extracted. +- **Alternative:** drop it — it's OCP, not the SRP focus of the phase. A reasonable trim if + Daniel wants Phase Q strictly scoped to the reorg. Kept in with a leading recommendation + because, once W3 has hoisted the registration code, tabling it is a small, high-legibility + finish — but it is the most droppable point if the phase needs narrowing. + +--- + +## 7. What Phase Q does NOT change (guardrails) + +- **The pure-core / shell split is strengthened, never dissolved.** The whole point is to + make it *more* legible (top-level `core/` vs `shell/`). No file moves across the boundary; + no core file gains a REAPER type; the CMake per-module test executables stay green. +- **Every precision invariant holds.** Null-test, bit-identical repeats, non-destructive + capture, exact bounds, relative-paths-only — none is code Phase Q rewrites. `FxBypassGuard` + (the precision-critical guard) *moves* out of `main.cpp` but stays a stack RAII object with + identical behavior (§3.5). +- **Capture ≠ placement.** No `Run*` / capture-orchestrator path may gain an `InsertMedia` + call during the split. The load-bearing rule is invariant under reorganization. +- **The single file-deletion authority stays one obvious module.** `persist`'s `prune_fs` + split *concentrates* the deletion authority (`SHFileOperationW`) into one named module — + it must not spread it. This is a safety property the reorg improves, never dilutes. +- **Relative-paths-only in the persisted index** — untouched (a data invariant, not a + structural one). +- **Performance** — §3 is a hard acceptance bar: no hot-path indirection, ever. +- **No behavior change.** Phase Q is a pure structural refactor. If a point changes observable + behavior, it has exceeded its charter and must be reworked. The test suite passing + unchanged **is** the proof of correctness (green CTest at every point). + +--- + +## 8. Sources + +- Vital source structure — `github.com/mtytel/vital`, `src/` subsystem tree + (`common/ synthesis/ interface/ plugin/ headless/ standalone/`) and the nested + `synthesis/` functional sub-dirs (`synth_engine/ modulators/ filters/ effects/ producers/ + framework/ lookups/ utilities/`). Read from the repository tree, 2026-07-26. Vital is + GPLv3 — the borrowed artifact is the **structural pattern**, not code. +- The SOLID-compliance audit (§2) — grep-verified staff-engineer analysis of the actual + ReaSampler `src/` tree (45 files / ~19,800 LOC), 2026-07-26. The evidence base for every + reorg move and the performance guardrails (§3).