From 86aa8dabbafe0687e17e843d68141179443d3a8d Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Fri, 7 Aug 2026 12:53:42 -0400 Subject: [PATCH] Collapse a Design View mode switch and its background FX parking to ONE undo point applyMode now returns with its undo block open; the reclaimer closes it when the drain settles. A transport deferral or a tab switch ends it early -- two points, never a leak. --- src/app/main.cpp | 4 + src/shell/view/CLAUDE.md | 71 +++++++++---- src/shell/view/fx_reclaim.cpp | 61 +++++++++-- src/shell/view/fx_reclaim.h | 135 +++++++++++++++++++++-- src/shell/view/view.cpp | 27 +++-- src/shell/view/view.h | 7 +- tests/test_fx_reclaim.cpp | 194 +++++++++++++++++++++++++++++++++- 7 files changed, 449 insertions(+), 50 deletions(-) diff --git a/src/app/main.cpp b/src/app/main.cpp index 554568b..2d7e60e 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -324,6 +324,10 @@ extern "C" REAPER_PLUGIN_DLL_EXPORT int REAPER_PLUGIN_ENTRYPOINT( // altered transport/cursor in the user's project on unload. capture::AbortRealtimeCaptureForUnload(g_session); + // Same reason, for the reclaimer's undo span: a block left open here has no + // owner left to close it and would swallow every edit the user makes after. + reasampler::fxReclaimEndUndoSpan(); + g_rec->Register("-timer", (void*)&OnTimer); g_rec->Register("-projectconfig", (void*)&g_projectConfig); g_rec->Register("-toggleaction", (void*)&OnToggleAction); diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index d20eff5..571d5d3 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -64,17 +64,35 @@ decide membership or mode rules. offline/online runs on the background reclaimer (`fx_reclaim`), in ticks bounded by `kOnlineBudgetMs`/`kOfflineBudgetMs`, off `applyMode`'s block entirely (measured: `TrackFX_SetOffline` was 1897ms of a 1942ms switch). -- **ONE mode switch is ONE undo point.** The driven flags and the lane state - written inside `applyMode`'s block roll back in a single Ctrl-Z. (The - `view_state` ext-state write is NOT among them; it lands after the block, see - Gotchas.) Per-FX offline/online moved off this block entirely, onto the - reclaimer (`fx_reclaim`) — each write there runs inside its OWN discarded - block (`Undo_EndBlock2(proj, "", 0)`), so it mints nothing. DAW-VERIFIED: ten - unblocked `TrackFX_SetOffline` calls minted ten undo points; the same ten - calls wrapped in `Undo_BeginBlock2`/`Undo_EndBlock2(proj, "", 0)` minted zero - — confirming the discard-block premise the reclaimer's per-write block - (`fx_reclaim.cpp`'s `writeOffline`) rests on. -- **A reapply that wrote nothing leaves no undo point.** The load-tick reapply +- **ONE mode switch is ONE undo point — the background FX parking included.** The + driven flags, the lane state, AND the per-FX offline/online the reclaimer writes + minutes-of-plugin-loads later all roll back in a single Ctrl-Z, because they share + ONE undo block: `applyMode` opens it (`fx_reclaim`'s `UndoSpan`) and a REAL switch + RETURNS WITH IT OPEN, for the reclaimer to close once the drain has converged. A + reapply closes its own before returning. This does not move an FX write back onto + the switch — the writes still run on the timer, off the 1897ms path. + DAW-MEASURED, and the reason the design is shaped this way: block WIDTH is the + entire effect. For ten `TrackFX_SetOffline` calls from a TIMER — unblocked, or + under a block per write, by every dispatch route — REAPER minted TEN points; under + ONE block spanning all ten it minted ONE. The `("", 0)` discard form does NOT + discard outside an action, it coalesces; it discards only from a hand-fired action. + A block opened on one timer callback and closed on a LATER one holds. Whether one + held from an ACTION into a later timer callback holds is the same property but is + `[verify — DAW]`; if it does not, the switch and its parking land as two points, + which is the intended degradation. +- **No path may return past a terminating condition with the span open.** A leaked + block swallows every edit the user makes afterwards, indefinitely, so every + terminating path routes through ONE idempotent close (`fxReclaimEndUndoSpan`): the + drain settling, aborting on a generation bump, or deferring on the transport; the + project retargeting or closing; the next apply superseding it; a project save; + extension unload. The span may stay open only across a wait it can BOUND — the 3s + debounce, which is the dead time between the switch and its first FX write, and + the only reason the span exists. A TRANSPORT deferral is not bounded (it lasts as + long as the playback pass), so it ends the span and the parking that follows mints + its own point (`spanEndsAfter`). +- **A reapply that wrote nothing leaves no undo point** — and a reapply never holds + the span open, so a project open, a membership edit and `render_in_place`'s inner + reapply all keep the block shape they had. The load-tick reapply re-plans a park for every inactive leaf, and over a project saved fully parked it finds every flag and every FX already where the plan wants them. The block then closes on the `("", 0)` discard form (`view.h`'s `applyMintsUndoPoint`; @@ -124,13 +142,13 @@ applies the resulting lane state to live tracks. ## Modules -- `view` — Design View shell: runs the pure planner over the live folder tree and iterates its park/restore plans, applies managed-lane and parent-visibility writes, and owns the ONE undo block all of that rides (`applyMintsUndoPoint` decides whether it leaves a point). Owns the one discriminator (`target != active`) that separates a real switch from a reapply, and with it both the playback gate (`transportBlocksModeSwitch`) and the solo cache/clear/restore seams. **Never touches master or `B_MUTE`.** FX are NOT written here — see `fx_reclaim`. +- `view` — Design View shell: runs the pure planner over the live folder tree and iterates its park/restore plans, applies managed-lane and parent-visibility writes, and OPENS the ONE undo span all of that rides (`applyMintsUndoPoint` decides whether it leaves a point; on a real switch `fx_reclaim` is what closes it). Owns the one discriminator (`target != active`) that separates a real switch from a reapply, and with it both the playback gate (`transportBlocksModeSwitch`) and the solo cache/clear/restore seams. **Never touches master or `B_MUTE`.** FX are NOT written here — see `fx_reclaim`. - `view_park` — the park surface, FLAG DOMAIN ONLY. `parkTrack` captures the pre-park snapshot (flags AND the FX chain's identity-keyed offline state) when the model holds none, drives the four flags, and EXPOSEs a track that reads park-shaped with no record behind it rather than adopting it; `restoreTrack` writes the flags back from the snapshot and spends its flag half (`releaseSnapshotFlags`). Also `trackFlagParm` (the one `Flag` → REAPER-parameter mapping), `writeIfChanged` (the read-before-write every driven parameter goes through), `fxGuidString` (the one FX-identity read snapshot/reclaim both address FX through), `kViewUndoMask`, and `reportExposedTracks`/`describeExposedTracks` (named tracks, once per changed set per project). - `view_lanes` — managed fixed lanes: `mintManagedLanes` splits multi-mode content onto per-mode lanes, `applyLaneOps` drives `C_LANEPLAYS` for the active mode, `reconcileManagedLanes` recovers ownership from durable `P_LANENAME` on project open. Manual lanes are never minted, driven, renamed or reassigned. - `view_reconcile` — the repair pass for a project state REAPER reloaded under us: `noteAppliedState` records what an apply left driven; `reconcileAppliedFlags` diffs that fingerprint against the reloaded model and hands back any track the reload no longer records but still reads driven — the fix for `UNDO_STATE_TRACKCFG` not carrying `B_SHOWINTCP`/`B_SHOWINMIXER` (DAW-confirmed). - `view_tracks` — the scaffolding every write pass sets up: `TrackHandles`/`TrackByGuid`, `readFolderEntries`/`indexHandles`, and `UiRefreshHold` (the `PreventUIRefresh` RAII pair every write phase runs under). - `view_solo` — the `I_SOLO` read/write pair behind the per-mode solo surface, plus `clearTrackSolos`/`restoreTrackSolos`, the outgoing-clear and incoming-replay entry points `view` drives them through. Holds no policy: what to cache, clear, or replay is `core/view/solo_cache`. -- `fx_reclaim` — the FX convergence loop, off the switch's synchronous path (`TrackFX_SetOffline` was 1897ms of a measured 1942ms switch). `drainTick` moves one track's FX per step against `core/view/fx_state`'s derived work list (Online before Offline; an Offline item waits out `kParkHoldMs` so a fast A→B→A flip moves nothing), budgeted per tick (`kOnlineBudgetMs`/`kOfflineBudgetMs`) and gated off the transport for the Offline direction only. `FxReclaimQueue` owns the derived queue's lifetime across re-entrancy (a plugin unload is assumed to pump); each `writeOffline` runs inside its own discarded undo block, DAW-verified to mint nothing. `ReleaseFlush`/`fxReclaimFlushRelease` make a completed record's spend durable on the project's next save, never on the drain's own initiative. +- `fx_reclaim` — the FX convergence loop, off the switch's synchronous path (`TrackFX_SetOffline` was 1897ms of a measured 1942ms switch). `drainTick` moves one track's FX per step against `core/view/fx_state`'s derived work list (Online before Offline; an Offline item waits out `kParkHoldMs` so a fast A→B→A flip moves nothing), budgeted per tick (`kOnlineBudgetMs`/`kOfflineBudgetMs`) and gated off the transport for the Offline direction only. `FxReclaimQueue` owns the derived queue's lifetime across re-entrancy (a plugin unload is assumed to pump). `UndoSpan` owns the ONE undo block the whole drain writes under — opened by `applyMode` on a real switch, else lazily by the first `writeOffline`, and closed through the single idempotent `fxReclaimEndUndoSpan` that every terminating path routes through (`spanEndsAfter` decides which ticks terminate). `ReleaseFlush`/`fxReclaimFlushRelease` make a completed record's spend durable on the project's next save, never on the drain's own initiative. ## Gotchas @@ -158,15 +176,24 @@ applies the resulting lane state to live tracks. reads identically and is EXPOSEd too — no lost state there at all, which is why the report never asserts a cause. A REDO of a mode switch is a further, hypothetical route — see the `view_state` gotcha below. -- **An UNDO of a mode switch no longer rolls back FX at all.** FX offline/online - moved off `applyMode`'s block entirely onto the reclaimer, so a Ctrl-Z restores - only the flags and lane state the block wrote — whatever the reclaimer had - converged (or had not yet converged) survives the undo untouched. Whether this - reads as correct in the DAW is `[verify — DAW]`, `docs/VERIFICATION.md` - §"Mode switching". -- **The `view_state` write is NOT inside `applyMode`'s block.** `persistViewState` - (`design_view_actions.cpp`) runs after `applyMode` has returned, so - `SetProjExtState` lands past `Undo_EndBlock2`. The UNDO direction still comes +- **An UNDO of a mode switch rolls back the FX the span covered, and only those.** + The span carries `UNDO_STATE_FX` only once the drain has actually written FX under + it (`UndoSpan::widen`), so a switch whose parking never matured — a fast A→B→A + flip — still closes on `kViewUndoMask` alone and costs no FX serialization. What a + Ctrl-Z cannot restore is anything the reclaimer converges AFTER the span closed: a + transport deferral, and a drain no switch opened a span for, each mint their own + point. Whether that reads as correct in the DAW is `[verify — DAW]`, + `docs/VERIFICATION.md` §"Mode switching". +- **The `view_state` write is NOT inside `applyMode`'s block — but on a REAL switch + it is now inside the SPAN.** `persistViewState` (`design_view_actions.cpp`) runs + after `applyMode` has returned, and a real switch returns with the span open, so + the `SetProjExtState` — and the modal Save-As `persistViewState` can pop on an + unsaved project — land inside the block after all. That is a consequence of the + span, not a decision made here; it does not resolve the redo hypothesis below, + which turns on capture-time semantics the SDK does not document. `[verify — DAW]` + that a Save-As dialog opening under an open undo block is harmless. The rest of + this gotcha describes the reapply path, where nothing changed. The UNDO direction + still comes out right — the point closed over the ext state as it stood BEFORE the switch, which is what a Ctrl-Z should restore — but by position, not by design. The REDO direction is what this leaves open, and the cost is worse than the diff --git a/src/shell/view/fx_reclaim.cpp b/src/shell/view/fx_reclaim.cpp index 36ef36e..f5c60ee 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -38,6 +38,28 @@ FxReclaimQueue g_queue; // A release applied to the in-memory model and not yet written — see ReleaseFlush. ReleaseFlush g_releaseFlush; +// The one open undo block a mode switch and its parking share — see UndoSpan. +UndoSpan g_span; + +// The ONE open and the ONE close. Both idempotent, so every caller may say either +// unconditionally; the close reads the span's values before clearing it, because the +// clear has to precede the SDK call — nothing re-entered through Undo_EndBlock2 may +// find a span it could close a second time. +void openSpan(ReaProject* proj, const char* label, int mask) { + if (g_span.begin(proj, label, mask)) Undo_BeginBlock2(proj); +} + +void closeSpan() { + if (!g_span.open()) return; + ReaProject* const proj = g_span.project(); + const std::string label = g_span.label(); + const int mask = g_span.mask(); + g_span.end(); + // The span outlives its opener, so by here the project may be the one that just + // closed — its undo history went with it and there is no block left to end. + if (ValidatePtr2(nullptr, proj, "ReaProject*")) Undo_EndBlock2(proj, label.c_str(), mask); +} + // TrackFX_SetOffline is ASSUMED to pump the message loop, so a plugin load can re-enter // the timer and land back here mid-drain. Fail closed: a re-entered drain would derive a // second queue for the same project and race the outer one's writes track for track. @@ -132,16 +154,15 @@ struct ReclaimHost { if (fxIndex < 0 || fxIndex >= TrackFX_GetCount(tr)) return false; // chain shrank if (TrackFX_GetOffline(tr, fxIndex) == offline) return false; // already there - // DAW-VERIFIED: the same ten TrackFX_SetOffline calls mint ten undo points - // unblocked and ZERO inside this pair. One block per WRITE even though a step is - // now one whole track: the write itself is what pumps, so the block cannot avoid - // spanning a pump — it can only span as little of one as possible. Widening it to - // the track would multiply that exposure by the chain length while suppressing no - // extra undo point (the discard form mints none either way), and a user action - // landing inside a discarded block can lose its own Ctrl-Z. - Undo_BeginBlock2(project); + // Every write in the drain rides ONE span. Do not reintroduce a block per write: + // it minted one undo point per FX toggle, because the ("", 0) discard form + // coalesces rather than discards outside an action (see UndoSpan). The open is a + // no-op under a switch's span, which is why the FX bit is added separately — + // here, at the first real write, so a switch whose parking never matured closes + // on the flags-and-items mask it opened with and serializes no FX state. + openSpan(project, kReclaimUndoLabel, UNDO_STATE_FX); + g_span.widen(UNDO_STATE_FX); TrackFX_SetOffline(tr, fxIndex, offline); - Undo_EndBlock2(project, "", 0); return true; } @@ -188,6 +209,8 @@ void invalidateFxReclaim() { bool fxReclaimDrainInProgress() { return g_draining; } void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { + // Neither guard may close the span: both name a caller that is still on the stack and + // still owns it — the outer drain, or the applyMode that opened it. Both re-tick. if (g_draining) return; // The model is mid-rewrite and its records are being consumed out from under any // plan derived from it. The apply invalidates on its way out, so nothing is lost. @@ -200,6 +223,12 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { // that gate that poll. ReaProject* project = proj ? proj : EnumProjects(-1, nullptr, 0); if (!ValidatePtr2(nullptr, project, "ReaProject*")) project = nullptr; + + // A span belongs to the project it was opened on. Keyed on the SPAN's own project + // rather than the queue's pin, which moves independently: a tab switch, a closed + // project, or no active project at all ends it where it stands rather than carrying + // it into another project's history — and the no-project arm below returns. + if (g_span.open() && g_span.project() != project) closeSpan(); if (!g_queue.beginTick(project)) return; // DERIVED, never persisted: recomputed from view state every time the queue was @@ -213,9 +242,20 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { ReclaimHost host{project, &session}; const ReclaimRunResult run = g_queue.run(host); reportDrops(project, run.drops); + if (spanEndsAfter(run)) closeSpan(); if (!run.aborted && run.settled) g_queue.goIdle(); } +void fxReclaimBeginUndoSpan(ReaProject* proj) { + openSpan(proj, "", 0); +} + +void fxReclaimLabelUndoSpan(const char* label, int mask) { + g_span.relabel(label ? label : "", mask); +} + +void fxReclaimEndUndoSpan() { closeSpan(); } + void fxReclaimFlushRelease(ReaSamplerSession& session) { // GetCurrentProjectInLoadSave() names the project THIS hook is being asked to save // (reaper_plugin_functions.h:1582-1587) -- not merely the active tab, which a @@ -224,6 +264,9 @@ void fxReclaimFlushRelease(ReaSamplerSession& session) { // the project actually being written, or match the arm against the active tab while a // different project saved and write into that wrong project's ext state. ReaProject* saving = GetCurrentProjectInLoadSave(); + // BEFORE the arm test, and unconditionally: a save is a user act the span must not + // absorb, and the arm is not what decides whether one is open. + closeSpan(); if (!g_releaseFlush.pendingFor(saving)) return; // Promptless, unlike design_view_actions' persistViewState, which can open a modal // Save-As. [verify — DAW] whether this write reaches the block of the diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index b56fa28..e49d618 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -7,6 +7,7 @@ #include #include #include +#include #include #include "core/view/fx_state.h" @@ -155,6 +156,12 @@ struct ReclaimTickResult { int writes = 0; bool settled = false; + // The transport gate — and only it — held an otherwise-eligible item back this + // tick. Distinguished from the debounce because the two waits differ in KIND: a + // hold expires at kParkHoldMs, a deferral lasts as long as the playback pass does, + // and the undo span may only stay open across a wait it can bound (spanEndsAfter). + bool transportDeferred = false; + // Tracks whose park record the tick drove fully online, so the record's FX half is // spent. REPORTED, not applied: releasing is a model write and the model is exactly // what a re-entered load can replace mid-drain, so only the owner may commit it — @@ -178,8 +185,9 @@ struct ReclaimTickResult { // may still online while it rolls // bool liveChain(guid, std::vector&) FX identity per live slot; // false when the track is gone -// bool writeOffline(guid, fxIndex, offline) compare-before-write inside its -// own discarded undo block; +// bool writeOffline(guid, fxIndex, offline) compare-before-write, opening +// the drain's undo span if no +// switch already did (UndoSpan); // true iff it actually wrote // void releaseFx(guid) release the record's FX half and // arm the save-time flush that @@ -213,8 +221,10 @@ ReclaimTickResult drainTick(std::vector& items, ParkClock& clock, H auto nextAllowed = [&](double nowMs) { const int i = nextEligible(items, clock, nowMs); if (i >= 0 && !offlineAllowed && - items[static_cast(i)].work.kind == view::FxWorkKind::Offline) + items[static_cast(i)].work.kind == view::FxWorkKind::Offline) { + result.transportDeferred = true; return -1; + } return i; }; @@ -274,6 +284,9 @@ struct ReclaimRunResult { int writes = 0; bool settled = false; + // Carried straight out of the tick — see ReclaimTickResult::transportDeferred. + bool transportDeferred = false; + // Something invalidated the queue while the drain was on the stack. The work and // the releases are DISCARDED, `settled` is never true, and the caller must not go // idle — the next tick re-derives against whatever changed. @@ -376,8 +389,18 @@ public: // back whether or not the drain's own work survives. clock_.swap(clock); - if (generation_ != entered) - return ReclaimRunResult{tick.writes, false, true, tick.drops}; + // Assembled field by field rather than positionally: the aggregate is read by + // the undo span's close decision now, and a silently mis-slotted bool there + // leaks an open block. + ReclaimRunResult run; + run.writes = tick.writes; + run.transportDeferred = tick.transportDeferred; + run.drops = tick.drops; + + if (generation_ != entered) { + run.aborted = true; + return run; + } items_.swap(work); // A release dropped by an abort is re-owed, not lost: the next derivation plans @@ -385,7 +408,8 @@ public: // release fires then. Applied in MEMORY only — the spend reaches disk on the // project's next save (ReleaseFlush), never on the drain's own initiative. for (const std::string& guid : tick.released) host.releaseFx(guid); - return ReclaimRunResult{tick.writes, tick.settled, false, tick.drops}; + run.settled = tick.settled; + return run; } private: @@ -433,6 +457,87 @@ private: ReaProject* project_ = nullptr; // COMPARE-ONLY, never dereferenced }; +// --- the undo span --------------------------------------------------------------- + +// The label a drain that no mode switch opened a span for closes under. Names the +// feature the user knows — Design View, and its CPU reclaim (the toggle's own tooltip +// wording) — rather than the loop behind it, and covers both directions: one act, +// converging the parked/returning chains to what the view state says. +inline constexpr const char* kReclaimUndoLabel = "ReaSampler: reclaim Design View FX"; + +// One open REAPER undo block, held across the timer callbacks the drain runs on. +// +// DAW-measured this phase, and the whole reason this type exists: block WIDTH is the +// entire effect. From a timer, ten TrackFX_SetOffline calls mint ten undo points +// unblocked, ten with a block PER WRITE, and ONE under a single block spanning all +// ten — the ("", 0) discard form does not discard outside an action, it coalesces. A +// block opened on one timer callback and closed on a LATER one holds. +// +// Pure: the Undo_BeginBlock2/EndBlock2 calls are fx_reclaim.cpp's, which owns the ONE +// instance and routes every path through one open and one close. Both moves report +// whether they actually moved, so a caller may say either unconditionally and still +// make no second BeginBlock2 and no unbalanced EndBlock2. +class UndoSpan { +public: + bool open() const { return open_; } + ReaProject* project() const { return project_; } + const std::string& label() const { return label_; } + int mask() const { return mask_; } + + // True iff this call opened it. `label`/`mask` are adopted only then, so a drain + // writing under a switch's span cannot displace the switch's own label. + bool begin(ReaProject* proj, std::string label, int mask) { + if (open_) return false; + project_ = proj; + label_ = std::move(label); + mask_ = mask; + open_ = true; + return true; + } + + // A mode switch decides its label only after its writes (applyMintsUndoPoint). + void relabel(std::string label, int mask) { + if (!open_) return; + label_ = std::move(label); + mask_ = mask; + } + + // Add state the span did not originally promise to serialize — the FX bit, once + // this drain has actually written FX under a switch's flags-and-items mask. + void widen(int extraMask) { + if (open_) mask_ |= extraMask; + } + + // True iff this call closed it; the caller owes Undo_EndBlock2 with the values it + // read BEFORE calling, since this clears them. + bool end() { + if (!open_) return false; + project_ = nullptr; + label_.clear(); + mask_ = 0; + open_ = false; + return true; + } + +private: + ReaProject* project_ = nullptr; // passed back to Undo_EndBlock2, never dereferenced here + std::string label_; + int mask_ = 0; + bool open_ = false; +}; + +// Must the tick that produced `run` end the span? +// +// The span may stay open only across a wait whose length it can bound. Waiting out the +// 3s debounce qualifies and is the wait the whole design turns on — that dead time sits +// between the switch and its first FX write. A TRANSPORT deferral does not: it lasts as +// long as the user plays, and a block held open for a playback pass swallows every edit +// made during it. Ending there costs a second undo point for the parking that follows, +// which is the documented degradation, not a failure. +inline bool spanEndsAfter(const ReclaimRunResult& run) { + return run.settled || run.aborted || run.transportDeferred; +} + // --- shell entry points -------------------------------------------------------- // The ONE test the timer makes per tick, mirroring the realtime capture's single @@ -460,6 +565,24 @@ bool fxReclaimDrainInProgress(); // park record's FX half — see FxReclaimQueue::run. void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj); +// Opens the undo span a mode switch and its background FX parking share, so the two are +// ONE Ctrl-Z. Called by applyMode in place of its own Undo_BeginBlock2; a REAL switch +// then returns with the span still open and the reclaimer closes it once the parking has +// drained. No-op if a span is already open, so callers END first (see applyMode) rather +// than nesting. +void fxReclaimBeginUndoSpan(ReaProject* proj); + +// Sets what the eventual close names the point. Separate from the begin because the +// switch's own label is only decided after its writes (applyMintsUndoPoint). +void fxReclaimLabelUndoSpan(const char* label, int mask); + +// The ONE close, idempotent: every terminating path routes through it — the drain +// settling, aborting, or deferring on the transport; the project retargeting or closing; +// the next apply superseding it; a project save; extension unload. A leaked open block +// would swallow everything the user does afterwards, so nothing may return past a +// terminating condition without calling this. +void fxReclaimEndUndoSpan(); + // Writes an in-memory release into the SAVING project's view state (identified via // GetCurrentProjectInLoadSave(), not the active tab). Called from the project-save // hook and nowhere else: this is the ONE path that makes a spent record durable, and diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index 685f48a..f33d5f6 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -2,7 +2,8 @@ // reaper_plugin_functions.h without REAPERAPI_IMPLEMENT (main.cpp owns that). // Tree arithmetic lives in view_tree (pure), one track's park/restore in view_park // and the lane writes in view_lanes; this file owns the plan iteration, parent -// visibility, and the one undo block they all ride. +// visibility, and opening the one undo span they all ride (fx_reclaim's UndoSpan, +// which a real switch hands to the reclaimer still open). #include "shell/view/view.h" @@ -23,8 +24,6 @@ #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_EnumProjects #define REAPERAPI_WANT_GetPlayStateEx -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #define REAPERAPI_WANT_TrackList_AdjustWindows #define REAPERAPI_WANT_UpdateArrange #define REAPERAPI_WANT_UpdateTimeline @@ -117,9 +116,13 @@ bool applyModeImpl(ViewModeModel& model, const std::string& targetModeId, ReaPro bool laneModeChanged = false; bool wrote = false; // anything at all this apply changed — see applyMintsUndoPoint - std::vector exposedNames; // named while handles are fresh, reported after the block + std::vector exposedNames; // named while handles are fresh, reported after the writes - Undo_BeginBlock2(project); + // A prior switch's span, if the reclaimer has not closed it yet: this apply + // supersedes it, so that parking lands as its own point rather than nesting under + // ours. Both calls are idempotent — the end reaches nothing when none is open. + fxReclaimEndUndoSpan(); + fxReclaimBeginUndoSpan(project); { UiRefreshHold uiHold; // every write below lands with the TCP/MCP frozen @@ -195,14 +198,20 @@ bool applyModeImpl(ViewModeModel& model, const std::string& targetModeId, ReaPro if (laneModeChanged) UpdateTimeline(); const bool mint = applyMintsUndoPoint(realSwitch, wrote); - Undo_EndBlock2(project, mint ? undoLabel.c_str() : "", mint ? kViewUndoMask : 0); + fxReclaimLabelUndoSpan(mint ? undoLabel.c_str() : "", mint ? kViewUndoMask : 0); - // After the undo block: an EXPOSE report is a console message, not a project - // write, and must not join what a Ctrl-Z rolls back. + // A REAL switch returns with the span STILL OPEN: the reclaimer closes it once the + // parking this apply is about to invalidate has drained, so the switch and its FX + // writes are ONE Ctrl-Z. Everything else closes here as it always did — a reapply is + // not a switch, and the load tick must not leave a block open across a project open. + if (!realSwitch) fxReclaimEndUndoSpan(); + + // Neither of the two below is a project write — a console message and an in-memory + // fingerprint — so whether the span is still open over them is immaterial. reportExposedTracks(project, exposedNames); // The tool's own account of what it just left driven — the only one that survives - // an undo rolling the model back underneath it. + // an undo rolling the model back underneath it. In memory, never on disk. noteAppliedState(project, model); return true; diff --git a/src/shell/view/view.h b/src/shell/view/view.h index d91e5c4..643f7ba 100644 --- a/src/shell/view/view.h +++ b/src/shell/view/view.h @@ -32,9 +32,6 @@ bool transportBlocksModeSwitch(ReaProject* proj); // // A real switch mints regardless, fail-safe: it is an explicitly fired action and has // to stay undoable even in the degenerate case where the plan found nothing to write. -// Note what applyMode CANNOT see when it decides — the `view_state` ext-state write -// lands in persistViewState, after this block has closed (src/shell/view/CLAUDE.md's -// Gotchas). inline bool applyMintsUndoPoint(bool realSwitch, bool wroteAnything) { return realSwitch || wroteAnything; } @@ -54,6 +51,10 @@ bool modeApplyInProgress(); // path: TrackFX_SetOffline was 1897ms of a measured 1942ms switch, against 0.02ms for // all the flag work below. Do not reintroduce a synchronous FX write. // +// A REAL switch therefore RETURNS WITH ITS UNDO BLOCK STILL OPEN (fx_reclaim's +// UndoSpan): the reclaimer closes it once the parking has drained, which is what keeps +// the switch and its FX writes ONE point rather than two. A reapply closes its own. +// // Returns false (no mutation) if `targetModeId` isn't registered, if this is a real // switch (target != active) while the transport is running, if it is re-entered // (modeApplyInProgress), or if the project does not validate. A reapply (target == diff --git a/tests/test_fx_reclaim.cpp b/tests/test_fx_reclaim.cpp index f28a070..77c3551 100644 --- a/tests/test_fx_reclaim.cpp +++ b/tests/test_fx_reclaim.cpp @@ -6,7 +6,9 @@ // the user's save rather than on the drain, and — via a host that invalidates from inside // a write — that a re-entered invalidation reaches nothing the drain is using. The queue // owns the project pin and the timer's wakeup for the same reason: a compare-only pointer -// makes the tab switch assertable. +// makes the tab switch assertable. The undo span is here on the same terms: its state +// machine is pure, so open-once/close-once and the wait it may or may not outlast are +// assertable; only the Undo_BeginBlock2/EndBlock2 calls themselves need a DAW. #include "../src/shell/view/fx_reclaim.h" @@ -59,6 +61,12 @@ struct FakeHost { bool rolling = false; // the transport gate's input + // Stands in for the shell's one span instance: fx_reclaim.cpp opens it on the first + // real write of a drain, exactly here, and widens it to carry the FX bit. Only the + // Undo_BeginBlock2/EndBlock2 calls themselves are out of reach without a DAW. + UndoSpan* span = nullptr; + int spanOpens = 0; + double nowMs() const { return clock; } bool transportRolling() const { return rolling; } @@ -76,6 +84,10 @@ struct FakeHost { if (fxIndex < 0 || fxIndex >= static_cast(it->second.size())) return false; if (it->second[static_cast(fxIndex)].offline == offline) return false; + if (span) { + if (span->begin(project, kReclaimUndoLabel, 2)) ++spanOpens; + span->widen(2); + } it->second[static_cast(fxIndex)].offline = offline; clock += stepCostMs; writeLog.push_back(guid + ":" + std::to_string(fxIndex) + (offline ? "=off" : "=on")); @@ -1011,6 +1023,180 @@ static void testAGatedTickStillGoesIdleWithNothingLeftToConverge() { CHECK(q.run(host).settled); } +// -- 10. the undo span ---------------------------------------------------------------- + +static void testTheSpanOpensOnceAndClosesOnceHoweverManyTimesItIsAsked() { + // The whole safety of the design is in these two answers: a caller may say begin or + // end unconditionally and still make no second Undo_BeginBlock2 and no unbalanced + // Undo_EndBlock2. A leaked open block swallows everything the user does afterwards. + UndoSpan span; + CHECK(!span.open()); + CHECK(!span.end()); // nothing open — the caller owes REAPER nothing + + CHECK(span.begin(kProjA, "ReaSampler: activate Design view", 9)); + CHECK(span.open()); + CHECK(span.project() == kProjA); + CHECK(span.label() == "ReaSampler: activate Design view"); + CHECK(span.mask() == 9); + + // The drain's own begin, arriving under a switch's span: it must adopt nothing — + // neither the other project, nor a label that would rename the user's switch. + CHECK(!span.begin(kProjB, kReclaimUndoLabel, 2)); + CHECK(span.project() == kProjA); + CHECK(span.label() == "ReaSampler: activate Design view"); + CHECK(span.mask() == 9); + + span.widen(2); // ...but the FX bit it DID write under still has to reach the point + CHECK(span.mask() == 11); + + span.relabel("ReaSampler: activate Arrange view", 1); + CHECK(span.label() == "ReaSampler: activate Arrange view"); + CHECK(span.mask() == 1); + + CHECK(span.end()); + CHECK(!span.open()); + CHECK(!span.end()); // the second close reaches nothing + CHECK(span.project() == nullptr); + + // And nothing a closed span carries can be revived out from under the next opener. + span.widen(4); + span.relabel("stale", 8); + CHECK(span.mask() == 0); + CHECK(span.label().empty()); +} + +static void testTheSpanOutlastsADebounceWaitButNeverATransportDeferral() { + // The one policy call: the span may stay open across a wait it can BOUND. The 3s + // debounce is the dead time between the switch and its first FX write, and spanning + // it is the whole point. A transport deferral lasts as long as the user plays. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + const ReclaimRunResult holding = q.run(host); // the hold starts at t=0 + CHECK(!holding.settled); + CHECK(!holding.transportDeferred); + CHECK(!spanEndsAfter(holding)); // bounded by kParkHoldMs — the span holds + + host.clock += kParkHoldMs; + host.rolling = true; + const ReclaimRunResult gated = q.run(host); + CHECK(gated.writes == 0); + CHECK(!gated.settled); + CHECK(gated.transportDeferred); + CHECK(spanEndsAfter(gated)); // unbounded — the switch's point closes without it + + host.rolling = false; + const ReclaimRunResult done = q.run(host); + CHECK(done.writes == 1); + CHECK(done.settled); + CHECK(!done.transportDeferred); + CHECK(spanEndsAfter(done)); +} + +static void testAnAbortEndsTheSpanSoNoLaterSwitchNestsUnderIt() { + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + q.run(host); + host.clock += kParkHoldMs; + + host.reentrant = &q; + host.invalidateAfterWrite = 1; + const ReclaimRunResult aborted = q.run(host); + CHECK(aborted.aborted); + CHECK(!aborted.settled); + CHECK(spanEndsAfter(aborted)); // the model moved; this span's work is superseded +} + +static void testOneSwitchAndItsMultiTickParkingAreOneSpan() { + // Three tracks at a cost that admits one per tick, so the parking really does span + // several timer callbacks — the case a per-write block turned into one undo point per + // FX toggle. The span must open exactly once and close exactly once, on the tick that + // settles, under the SWITCH's label rather than the reclaimer's. + ViewModeModel vm = parkedDesignLeaves({{"{T1}", {"{FX-A}"}}, + {"{T2}", {"{FX-B}"}}, + {"{T3}", {"{FX-C}"}}}); + FakeHost host; + host.chains["{T1}"] = onlineChain({"{FX-A}"}); + host.chains["{T2}"] = onlineChain({"{FX-B}"}); + host.chains["{T3}"] = onlineChain({"{FX-C}"}); + host.stepCostMs = 50.0; // dearer than kOfflineBudgetMs ⇒ one track per tick + + // What applyMode leaves behind on a real switch: an open span, its label set, and no + // close — the reclaimer's to finish. + UndoSpan span; + CHECK(span.begin(kProjA, "", 0)); + span.relabel("ReaSampler: activate Design view", 9); + host.span = &span; + + int closes = 0; + FxReclaimQueue q = queueOf(vm, {"{T1}", "{T2}", "{T3}"}); + auto tick = [&]() { + const ReclaimRunResult r = q.run(host); + if (spanEndsAfter(r) && span.end()) ++closes; + return r; + }; + + CHECK(!tick().settled); // hold starts + host.clock += kParkHoldMs; + CHECK(!tick().settled); // {T1} + CHECK(!tick().settled); // {T2} + CHECK(tick().settled); // {T3}, and the queue is exhausted in the same pass + + CHECK(host.writeLog.size() == 3); + CHECK(host.spanOpens == 0); // the switch's span was already open — no second begin + CHECK(closes == 1); // ...and exactly one close, on the settling tick + CHECK(!span.open()); + + // Extra ticks after the settle must not produce a second close. + tick(); + CHECK(closes == 1); +} + +static void testADrainNoSwitchOpenedMintsOnePointOfItsOwn() { + // The non-switch origins — a membership edit's reapply, the load tick, a drain + // resuming after the transport stopped. Nobody opened a span, so the drain opens its + // own: one point for the whole convergence, never one per FX toggle. + ViewModeModel vm = parkedDesignLeaves({{"{T1}", {"{FX-A}", "{FX-B}"}}, + {"{T2}", {"{FX-C}"}}}); + FakeHost host; + host.chains["{T1}"] = onlineChain({"{FX-A}", "{FX-B}"}); + host.chains["{T2}"] = onlineChain({"{FX-C}"}); + host.stepCostMs = 50.0; + + UndoSpan span; + host.span = &span; + + int closes = 0; + FxReclaimQueue q = queueOf(vm, {"{T1}", "{T2}"}); + auto tick = [&]() { + const ReclaimRunResult r = q.run(host); + if (spanEndsAfter(r) && span.end()) ++closes; + return r; + }; + + CHECK(!tick().settled); // hold starts; nothing written, so nothing opened yet + CHECK(!span.open()); + CHECK(host.spanOpens == 0); + + host.clock += kParkHoldMs; + CHECK(!tick().settled); // {T1}'s chain: the first write opens the span + CHECK(host.spanOpens == 1); + CHECK(span.open()); + CHECK(span.label() == kReclaimUndoLabel); + CHECK(span.project() == kProjA); + + CHECK(tick().settled); // {T2}, still under the SAME span + CHECK(host.spanOpens == 1); + CHECK(host.writeLog.size() == 3); + CHECK(closes == 1); + CHECK(!span.open()); +} + int main() { testFastFlipMovesZeroPluginsButAHeldParkStillReclaims(); @@ -1048,6 +1234,12 @@ int main() { testAHoldDropsWhenItsTrackStopsOwingOfflineWorkWhileRolling(); testAGatedTickStillGoesIdleWithNothingLeftToConverge(); + testTheSpanOpensOnceAndClosesOnceHoweverManyTimesItIsAsked(); + testTheSpanOutlastsADebounceWaitButNeverATransportDeferral(); + testAnAbortEndsTheSpanSoNoLaterSwitchNestsUnderIt(); + testOneSwitchAndItsMultiTickParkingAreOneSpan(); + testADrainNoSwitchOpenedMintsOnePointOfItsOwn(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; }