docs: spec Phase Q (Quality) — structural reorganization pillar
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.
This commit is contained in:
+217
@@ -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::<subsystem>`):**
|
||||
- `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<float>&`** 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<float>` 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.
|
||||
|
||||
Reference in New Issue
Block a user