From 7169d7f22b79ed354a6467d0de6d5c9b9b7eee7c Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 13:35:30 -0400 Subject: [PATCH] Clear the park snapshot where the restore is planned, not where it drains; make the drain re-entrant and reload-aware --- docs/TODO.md | 2 +- src/app/main.cpp | 18 +++-- src/shell/view/CLAUDE.md | 9 ++- src/shell/view/view.cpp | 94 +++++++++++------------ src/shell/view/view_fx_park.cpp | 57 ++++++++++---- src/shell/view/view_fx_park.h | 79 +++++++++++++++---- tests/test_view_fx_park.cpp | 130 ++++++++++++++++++++++++++++++-- 7 files changed, 298 insertions(+), 91 deletions(-) diff --git a/docs/TODO.md b/docs/TODO.md index 947a354..1585b3d 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -881,7 +881,7 @@ there costs one `target_sources` line in `src/app/CMakeLists.txt` instead. sidestepped), dropping the file under the ~600-line ceiling; `view.cpp`'s own path is unblocked once the build-file ownership question is resolved. -## FX-GUID stability for `restoreFxOffline` is unverified in the DAW +## FX-GUID stability for `applyRestore` is unverified in the DAW **Context.** The Design View park/restore FX keying (`applyRestore`, `src/shell/view/view_fx_park.cpp`) rests on `TrackFX_GetFXGUID` returning an identity that diff --git a/src/app/main.cpp b/src/app/main.cpp index ca7b52b..140ca94 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -173,12 +173,6 @@ static void OnTimer() g_session.poll(); - // A mode switch applies its visibility/routing writes synchronously and leaves - // the per-FX offline work here, so the new mode paints before the plugins - // unload. Drained AFTER poll() so a project switch discards the queue instead - // of applying it to the project that replaced it; idle cost is one empty test. - reasampler::drainDeferredFxParks(g_session.view()); - // persist stays MODEL-ONLY (loads the saved view model but does not apply // visibility, to avoid coupling persist to the view shell); poll() raises a // one-shot load signal that we drain here to reapply the SAVED active mode so a @@ -187,12 +181,24 @@ static void OnTimer() // reapply so re-arm and model restore ride the one load event (otherwise // pre-existing tracks can be mis-detected as "new" and mass-tagged). if (g_session.consumeLoadSignal()) { + // Every path that raises this signal — open, project switch, recycled + // pointer, undo/redo state restore — replaced the model the pending FX + // intents were planned against, so they are discarded rather than applied + // to the project that replaced it. This is the queue's ONLY guard against + // a recycled ReaProject*, which a pointer compare cannot see. + reasampler::discardDeferredFxParks(); reasampler::bankPanelNotifyProjectLoaded(); // Reconcile lane ownership against the live project's lanes (P_LANENAME, // the cross-session source of truth) BEFORE reapplying visibility. Never // re-mints, never mass-tags. reasampler::reconcileManagedLanes(g_session.view(), nullptr); reasampler::applyMode(g_session.view(), g_session.view().activeModeId(), nullptr); + } else { + // A mode switch applies its visibility/routing writes synchronously and + // leaves the per-FX offline work here, so the new mode paints before the + // plugins unload. Idle cost is one empty test. Skipped on a load tick so + // the reapply's own intents defer one tick like any other switch's. + reasampler::drainDeferredFxParks(); } reasampler::bankPanelRefresh(); // cheap fingerprint compare; no-op when unchanged/closed diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index e4f719d..bd80068 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -55,6 +55,13 @@ decide membership or mode rules. 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]`. +- **Stated DEVIATION — the undo mask does not keep FX out of a real switch.** The + apply mask (`kApplyUndoMask`) drops `UNDO_STATE_FX` and ORs it back in when a + driven flag in that domain moved; the only such flag is `I_FXEN`, which every + park writes. So any switch that parks at least one track still makes REAPER + marshal the project-wide FX chunk into its undo record. The saving is real only + on a reapply and on a no-op switch. Narrowing it further would mean not carrying + `I_FXEN` in the undo record at all, which would break "one switch is one Ctrl-Z." - **Show-both semantics:** a per-track "pin visible across modes" flag re-enables processing whenever shown. A show-both leaf appears in every mode's visible set and is never parked — its driven flags stay at snapshot/restored values, FX @@ -93,7 +100,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 per-FX offline 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), and the idle-tick drain `main.cpp`'s `OnTimer` calls. The drain is also where a consumed snapshot is cleared — not where the restore was planned — so a second switch arriving before the first drained re-parks against the still-true captured state instead of re-capturing parked values. +- `view_fx_park` — the per-FX offline 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), and the idle-tick drain `main.cpp`'s `OnTimer` calls. **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 diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index 4f48614..2bc9466 100644 --- a/src/shell/view/view.cpp +++ b/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 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& cancelledRestore) { TrackSnapshot snap; snap.showInTcp = static_cast(GetMediaTrackInfo_Value(tr, "B_SHOWINTCP")); snap.showInMixer = static_cast(GetMediaTrackInfo_Value(tr, "B_SHOWINMIXER")); snap.mainSend = static_cast(GetMediaTrackInfo_Value(tr, "B_MAINSEND")); snap.fxEnable = static_cast(GetMediaTrackInfo_Value(tr, "I_FXEN")); - const std::vector 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(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& 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 itemHandlesByGuid(MediaTrack* tr) { std::map 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 readLaneTracks( const ViewModeModel& model, const TrackHandles& handleByGuid) { @@ -329,9 +327,8 @@ std::vector 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(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 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; } diff --git a/src/shell/view/view_fx_park.cpp b/src/shell/view/view_fx_park.cpp index 95190aa..22f99cf 100644 --- a/src/shell/view/view_fx_park.cpp +++ b/src/shell/view/view_fx_park.cpp @@ -9,7 +9,6 @@ #include #include -#include "core/view/view_mode_model.h" #include "shell/capture/track_guid.h" #define REAPERAPI_MINIMAL @@ -40,9 +39,11 @@ void adoptOwner(ReaProject* proj) { g_owner = p; } -// TrackFX_SetOffline unloads and re-instantiates the plugin, so writing a state -// that already holds is not a no-op — it is the expensive half of a mode switch -// spent on nothing. +// [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) { if (TrackFX_GetOffline(tr, fx) == offline) return; TrackFX_SetOffline(tr, fx, offline); @@ -94,9 +95,24 @@ std::vector liveFxGuids(MediaTrack* tr) { return guids; } -void deferFxPark(ReaProject* proj, const std::string& guid) { +PreParkFx snapshotFxOffline(MediaTrack* tr, const std::vector& cancelled) { + PreParkFx carried = preParkFxFromCancelledRestore(cancelled); + if (!carried.states.empty()) return carried; + + // Nothing was cancelled, so the chain reads true. fxKeying stays Identity — + // a live capture always knows the chain. + PreParkFx live; + const std::vector guids = liveFxGuids(tr); + live.states.reserve(guids.size()); + for (std::size_t fx = 0; fx < guids.size(); ++fx) + live.states.push_back( + FxOfflineState{guids[fx], TrackFX_GetOffline(tr, static_cast(fx)) ? 1 : 0}); + return live; +} + +std::vector deferFxPark(ReaProject* proj, const std::string& guid) { adoptOwner(proj); - g_queue.park(guid); + return g_queue.park(guid); } void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector ops) { @@ -104,23 +120,35 @@ void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector draining = g_queue.take(); + std::unordered_map byGuid; - const int count = CountTracks(g_owner); + const int count = CountTracks(proj); byGuid.reserve(static_cast(count)); for (int i = 0; i < count; ++i) { - MediaTrack* tr = GetTrack(g_owner, i); + MediaTrack* tr = GetTrack(proj, i); if (!tr) continue; std::string guid = guidString(tr); if (!guid.empty()) byGuid.emplace(std::move(guid), tr); @@ -128,7 +156,7 @@ void drainDeferredFxParks(ViewModeModel& model) { FxRestoreDrops drops; int dropTracks = 0; - for (const FxParkIntent& intent : g_queue.pending()) { + for (const FxParkIntent& intent : draining) { auto it = byGuid.find(intent.guid); if (it == byGuid.end()) continue; // track deleted since the switch — prune if (intent.park) { @@ -140,11 +168,8 @@ void drainDeferredFxParks(ViewModeModel& model) { drops.add(d); ++dropTracks; } - model.clearSnapshot(intent.guid); } - g_queue.clear(); - // Captured FX state that could not be applied is REPORTED — silence would read // to the user as "restore worked" while an FX sat at whatever state the park // left it in. The "!SHOW:" prefix (reaper_plugin_functions.h:6536) keeps it from diff --git a/src/shell/view/view_fx_park.h b/src/shell/view/view_fx_park.h index 2377277..ad054f3 100644 --- a/src/shell/view/view_fx_park.h +++ b/src/shell/view/view_fx_park.h @@ -15,8 +15,6 @@ class ReaProject; namespace reasampler { -class ViewModeModel; - // The chain as it stands now: identity by current slot. Snapshot, park and // restore all address FX through this one plain 0..TrackFX_GetCount-1 // enumeration — never the 0x1000000/0x2000000 input-FX or container forms — so @@ -39,15 +37,24 @@ struct FxParkIntent { // observably wrong. class FxParkQueue { public: - void park(const std::string& guid) { + // Returns the ops of a pending restore this park CANCELLED, empty otherwise. + // The caller needs them: that restore never ran, so the live chain still + // reads the PARKED offline states and is no longer a source for a fresh + // pre-park snapshot (see preParkFxFromCancelledRestore). + std::vector park(const std::string& guid) { FxParkIntent* held = find(guid); if (!held) { pending_.push_back(FxParkIntent{guid, true, {}}); - } else if (!held->park) { - erase(held); + return {}; } + if (held->park) return {}; + std::vector cancelled = std::move(held->restoreOps); + erase(held); + return cancelled; } + // A restore cancelling a pending park is COMPLETE at that point — the park + // never ran, so no drain will ever come for this GUID. void restore(const std::string& guid, std::vector ops) { FxParkIntent* held = find(guid); if (!held) { @@ -65,6 +72,17 @@ public: bool empty() const { return pending_.empty(); } void clear() { pending_.clear(); } + // Detaches everything pending, leaving the queue able to accept intents + // enqueued WHILE the caller applies what it took. Applying loads/unloads + // plugins, which pumps the message loop, so a re-entrant switch can enqueue + // mid-apply: iterating the live queue would dangle on the push_back, and + // clearing it afterwards would discard whatever arrived during the apply. + std::vector take() { + std::vector taken; + taken.swap(pending_); + return taken; + } + private: FxParkIntent* find(const std::string& guid) { for (FxParkIntent& i : pending_) @@ -78,18 +96,49 @@ private: std::vector pending_; }; +// The FX half of a snapshot, with the keying it must be read back under. +struct PreParkFx { + std::vector states; + FxKeying keying = FxKeying::Identity; +}; + +// The FX half a fresh pre-park snapshot must carry when the park CANCELLED a +// pending restore: those ops are the only surviving record of the pre-park +// state, because the chain still reads the parked values until that restore +// drains — and it never will, the cancel dropped it. Empty in (nothing was +// cancelled) means the caller reads the live chain instead. The restore's own +// keying travels with it so a slot-keyed snapshot lifted from a legacy +// view_state does not silently become an identity-keyed one with no identities. +inline PreParkFx preParkFxFromCancelledRestore(const std::vector& cancelled) { + PreParkFx out; + if (cancelled.empty()) return out; + out.keying = cancelled.front().keying; + out.states.reserve(cancelled.size()); + for (const FxOfflineOp& op : cancelled) + out.states.push_back(FxOfflineState{op.fxGuid, op.offline ? 1 : 0}); + return out; +} + +// The FX half of a fresh pre-park snapshot for `tr`: whatever the accompanying +// park cancelled, else a live read of the chain. +PreParkFx snapshotFxOffline(MediaTrack* tr, const std::vector& cancelled); + // Enqueue against `proj` (nullptr = current project). An enqueue naming a -// different project than the pending intents discards those unapplied. -void deferFxPark(ReaProject* proj, const std::string& guid); +// different project than the pending intents discards those unapplied. The park +// returns whatever pending restore it cancelled, per FxParkQueue::park. +std::vector deferFxPark(ReaProject* proj, const std::string& guid); void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector ops); -// Applies every pending intent, then clears each drained restore's snapshot -// from `model` — the snapshot is consumed when the restore actually lands, not -// when it was planned, so a second switch arriving first re-parks against the -// still-true captured state instead of re-capturing parked values. Discards the -// queue unapplied if the project it was enqueued against is no longer current -// (close / switch); an intent whose track is gone is pruned. Idle cost is one -// empty-queue test. -void drainDeferredFxParks(ViewModeModel& model); +// 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); +// an intent whose track is gone is pruned. Idle cost is one empty-queue test. +void drainDeferredFxParks(); + +// Drops every pending intent without applying it. Called when the model the +// intents were planned against has been replaced (project load/switch, undo/redo +// state restore) — applying them then would write the pre-reload plan over the +// project that replaced it. +void discardDeferredFxParks(); } // namespace reasampler diff --git a/tests/test_view_fx_park.cpp b/tests/test_view_fx_park.cpp index edc4e41..a132ffd 100644 --- a/tests/test_view_fx_park.cpp +++ b/tests/test_view_fx_park.cpp @@ -1,9 +1,10 @@ -// Standalone tests for the deferred FX-park queue's re-entrancy rule — no -// REAPER, no test framework. +// Standalone tests for the deferred FX-park queue's re-entrancy rule and the +// snapshot-lifecycle contract that rides on it — no REAPER, no test framework. // -// The property 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. +// 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; +// and a cancel must not strand the pre-park FX state it was the last record of. #include "../src/shell/view/view_fx_park.h" @@ -43,6 +44,17 @@ static void testParkEnqueuesOneIntentCarryingNoOps() { CHECK(held && held->restoreOps.empty()); } +static void testParkReportsNothingCancelledWhenNoIntentWasPending() { + FxParkQueue q; + CHECK(q.park("{A}").empty()); +} + +static void testParkOnItsOwnPendingParkReportsNothingCancelled() { + FxParkQueue q; + q.park("{A}"); + CHECK(q.park("{A}").empty()); +} + static void testRestoreOnAnUndrainedParkCancelsRatherThanStacks() { // The park never ran, so the track's FX still hold their captured state — // exactly what the restore would write. Replaying both would unload every @@ -124,8 +136,109 @@ static void testClearDropsEverythingPending() { CHECK(q.pending().empty()); } +// -- snapshot lifecycle ------------------------------------------------------ + +static void testParkHandsBackTheOpsOfTheRestoreItCancelled() { + // The cancelled restore is the LAST record of the pre-park FX state: the + // track's chain still reads the parked values (the restore never ran), and + // the cancel means no drain will ever put them back. A park that drops these + // snapshots the park's own offline zeros as if they were the user's state. + FxParkQueue q; + q.restore("{A}", ops("{FX}", false)); + + const std::vector cancelled = q.park("{A}"); + + CHECK(cancelled.size() == 1); + CHECK(cancelled.size() == 1 && cancelled.front().fxGuid == "{FX}"); + CHECK(cancelled.size() == 1 && !cancelled.front().offline); + CHECK(q.empty()); // annihilated: the chain already holds what the park wants +} + +static void testCancelledRestoreOpsBecomeTheFreshSnapshotsFxHalf() { + std::vector cancelled = ops("{ONE}", true); + cancelled.push_back(FxOfflineOp{"{TRACK}", FxKeying::Identity, "{TWO}", 1, false}); + + const PreParkFx fx = preParkFxFromCancelledRestore(cancelled); + + CHECK(fx.keying == FxKeying::Identity); + CHECK(fx.states.size() == 2); + CHECK(fx.states.size() == 2 && fx.states[0].fxGuid == "{ONE}" && fx.states[0].offline == 1); + CHECK(fx.states.size() == 2 && fx.states[1].fxGuid == "{TWO}" && fx.states[1].offline == 0); +} + +static void testNothingCancelledLeavesTheFxHalfToTheCaller() { + const PreParkFx fx = preParkFxFromCancelledRestore({}); + + CHECK(fx.states.empty()); // caller reads the live chain instead + CHECK(fx.keying == FxKeying::Identity); +} + +static void testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities() { + // A snapshot lifted from a pre-identity view_state is slot-keyed and carries + // no fxGuid. Re-labelling it Identity would make resolveFxRestore drop every + // entry as unidentified instead of writing it by slot. + std::vector cancelled = { + FxOfflineOp{"{TRACK}", FxKeying::Slot, "", 0, true}, + FxOfflineOp{"{TRACK}", FxKeying::Slot, "", 1, false}, + }; + + const PreParkFx fx = preParkFxFromCancelledRestore(cancelled); + + CHECK(fx.keying == FxKeying::Slot); + CHECK(fx.states.size() == 2); + CHECK(fx.states.size() == 2 && fx.states[0].offline == 1 && fx.states[1].offline == 0); +} + +// -- re-entrancy ------------------------------------------------------------- + +static void testTakeDetachesEverythingAndLeavesTheQueueEmpty() { + FxParkQueue q; + q.park("{A}"); + q.restore("{B}", ops("{FX}", true)); + + const std::vector taken = q.take(); + + CHECK(taken.size() == 2); + CHECK(taken.size() == 2 && taken[0].guid == "{A}" && taken[0].park); + CHECK(taken.size() == 2 && taken[1].guid == "{B}" && !taken[1].park); + CHECK(q.empty()); +} + +static void testIntentsArrivingDuringADrainSurviveIt() { + // Applying an intent loads/unloads plugins, which pumps the message loop, so + // a switch can re-enter and enqueue mid-drain. Those intents belong to the + // NEXT drain — the one in progress must neither see them nor discard them. + FxParkQueue q; + q.park("{A}"); + + const std::vector draining = q.take(); + q.restore("{B}", ops("{FX}", false)); // arrives while {A} is being applied + + CHECK(draining.size() == 1); + CHECK(draining.size() == 1 && draining.front().guid == "{A}"); + CHECK(q.pending().size() == 1); + CHECK(intentFor(q, "{B}") != nullptr); +} + +static void testAReEntrantParkCancelsOnlyWhatIsStillPending() { + // {A}'s restore was already taken for the in-flight drain, so a park arriving + // mid-drain has nothing to cancel — it must queue as a fresh park rather than + // silently annihilate against an intent that has already been applied. + FxParkQueue q; + q.restore("{A}", ops("{FX}", false)); + q.take(); + + const std::vector cancelled = q.park("{A}"); + + CHECK(cancelled.empty()); + CHECK(q.pending().size() == 1); + CHECK(intentFor(q, "{A}") && intentFor(q, "{A}")->park); +} + int main() { testParkEnqueuesOneIntentCarryingNoOps(); + testParkReportsNothingCancelledWhenNoIntentWasPending(); + testParkOnItsOwnPendingParkReportsNothingCancelled(); testRestoreOnAnUndrainedParkCancelsRatherThanStacks(); testParkOnAnUndrainedRestoreCancelsRatherThanStacks(); testRepeatedParkStaysOneIntent(); @@ -133,6 +246,13 @@ int main() { testOneTracksCancelLeavesEveryOtherTrackAlone(); testCancelledTrackCanBeQueuedAgain(); testClearDropsEverythingPending(); + testParkHandsBackTheOpsOfTheRestoreItCancelled(); + testCancelledRestoreOpsBecomeTheFreshSnapshotsFxHalf(); + testNothingCancelledLeavesTheFxHalfToTheCaller(); + testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities(); + testTakeDetachesEverythingAndLeavesTheQueueEmpty(); + testIntentsArrivingDuringADrainSurviveIt(); + testAReEntrantParkCancelsOnlyWhatIsStillPending(); if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0;