Files

T

daniel a20cb46d65 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.

2026-07-26 20:56:31 -04:00

25 KiB

Raw Blame History

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

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).
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.
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<float> 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:

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<float>& so it inlines exactly as today. Relocating the file + namespacing it is fine; wrapping it in an abstraction is forbidden.
Hot path — audition / preview: splitting panel_audition into its own TU is fine — but the call must stay a direct call-through, not virtual.
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.
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).
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 Parsers, 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).

25 KiB Raw Blame History Unescape Escape