Collapse the FX-park drain's writes into one undo point instead of one per TrackFX_SetOffline

docs/VERIFICATION.md

@@ -48,6 +48,10 @@ 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")
 - [ ] 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]` **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]` 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]` 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]` 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]` 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

src/shell/view/CLAUDE.md

@@ -71,17 +71,24 @@ decide membership or mode rules.
   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
   lost. What it does change is the undo record, and REAPER's behaviour there is
-  now OBSERVED, not assumed: the drain's `TrackFX_SetOffline` calls (the only
+  now OBSERVED, not assumed. What was measured, before the fix below: a
-  `TrackFX_SetOffline` in the tree) run outside any undo block, and REAPER mints
+  `TrackFX_SetOffline` made outside any undo block mints ONE implicit undo point
-  ONE implicit undo point per call — a small project measured 10 points for one
+  per call, and a small project showed 10 points for one switch — 1 for the switch
-  switch, 1 for the switch itself and 9 for its FX. Ctrl-Z walks them one at a
+  itself and 9 for its FX — which Ctrl-Z then walked one FX at a time. **The drain
-  time, each step re-onlining a single FX, so one Ctrl-Z after a switch does not
+  now opens its own undo block** (`fxParkUndoClose`, `UNDO_STATE_FX` only, no other
-  bring the plugins back. It also puts the switch point BELOW every FX point: an
+  domain), so one drain is one point and one switch costs two: the switch's, then
-  undo deep enough to restore the driven flags and roll the `"reasampler"` ext
+  the drain's. A drain that wrote no FX state closes with REAPER's discard form
-  state back has already re-onlined every FX, so flags, FX and snapshot move
+  and mints nothing at all, so a plain project open costs no undo point
-  together. The per-FX fragmentation is itself a defect to be closed by collapsing
+  (`fxParkUndoClose` owns why). The ordering property the observation established is DELIBERATELY PRESERVED: the
-  the drain's writes into one undo point; nothing here depends on them staying
+  drain's block is its own and never the switch's, so the FX point still sits ABOVE
-  separate. **The idle tick is not the only drain point.** Any path that
+  the switch point, and an undo deep enough to restore the driven flags and roll
+  the `"reasampler"` ext state back has already re-onlined every FX — flags, FX and
+  snapshot still move together. Folding the FX writes into the switch's own block,
+  or making them undo-silent, would break that and is not an option here. So ONE
+  Ctrl-Z after a switch now re-onlines every FX at once, and a second rolls the
+  flags and the ext state back — two steps rather than the measured ten, and never
+  one, which is the standing price of the drain being a separate act.
+  **The idle tick is not the only drain point.** Any path that
   serializes the view model drains synchronously first (`persistViewState`,
   `render_in_place`), because a save landing between a restore's synchronous flag
   writes and its drain would record offline FX beside a model that no longer
@@ -134,7 +141,7 @@ applies the resulting lane state to live tracks.
 ## Modules
 
 - `view` — Design View shell: snapshots flag values before parking, drives hide + CPU-park on inactive-mode leaves (`B_SHOWINTCP`/`B_SHOWINMIXER`/`B_MAINSEND`/`I_FXEN`, with per-FX offline deferred to `view_fx_park`), restores from snapshot. 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`.**
-- `view_fx_park` — the park surface: the `TrackFX_GetFXGUID` identity read snapshot/park/restore share, the deferred intent queue that keeps `TrackFX_SetOffline` off the switch's synchronous path (at most one intent per track GUID, latest wins, an intent landing on its own pending inverse cancels it), the idle-tick drain `main.cpp`'s `OnTimer` calls, the per-track park decision (`decidePark` over "does the model hold a snapshot" × "does the chain already read parked", with `decideParkForTrack` owning the live reads BOTH halves of that second term needs, and `trackFlagParm` the ONE `Flag` → REAPER-parameter mapping snapshot's read, park's read and restore's write all address), and the refusal report (`reportRefusedParks`, which names the tracks and holds the last-reported set so a reapply does not reprint it). **The drain owns no model state.** A snapshot is dropped where the restore is PLANNED — the flags are back at their captured values from that moment, and a model that still described the track as parked would let a persist or a reapply inside the drain window replan a restore over whatever the user changed since. What the deferral costs instead is that the live FX chain stops being a trustworthy snapshot source while an intent is pending: a park that CANCELS a pending restore takes the pre-park FX states from that restore's ops (`preParkFxFromCancelledRestore`), because the chain still reads the parked values and the cancel means no drain will ever fix them.
+- `view_fx_park` — the park surface: the `TrackFX_GetFXGUID` identity read snapshot/park/restore share, the deferred intent queue that keeps `TrackFX_SetOffline` off the switch's synchronous path (at most one intent per track GUID, latest wins, an intent landing on its own pending inverse cancels it), the idle-tick drain `main.cpp`'s `OnTimer` calls and the ONE undo point it collapses its writes into (`fxParkUndoClose` — it opens that block itself, so no caller may drain while holding one), the per-track park decision (`decidePark` over "does the model hold a snapshot" × "does the chain already read parked", with `decideParkForTrack` owning the live reads BOTH halves of that second term needs, and `trackFlagParm` the ONE `Flag` → REAPER-parameter mapping snapshot's read, park's read and restore's write all address), and the refusal report (`reportRefusedParks`, which names the tracks and holds the last-reported set so a reapply does not reprint it). **The drain owns no model state.** A snapshot is dropped where the restore is PLANNED — the flags are back at their captured values from that moment, and a model that still described the track as parked would let a persist or a reapply inside the drain window replan a restore over whatever the user changed since. What the deferral costs instead is that the live FX chain stops being a trustworthy snapshot source while an intent is pending: a park that CANCELS a pending restore takes the pre-park FX states from that restore's ops (`preParkFxFromCancelledRestore`), because the chain still reads the parked values and the cancel means no drain will ever fix them.
 - `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`.
 
 ## Gotchas
@@ -163,7 +170,10 @@ applies the resulting lane state to live tracks.
   FX enqueue too. Full contract at `discardDeferredFxParks`.
 - **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
-  above, the flags and the FX come back at the same step. Whether the ext-state
+  above, the drain's point sits above the switch's, so every FX is back online by
+  the step that 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
   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
   also restore a `view_state` that is snapshot-free for it (matching a fresh

src/shell/view/view_fx_park.cpp

@@ -23,10 +23,15 @@
 #define REAPERAPI_WANT_TrackFX_GetFXGUID
 #define REAPERAPI_WANT_TrackFX_GetOffline
 #define REAPERAPI_WANT_TrackFX_SetOffline
+#define REAPERAPI_WANT_Undo_BeginBlock2
+#define REAPERAPI_WANT_Undo_EndBlock2
 #define REAPERAPI_WANT_ValidatePtr2
 #define REAPERAPI_WANT_guidToString
 #include "reaper_plugin_functions.h"
 
+static_assert(reasampler::kFxParkUndoMask == UNDO_STATE_FX,
+              "the SDK-free copy of the drain's undo mask has drifted from UNDO_STATE_FX");
+
 namespace reasampler {
 
 namespace {
@@ -58,30 +63,70 @@ void adoptOwner(ReaProject* proj) {
     g_owner = p;
 }
 
+// Returns whether it actually wrote — the drain's undo point is discarded unless
+// something did (fxParkUndoClose).
+//
 // [verify — DAW] TrackFX_SetOffline unloads and re-instantiates the plugin, so
 // writing a state that already holds is assumed to cost the same unload/reload
 // as a real change — the header documents no internal short-circuit either way.
 // The compare is cheap and correct regardless; only the size of what it saves is
 // unconfirmed.
-void setOfflineIfChanged(MediaTrack* tr, int fx, bool offline) {
+bool setOfflineIfChanged(MediaTrack* tr, int fx, bool offline) {
-    if (TrackFX_GetOffline(tr, fx) == offline) return;
+    if (TrackFX_GetOffline(tr, fx) == offline) return false;
     TrackFX_SetOffline(tr, fx, offline);
+    return true;
 }
 
-void applyPark(MediaTrack* tr) {
+bool applyPark(MediaTrack* tr) {
+    bool wrote = false;
     const int fxCount = TrackFX_GetCount(tr);
-    for (int fx = 0; fx < fxCount; ++fx) setOfflineIfChanged(tr, fx, true);
+    for (int fx = 0; fx < fxCount; ++fx)
+        if (setOfflineIfChanged(tr, fx, true)) wrote = true;
+    return wrote;
 }
 
+struct RestoreOutcome {
+    FxRestoreDrops drops;
+    bool           wrote = false;
+};
+
 // Restores per-FX offline from the plan verbatim — never a blanket "online".
 // Which live FX each captured state belongs to is resolveFxRestore's call, and
 // what it could not place comes back for the caller to report.
-FxRestoreDrops applyRestore(MediaTrack* tr, const std::vector<FxOfflineOp>& ops) {
+RestoreOutcome applyRestore(MediaTrack* tr, const std::vector<FxOfflineOp>& ops) {
     const FxRestoreResolution res = resolveFxRestore(ops, liveFxGuids(tr));
-    for (const FxOfflineWrite& w : res.writes) setOfflineIfChanged(tr, w.fxIndex, w.offline);
+    RestoreOutcome out;
-    return res.drops;
+    for (const FxOfflineWrite& w : res.writes)
+        if (setOfflineIfChanged(tr, w.fxIndex, w.offline)) out.wrote = true;
+    out.drops = res.drops;
+    return out;
 }
 
+// ONE drain is ONE undo point (see src/shell/view/CLAUDE.md for the measured
+// 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.
+//
+// Closing is unconditional on every exit, the abort path included: an unbalanced
+// block would swallow every later action into one point, which is far worse than
+// [verify — DAW] whatever Undo_EndBlock2 does with a ReaProject* that has closed
+// since Undo_BeginBlock2 took it.
+class FxParkUndoBlock {
+public:
+    explicit FxParkUndoBlock(ReaProject* proj) : proj_(proj) { Undo_BeginBlock2(proj_); }
+    ~FxParkUndoBlock() {
+        const FxParkUndoClose close = fxParkUndoClose(wrote_);
+        Undo_EndBlock2(proj_, close.label, close.mask);
+    }
+    FxParkUndoBlock(const FxParkUndoBlock&) = delete;
+    FxParkUndoBlock& operator=(const FxParkUndoBlock&) = delete;
+
+    void noteWrite() { wrote_ = true; }
+
+private:
+    ReaProject* proj_;
+    bool        wrote_ = false;
+};
+
 // The FX's own durable identity, braced exactly like the track GUID keys. Empty
 // when REAPER reports none — an FX we cannot name is one we cannot restore, and
 // fx_offline treats it that way rather than guessing at its slot. Lifetime is
@@ -221,27 +266,34 @@ void drainDeferredFxParks() {
 
     FxRestoreDrops drops;
     int dropTracks = 0;
-    for (const FxParkIntent& intent : draining) {
+    {
-        auto it = byGuid.find(intent.guid);
+        // Scoped so the block closes before the console report below: a report is
-        if (it == byGuid.end()) continue;  // track deleted since the switch — prune
+        // not a project write and must not join what a Ctrl-Z rolls back, the same
+        // ordering applyMode gives reportRefusedParks.
+        FxParkUndoBlock undo(proj);
+        for (const FxParkIntent& intent : draining) {
+            auto it = byGuid.find(intent.guid);
+            if (it == byGuid.end()) continue;  // track deleted since the switch — prune
 
-        // Re-validated PER INTENT, not once above: under the same pumping
+            // Re-validated PER INTENT, not once above: under the same pumping
-        // assumption, a project closed or a track deleted between two applies
+            // assumption, a project closed or a track deleted between two applies
-        // leaves the handle resolved above dangling — a use-after-free, not a
+            // leaves the handle resolved above dangling — a use-after-free, not a
-        // pruned intent. Same gate capture_realtime_shell's teardown uses; a null
+            // pruned intent. Same gate capture_realtime_shell's teardown uses; a null
-        // first argument validates the ReaProject* itself (SDK: proj is ignored
+            // first argument validates the ReaProject* itself (SDK: proj is ignored
-        // when the pointer is a project).
+            // when the pointer is a project).
-        if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return;
+            if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return;
-        if (!ValidatePtr2(proj, it->second, "MediaTrack*")) continue;
+            if (!ValidatePtr2(proj, it->second, "MediaTrack*")) continue;
 
-        if (intent.park) {
+            if (intent.park) {
-            applyPark(it->second);
+                if (applyPark(it->second)) undo.noteWrite();
-            continue;
+                continue;
-        }
+            }
-        const FxRestoreDrops d = applyRestore(it->second, intent.restoreOps);
+            const RestoreOutcome r = applyRestore(it->second, intent.restoreOps);
-        if (d.total() > 0) {
+            if (r.wrote) undo.noteWrite();
-            drops.add(d);
+            if (r.drops.total() > 0) {
-            ++dropTracks;
+                drops.add(r.drops);
+                ++dropTracks;
+            }
         }
     }
 

src/shell/view/view_fx_park.h

@@ -263,6 +263,29 @@ PreParkFx snapshotFxOffline(MediaTrack* tr, const std::vector<FxOfflineOp>& canc
 std::vector<FxOfflineOp> deferFxPark(ReaProject* proj, const std::string& guid);
 void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector<FxOfflineOp> ops);
 
+// -- the drain's own undo point -----------------------------------------------
+//
+// UNDO_STATE_FX and nothing else: per-FX offline is all the drain writes. The
+// value is repeated here rather than included so this header stays SDK-free;
+// view_fx_park.cpp static_asserts it against the macro (reaper_plugin.h:1542).
+inline constexpr int kFxParkUndoMask = 2;
+
+// Undo_EndBlock2's two arguments for ONE drain. An empty label with a zero mask
+// is REAPER's discard form — view.cpp's lane-mint path uses the same idiom — and
+// discarding matters here rather than merely tidying: a project-load reapply
+// plans a park for every inactive leaf, and a project saved parked already holds
+// every one of those FX offline, so a block opened unconditionally would mint an
+// empty undo point on each open.
+struct FxParkUndoClose {
+    const char* label = "";
+    int         mask  = 0;
+};
+
+inline FxParkUndoClose fxParkUndoClose(bool wroteAnyFx) {
+    if (!wroteAnyFx) return {};
+    return FxParkUndoClose{"ReaSampler: Design View FX park", kFxParkUndoMask};
+}
+
 // Applies every pending intent. Touches NO model state — a restore's snapshot is
 // dropped where the restore is planned (applyMode). Discards the queue unapplied
 // if the project it was enqueued against is no longer current (close / switch);
@@ -278,6 +301,12 @@ void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector<FxOff
 // model, and nothing on reopen brings them back online. The cost is that an
 // action-driven switch pays the FX hitch before it returns; its repaint and undo
 // block have both closed by then, which is what the deferral was for.
+//
+// OPENS ITS OWN UNDO BLOCK, so a caller must not already hold one: the drain's
+// point has to sit ABOVE the switch's, and nesting is undocumented in the SDK
+// header either way. All three call sites drain outside any block they open
+// (OnTimer holds none; persistViewState is reached only after applyMode's and
+// doMoveItems' blocks have closed; render_in_place drains after its EndBlock2).
 void drainDeferredFxParks();
 
 // Drops every pending intent without applying it. Called when the model the

tests/test_view_fx_park.cpp

@@ -4,8 +4,9 @@
 // The properties under test: a mode switch leaves its per-FX offline work here,
 // so a second switch arriving before the first drained must leave every track in
 // the state the SECOND switch specifies — never the first's, never both replayed;
-// a cancel must not strand the pre-park FX state it was the last record of; and
+// a cancel must not strand the pre-park FX state it was the last record of; a
-// a pre-park snapshot is never taken from a chain a park has already touched.
+// pre-park snapshot is never taken from a chain a park has already touched; and
+// one drain closes as at most ONE undo point, in the FX domain only.
 
 #include "../src/shell/view/view_fx_park.h"
 
@@ -406,6 +407,30 @@ static void testAReEntrantParkCancelsOnlyWhatIsStillPending() {
     CHECK(intentFor(q, "{A}") && intentFor(q, "{A}")->park);
 }
 
+// -- the drain's undo point --------------------------------------------------
+
+static void testADrainThatWroteFxClosesItsBlockAsOneNamedFxPoint() {
+    const FxParkUndoClose close = fxParkUndoClose(true);
+
+    // 2 is UNDO_STATE_FX (reaper_plugin.h:1542), pinned as a literal here and
+    // static_asserted against the macro in view_fx_park.cpp. The drain writes
+    // per-FX offline and nothing else, so any wider mask would make it marshal
+    // track config or items it never touched.
+    CHECK(close.mask == 2);
+    CHECK(close.label != nullptr && std::string(close.label) ==
+                                        "ReaSampler: Design View FX park");
+}
+
+static void testADrainThatWroteNothingClosesItsBlockAsADiscard() {
+    // The project-load reapply: a park is planned for every inactive leaf, and a
+    // project saved parked already holds every one of those FX offline. Nothing is
+    // written, so the block must leave no undo point behind at all.
+    const FxParkUndoClose close = fxParkUndoClose(false);
+
+    CHECK(close.mask == 0);
+    CHECK(close.label != nullptr && std::string(close.label).empty());
+}
+
 int main() {
     testParkEnqueuesOneIntentCarryingNoOps();
     testParkReportsNothingCancelledWhenNoIntentWasPending();
@@ -442,6 +467,9 @@ int main() {
     testIntentsArrivingDuringADrainSurviveIt();
     testAReEntrantParkCancelsOnlyWhatIsStillPending();
 
+    testADrainThatWroteFxClosesItsBlockAsOneNamedFxPoint();
+    testADrainThatWroteNothingClosesItsBlockAsADiscard();
+
     if (g_fail == 0) std::printf("All tests passed.\n");
     return g_fail ? 1 : 0;
 }