Clear the park snapshot where the restore is planned, not where it drains; make the drain re-entrant and reload-aware

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_TrackFX_GetOffline
 #define REAPERAPI_WANT_Undo_BeginBlock2
 #define REAPERAPI_WANT_Undo_EndBlock2
 #define REAPERAPI_WANT_TrackList_AdjustWindows
@@ -69,6 +68,10 @@ constexpr int kTransportMoving = 1 | 4;
 // state — a cost that scales with the project's plugin count rather than with
 // what the switch changed. FX is OR'd back in per apply, only when an FX-domain
 // flag really moved.
+// [verify — DAW] INFERRED, not documented: UNDO_STATE_TRACKCFG reads
+// "track/master vol/pan/routing" (reaper_plugin.h:1541) and names neither
+// B_SHOWINTCP/B_SHOWINMIXER nor the fixed-lane properties (assumed to ride
+// UNDO_STATE_ITEMS, :1543). Wrong ⇒ a Ctrl-Z restores less than the switch did.
 constexpr int kApplyUndoMask = UNDO_STATE_TRACKCFG | UNDO_STATE_ITEMS | UNDO_STATE_MISCCFG;
 
 // Holds REAPER's UI refresh off for the write phase; the deliberate rebuild
@@ -117,7 +120,7 @@ const char* flagParm(Flag f) {
 
 // The master track is not enumerated by GetTrack (index space excludes it),
 // so it can never enter the tree — the master-untouched invariant holds by
-// construction. Also caches each MediaTrack* by GUID for later resolve().
+// construction.
 std::vector<TrackFolderEntry> readFolderEntries(
         ReaProject* proj,
         TrackHandles& handleByGuid) {
@@ -156,26 +159,25 @@ MediaTrack* resolve(const TrackByGuid& byGuid, const std::string& guid) {
 
 // Captures prior driven-flag state before parking. Never reads B_MUTE/I_SOLO;
 // ints preserve whatever REAPER reported (TrackSnapshot's defensive contract).
-TrackSnapshot snapshotTrack(MediaTrack* tr) {
+// 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) {
     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"));
 
-    const std::vector<std::string> guids = liveFxGuids(tr);
-    snap.fxOffline.reserve(guids.size());
-    for (std::size_t fx = 0; fx < guids.size(); ++fx) {
-        snap.fxOffline.push_back(
-            FxOfflineState{guids[fx], TrackFX_GetOffline(tr, static_cast<int>(fx)) ? 1 : 0});
-    }
-    return snap;  // fxKeying stays Identity — a live capture always knows the chain
+    PreParkFx fx = snapshotFxOffline(tr, cancelledRestore);
+    snap.fxOffline = std::move(fx.states);
+    snap.fxKeying  = fx.keying;
+    return snap;
 }
 
 // A reapply (tag/untag, project load) re-plans every flag it already applied, so
-// most of what it writes is a value the track already holds; the read is the
-// cheap half of the pair, and it also keeps those no-op writes out of the undo
-// record's mask.
+// most of what it writes is a value the track already holds. The read is ASSUMED
+// cheaper than the write it elides (unmeasured); what it certainly does is keep
+// those no-op writes out of the undo record's mask.
 bool writeIfChanged(MediaTrack* tr, const char* parm, double value) {
     if (GetMediaTrackInfo_Value(tr, parm) == value) return false;
     SetMediaTrackInfo_Value(tr, parm, value);
@@ -191,11 +193,10 @@ void applyFlags(MediaTrack* tr, const std::vector<TrackFlagOp>& flags, int& undo
     }
 }
 
-// Managed-lane application: the pure planner keys LanePlayOps by the lane's
-// DURABLE name; REAPER's C_LANEPLAYS:N is keyed by current ordinal, which
-// renumbers on reorder. So every write here re-resolves durable key -> current
-// ordinal first. A lane whose name lacks the managed prefix never enters this
-// map and so can never be driven.
+// Managed-lane application: LanePlayOps are keyed by the lane's DURABLE name but
+// C_LANEPLAYS:N by current ordinal, which renumbers on reorder — so every write
+// re-resolves durable key -> ordinal first, and a lane whose name lacks the
+// managed prefix never enters the map and so can never be driven.
 
 // Lane `laneIdx`'s durable name (P_LANENAME:n) on `tr`, or empty if unnamed /
 // unavailable (non-fixed-lane track).
@@ -268,12 +269,10 @@ bool applyLaneOps(const TrackByGuid& handleByGuid,
     return touchedFreeMode;
 }
 
-// Managed-lane minting: the DECISION (which tracks split, which lanes, which
-// item goes where) is planLaneMinting; this shell only reads live per-item
-// mode+lane state, calls it, and applies the resulting writes.
+// Managed-lane minting: the DECISION (which tracks split, which lanes, which item
+// goes where) is planLaneMinting's; this shell only reads, calls and applies.
 
-// Maps every item GUID on `tr` to its handle in one pass (avoids a per-item
-// re-scan in the assign loop).
+// One pass, so the assign loop needs no per-item re-scan.
 std::map<std::string, MediaItem*> itemHandlesByGuid(MediaTrack* tr) {
     std::map<std::string, MediaItem*> byGuid;
     const int itemCount = CountTrackMediaItems(tr);
@@ -295,9 +294,8 @@ std::string itemModeFromMembership(const ViewModeModel& model, const std::string
     return *modes.begin();
 }
 
-// Builds the per-track LaneItem picture the pure decision consumes. Manual-
-// lane reads are skipped on a non-fixed-lane track (isOnManualLane is false
-// there regardless of name).
+// The per-track LaneItem picture the pure decision consumes. Manual-lane reads
+// are skipped on a non-fixed-lane track (isOnManualLane is false there anyway).
 std::vector<LaneTrack> readLaneTracks(
         const ViewModeModel& model,
         const TrackHandles& handleByGuid) {
@@ -329,9 +327,8 @@ std::vector<LaneTrack> readLaneTracks(
     return tracks;
 }
 
-// Idempotent: writes I_FIXEDLANE only when it differs from the item's current
-// lane. Non-destructive — only this reversible flag is written, never a move
-// in time or across tracks.
+// Idempotent, and non-destructive: only this reversible flag is written, never
+// a move in time or across tracks.
 bool assignItemToLane(MediaTrack* tr, MediaItem* it, int laneOrdinal) {
     const int current = static_cast<int>(GetMediaItemInfo_Value(it, "I_FIXEDLANE"));
     if (current == laneOrdinal) return false;  // already there — no-op
@@ -465,28 +462,26 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject
             clearTrackSolos(handleByGuid, outgoingSolo);
         }
 
-        // PARK: snapshot before mutating, store into the model, then apply. The
-        // per-FX offline half is deferred (view_fx_park) — it is the expensive
-        // half and nothing about the new mode's appearance waits on it.
+        // PARK: snapshot before mutating. The per-FX offline half is deferred
+        // (view_fx_park) — the expensive half, and the new mode's appearance
+        // does not wait on it.
         for (const TrackPlan& tp : plan.park) {
             if (tp.flags.empty()) continue;  // every op in a TrackPlan targets one track
             const std::string& guid = tp.flags.front().guid;
             MediaTrack* tr = resolve(trackByGuid, guid);
             if (!tr) continue;  // stale GUID — prune
 
-            // Snapshot ONCE, at first park: a snapshot already present means the
-            // track is still parked from a prior apply (or its deferred restore
-            // has not landed yet), so its live flags are the parked values —
-            // recapturing would overwrite the true pre-park state with zeros and
-            // a later restore would hide it for good.
+            // 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
+            // pre-park FX state. Snapshot ONCE — a snapshot already present means
+            // the track is still parked, so its live flags read as parked.
+            const std::vector<FxOfflineOp> cancelled = deferFxPark(proj, guid);
             if (model.snapshot(guid) == nullptr)
-                model.storeSnapshot(guid, snapshotTrack(tr));
+                model.storeSnapshot(guid, snapshotTrack(tr, cancelled));
             applyFlags(tr, tp.flags, undoMask);
-            deferFxPark(proj, guid);
         }
 
-        // RESTORE: flags verbatim now, per-FX offline on the drain, which is also
-        // where the consumed snapshot is dropped.
+        // RESTORE: flags verbatim now, per-FX offline on the drain.
         for (const TrackPlan& tp : plan.restore) {
             if (tp.flags.empty()) continue;
             const std::string& guid = tp.flags.front().guid;
@@ -495,11 +490,15 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject
 
             applyFlags(tr, tp.flags, undoMask);
             deferFxRestore(proj, guid, tp.fxOffline);
+            // Consumed HERE, not on the drain: from this line the flags are back
+            // at their captured values, and a persist or reapply landing inside
+            // the drain window must not replan a restore over what the user has
+            // changed since.
+            model.clearSnapshot(guid);
         }
 
-        // MANAGED LANES: drive C_LANEPLAYS so the active mode's lane plays+shows
-        // and every other managed lane is silenced+hidden. Empty for a D1-only
-        // project, leaving that behavior byte-identical.
+        // MANAGED LANES: the active mode's lane plays+shows, every other managed
+        // lane is silenced+hidden. Empty on a D1-only project — byte-identical there.
         laneModeChanged = applyLaneOps(trackByGuid, plan.lanes);
 
         // PARENT VISIBILITY (never parked): recomputed every toggle, never
@@ -530,8 +529,8 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject
     }
 
     // Force REAPER to rebuild the TCP/MCP now rather than on the next user
-    // interaction: TrackList_AdjustWindows(false) does the full relayout owed
-    // when tracks appear/disappear; UpdateArrange() repaints.
+    // interaction — AdjustWindows(false) is the full relayout tracks appearing
+    // and disappearing owes.
     TrackList_AdjustWindows(false);
     UpdateArrange();
 
@@ -586,7 +585,8 @@ bool mintManagedLanes(ViewModeModel& model, ReaProject* proj) {
     UpdateArrange();
 
     // Lane minting writes track lane config and item lane assignment and nothing
-    // else — never an FX state — so it takes the same honest mask applyMode does.
+    // else — never an FX state — so it takes the same honest mask applyMode does
+    // (kApplyUndoMask, whose [verify — DAW] on lane-property domain binds here too).
     Undo_EndBlock2(proj, "ReaSampler: separate cross-mode content into lanes", kApplyUndoMask);
     return true;
 }