From 024c6198849ee6d9ba73693ac19410ea1a6f8a79 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 17:50:49 -0400 Subject: [PATCH] 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 | 14 +--- src/shell/view/view_lanes.cpp | 8 +-- src/shell/view/view_park.cpp | 21 +++++- src/shell/view/view_park.h | 37 ++++++++++ src/shell/view/view_reconcile.cpp | 30 ++++---- src/shell/view/view_reconcile.h | 35 +++++++++ tests/test_view_mode_model.cpp | 114 ++++++++++++------------------ tests/test_view_park.cpp | 44 ++++++++++++ tests/test_view_reconcile.cpp | 46 ++++++++++++ 9 files changed, 244 insertions(+), 105 deletions(-) diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index 4bc63ce..651476d 100644 --- a/src/shell/view/view.cpp +++ b/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. diff --git a/src/shell/view/view_lanes.cpp b/src/shell/view/view_lanes.cpp index 5dba34d..087589c 100644 --- a/src/shell/view/view_lanes.cpp +++ b/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; } diff --git a/src/shell/view/view_park.cpp b/src/shell/view/view_park.cpp index fb60ebf..4b0c95d 100644 --- a/src/shell/view/view_park.cpp +++ b/src/shell/view/view_park.cpp @@ -75,6 +75,8 @@ bool applyFlags(MediaTrack* tr, const std::vector& 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; } diff --git a/src/shell/view/view_park.h b/src/shell/view/view_park.h index c4ef8c8..650f0fc 100644 --- a/src/shell/view/view_park.h +++ b/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 #include #include @@ -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 unionFxOfflineOnRepark( + const std::vector& prior, const std::vector& live) { + std::vector merged = prior; + std::set 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); diff --git a/src/shell/view/view_reconcile.cpp b/src/shell/view/view_reconcile.cpp index 3accb85..6d605e8 100644 --- a/src/shell/view/view_reconcile.cpp +++ b/src/shell/view/view_reconcile.cpp @@ -5,6 +5,7 @@ #include "shell/view/view_reconcile.h" +#include #include #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 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 ops = - project == g_appliedOwner ? planReconcile(g_applied, reloaded) - : std::vector{}; + 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(op.value))) diff --git a/src/shell/view/view_reconcile.h b/src/shell/view/view_reconcile.h index 62fec33..6c33328 100644 --- a/src/shell/view/view_reconcile.h +++ b/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 #include #include #include @@ -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 planReconcile(const view::AppliedState& applied, const view::AppliedState& reloaded) { std::vector ops; @@ -45,6 +54,32 @@ inline std::vector 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 ops; + view::AppliedState newEntry; +}; + +inline ProjectReconcileResult planReconcilePerProject( + const std::map& 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{}; + 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. diff --git a/tests/test_view_mode_model.cpp b/tests/test_view_mode_model.cpp index 78dfd5e..93345d9 100644 --- a/tests/test_view_mode_model.cpp +++ b/tests/test_view_mode_model.cpp @@ -814,13 +814,13 @@ using LiveFlags = std::map; // Runs one applyMode-equivalent toggle against `vm` + `live`. `guard` selects the // fixed behavior vs. the original buggy (snapshot-every-park) one. // Returns nothing; mutates `vm` snapshots/active mode and `live` flags in place, -// exactly mirroring view.cpp's park then restore then setActiveMode ordering. -// -// Guarded, the park recaptures only when the model holds no record — the property -// the shell's parkTrack keeps. Its second guarded branch (a parked-looking chain -// with no record is left alone) outlived the shell rule it mirrored and is kept -// here only as a second unguarded-vs-guarded contrast; the model half of the defect -// is identical either way. +// mirroring view.cpp's park then restore then setActiveMode ordering: guarded park +// gates on holdsSnapshotFlags (parkTrack's own gate, current shell — there is no +// refusal branch any more, see view_park.h) and guarded restore spends the flag half +// through releaseSnapshotFlags (restoreTrack's own call), not a full clearSnapshot. +// The FX half of the two-half record is never exercised here (nothing sets fxHeld +// false), so it stays held throughout and never blocks a re-capture the way +// flagsHeld does — the one place this harness is narrower than the real record. void simulateApplyMode(ViewModeModel& vm, LiveFlags& live, const FolderTree& tree, const std::string& target, bool guard) { TogglePlan plan = vm.planToggle(tree, target); @@ -830,8 +830,7 @@ void simulateApplyMode(ViewModeModel& vm, LiveFlags& live, const FolderTree& tre if (tp.flags.empty()) continue; const std::string& guid = tp.flags.front().guid; if (guard) { - if (vm.snapshot(guid) == nullptr) { - if (live[guid] == 0) continue; // reads parked, no snapshot — refuse + if (!vm.holdsSnapshotFlags(guid)) { TrackSnapshot snap; snap.showInTcp = live[guid]; vm.storeSnapshot(guid, snap); @@ -844,13 +843,13 @@ void simulateApplyMode(ViewModeModel& vm, LiveFlags& live, const FolderTree& tre live[guid] = 0; // park hides it } - // RESTORE: write snapshot flag back, then drop the snapshot. + // RESTORE: write snapshot flag back, then spend the flag half. for (const auto& tp : plan.restore) { if (tp.flags.empty()) continue; const std::string& guid = tp.flags.front().guid; if (const TrackSnapshot* snap = vm.snapshot(guid)) live[guid] = snap->showInTcp; // restore the captured visible flag - vm.clearSnapshot(guid); + vm.releaseSnapshotFlags(guid); } vm.setActiveMode(target); @@ -939,70 +938,47 @@ static void testNestedToggleSnapshotSurvivesRepark() { // -- 9b. Reload strand: snapshots gone, live flags still parked --------------- // -// The pair the park refusal exists for, driven through the same harness. A model -// replaced without the project rolling back with it — an unreadable view_state, a -// snapshot reconciled away while its track was deleted, a hand or script edit — -// leaves tracks sitting at parked values with nothing recording what they were -// before. Snapshotting there commits the parked state as the user's, and the very -// next toggle writes it back over whatever they have since fixed by hand. +// A model replaced without the project rolling back with it — an unreadable +// view_state, a snapshot reconciled away while its track was deleted, a hand or +// script edit — leaves tracks sitting at parked values with nothing recording what +// they were before. The park refusal that once existed for this pair (view_fx_park's +// decidePark) was retired by the Wave-2 shell restructure: view_park.h now states +// plainly that nothing infers a park from a live value any more, so a parked-looking +// chain with no record is treated as the user's own state and captured, not refused. +// This test documents the CURRENT consequence — it is not asserting the consequence +// is desirable: a hand recovery made before the next reapply survives, but one made +// after it (the reapply has already re-captured the still-hidden value as "the +// user's") is overwritten by the next restore, converging with the always-recapture +// defect test 9's unguarded block pins. Guarded and unguarded no longer differ here. -static void testReloadedModelWithParkedTracksRefusesRatherThanResnapshotting() { +static void testReloadedModelWithParkedTracksIsCapturedNotRefused() { const FolderTree tree = nestedTree(); - // GUARDED (fixed shell). - { - ViewModeModel vm; - vm.membership().tag("{L1}", kDesignModeId); - LiveFlags live{{"{L1}", 1}, {"{L2}", 1}, {"{L3}", 1}}; + ViewModeModel vm; + vm.membership().tag("{L1}", kDesignModeId); + LiveFlags live{{"{L1}", 1}, {"{L2}", 1}, {"{L3}", 1}}; - simulateApplyMode(vm, live, tree, kDesignModeId, /*guard=*/true); - CHECK(live["{L2}"] == 0 && live["{L3}"] == 0); - CHECK(vm.snapshot("{L2}") != nullptr); + simulateApplyMode(vm, live, tree, kDesignModeId, /*guard=*/true); + CHECK(live["{L2}"] == 0 && live["{L3}"] == 0); + CHECK(vm.snapshot("{L2}") != nullptr); - // The reload: the model comes back with no snapshots while the project's - // tracks are still parked. - vm.clearSnapshot("{L2}"); - vm.clearSnapshot("{L3}"); + // The reload: the model comes back with no snapshots while the project's + // tracks are still parked. + vm.clearSnapshot("{L2}"); + vm.clearSnapshot("{L3}"); - // The load tick reapplies the saved active mode. Both leaves are refused — - // nothing written, and nothing false captured. - simulateApplyMode(vm, live, tree, kDesignModeId, /*guard=*/true); - CHECK(vm.snapshot("{L2}") == nullptr && vm.snapshot("{L3}") == nullptr); - CHECK(live["{L2}"] == 0 && live["{L3}"] == 0); + // The load tick reapplies the saved active mode. Both leaves are captured + // fresh from their still-hidden live value — not refused. + simulateApplyMode(vm, live, tree, kDesignModeId, /*guard=*/true); + CHECK(vm.snapshot("{L2}") != nullptr && vm.snapshot("{L3}") != nullptr); + CHECK(live["{L2}"] == 0 && live["{L3}"] == 0); - // The documented hand recovery, on {L2} only, while still in Design. - // Toggling back must leave it where the user put it: with no snapshot - // there is nothing to restore FROM, so the restore writes nothing. - live["{L2}"] = 1; - simulateApplyMode(vm, live, tree, kArrangeModeId, /*guard=*/true); - CHECK(live["{L2}"] == 1); // the hand fix survives - CHECK(live["{L3}"] == 0); // never recovered — stuck, but never falsely committed - - // And {L2} is back under mode control from there: the next park sees a - // clean chain, captures the user's value, and restores it. - simulateApplyMode(vm, live, tree, kDesignModeId, /*guard=*/true); - CHECK(live["{L2}"] == 0 && vm.snapshot("{L2}") != nullptr); - simulateApplyMode(vm, live, tree, kArrangeModeId, /*guard=*/true); - CHECK(live["{L2}"] == 1); - } - - // UNGUARDED (the defect): the reapply after the reload recaptures the parked - // zero, so the very next toggle writes it back over the hand recovery. - { - ViewModeModel vm; - vm.membership().tag("{L1}", kDesignModeId); - LiveFlags live{{"{L1}", 1}, {"{L2}", 1}, {"{L3}", 1}}; - - simulateApplyMode(vm, live, tree, kDesignModeId, /*guard=*/false); - vm.clearSnapshot("{L2}"); - vm.clearSnapshot("{L3}"); - simulateApplyMode(vm, live, tree, kDesignModeId, /*guard=*/false); - - live["{L2}"] = 1; // the same hand recovery - simulateApplyMode(vm, live, tree, kArrangeModeId, /*guard=*/false); - - CHECK(live["{L2}"] == 0); // wiped — the false snapshot won - } + // A hand recovery made NOW, after that recapture, is not what the next restore + // reads back — the fresh (hidden) capture just above is, so it is lost. + live["{L2}"] = 1; + simulateApplyMode(vm, live, tree, kArrangeModeId, /*guard=*/true); + CHECK(live["{L2}"] == 0); // wiped by the restore + CHECK(live["{L3}"] == 0); // never recovered } // =========================================================================== @@ -2190,7 +2166,7 @@ int main() { testUntaggedLeavesManagedByModeSystem(); testTaggedLeafBehaviorUnchangedWithUntagged(); testNestedToggleSnapshotSurvivesRepark(); - testReloadedModelWithParkedTracksRefusesRatherThanResnapshotting(); + testReloadedModelWithParkedTracksIsCapturedNotRefused(); testReconcilePrunesOrphanedSnapshots(); testReconcileFullLiveSetIsNoOp(); testReconcileThenReparkLifecycleIntact(); diff --git a/tests/test_view_park.cpp b/tests/test_view_park.cpp index 8f875c8..538231e 100644 --- a/tests/test_view_park.cpp +++ b/tests/test_view_park.cpp @@ -91,6 +91,47 @@ static void testAReapplyThatWroteNothingLeavesNoPointAtAll() { CHECK(!applyMintsUndoPoint(/*realSwitch=*/false, /*wroteAnything=*/false)); } +// -- the re-park union: a plugin added between a restore and the next park -------- +// +// Sequence under test: park -> restore -> user adds plugin P -> re-park. Without the +// union, the record's prior verbatim-keep would drop P silently -- the reclaimer +// offlines it with everything else, and no snapshot ever names it to bring it back. + +static void testAPluginAddedWhileRestoredJoinsTheUnionRatherThanBeingDropped() { + const std::vector prior{ + FxOfflineState{"{FX-A}", 1}, FxOfflineState{"{FX-B}", 0}}; + const std::vector live{ + FxOfflineState{"{FX-A}", 0}, FxOfflineState{"{FX-B}", 0}, FxOfflineState{"{FX-P}", 0}}; + + const std::vector merged = unionFxOfflineOnRepark(prior, live); + + CHECK(merged.size() == 3); + bool foundA = false, foundB = false, foundP = false; + for (const FxOfflineState& fx : merged) { + if (fx.fxGuid == "{FX-A}") { foundA = true; CHECK(fx.offline == 1); } // record wins + if (fx.fxGuid == "{FX-B}") { foundB = true; CHECK(fx.offline == 0); } // record wins + if (fx.fxGuid == "{FX-P}") { foundP = true; CHECK(fx.offline == 0); } // added from live + } + CHECK(foundA && foundB && foundP); +} + +static void testAPluginRemovedWhileRestoredStaysInTheUnionForResolveFxRestoreToDrop() { + // The union is not where a removed plugin's entry disappears -- resolveFxRestore + // owns that at restore time (missingIdentity, reported), so the merge must keep + // naming it rather than quietly forgetting it here. + const std::vector prior{ + FxOfflineState{"{FX-A}", 1}, FxOfflineState{"{FX-REMOVED}", 1}}; + const std::vector live{FxOfflineState{"{FX-A}", 0}}; + + const std::vector merged = unionFxOfflineOnRepark(prior, live); + + CHECK(merged.size() == 2); + bool foundRemoved = false; + for (const FxOfflineState& fx : merged) + if (fx.fxGuid == "{FX-REMOVED}") foundRemoved = true; + CHECK(foundRemoved); +} + int main() { testEachDrivenFlagAddressesItsOwnReaperParameter(); testNoDrivenFlagCanEverAddressMuteOrSolo(); @@ -100,6 +141,9 @@ int main() { testAReapplyThatWroteSomethingLeavesAPoint(); testAReapplyThatWroteNothingLeavesNoPointAtAll(); + testAPluginAddedWhileRestoredJoinsTheUnionRatherThanBeingDropped(); + testAPluginRemovedWhileRestoredStaysInTheUnionForResolveFxRestoreToDrop(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; } diff --git a/tests/test_view_reconcile.cpp b/tests/test_view_reconcile.cpp index 0d8db11..dda1122 100644 --- a/tests/test_view_reconcile.cpp +++ b/tests/test_view_reconcile.cpp @@ -11,6 +11,7 @@ #include "../src/shell/view/view_reconcile.h" #include +#include #include #include @@ -144,6 +145,48 @@ static void testASpentFlagHalfIsNotARecordToRepairFrom() { CHECK(planReconcile(view::appliedState(applied), view::appliedState(reloaded)).empty()); } +// -- the per-project memo: a tab switch must not erase another project's account -- + +static void testASecondProjectNotedInBetweenDoesNotEraseTheFirstProjectsMemo() { + // The bug this replaced: a single global slot meant noting project B's applied + // state overwrote project A's entirely, so a repair for A run afterward found no + // memo to compare against and silently skipped -- exactly what a tab-switch then + // an undo on the ORIGINAL project produces within one timer period. + ReaProject* const projA = reinterpret_cast(1); + ReaProject* const projB = reinterpret_cast(2); + + ViewModeModel appliedA; + appliedA.setActiveMode(kDesignModeId); + appliedA.storeSnapshot("{A}", captured(1, 1, 1, 1)); + + const ViewModeModel appliedB; // untouched — B has nothing driven + + std::map memo; + memo[projA] = view::appliedState(appliedA); + memo[projB] = view::appliedState(appliedB); // noted in between; must not clobber A + + const ViewModeModel reloadedA; // A's park record dropped, as an undo would leave it + + const ProjectReconcileResult result = + planReconcilePerProject(memo, projA, view::appliedState(reloadedA)); + + CHECK(result.ops.size() == 4); + CHECK(hasOp(result.ops, "{A}", Flag::MainSend, 1)); +} + +static void testAProjectWithNoPriorMemoGetsNoRepair() { + // "A project we hold no memo for gets no repair" -- the tool has no account of + // driving anything there, so it has no opinion (see the header's SCOPE note). + ReaProject* const projC = reinterpret_cast(3); + const std::map memo; // never noted + + const ViewModeModel reloaded; + const ProjectReconcileResult result = + planReconcilePerProject(memo, projC, view::appliedState(reloaded)); + + CHECK(result.ops.empty()); +} + int main() { testAnUndoThatLeftOurAppliedOutputAloneRepairsNothing(); testADriftedActiveModeAloneIsNotAReasonToDriveATrack(); @@ -153,6 +196,9 @@ int main() { testATrackNeitherStateRecordsIsNeverDrivenAtAll(); testASpentFlagHalfIsNotARecordToRepairFrom(); + testASecondProjectNotedInBetweenDoesNotEraseTheFirstProjectsMemo(); + testAProjectWithNoPriorMemoGetsNoRepair(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; }