Fix Wave-2 review findings: FX union on re-park, per-project reconcile memo, shared undo mask, decayed test oracle

src/shell/view/view.cpp

@@ -41,18 +41,6 @@ namespace {
 // deliberately excluded, nothing is moving there.
 constexpr int kTransportMoving = 1 | 4;
 
-// The domains one apply actually writes: track config/routing (visibility,
-// B_MAINSEND, I_FXEN, I_SOLO), item lane assignment, and the ext-state block
-// (UNDO_STATE_MISCCFG covers extension state). NOT UNDO_STATE_ALL, and no
-// UNDO_STATE_FX at all — the apply moves no FX state, so making every undo record
-// carry the project's whole FX chunk would cost with the plugin count for nothing.
-// [verify — DAW] INFERRED, not documented: UNDO_STATE_TRACKCFG reads "track/master
-// vol/pan/routing" (reaper_plugin.h:1541) and names neither the fixed-lane properties
-// (assumed to ride UNDO_STATE_ITEMS, :1543) nor B_SHOWINTCP/B_SHOWINMIXER — and for
-// the visibility pair that inference is now DISPROVEN in the DAW: REAPER's undo does
-// not restore them, which is what view_reconcile exists to repair.
-constexpr int kApplyUndoMask = UNDO_STATE_TRACKCFG | UNDO_STATE_ITEMS | UNDO_STATE_MISCCFG;
-
 // applyMode is reachable from the load tick, the actions and render_in_place. Nothing
 // it calls is known to pump the message loop now that the FX writes are gone, but a
 // re-entrant apply would plan against the same model the outer loop is iterating and
@@ -199,7 +187,7 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject
     if (laneModeChanged) UpdateTimeline();
 
     const bool mint = applyMintsUndoPoint(realSwitch, wrote);
-    Undo_EndBlock2(project, mint ? undoLabel.c_str() : "", mint ? kApplyUndoMask : 0);
+    Undo_EndBlock2(project, mint ? undoLabel.c_str() : "", mint ? kViewUndoMask : 0);
 
     // The tool's own account of what it just left driven — the only one that survives
     // an undo rolling the model back underneath it.

src/shell/view/view_lanes.cpp

@@ -45,10 +45,6 @@ namespace {
 // I_FREEMODE value for fixed lanes. SDK: 0=normal, 1=free item positioning, 2=fixed lanes.
 constexpr int kFreeModeFixedLanes = 2;
 
-// The same domains the apply's own mask names (view.cpp's kApplyUndoMask, which owns
-// the rationale): lane minting writes track lane config and item lane assignment.
-constexpr int kLaneUndoMask = UNDO_STATE_TRACKCFG | UNDO_STATE_ITEMS | UNDO_STATE_MISCCFG;
-
 // C_LANESCOLLAPSED=2: render a tool-split track like a normal single-lane track
 // showing only the playing lane (SDK: 1=collapsed, 2=hidden-lanes-exist but displays
 // as non-fixed-lane).
@@ -280,6 +276,8 @@ bool mintManagedLanes(ViewModeModel& model, ReaProject* proj) {
 
     const TrackByGuid trackByGuid = indexHandles(handleByGuid);
 
+    // `proj` deliberately NOT pinned the way applyMode pins it: nothing between this
+    // begin and its end pumps the message loop, so no tab switch can land between them.
     Undo_BeginBlock2(proj);
     const bool changed = applyMintPlan(model, plan, trackByGuid);
 
@@ -308,7 +306,7 @@ bool mintManagedLanes(ViewModeModel& model, ReaProject* proj) {
     UpdateTimeline();  // a split happened this call — refresh is owed
     UpdateArrange();
 
-    Undo_EndBlock2(proj, "ReaSampler: separate cross-mode content into lanes", kLaneUndoMask);
+    Undo_EndBlock2(proj, "ReaSampler: separate cross-mode content into lanes", kViewUndoMask);
     return true;
 }
 

src/shell/view/view_park.cpp

@@ -75,6 +75,8 @@ bool applyFlags(MediaTrack* tr, const std::vector<TrackFlagOp>& flags) {
 
 }  // namespace
 
+const int kViewUndoMask = UNDO_STATE_TRACKCFG | UNDO_STATE_ITEMS | UNDO_STATE_MISCCFG;
+
 bool writeIfChanged(MediaTrack* tr, const char* parm, double value) {
     if (GetMediaTrackInfo_Value(tr, parm) == value) return false;
     SetMediaTrackInfo_Value(tr, parm, value);
@@ -87,12 +89,25 @@ bool parkTrack(ViewModeModel& model, const std::string& guid, MediaTrack* tr,
 
     if (!model.holdsSnapshotFlags(guid)) {
         TrackSnapshot snap = snapshotTrack(tr);
+
         // A restore hands the flags back immediately but leaves the chain offline
         // until the reclaimer gets to it, so a re-park lands here with the FX half
-        // still held over a still-parked chain. Keep it: the live read above would
-        // record the park's own offline state as the user's, permanently.
+        // still held over a still-parked chain. Union the live read against the
+        // record rather than overwriting either way — see unionFxOfflineOnRepark.
+        //
+        // `prior->fxHeld` is provably redundant here today — eraseIfSpent
+        // (view_mode_model.cpp) guarantees a surviving record has fxHeld == true
+        // — so the false arm is unreached. Kept anyway: it is the direct
+        // statement of what this branch actually needs, and cheaper to keep true
+        // than to re-derive if that guarantee ever loosens.
         const TrackSnapshot* prior = model.snapshot(guid);
-        if (prior && prior->fxHeld) {
+        if (prior && prior->fxHeld && prior->fxKeying == FxKeying::Identity) {
+            snap.fxOffline = unionFxOfflineOnRepark(prior->fxOffline, snap.fxOffline);
+            snap.fxKeying  = FxKeying::Identity;
+        } else if (prior && prior->fxHeld) {
+            // Slot-keyed legacy record (lifted from a pre-identity view_state): no
+            // identities to union against, so fall back to the old behavior and
+            // trust the record verbatim over live slots that may have moved since.
             snap.fxOffline = prior->fxOffline;
             snap.fxKeying  = prior->fxKeying;
         }

src/shell/view/view_park.h

@@ -4,6 +4,7 @@
 // FX half is captured here (a read) but never written here; the reclaimer owns
 // every TrackFX_SetOffline, off the switch's synchronous path.
 
+#include <set>
 #include <string>
 #include <vector>
 
@@ -27,6 +28,42 @@ inline const char* trackFlagParm(Flag f) {
     return "B_SHOWINTCP";  // unreachable; keeps the compiler quiet
 }
 
+// A re-park's FX half, unioned rather than overwritten: keeps every FX identity
+// `prior` already names (the user's real pre-park state, still offline while the
+// reclaimer hasn't run) and appends whatever identity `live` sees that `prior`
+// does not — a plugin added during the restored interval, which a verbatim keep
+// of `prior` would otherwise never capture and leave stuck offline for good once
+// the reclaimer lands. Only meaningful when `prior` is Identity-keyed; a
+// Slot-keyed legacy record has no identities to union against, so parkTrack falls
+// back to trusting it verbatim instead of calling this. An empty fxGuid (REAPER
+// reported no identity) is never deduped against another empty one — it is
+// unrestorable either way, so appending it costs nothing and dedup would be a
+// coin flip.
+inline std::vector<FxOfflineState> unionFxOfflineOnRepark(
+        const std::vector<FxOfflineState>& prior, const std::vector<FxOfflineState>& live) {
+    std::vector<FxOfflineState> merged = prior;
+    std::set<std::string> named;
+    for (const FxOfflineState& fx : merged)
+        if (!fx.fxGuid.empty()) named.insert(fx.fxGuid);
+    for (const FxOfflineState& fx : live)
+        if (fx.fxGuid.empty() || named.insert(fx.fxGuid).second) merged.push_back(fx);
+    return merged;
+}
+
+// The undo-state domains both the mode-switch apply (view.cpp) and lane minting
+// (view_lanes.cpp) write: track config/routing, item lane assignment, and the
+// ext-state block (UNDO_STATE_MISCCFG covers extension state). NOT UNDO_STATE_ALL,
+// and no UNDO_STATE_FX at all — neither writer moves FX state, so carrying the
+// project's whole FX chunk on every point would cost with the plugin count for
+// nothing. One symbol, not two identically-valued constants free to diverge.
+// [verify — DAW] INFERRED, not documented: UNDO_STATE_TRACKCFG reads "track/master
+// vol/pan/routing" (reaper_plugin.h:1541) and names neither the fixed-lane
+// properties (assumed to ride UNDO_STATE_ITEMS, :1543) nor
+// B_SHOWINTCP/B_SHOWINMIXER — and for the visibility pair that inference is now
+// DISPROVEN in the DAW: REAPER's undo does not restore them, which is what
+// view_reconcile exists to repair.
+extern const int kViewUndoMask;
+
 // A track parameter written only when it is not already there. A reapply re-plans
 // every flag it already applied, so most of what it writes is a value the track
 // already holds. The read is ASSUMED cheaper than the write it elides (unmeasured);

src/shell/view/view_reconcile.cpp

@@ -5,6 +5,7 @@
 
 #include "shell/view/view_reconcile.h"
 
+#include <map>
 #include <vector>
 
 #include "shell/view/view_park.h"    // trackFlagParm / writeIfChanged
@@ -23,19 +24,21 @@ namespace reasampler {
 
 namespace {
 
-// What the last apply left driven, and in which project. The owner is COMPARE-ONLY
+// What the last apply left driven, per project. Keyed by ReaProject*, COMPARE-ONLY
 // and never dereferenced (the same discipline the panel's own project memos take): a
-// closed project's pointer must never be followed, only found unequal. A project we
-// hold no memo for gets no repair — the tool has no account of driving anything
-// there, so it has no opinion.
-ReaProject*        g_appliedOwner = nullptr;
-view::AppliedState g_applied;
+// closed project's pointer must never be followed, only found unequal or used as a
+// map key. A project absent here gets no repair — the tool has no account of driving
+// anything there, so it has no opinion. Never pruned on project close: churn is a
+// handful of entries per session, cheap enough that a close hook would cost more than
+// it saves. One slot used to serve every project; a tab-switch mid-session silently
+// discarded whichever project's memo wasn't "current" — see planReconcilePerProject.
+std::map<ReaProject*, view::AppliedState> g_applied;
 
 }  // namespace
 
 void noteAppliedState(ReaProject* proj, const ViewModeModel& model) {
-    g_appliedOwner = proj ? proj : EnumProjects(-1, nullptr, 0);
-    g_applied      = view::appliedState(model);
+    ReaProject* const project = proj ? proj : EnumProjects(-1, nullptr, 0);
+    g_applied[project] = view::appliedState(model);
 }
 
 void reconcileAppliedFlags(ViewModeModel& model, ReaProject* proj) {
@@ -43,16 +46,13 @@ void reconcileAppliedFlags(ViewModeModel& model, ReaProject* proj) {
     if (!ValidatePtr2(nullptr, project, "ReaProject*")) return;
 
     const view::AppliedState reloaded = view::appliedState(model);
-    const std::vector<TrackFlagOp> ops =
-        project == g_appliedOwner ? planReconcile(g_applied, reloaded)
-                                  : std::vector<TrackFlagOp>{};
+    const ProjectReconcileResult result = planReconcilePerProject(g_applied, project, reloaded);
 
     // Adopted whether or not anything is written: from here the memo describes the
     // project as it now stands, so a second reload cannot replay a repair that has
     // already happened over values the user has since changed.
-    g_appliedOwner = project;
-    g_applied      = reloaded;
-    if (ops.empty()) return;
+    g_applied[project] = result.newEntry;
+    if (result.ops.empty()) return;
 
     TrackHandles handleByGuid;
     readFolderEntries(project, handleByGuid);
@@ -62,7 +62,7 @@ void reconcileAppliedFlags(ViewModeModel& model, ReaProject* proj) {
     Undo_BeginBlock2(project);
     {
         UiRefreshHold uiHold;
-        for (const TrackFlagOp& op : ops) {
+        for (const TrackFlagOp& op : result.ops) {
             MediaTrack* tr = resolve(trackByGuid, op.guid);
             if (!tr) continue;  // stale/deleted GUID — prune
             if (writeIfChanged(tr, trackFlagParm(op.flag), static_cast<double>(op.value)))

src/shell/view/view_reconcile.h

@@ -4,6 +4,7 @@
 // does not carry them (DAW-confirmed) — so a park the model no longer records can be
 // left driven on the track with nobody holding its captured values but us.
 
+#include <map>
 #include <set>
 #include <string>
 #include <vector>
@@ -27,6 +28,14 @@ namespace reasampler {
 //
 // A track the RELOADED model still parks is left out too — it still has an owner, and
 // the reapply that follows this pass drives it from the live model.
+//
+// THE LOAD-BEARING RULE is narrower than the `applied == reloaded` early-out below
+// looks: only a GUID in `applied.parked` that is ABSENT from `reloaded.parked` can
+// ever produce an op. A drifted `activeModeId` alone, or a drifted captured VALUE
+// under a GUID both sides still name, fails the equality check (so the early-out is
+// skipped) but the loop below finds that GUID still recorded and writes nothing —
+// the check is a cheap skip, not a "did anything change" decision. See
+// tests/test_view_reconcile.cpp's drift-alone case for the documented inertness.
 inline std::vector<TrackFlagOp> planReconcile(const view::AppliedState& applied,
                                               const view::AppliedState& reloaded) {
     std::vector<TrackFlagOp> ops;
@@ -45,6 +54,32 @@ inline std::vector<TrackFlagOp> planReconcile(const view::AppliedState& applied,
     return ops;
 }
 
+// The per-project memo's pure decision: given every project the tool has a memo for
+// so far, which project just reloaded, and its fresh fingerprint, produce the ops to
+// write back plus the entry the memo should hold for that project from here on.
+// `ReaProject*` is used only as an opaque map key here — never dereferenced, the same
+// compare-only discipline the .cpp's memo itself keeps.
+//
+// A project absent from `applied` gets no repair (see SCOPE above: no memo, no
+// opinion), but its `reloaded` fingerprint is still the entry handed back — a project
+// this pass has never seen before starts its own history here rather than being
+// re-skipped on every future compare too.
+struct ProjectReconcileResult {
+    std::vector<TrackFlagOp> ops;
+    view::AppliedState       newEntry;
+};
+
+inline ProjectReconcileResult planReconcilePerProject(
+        const std::map<ReaProject*, view::AppliedState>& applied, ReaProject* project,
+        const view::AppliedState& reloaded) {
+    auto it = applied.find(project);
+    ProjectReconcileResult result;
+    result.ops = it != applied.end() ? planReconcile(it->second, reloaded)
+                                     : std::vector<TrackFlagOp>{};
+    result.newEntry = reloaded;
+    return result;
+}
+
 // Records what `model` left driven in `proj` (nullptr = current project). Called at
 // the end of every apply: the fingerprint is the ONLY account of what the tool drove
 // once an undo has rolled the model back underneath it.