Fix undo-label misnomer, tighten drain-nesting/abort-path docs, mark inferred undo behavior as unverified

Renamed the drain's undo point to "Design View FX state" (it restores as often as it parks). Extended the nesting audit to cover a block opening inside the drain's own block. Marked post-fix undo behavior as inference pending DAW checks.
This commit is contained in:
2026-08-05 20:54:31 -04:00
parent 13c824c63d
commit 8a7056d19d
5 changed files with 59 additions and 32 deletions
+5 -4
View File
@@ -48,10 +48,11 @@ Checks for Θ, Ξ, Ψ, Ε, Ρ, Γ, and Ω work that no unit test can close. Buil
- [ ] Attempt a mode switch while the transport is playing, then while recording — both refuse, visibly (`docs/COMPLETED.md` §"Ψ-W1-T2") - [ ] Attempt a mode switch while the transport is playing, then while recording — both refuse, visibly (`docs/COMPLETED.md` §"Ψ-W1-T2")
- [ ] Click the footer mode segment, save, reopen the project — the mode persisted (`docs/COMPLETED.md` §"Ψ-W1-T2") - [ ] Click the footer mode segment, save, reopen the project — the mode persisted (`docs/COMPLETED.md` §"Ψ-W1-T2")
- [ ] `[verify — DAW]` Reproduce the strand: project with FX in both modes, switch to Design, save, close, reopen, toggle to Arrange. Toggle back and forth several more times — the arrangement's FX must NOT converge on permanently offline/hidden. A track the tool refuses to park is NAMED in the console, and the same refusal is not reprinted on a later reapply unless the refused set changed (`src/shell/view/view_fx_park.h`'s `decidePark` / `reportRefusedParks`) - [ ] `[verify — DAW]` Reproduce the strand: project with FX in both modes, switch to Design, save, close, reopen, toggle to Arrange. Toggle back and forth several more times — the arrangement's FX must NOT converge on permanently offline/hidden. A track the tool refuses to park is NAMED in the console, and the same refusal is not reprinted on a later reapply unless the refused set changed (`src/shell/view/view_fx_park.h`'s `decidePark` / `reportRefusedParks`)
- [ ] `[verify — DAW]` **The undo collapse.** In a project with several FX across several tracks, switch modes once and count the undo points REAPER shows (Ctrl-Z tooltip / undo history): expect exactly TWO — `ReaSampler: Design View FX park` above `ReaSampler: activate <mode> view` — never one per FX (`src/shell/view/view_fx_park.h`'s `fxParkUndoClose`) - [ ] `[verify — DAW]` **The undo collapse.** In a project with several FX across several tracks, switch modes once and count the undo points REAPER shows (Ctrl-Z tooltip / undo history): expect exactly TWO — `ReaSampler: Design View FX state` above `ReaSampler: activate <mode> view` — never one per FX (`src/shell/view/view_fx_park.h`'s `fxParkUndoClose`)
- [ ] `[verify — DAW]` Then press Ctrl-Z once: every FX re-onlines in that one step and the flags stay parked. A second Ctrl-Z rolls the flags and the ext state back. The FX point must sit ABOVE the switch point, never below it and never folded into it (`src/shell/view/CLAUDE.md` §Invariants — the observed undo semantics) - [ ] `[verify — DAW]` Then press Ctrl-Z once: every FX re-onlines in that one step and the flags stay parked. A second Ctrl-Z rolls the flags and the ext state back. The FX point must sit ABOVE the switch point, never below it and never folded into it. Verify the EFFECT, not merely that a point exists in the history — a mask narrower than `UNDO_STATE_FX` covers would still show an entry there while doing nothing to FX state on Ctrl-Z, which would look like a pass and isn't (`src/shell/view/CLAUDE.md` §Invariants — the undo-ordering premise, still `[verify — DAW]` until this item and the next close it)
- [ ] `[verify — DAW]` Open a project saved in Design mode and check the undo history: the load-tick reapply writes no FX state (they are already offline), so it must add NO undo point at all — the discard form. A `ReaSampler: Design View FX park` entry appearing on a plain project open means the discard is not working (`src/shell/view/view_fx_park.h`'s `fxParkUndoClose`) - [ ] `[verify — DAW]` Open a project saved in Design mode with every inactive leaf's FX already offline, and check the undo history: the load-tick reapply then writes no FX state, so it must add NO undo point at all — the discard form. This only holds when the saved state was actually fully parked — a first open after tagging a new leaf, an FX added to a parked track while it was offline, or a plugin hand-onlined since the last save all have real FX state to write and legitimately mint one correct `ReaSampler: Design View FX state` point; don't record those as a false failure. An entry appearing on a project that WAS fully parked means the discard is not working (`src/shell/view/view_fx_park.h`'s `fxParkUndoClose`)
- [ ] `[verify — DAW]` Whether `Undo_EndBlock2` tolerates a `ReaProject*` that closed between it and its `Undo_BeginBlock2`. The drain closes its block unconditionally on the project-gone abort path — an unbalanced block would swallow every later action into one point, which is the worse failure — but the SDK header promises nothing here. Reproduce by closing a project tab while a mode switch's FX drain is in flight (`src/shell/view/view_fx_park.cpp`'s `FxParkUndoBlock`) - [ ] `[verify — DAW]` Whether `Undo_EndBlock2` tolerates a `ReaProject*` that closed between it and its `Undo_BeginBlock2`, AND which of two undocumented block-nesting models REAPER implements — a GLOBAL counter (an unbalanced block swallows every later action, in ANY project, into one point) or a PER-PROJECT one (an unbalanced block on a now-dead project costs nothing). The drain closes its block unconditionally on the project-gone abort path on the assumption the global model applies; record which model is actually observed, not just whether the call survives. Reproduce by closing a project tab while a mode switch's FX drain is in flight (`src/shell/view/view_fx_park.cpp`'s `FxParkUndoBlock`)
- [ ] `[verify — DAW]` **Block-inside-drain.** The nesting audit at `drainDeferredFxParks` only covers the drain opening inside an already-open caller block; the pumping premise it documents (`FxParkQueue::take`) also makes the inverse possible — a re-entrant `applyMode` (an action fired mid-drain) or a re-entrant `OnTimer` -> `bankPanelRefresh` -> `mintManagedLanes` opening ITS OWN `Undo_BeginBlock2`/`Undo_EndBlock2` pair while the drain's block is still open. `g_draining` blocks only a second `drainDeferredFxParks` call, not this. Reproduce by triggering a mode-switch action or a lane-mint mid-drain (a heavy FX chain widens the window) and check the undo history for a split or misordered point (`src/shell/view/view_fx_park.h`'s `drainDeferredFxParks`)
- [ ] `[verify — DAW]` For a track the console names as left unparked, follow the message's recovery IN FULL: re-enable its TCP/mixer visibility, main send and FX enable, **and set every FX in its chain online** (FX enable is the chain bypass — it does not bring an individually offlined FX back). Then switch modes twice: it parks and restores normally, with no further console line. Do the flag half ONLY on a second such track and confirm it is still refused — the FX half of the trust test, and the reason the message spells the step out (unit-tested only as a decision, never against a live chain) - [ ] `[verify — DAW]` For a track the console names as left unparked, follow the message's recovery IN FULL: re-enable its TCP/mixer visibility, main send and FX enable, **and set every FX in its chain online** (FX enable is the chain bypass — it does not bring an individually offlined FX back). Then switch modes twice: it parks and restores normally, with no further console line. Do the flag half ONLY on a second such track and confirm it is still refused — the FX half of the trust test, and the reason the message spells the step out (unit-tested only as a decision, never against a live chain)
## Actions and drops ## Actions and drops
+23 -22
View File
@@ -70,24 +70,24 @@ decide membership or mode rules.
offline/online is enqueued and applied on a later idle tick (`view_fx_park`), offline/online is enqueued and applied on a later idle tick (`view_fx_park`),
so the new mode paints first. The deferral changes only WHEN the plugins move — so the new mode paints first. The deferral changes only WHEN the plugins move —
they still unload and re-instantiate, and un-persisted internal state is still they still unload and re-instantiate, and un-persisted internal state is still
lost. What it does change is the undo record, and REAPER's behaviour there is lost. What it does change is the undo record. What was MEASURED, before the fix
now OBSERVED, not assumed. What was measured, before the fix below: a below: a `TrackFX_SetOffline` made outside any undo block mints ONE implicit
`TrackFX_SetOffline` made outside any undo block mints ONE implicit undo point undo point per call, and a small project showed 10 points for one switch — 1
per call, and a small project showed 10 points for one switch — 1 for the switch for the switch itself and 9 for its FX — which Ctrl-Z then walked one FX at a
itself and 9 for its FX — which Ctrl-Z then walked one FX at a time. **The drain time. **The drain now opens its own undo block** (`fxParkUndoClose`,
now opens its own undo block** (`fxParkUndoClose`, `UNDO_STATE_FX` only, no other `UNDO_STATE_FX` only, no other domain) on the PREMISE that an explicit block
domain), so one drain is one point and one switch costs two: the switch's, then suppresses those per-call implicit points. That premise, and everything below
the drain's. A drain that wrote no FX state closes with REAPER's discard form that follows from it — one drain being one point, a switch costing two, a
and mints nothing at all, so a plain project open costs no undo point no-write drain minting nothing (`fxParkUndoClose` owns why), and ONE Ctrl-Z
(`fxParkUndoClose` owns why). The ordering property the observation established is DELIBERATELY PRESERVED: the re-onlining every FX at once with a second rolling the flags and the ext state
drain's block is its own and never the switch's, so the FX point still sits ABOVE back is INFERENCE from the code's design, not observed: `docs/VERIFICATION.md`
the switch point, and an undo deep enough to restore the driven flags and roll §"Mode switching" (items 5153) is where this gets checked against REAPER. The
the `"reasampler"` ext state back has already re-onlined every FX — flags, FX and ordering property the pre-fix measurement established is DELIBERATELY
snapshot still move together. Folding the FX writes into the switch's own block, PRESERVED BY DESIGN regardless: the drain's block is its own and never the
or making them undo-silent, would break that and is not an option here. So ONE switch's, so the FX point is meant to sit ABOVE the switch point, with flags,
Ctrl-Z after a switch now re-onlines every FX at once, and a second rolls the FX and snapshot moving together on undo. Folding the FX writes into the
flags and the ext state back — two steps rather than the measured ten, and never switch's own block, or making them undo-silent, would break that and is not an
one, which is the standing price of the drain being a separate act. option here.
**The idle tick is not the only drain point.** Any path that **The idle tick is not the only drain point.** Any path that
serializes the view model drains synchronously first (`persistViewState`, serializes the view model drains synchronously first (`persistViewState`,
`render_in_place`), because a save landing between a restore's synchronous flag `render_in_place`), because a save landing between a restore's synchronous flag
@@ -169,10 +169,11 @@ applies the resulting lane state to live tracks.
never falsely committed — and never finished either, since the refusal skips the never falsely committed — and never finished either, since the refusal skips the
FX enqueue too. Full contract at `discardDeferredFxParks`. FX enqueue too. Full contract at `discardDeferredFxParks`.
- **Which routes actually reach that no-snapshot-plus-parked-chain pair.** An undo - **Which routes actually reach that no-snapshot-plus-parked-chain pair.** An undo
of a mode switch (Ctrl-Z) is NOT one of them: per the observed undo semantics of a mode switch (Ctrl-Z) is NOT one of them, ON THE UNDO-ORDERING PREMISE
above, the drain's point sits above the switch's, so every FX is back online by above (`[verify — DAW]`, not yet observed — see Invariants): if the drain's
the step that rolls the flags and the ext state back — and the step in between point does sit above the switch's, every FX is back online by the step that
(FX online, flags still parked) still holds the model's snapshot, which decides rolls the flags and the ext state back — and the step in between (FX online,
flags still parked) still holds the model's snapshot, which decides
`ParkOnly`, not a refusal. Whether the ext-state `ParkOnly`, not a refusal. Whether the ext-state
snapshot comes back with them is INFERENCE, not observed, and untested in the snapshot comes back with them is INFERENCE, not observed, and untested in the
REDO direction: `[verify — DAW]` does a redo that re-parks a track's flags/FX REDO direction: `[verify — DAW]` does a redo that re-parks a track's flags/FX
+10 -4
View File
@@ -106,10 +106,16 @@ RestoreOutcome applyRestore(MediaTrack* tr, const std::vector<FxOfflineOp>& ops)
// per-FX fragmentation this closes). The block is deliberately the drain's OWN // per-FX fragmentation this closes). The block is deliberately the drain's OWN
// and never the switch's — the FX point has to stay ABOVE the switch's. // and never the switch's — the FX point has to stay ABOVE the switch's.
// //
// Closing is unconditional on every exit, the abort path included: an unbalanced // Closing is unconditional on every exit, the abort path included. Which of two
// block would swallow every later action into one point, which is far worse than // undocumented models REAPER uses decides whether that is the safe call:
// [verify — DAW] whatever Undo_EndBlock2 does with a ReaProject* that has closed // under a GLOBAL block-nesting counter, leaving this one unbalanced would
// since Undo_BeginBlock2 took it. // swallow every later action — in ANY project — into one point, far worse than
// calling Undo_EndBlock2 on a ReaProject* that closed since Undo_BeginBlock2
// took it. Under a PER-PROJECT counter, an unbalanced block on a now-dead
// project costs nothing, and the closing call against a stale pointer is the
// riskier of the two. Closing unconditionally is the safer bet under the
// global model and merely unnecessary under the per-project one, hence the
// choice. [verify — DAW] which model REAPER actually implements.
class FxParkUndoBlock { class FxParkUndoBlock {
public: public:
explicit FxParkUndoBlock(ReaProject* proj) : proj_(proj) { Undo_BeginBlock2(proj_); } explicit FxParkUndoBlock(ReaProject* proj) : proj_(proj) { Undo_BeginBlock2(proj_); }
+15 -1
View File
@@ -283,7 +283,9 @@ struct FxParkUndoClose {
inline FxParkUndoClose fxParkUndoClose(bool wroteAnyFx) { inline FxParkUndoClose fxParkUndoClose(bool wroteAnyFx) {
if (!wroteAnyFx) return {}; if (!wroteAnyFx) return {};
return FxParkUndoClose{"ReaSampler: Design View FX park", kFxParkUndoMask}; // "FX state", not "FX park": the drain restores as often as it parks, and a
// direction-coded label would misname the point that brings FX back online.
return FxParkUndoClose{"ReaSampler: Design View FX state", kFxParkUndoMask};
} }
// Applies every pending intent. Touches NO model state — a restore's snapshot is // Applies every pending intent. Touches NO model state — a restore's snapshot is
@@ -307,6 +309,18 @@ inline FxParkUndoClose fxParkUndoClose(bool wroteAnyFx) {
// header either way. All three call sites drain outside any block they open // header either way. All three call sites drain outside any block they open
// (OnTimer holds none; persistViewState is reached only after applyMode's and // (OnTimer holds none; persistViewState is reached only after applyMode's and
// doMoveItems' blocks have closed; render_in_place drains after its EndBlock2). // doMoveItems' blocks have closed; render_in_place drains after its EndBlock2).
//
// That audit is ONE-DIRECTIONAL — it covers the drain opening inside a caller's
// block, not the inverse the pumping premise above (FxParkQueue::take) makes
// possible: a block opening INSIDE the drain's own open block. Two routes are
// live if that premise holds: a re-entrant applyMode (view.cpp) fired by an
// action mid-drain, and a re-entrant OnTimer -> bankPanelRefresh ->
// mintManagedLanes (view.cpp) — both open their OWN Undo_BeginBlock2/EndBlock2
// pair. g_draining (view_fx_park.cpp) only blocks a SECOND drainDeferredFxParks
// call; it does nothing to stop a different subsystem's block opening while
// this one is still open. `[verify — DAW]` whether that nested pair (undocumented
// either way, as above) does anything worse than the ordering already assumed —
// see docs/VERIFICATION.md.
void drainDeferredFxParks(); void drainDeferredFxParks();
// Drops every pending intent without applying it. Called when the model the // Drops every pending intent without applying it. Called when the model the
+6 -1
View File
@@ -408,6 +408,11 @@ static void testAReEntrantParkCancelsOnlyWhatIsStillPending() {
} }
// -- the drain's undo point -------------------------------------------------- // -- the drain's undo point --------------------------------------------------
//
// These pin fxParkUndoClose's own fold only. The real wroteAnyFx verdict —
// setOfflineIfChanged -> applyPark/applyRestore -> FxParkUndoBlock::noteWrite
// -- runs against live REAPER calls and cannot be pinned here; docs/VERIFICATION.md
// items 51 and 53 are the sole cover for that plumbing being wired correctly.
static void testADrainThatWroteFxClosesItsBlockAsOneNamedFxPoint() { static void testADrainThatWroteFxClosesItsBlockAsOneNamedFxPoint() {
const FxParkUndoClose close = fxParkUndoClose(true); const FxParkUndoClose close = fxParkUndoClose(true);
@@ -418,7 +423,7 @@ static void testADrainThatWroteFxClosesItsBlockAsOneNamedFxPoint() {
// track config or items it never touched. // track config or items it never touched.
CHECK(close.mask == 2); CHECK(close.mask == 2);
CHECK(close.label != nullptr && std::string(close.label) == CHECK(close.label != nullptr && std::string(close.label) ==
"ReaSampler: Design View FX park"); "ReaSampler: Design View FX state");
} }
static void testADrainThatWroteNothingClosesItsBlockAsADiscard() { static void testADrainThatWroteNothingClosesItsBlockAsADiscard() {