From 203961f41cc4e6e9121db343b66b1583bb77e260 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 18:30:40 -0400 Subject: [PATCH] Refuse to park a track whose pre-park snapshot is gone, rather than re-snapshotting the already-parked chain as the user's state --- docs/VERIFICATION.md | 2 + src/shell/view/CLAUDE.md | 15 ++++++- src/shell/view/view.cpp | 38 ++++++++++------- src/shell/view/view_fx_park.cpp | 9 ++++ src/shell/view/view_fx_park.h | 75 +++++++++++++++++++++++++++++++-- tests/test_view_fx_park.cpp | 72 ++++++++++++++++++++++++++++++- 6 files changed, 189 insertions(+), 22 deletions(-) diff --git a/docs/VERIFICATION.md b/docs/VERIFICATION.md index 4ad21bb..599f933 100644 --- a/docs/VERIFICATION.md +++ b/docs/VERIFICATION.md @@ -47,6 +47,8 @@ Checks for Θ, Ξ, Ψ, Ε, Ρ, Γ, and Ω work that no unit test can close. Buil - [ ] Solo tracks in Arrange, switch to Design, solo different tracks, switch back — each mode restores its own solo set verbatim (`docs/COMPLETED.md` §"Ψ-W1-T2") - [ ] Attempt a mode switch while the transport is playing, then while recording — both refuse, visibly (`docs/COMPLETED.md` §"Ψ-W1-T2") - [ ] Click the footer mode segment, save, reopen the project — the mode persisted (`docs/COMPLETED.md` §"Ψ-W1-T2") +- [ ] `[verify — DAW]` Reproduce the strand: project with FX in both modes, switch to Design, save, close, reopen, toggle to Arrange. Toggle back and forth several more times — the arrangement's FX must NOT converge on permanently offline/hidden. Any track the tool now refuses to park says so once in the console (`src/shell/view/view_fx_park.h`'s `decidePark`) +- [ ] `[verify — DAW]` For a track the console reports as left unparked: re-enable its TCP/mixer visibility, main send and FX enable by hand, then switch modes twice — it parks and restores normally from there, with no further console line (the refusal's self-healing claim; unit-tested only as a decision, never against a live chain) ## Actions and drops diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index caa8743..b129464 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -30,6 +30,15 @@ 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`. - **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 @@ -107,7 +116,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 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, 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_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 @@ -130,7 +139,9 @@ applies the resulting lane state to live tracks. over the reloaded model, re-planning a park for every inactive leaf, so parked FX converge on the following drain. The one case that does not self-heal is a track whose reloaded model carries no snapshot — nothing plans its restore, so - FX left offline stay offline. Full contract at `discardDeferredFxParks`. + 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`. - `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 diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index e06cd3f..43c9d41 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -31,6 +31,7 @@ #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 @@ -107,16 +108,6 @@ void applyTransparentLaneDisplay(MediaTrack* tr) { static_cast(settings | kLaneSettingsHideButtons)); } -const char* flagParm(Flag f) { - switch (f) { - case Flag::ShowInTcp: return "B_SHOWINTCP"; - case Flag::ShowInMixer: return "B_SHOWINMIXER"; - case Flag::MainSend: return "B_MAINSEND"; - case Flag::FxEnable: return "I_FXEN"; - } - return "B_SHOWINTCP"; // unreachable; keeps the compiler quiet -} - // 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. @@ -185,7 +176,7 @@ bool writeIfChanged(MediaTrack* tr, const char* parm, double value) { void applyFlags(MediaTrack* tr, const std::vector& flags, int& undoMask) { for (const TrackFlagOp& op : flags) { - if (!writeIfChanged(tr, flagParm(op.flag), static_cast(op.value))) continue; + if (!writeIfChanged(tr, trackFlagParm(op.flag), static_cast(op.value))) continue; // I_FXEN is the only FX-domain flag driven here, so it is the only one // whose undo record has to carry UNDO_STATE_FX. if (op.flag == Flag::FxEnable) undoMask |= UNDO_STATE_FX; @@ -449,6 +440,7 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject int undoMask = kApplyUndoMask; bool laneModeChanged = false; + int refusedParks = 0; Undo_BeginBlock2(proj); { @@ -470,13 +462,22 @@ 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; } + // 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 recapturing would overwrite the true - // pre-park state with zeros and a later restore would hide it for good. + // pre-park FX state. const std::vector cancelled = deferFxPark(proj, guid); - if (model.snapshot(guid) == nullptr) + if (action == ParkAction::SnapshotThenPark) model.storeSnapshot(guid, snapshotTrack(tr, cancelled)); applyFlags(tr, tp.flags, undoMask); } @@ -539,6 +540,13 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject if (laneModeChanged) UpdateTimeline(); 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()); return true; } diff --git a/src/shell/view/view_fx_park.cpp b/src/shell/view/view_fx_park.cpp index 467a80e..a03a5b6 100644 --- a/src/shell/view/view_fx_park.cpp +++ b/src/shell/view/view_fx_park.cpp @@ -14,6 +14,7 @@ #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_CountTracks #define REAPERAPI_WANT_EnumProjects +#define REAPERAPI_WANT_GetMediaTrackInfo_Value #define REAPERAPI_WANT_GetTrack #define REAPERAPI_WANT_ShowConsoleMsg #define REAPERAPI_WANT_TrackFX_GetCount @@ -99,6 +100,14 @@ std::string fxGuidString(MediaTrack* tr, int fx) { } // namespace +std::vector readParkFlagValues(MediaTrack* tr, const std::vector& parkFlags) { + std::vector live; + live.reserve(parkFlags.size()); + for (const TrackFlagOp& op : parkFlags) + live.push_back(static_cast(GetMediaTrackInfo_Value(tr, trackFlagParm(op.flag)))); + return live; +} + std::vector liveFxGuids(MediaTrack* tr) { const int fxCount = TrackFX_GetCount(tr); std::vector guids; diff --git a/src/shell/view/view_fx_park.h b/src/shell/view/view_fx_park.h index aebbf6d..ab9c980 100644 --- a/src/shell/view/view_fx_park.h +++ b/src/shell/view/view_fx_park.h @@ -1,13 +1,16 @@ #pragma once -// Design View's per-FX offline surface: the FX-identity read that snapshot, -// park and restore all address FX through, the deferred intent queue that keeps -// TrackFX_SetOffline off the mode switch's synchronous path, and the idle-tick -// drain that applies it. See src/shell/view/CLAUDE.md's FX-parking caveat. +// Design View's park surface: the FX-identity read that snapshot, park and +// restore all address FX through, the deferred intent queue that keeps +// TrackFX_SetOffline off the mode switch's synchronous path, the idle-tick drain +// that applies it, and the trust test deciding whether a chain may be +// snapshotted at all. See src/shell/view/CLAUDE.md's FX-parking caveat. +#include #include #include #include "core/view/fx_offline.h" +#include "core/view/view_mode_model.h" // Flag / TrackFlagOp — what a park writes // Forward-declared to keep this header SDK-free; the .cpp includes the real SDK header. class MediaTrack; @@ -103,6 +106,70 @@ private: std::vector pending_; }; +// -- may this chain be snapshotted? ------------------------------------------- +// +// A pre-park snapshot may only ever be taken from a chain NO park has touched. +// Its values are restore's sole source of truth, so a snapshot lifted from an +// already-parked chain records park state as the user's state, and every later +// restore then faithfully writes hidden/out-of-mix/FX-disabled back — permanent +// 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. + +// 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 +// never disagree about which parameter a Flag names. +inline const char* trackFlagParm(Flag f) { + switch (f) { + case Flag::ShowInTcp: return "B_SHOWINTCP"; + case Flag::ShowInMixer: return "B_SHOWINMIXER"; + case Flag::MainSend: return "B_MAINSEND"; + case Flag::FxEnable: return "I_FXEN"; + } + return "B_SHOWINTCP"; // unreachable; keeps the compiler quiet +} + +// The live values of exactly the flags `parkFlags` would write, in that order. +std::vector readParkFlagValues(MediaTrack* tr, const std::vector& 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& parkFlags, + const std::vector& 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; + return true; +} + +enum class ParkAction { + SnapshotThenPark, // clean chain — capture the pre-park truth, then park + ParkOnly, // still parked; the held snapshot IS the pre-park truth + Refuse, // no snapshot AND a parked chain — the truth is gone +}; + +// Refusing leaves the track exactly as found: no flag write, no FX-offline +// enqueue, no snapshot. Nothing false is committed, so the moment the user's own +// edit puts the chain back somewhere trustworthy the next park captures it +// correctly — where a snapshot of the parked state would have closed that door +// for good. +inline ParkAction decidePark(bool haveSnapshot, bool chainReadsParked) { + if (haveSnapshot) return ParkAction::ParkOnly; + 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"; +} + // The FX half of a snapshot, with the keying it must be read back under. struct PreParkFx { std::vector states; diff --git a/tests/test_view_fx_park.cpp b/tests/test_view_fx_park.cpp index 8dec3ac..a62dea1 100644 --- a/tests/test_view_fx_park.cpp +++ b/tests/test_view_fx_park.cpp @@ -4,7 +4,8 @@ // 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. +// a cancel must not strand the pre-park FX state it was the last record of; and +// a pre-park snapshot is never taken from a chain a park has already touched. #include "../src/shell/view/view_fx_park.h" @@ -233,6 +234,67 @@ static void testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim() { CHECK(rebuilt.states == snap.fxOffline); } +// -- may this chain be snapshotted? ------------------------------------------- +// +// The pre-park snapshot is restore's only source of truth, so one taken from an +// already-parked chain makes every later restore write hidden/out-of-mix/ +// FX-disabled back, permanently. The park site cannot infer a clean chain from +// an absent snapshot — discardDeferredFxParks drops intents whose flag writes +// already landed — so the chain itself has to be asked. + +static void testAChainSittingAtEveryValueTheParkWouldWriteReadsAsParked() { + // The four zeros are what a parked track's driven flags actually read; if + // makeParkPlan ever writes something else, this is the test that says so. + const TrackPlan park = makeParkPlan("{A}", /*fxCount=*/0); + + CHECK(park.flags.size() == 4); + CHECK(parkFlagsAlreadyApplied(park.flags, {0, 0, 0, 0})); +} + +static void testOneFlagStillAtTheUsersValueMeansNoParkReachedTheChain() { + const TrackPlan park = makeParkPlan("{A}", /*fxCount=*/0); + + // Flag order is ShowInTcp, ShowInMixer, MainSend, FxEnable — each alone is + // enough to prove the chain was never parked. + CHECK(!parkFlagsAlreadyApplied(park.flags, {1, 0, 0, 0})); + CHECK(!parkFlagsAlreadyApplied(park.flags, {0, 1, 0, 0})); + CHECK(!parkFlagsAlreadyApplied(park.flags, {0, 0, 1, 0})); + CHECK(!parkFlagsAlreadyApplied(park.flags, {0, 0, 0, 1})); +} + +static void testAnEmptyOrMismatchedPlanProvesNothing() { + const TrackPlan park = makeParkPlan("{A}", /*fxCount=*/0); + + CHECK(!parkFlagsAlreadyApplied({}, {})); + CHECK(!parkFlagsAlreadyApplied(park.flags, {0, 0, 0})); +} + +static void testACleanChainIsSnapshottedThenParked() { + CHECK(decidePark(/*haveSnapshot=*/false, /*chainReadsParked=*/false) == + ParkAction::SnapshotThenPark); +} + +static void testAHeldSnapshotIsNeverOverwrittenWhateverTheChainReads() { + // The held snapshot IS the pre-park truth, so the chain is not consulted — + // which is what lets the shell skip the live flag reads on this path. + CHECK(decidePark(/*haveSnapshot=*/true, /*chainReadsParked=*/false) == ParkAction::ParkOnly); + CHECK(decidePark(/*haveSnapshot=*/true, /*chainReadsParked=*/true) == ParkAction::ParkOnly); +} + +static void testAParkedChainWithNoSnapshotIsRefusedRatherThanResnapshotted() { + // The defect this whole section exists for: the truth is gone, so the only + // non-destructive act is to leave the track alone. Snapshotting here commits + // park state as the user's state and no later restore can undo it. + CHECK(decidePark(/*haveSnapshot=*/false, /*chainReadsParked=*/true) == ParkAction::Refuse); +} + +static void testRefusalIsReportedWithItsTrackCountAndSaysNothingWhenNoneWereRefused() { + CHECK(describeRefusedParks(0).empty()); + CHECK(describeRefusedParks(-1).empty()); + CHECK(describeRefusedParks(1).find("1 track ") != std::string::npos); + CHECK(describeRefusedParks(3).find("3 tracks ") != std::string::npos); +} + // -- re-entrancy ------------------------------------------------------------- static void testTakeDetachesEverythingAndLeavesTheQueueEmpty() { @@ -297,6 +359,14 @@ int main() { testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities(); testRestorePlanOpsRebuildTheIdentityKeyedSnapshotVerbatim(); testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim(); + testAChainSittingAtEveryValueTheParkWouldWriteReadsAsParked(); + testOneFlagStillAtTheUsersValueMeansNoParkReachedTheChain(); + testAnEmptyOrMismatchedPlanProvesNothing(); + testACleanChainIsSnapshottedThenParked(); + testAHeldSnapshotIsNeverOverwrittenWhateverTheChainReads(); + testAParkedChainWithNoSnapshotIsRefusedRatherThanResnapshotted(); + testRefusalIsReportedWithItsTrackCountAndSaysNothingWhenNoneWereRefused(); + testTakeDetachesEverythingAndLeavesTheQueueEmpty(); testIntentsArrivingDuringADrainSurviveIt(); testAReEntrantParkCancelsOnlyWhatIsStillPending();