Extend the park trust test to the FX chain, name the refused tracks, and stop reprinting an unchanged refusal

src/shell/view/CLAUDE.md

@@ -30,15 +30,25 @@ decide membership or mode rules.
   (hide both panels), `B_MAINSEND=0` (out of mix), `I_FXEN=0` (FX bypassed), and
   `TrackFX_SetOffline(track, fx, true)` for each FX (reclaim CPU) — full CPU-park,
   not mix-removal-only.
-- **A snapshot is only ever taken from a chain no park has touched.** An absent
-  snapshot is NOT evidence of a clean chain — `discardDeferredFxParks` drops
-  intents whose flag writes already landed, so a reloaded model and an
-  already-parked project routinely coexist. A park meeting that pair REFUSES:
-  no flag write, no FX-offline enqueue, no snapshot, and one console line naming
-  the hand recovery. Leaving the track as found is the only non-destructive
-  answer once the pre-park truth is gone — snapshotting there would record park
-  state as the user's state and every later restore would faithfully write it
-  back. Decision at `view_fx_park`'s `decidePark`.
+- **A snapshot is only ever taken from a chain no park has touched — BOTH halves
+  of it.** The trust test reads the four driven flags AND the FX chain, and either
+  reading parked alone is enough to refuse: each half is the only source of truth
+  for its own half of the snapshot, so one of them lying poisons the whole record.
+  The FX half asks "is ANY FX offline", not "every" — no read distinguishes a
+  park's leftover from one the user set by hand, and only "any" errs toward
+  committing nothing false. An absent snapshot is NOT evidence of a clean chain —
+  `discardDeferredFxParks` drops intents whose flag writes already landed, so a
+  reloaded model and an already-parked project routinely coexist. A park meeting
+  that pair REFUSES: no flag write, no FX-offline enqueue, no snapshot, and a
+  console report NAMING the refused tracks (they are hidden from both the TCP and
+  the mixer, so a count points at nothing the user can go and find) plus the hand
+  recovery, printed once per changed refused set per project rather than on every
+  reapply. Leaving the track as found is the only non-destructive answer once the
+  pre-park truth is gone. **The refusal's cost is deliberate and it is not
+  symmetric with "hidden forever":** because it skips the FX enqueue too, a track
+  whose flags read parked but whose FX are still online keeps them online for
+  good. Refusing guarantees nothing false is written; it does not promise to
+  finish the park. Decision at `view_fx_park`'s `decidePark`.
 - **Non-destructive restore.** For every flag the tool drives, snapshot the prior
   value BEFORE parking; on toggle-back restore FROM the snapshot, never to a
   hardcoded "on." Round-trip (snapshot → park → restore) returns every driven flag
@@ -60,10 +70,18 @@ decide membership or mode rules.
   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 —
   they still unload and re-instantiate, and un-persisted internal state is still
-  lost. What it does change is the undo record: the offline writes land outside
-  the switch's undo block, so the tool no longer re-drives them on an undo or a
-  redo — what a Ctrl-Z then leaves the chain at is REAPER's own FX-state record,
-  `[verify — DAW]`. **The idle tick is not the only drain point.** Any path that
+  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
+  `TrackFX_SetOffline` in the tree) run outside any undo block, and REAPER mints
+  ONE implicit undo point per call — a small project measured 10 points for one
+  switch, 1 for the switch itself and 9 for its FX. Ctrl-Z walks them one at a
+  time, each step re-onlining a single FX, so one Ctrl-Z after a switch does not
+  bring the plugins back. It also puts the switch point BELOW every FX point: an
+  undo deep enough to restore the driven flags and roll the `"reasampler"` ext
+  state back has already re-onlined every FX, so flags, FX and snapshot move
+  together. The per-FX fragmentation is itself a defect to be closed by collapsing
+  the drain's writes into one undo point; nothing here depends on them staying
+  separate. **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
@@ -116,7 +134,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, and the pure per-track park decision (`decidePark` over "does the model hold a snapshot" × "does the chain already read parked", plus `trackFlagParm` — the ONE `Flag` → REAPER-parameter mapping park's live read and restore's write both address). **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, 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
@@ -141,7 +159,19 @@ applies the resulting lane state to live tracks.
   track whose reloaded model carries no snapshot — nothing plans its restore, so
   FX left offline stay offline, and its next park REFUSES rather than
   re-snapshotting the parked chain (snapshot-source invariant above). Stuck, but
-  never falsely committed. Full contract at `discardDeferredFxParks`.
+  never falsely committed — and never finished either, since the refusal skips the
+  FX enqueue too. Full contract at `discardDeferredFxParks`.
+- **Which routes actually reach that no-snapshot-plus-parked-chain pair.** An undo
+  of a mode switch is NOT one of them: per the observed undo semantics above, the
+  flags, the FX and the ext-state snapshots all come back at the same step, and
+  stopping short of it leaves the snapshot in place (so the next park is a
+  `ParkOnly`, not a refusal). What remains: a model replaced without the project
+  rolling back with it (an unreadable or absent `view_state` against an `.rpp`
+  saved parked), a snapshot `reconcile` pruned while its track was out of the live
+  enumeration and then undo-restored with the track, and a hand or script edit
+  that leaves a chain park-shaped. A track the USER keeps hidden from both panels,
+  out of the mix and FX-bypassed reads identically to all of them and is refused
+  too — no lost state there at all, which is why the report asserts no cause.
 - `fx_offline`'s identity keying (`TrackFX_GetFXGUID`) assumes the GUID stays
   attached to its plugin across a chain mutation while parked. That is
   `[verify — DAW]` (see `fxGuidString` in `view_fx_park.cpp`) and SWS issue #802 is a

src/shell/view/view.cpp

@@ -31,7 +31,6 @@
 #define REAPERAPI_WANT_SetMediaTrackInfo_Value
 #define REAPERAPI_WANT_GetSetMediaTrackInfo_String
 #define REAPERAPI_WANT_PreventUIRefresh
-#define REAPERAPI_WANT_ShowConsoleMsg
 #define REAPERAPI_WANT_Undo_BeginBlock2
 #define REAPERAPI_WANT_Undo_EndBlock2
 #define REAPERAPI_WANT_TrackList_AdjustWindows
@@ -152,11 +151,14 @@ MediaTrack* resolve(const TrackByGuid& byGuid, const std::string& guid) {
 // The FX half is snapshotFxOffline's — only it knows when the live chain has
 // stopped being a trustworthy source.
 TrackSnapshot snapshotTrack(MediaTrack* tr, const std::vector<FxOfflineOp>& cancelledRestore) {
+    auto read = [tr](Flag f) {
+        return static_cast<int>(GetMediaTrackInfo_Value(tr, trackFlagParm(f)));
+    };
     TrackSnapshot snap;
-    snap.showInTcp   = static_cast<int>(GetMediaTrackInfo_Value(tr, "B_SHOWINTCP"));
-    snap.showInMixer = static_cast<int>(GetMediaTrackInfo_Value(tr, "B_SHOWINMIXER"));
-    snap.mainSend    = static_cast<int>(GetMediaTrackInfo_Value(tr, "B_MAINSEND"));
-    snap.fxEnable    = static_cast<int>(GetMediaTrackInfo_Value(tr, "I_FXEN"));
+    snap.showInTcp   = read(Flag::ShowInTcp);
+    snap.showInMixer = read(Flag::ShowInMixer);
+    snap.mainSend    = read(Flag::MainSend);
+    snap.fxEnable    = read(Flag::FxEnable);
 
     PreParkFx fx = snapshotFxOffline(tr, cancelledRestore);
     snap.fxOffline = std::move(fx.states);
@@ -440,7 +442,7 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject
 
     int undoMask = kApplyUndoMask;
     bool laneModeChanged = false;
-    int refusedParks = 0;
+    std::vector<MediaTrack*> refusedParks;
 
     Undo_BeginBlock2(proj);
     {
@@ -462,16 +464,10 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject
             MediaTrack* tr = resolve(trackByGuid, guid);
             if (!tr) continue;  // stale GUID — prune
 
-            // Full contract at decidePark. Only an absent snapshot can refuse, so
-            // the short-circuit cannot change the answer (ParkOnly ignores the
-            // second term); a first park still pays the four reads, a reapply's
-            // re-park of a still-parked track does not.
-            const bool haveSnapshot = model.snapshot(guid) != nullptr;
-            const ParkAction action = decidePark(
-                haveSnapshot,
-                !haveSnapshot &&
-                    parkFlagsAlreadyApplied(tp.flags, readParkFlagValues(tr, tp.flags)));
-            if (action == ParkAction::Refuse) { ++refusedParks; continue; }
+            // Full contract at decidePark; view_fx_park owns the live reads.
+            const ParkAction action =
+                decideParkForTrack(tr, model.snapshot(guid) != nullptr, tp.flags);
+            if (action == ParkAction::Refuse) { refusedParks.push_back(tr); continue; }
 
             // Enqueued FIRST: a park landing on this track's own pending restore
             // cancels it, and those ops are then the only surviving record of the
@@ -542,11 +538,8 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject
     Undo_EndBlock2(proj, undoLabel.c_str(), undoMask);
 
     // After the undo block and the UI hold: a refusal is a report, not a project
-    // write, and it must not join what a Ctrl-Z rolls back. "!SHOW:" so an
-    // unattended project-load reapply cannot force the console open
-    // (reaper_plugin_functions.h:6536), same as the drain's drop report.
-    const std::string refusedMsg = describeRefusedParks(refusedParks);
-    if (!refusedMsg.empty()) ShowConsoleMsg(("!SHOW:" + refusedMsg).c_str());
+    // write, and it must not join what a Ctrl-Z rolls back.
+    reportRefusedParks(proj, refusedParks);
     return true;
 }
 

src/shell/view/view_fx_park.cpp

@@ -5,6 +5,7 @@
 
 #include "shell/view/view_fx_park.h"
 
+#include <cstring>  // strnlen — bounded read of GetTrackName's buffer
 #include <string>
 #include <unordered_map>
 #include <vector>
@@ -16,6 +17,7 @@
 #define REAPERAPI_WANT_EnumProjects
 #define REAPERAPI_WANT_GetMediaTrackInfo_Value
 #define REAPERAPI_WANT_GetTrack
+#define REAPERAPI_WANT_GetTrackName
 #define REAPERAPI_WANT_ShowConsoleMsg
 #define REAPERAPI_WANT_TrackFX_GetCount
 #define REAPERAPI_WANT_TrackFX_GetFXGUID
@@ -33,6 +35,10 @@ FxParkQueue g_queue;
 ReaProject* g_owner = nullptr;  // the project the pending intents were enqueued against
 bool        g_draining = false;
 
+// The last refusal report, so an unchanged one is not reprinted on every reapply.
+ReaProject*              g_reportedOwner = nullptr;
+std::vector<std::string> g_reportedRefusals;
+
 // Makes "one drain at a time" explicit rather than implied by the call sites.
 // RAII because an apply can throw and a stuck flag would silence the queue for
 // the rest of the session.
@@ -98,14 +104,56 @@ std::string fxGuidString(MediaTrack* tr, int fx) {
     return std::string(buf);
 }
 
+// ANY offline FX, not every one: a park offlines the whole chain, but a partial
+// undo or a half-finished hand recovery leaves some back online, and there is no
+// read that tells a park's leftover offline from one the user set. So the test
+// refuses in the direction that commits nothing false — the cost is a chain
+// carrying a hand-offlined plugin that the tool then declines to park.
+bool anyFxOffline(MediaTrack* tr) {
+    const int fxCount = TrackFX_GetCount(tr);
+    for (int fx = 0; fx < fxCount; ++fx)
+        if (TrackFX_GetOffline(tr, fx)) return true;
+    return false;
+}
+
+// "Track N" for an unnamed track, matching REAPER's own convention (GetTrackName,
+// not P_NAME, for the same reason capture::trackName picks it). The buffer is
+// read bounded — the SDK documents no NUL within bufOut_sz. A track the report
+// cannot name still has to appear in it, so the read failing yields a placeholder
+// rather than dropping the entry.
+std::string trackDisplayName(MediaTrack* tr) {
+    std::vector<char> buf(1024, '\0');
+    if (!GetTrackName || !GetTrackName(tr, buf.data(), static_cast<int>(buf.size())))
+        return "(name unavailable)";
+    return std::string(buf.data(), strnlen(buf.data(), buf.size()));
+}
+
 }  // namespace
 
-std::vector<int> readParkFlagValues(MediaTrack* tr, const std::vector<TrackFlagOp>& parkFlags) {
-    std::vector<int> live;
-    live.reserve(parkFlags.size());
-    for (const TrackFlagOp& op : parkFlags)
-        live.push_back(static_cast<int>(GetMediaTrackInfo_Value(tr, trackFlagParm(op.flag))));
-    return live;
+ParkAction decideParkForTrack(MediaTrack* tr, bool haveSnapshot,
+                              const std::vector<TrackFlagOp>& parkFlags) {
+    if (haveSnapshot) return decidePark(true, false);  // second term unread — skip the reads
+    const bool flagsRead = parkFlagsAlreadyApplied(parkFlags, [tr](Flag f) {
+        return static_cast<int>(GetMediaTrackInfo_Value(tr, trackFlagParm(f)));
+    });
+    return decidePark(false, chainReadsParked(flagsRead, anyFxOffline(tr)));
+}
+
+void reportRefusedParks(ReaProject* proj, const std::vector<MediaTrack*>& refused) {
+    ReaProject* const owner = proj ? proj : currentProject();
+
+    std::vector<std::string> names;
+    names.reserve(refused.size());
+    for (MediaTrack* tr : refused) names.push_back(trackDisplayName(tr));
+
+    if (owner == g_reportedOwner && names == g_reportedRefusals) return;
+    g_reportedOwner    = owner;
+    g_reportedRefusals = names;
+
+    const std::string msg = describeRefusedParks(names);
+    // "!SHOW:" so an unattended project-load reapply cannot force the console open
+    // (reaper_plugin_functions.h:6536), same as the drain's drop report.
+    if (!msg.empty()) ShowConsoleMsg(("!SHOW:" + msg).c_str());
 }
 
 std::vector<std::string> liveFxGuids(MediaTrack* tr) {

src/shell/view/view_fx_park.h

@@ -115,6 +115,11 @@ private:
 // and silent. A model carrying no snapshot is NOT evidence the chain is clean:
 // discardDeferredFxParks drops intents whose flag writes already landed, so a
 // reloaded model and an already-parked project routinely coexist.
+//
+// A snapshot has TWO halves and the test covers both, disjunctively: the flags
+// can read clean while the FX are still offline (a hand recovery that stopped at
+// the flags, a reload) and vice versa. Either half reading parked poisons the
+// whole snapshot, so either half alone refuses.
 
 // The REAPER track parameter each driven flag addresses. One home for the
 // mapping — park's live read, restore's write and the trust test below must
@@ -129,19 +134,28 @@ inline const char* trackFlagParm(Flag f) {
     return "B_SHOWINTCP";  // unreachable; keeps the compiler quiet
 }
 
-// The live values of exactly the flags `parkFlags` would write, in that order.
-std::vector<int> readParkFlagValues(MediaTrack* tr, const std::vector<TrackFlagOp>& parkFlags);
-
 // Does the track already sit at every value this park would write? Then a park
-// reached it. An empty plan is evidence of nothing and answers false.
-inline bool parkFlagsAlreadyApplied(const std::vector<TrackFlagOp>& parkFlags,
-                                    const std::vector<int>& liveValues) {
-    if (parkFlags.empty() || parkFlags.size() != liveValues.size()) return false;
-    for (std::size_t i = 0; i < parkFlags.size(); ++i)
-        if (liveValues[i] != parkFlags[i].value) return false;
+// reached it. `readFlag` answers one Flag's live value; templating on it lets the
+// live path compare as it reads (no per-track vector) while the tests drive the
+// same fold from a table, keyed by Flag rather than by plan order. An empty plan
+// is evidence of nothing and answers false.
+template <class ReadFlag>
+bool parkFlagsAlreadyApplied(const std::vector<TrackFlagOp>& parkFlags, ReadFlag readFlag) {
+    if (parkFlags.empty()) return false;
+    for (const TrackFlagOp& op : parkFlags)
+        if (readFlag(op.flag) != op.value) return false;
     return true;
 }
 
+// The two halves folded. DISJUNCTIVE, and that is the whole point: each half is
+// the sole source of truth for its own half of the snapshot, so one of them
+// reading parked poisons the snapshot whatever the other says. An AND would let a
+// flags-clean/FX-offline chain — what the documented hand recovery produces after
+// its first step — bake the parked FX states in as the user's.
+inline bool chainReadsParked(bool parkFlagsRead, bool anyFxOffline) {
+    return parkFlagsRead || anyFxOffline;
+}
+
 enum class ParkAction {
     SnapshotThenPark,  // clean chain — capture the pre-park truth, then park
     ParkOnly,          // still parked; the held snapshot IS the pre-park truth
@@ -158,18 +172,38 @@ inline ParkAction decidePark(bool haveSnapshot, bool chainReadsParked) {
     return chainReadsParked ? ParkAction::Refuse : ParkAction::SnapshotThenPark;
 }
 
-// One console line for the tracks an apply refused, empty when none. Names the
-// hand recovery: nothing else will bring a refused track back.
-inline std::string describeRefusedParks(int trackCount) {
-    if (trackCount <= 0) return {};
-    return "ReaSampler: " + std::to_string(trackCount) + " track" +
-           (trackCount == 1 ? "" : "s") +
-           " left unparked — the pre-park state was lost (an undo/redo or a project "
-           "reload dropped it), and parking now would record the parked state as yours. "
-           "Restore each track's TCP/mixer visibility, main send and FX enable by hand; "
-           "the next mode switch will then park it correctly.\n";
+// `decidePark` over the live track: the four driven flags AND the FX chain, which
+// a snapshot taken here would record both halves of. Skips the reads on the
+// ParkOnly path, where decidePark ignores the second term anyway.
+ParkAction decideParkForTrack(MediaTrack* tr, bool haveSnapshot,
+                              const std::vector<TrackFlagOp>& parkFlags);
+
+// The console report for the tracks one apply refused, empty when none. NAMES
+// them: a refused track is hidden from both the TCP and the mixer, so a count
+// alone points at nothing the user can go and find. It asserts no cause — the
+// pair it detects has several routes (src/shell/view/CLAUDE.md), and only some of
+// them are anyone's mistake.
+inline std::string describeRefusedParks(const std::vector<std::string>& trackNames) {
+    if (trackNames.empty()) return {};
+    std::string msg = "ReaSampler: " + std::to_string(trackNames.size()) + " track" +
+                      (trackNames.size() == 1 ? "" : "s") +
+                      " left unparked — no stored pre-park state, and a live state that "
+                      "already reads as parked; parking now would record that as your own "
+                      "state.\n";
+    for (const std::string& name : trackNames) msg += "  " + name + "\n";
+    msg += "To bring a track back under mode control: restore its TCP and mixer visibility, "
+           "its main send and its FX enable, AND set every FX in its chain online — FX enable "
+           "is the chain bypass and does not bring an individually offlined FX back. The next "
+           "mode switch will then park it correctly.\n";
+    return msg;
 }
 
+// Prints describeRefusedParks for `refused` (track handles, named here), ONCE per
+// changed set per project: the same refusal recurs on every reapply — every
+// project open, every tag/untag — and repeating it verbatim is noise the user
+// cannot act on any better the second time.
+void reportRefusedParks(ReaProject* proj, const std::vector<MediaTrack*>& refused);
+
 // The FX half of a snapshot, with the keying it must be read back under.
 struct PreParkFx {
     std::vector<FxOfflineState> states;