diff --git a/docs/VERIFICATION.md b/docs/VERIFICATION.md index 4ad21bb..d3ad8ad 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. A track the tool refuses to park is NAMED in the console, and the same refusal is not reprinted on a later reapply unless the refused set changed (`src/shell/view/view_fx_park.h`'s `decidePark` / `reportRefusedParks`) +- [ ] `[verify — DAW]` For a track the console names as left unparked, follow the message's recovery IN FULL: re-enable its TCP/mixer visibility, main send and FX enable, **and set every FX in its chain online** (FX enable is the chain bypass — it does not bring an individually offlined FX back). Then switch modes twice: it parks and restores normally, with no further console line. Do the flag half ONLY on a second such track and confirm it is still refused — the FX half of the trust test, and the reason the message spells the step out (unit-tested only as a decision, never against a live chain) ## Actions and drops diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt index d3c84f4..c16fe8a 100644 --- a/src/app/CMakeLists.txt +++ b/src/app/CMakeLists.txt @@ -65,9 +65,10 @@ target_include_directories(reaper_reasampler PRIVATE ${SDK_INC} ${WDL_INC}) # The deferred FX-park queue's re-entrancy rule is pure (header-inline, no REAPER # types), so it is CTest-covered like a core/ module. Declared here rather than in a # src/shell/view/CMakeLists.txt because that directory deliberately has none — its -# TUs are compiled into this target directly. view_mode_model is linked for -# makeRestorePlan alone: the cancel path's round trip is a contract between the two, -# and pinning it against hand-built ops would not catch a change to either half. +# TUs are compiled into this target directly. view_mode_model is linked for the plan +# builders the pure half is a contract WITH — makeRestorePlan's round trip through +# the cancel path, and makeParkPlan's flag set behind the trust test; pinning either +# against hand-built ops would not catch a change to the other half. reasampler_test(view_fx_park LINK fx_offline view_mode_model) # Bank-package import: the promptless verb plus its action skin. Kept as its own diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index caa8743..a165484 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -30,6 +30,25 @@ 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 — BOTH halves + of it.** The trust test reads the four driven flags AND the FX chain, and either + reading parked alone is enough to refuse: each half is the only source of truth + for its own half of the snapshot, so one of them lying poisons the whole record. + The FX half asks "is ANY FX offline", not "every" — no read distinguishes a + park's leftover from one the user set by hand, and only "any" errs toward + committing nothing false. 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 a + console report NAMING the refused tracks (they are hidden from both the TCP and + the mixer, so a count points at nothing the user can go and find) plus the hand + recovery, printed once per changed refused set per project rather than on every + reapply. Leaving the track as found is the only non-destructive answer once the + pre-park truth is gone. **The refusal's cost is deliberate and it is not + symmetric with "hidden forever":** because it skips the FX enqueue too, a track + whose flags read parked but whose FX are still online keeps them online for + good. Refusing guarantees nothing false is written; it does not promise to + finish the park. 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 @@ -51,10 +70,18 @@ decide membership or mode rules. offline/online is enqueued and applied on a later idle tick (`view_fx_park`), so the new mode paints first. The deferral changes only WHEN the plugins move — they still unload and re-instantiate, and un-persisted internal state is still - lost. What it does change is the undo record: the offline writes land outside - 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]`. **The idle tick is not the only drain point.** Any path that + lost. What it does change is the undo record, and REAPER's behaviour there is + now OBSERVED, not assumed: the drain's `TrackFX_SetOffline` calls (the only + `TrackFX_SetOffline` in the tree) run outside any undo block, and REAPER mints + ONE implicit undo point per call — a small project measured 10 points for one + switch, 1 for the switch itself and 9 for its FX. Ctrl-Z walks them one at a + time, each step re-onlining a single FX, so one Ctrl-Z after a switch does not + bring the plugins back. It also puts the switch point BELOW every FX point: an + undo deep enough to restore the driven flags and roll the `"reasampler"` ext + state back has already re-onlined every FX, so flags, FX and snapshot move + together. The per-FX fragmentation is itself a defect to be closed by collapsing + the drain's writes into one undo point; nothing here depends on them staying + separate. **The idle tick is not the only drain point.** Any path that serializes the view model drains synchronously first (`persistViewState`, `render_in_place`), because a save landing between a restore's synchronous flag writes and its drain would record offline FX beside a model that no longer @@ -107,7 +134,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, the per-track park decision (`decidePark` over "does the model hold a snapshot" × "does the chain already read parked", with `decideParkForTrack` owning the live reads BOTH halves of that second term needs, and `trackFlagParm` the ONE `Flag` → REAPER-parameter mapping snapshot's read, park's read and restore's write all address), and the refusal report (`reportRefusedParks`, which names the tracks and holds the last-reported set so a reapply does not reprint it). **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 +157,33 @@ 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 — and never finished either, since the refusal skips the + FX enqueue too. Full contract at `discardDeferredFxParks`. +- **Which routes actually reach that no-snapshot-plus-parked-chain pair.** An undo + of a mode switch (Ctrl-Z) is NOT one of them: per the observed undo semantics + above, the flags and the FX come back at the same step. Whether the ext-state + snapshot comes back with them is INFERENCE, not observed, and untested in the + REDO direction: `[verify — DAW]` does a redo that re-parks a track's flags/FX + also restore a `view_state` that is snapshot-free for it (matching a fresh + park), or can it re-park the live track while the model still holds a stale + snapshot? What is confirmed to reach the pair: a `view_state` that PARSED but + carries no snapshot for the track (a snapshot `reconcile` pruned while its + track was out of the live enumeration, then undo/redo-restored with the track + but not the snapshot), and a hand or script edit that leaves a chain + park-shaped — including the single likeliest real case, a track with untouched + flags and just ONE hand-offlined plugin (the FX half of the disjunction + refuses alone; the flags never have to read parked too). A track the USER + keeps hidden from both panels, out of the mix and FX-bypassed reads + identically to all of them and is refused too — no lost state there at all, + which is why the report asserts no cause. + A DIFFERENT strand entirely, which the refusal report does NOT cover: an + absent or malformed `view_state` (`loadViewModel`, `ext_state_io.cpp`) is not + a parked-chain reading at all — it falls back to a DEFAULT model (active mode + Arrange, empty membership), so nothing is planned to park the track, + `decidePark` never runs, and a track saved parked stays stuck with no console + line whatsoever. - `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..7bc57fd 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -107,16 +107,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. @@ -161,11 +151,14 @@ MediaTrack* resolve(const TrackByGuid& byGuid, const std::string& guid) { // 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) { + auto read = [tr](Flag f) { + return static_cast(GetMediaTrackInfo_Value(tr, trackFlagParm(f))); + }; 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")); + snap.showInTcp = read(Flag::ShowInTcp); + snap.showInMixer = read(Flag::ShowInMixer); + snap.mainSend = read(Flag::MainSend); + snap.fxEnable = read(Flag::FxEnable); PreParkFx fx = snapshotFxOffline(tr, cancelledRestore); snap.fxOffline = std::move(fx.states); @@ -185,7 +178,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 +442,7 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject int undoMask = kApplyUndoMask; bool laneModeChanged = false; + std::vector refusedParkNames; Undo_BeginBlock2(proj); { @@ -470,13 +464,16 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject MediaTrack* tr = resolve(trackByGuid, guid); if (!tr) continue; // stale GUID — prune + // decidePark's contract; view_fx_park owns the reads. A refusal is named HERE, not held past Undo_EndBlock2. + const ParkAction action = + decideParkForTrack(tr, model.snapshot(guid) != nullptr, tp.flags); + if (action == ParkAction::Refuse) { refusedParkNames.push_back(trackDisplayName(tr)); 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 +536,10 @@ 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. + reportRefusedParks(proj, refusedParkNames); return true; } diff --git a/src/shell/view/view_fx_park.cpp b/src/shell/view/view_fx_park.cpp index 467a80e..fbe2e1f 100644 --- a/src/shell/view/view_fx_park.cpp +++ b/src/shell/view/view_fx_park.cpp @@ -5,6 +5,7 @@ #include "shell/view/view_fx_park.h" +#include // strnlen — bounded read of GetTrackName's buffer #include #include #include @@ -14,7 +15,9 @@ #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_CountTracks #define REAPERAPI_WANT_EnumProjects +#define REAPERAPI_WANT_GetMediaTrackInfo_Value #define REAPERAPI_WANT_GetTrack +#define REAPERAPI_WANT_GetTrackName #define REAPERAPI_WANT_ShowConsoleMsg #define REAPERAPI_WANT_TrackFX_GetCount #define REAPERAPI_WANT_TrackFX_GetFXGUID @@ -32,6 +35,10 @@ FxParkQueue g_queue; ReaProject* g_owner = nullptr; // the project the pending intents were enqueued against bool g_draining = false; +// The last refusal report, so an unchanged one is not reprinted on every reapply. +ReaProject* g_reportedOwner = nullptr; +std::vector g_reportedRefusals; + // Makes "one drain at a time" explicit rather than implied by the call sites. // RAII because an apply can throw and a stuck flag would silence the queue for // the rest of the session. @@ -97,8 +104,50 @@ std::string fxGuidString(MediaTrack* tr, int fx) { return std::string(buf); } +// ANY offline FX, not every — see the snapshot-source invariant (view_fx_park.h, +// src/shell/view/CLAUDE.md) for why. +bool anyFxOffline(MediaTrack* tr) { + const int fxCount = TrackFX_GetCount(tr); + for (int fx = 0; fx < fxCount; ++fx) + if (TrackFX_GetOffline(tr, fx)) return true; + return false; +} + } // namespace +// GetTrackName, not P_NAME, matching capture::trackName's reason for the same +// choice. The buffer is read bounded — the SDK documents no NUL within +// bufOut_sz. A track the report cannot name still has to appear in it, so the +// read failing yields a placeholder rather than dropping the entry. +std::string trackDisplayName(MediaTrack* tr) { + std::vector buf(1024, '\0'); + if (!GetTrackName || !GetTrackName(tr, buf.data(), static_cast(buf.size()))) + return "(name unavailable)"; + return std::string(buf.data(), strnlen(buf.data(), buf.size())); +} + +ParkAction decideParkForTrack(MediaTrack* tr, bool haveSnapshot, + const std::vector& parkFlags) { + if (haveSnapshot) return decidePark(true, false); // second term unread — skip the reads + const bool flagsRead = parkFlagsAlreadyApplied(parkFlags, [tr](Flag f) { + return static_cast(GetMediaTrackInfo_Value(tr, trackFlagParm(f))); + }); + return decidePark(false, chainReadsParked(flagsRead, anyFxOffline(tr))); +} + +void reportRefusedParks(ReaProject* proj, const std::vector& refusedNames) { + ReaProject* const owner = proj ? proj : currentProject(); + + if (!shouldReport(owner == g_reportedOwner, g_reportedRefusals, refusedNames)) return; + g_reportedOwner = owner; + g_reportedRefusals = refusedNames; + + const std::string msg = describeRefusedParks(refusedNames); + // "!SHOW:" so an unattended project-load reapply cannot force the console open + // (reaper_plugin_functions.h:6536), same as the drain's drop report. + if (!msg.empty()) ShowConsoleMsg(("!SHOW:" + msg).c_str()); +} + 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..be46868 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,119 @@ 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. +// +// A snapshot has TWO halves and the test covers both, disjunctively: the flags +// can read clean while the FX are still offline (a hand recovery that stopped at +// the flags, a reload) and vice versa. Either half reading parked poisons the +// whole snapshot, so either half alone refuses. + +// 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 +} + +// Does the track already sit at every value this park would write? Then a park +// reached it. `readFlag` answers one Flag's live value; templating on it lets the +// live path compare as it reads (no per-track vector) while the tests drive the +// same fold from a table, keyed by Flag rather than by plan order. An empty plan +// is evidence of nothing and answers false. +template +bool parkFlagsAlreadyApplied(const std::vector& parkFlags, ReadFlag readFlag) { + if (parkFlags.empty()) return false; + for (const TrackFlagOp& op : parkFlags) + if (readFlag(op.flag) != op.value) return false; + return true; +} + +// DISJUNCTIVE fold — rationale is the snapshot-source invariant above (also +// src/shell/view/CLAUDE.md). +inline bool chainReadsParked(bool parkFlagsRead, bool anyFxOffline) { + return parkFlagsRead || anyFxOffline; +} + +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; +} + +// `decidePark` over the live track: the four driven flags AND the FX chain, which +// a snapshot taken here would record both halves of. Skips the reads on the +// ParkOnly path, where decidePark ignores the second term anyway. +ParkAction decideParkForTrack(MediaTrack* tr, bool haveSnapshot, + const std::vector& parkFlags); + +// "Track N" for an unnamed track (GetTrackName, matching capture::trackName's +// choice of API for the same reason). Exported so a caller holding the handle +// only briefly (view.cpp's park loop, before Undo_EndBlock2/TrackList_AdjustWindows +// invalidate it) can capture the name at resolve time rather than the handle. +std::string trackDisplayName(MediaTrack* tr); + +// The console report for the tracks one apply refused, empty when none. NAMES +// them: a refused track is hidden from both the TCP and the mixer, so a count +// alone points at nothing the user can go and find. It asserts no cause — the +// pair it detects has several routes (src/shell/view/CLAUDE.md), and only some of +// them are anyone's mistake. +inline std::string describeRefusedParks(const std::vector& trackNames) { + if (trackNames.empty()) return {}; + std::string msg = "ReaSampler: " + std::to_string(trackNames.size()) + " track" + + (trackNames.size() == 1 ? "" : "s") + + " left unparked -- no stored pre-park state, and a live state that " + "already reads as parked; parking now would record that as your own " + "state.\n"; + for (const std::string& name : trackNames) msg += " " + name + "\n"; + msg += "To bring a track back under mode control: restore its TCP and mixer visibility, " + "its main send and its FX enable, AND set every FX in its chain online -- FX enable " + "is the chain bypass and does not bring an individually offlined FX back. The next " + "mode switch will then park it correctly.\n"; + return msg; +} + +// Pure gate behind reportRefusedParks' one-per-changed-set memo: prints (i.e. +// updates the memo) on any change of owner or of the named set, stays silent on +// an exact repeat. An empty `now` still counts as a change from a non-empty +// `last` — that update itself prints nothing (describeRefusedParks({}) is +// empty) but resets the memo so a later refusal is never mistaken for a repeat +// of one that already healed. +inline bool shouldReport(bool sameOwnerAsLast, const std::vector& last, + const std::vector& now) { + return !(sameOwnerAsLast && last == now); +} + +// Prints describeRefusedParks for `refusedNames` (captured by the caller while its +// track handles are still fresh — see view.cpp's park loop), ONCE per changed set +// per project: the same refusal recurs on every reapply — every project open, +// every tag/untag — and repeating it verbatim is noise the user cannot act on any +// better the second time. Gated by shouldReport. +void reportRefusedParks(ReaProject* proj, const std::vector& refusedNames); + // 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..ba2112f 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,131 @@ static void testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim() { CHECK(rebuilt.states == snap.fxOffline); } +// -- may this chain be snapshotted? ------------------------------------------- +// +// Rationale: the snapshot-source invariant (view_fx_park.h, this directory's +// CLAUDE.md). + +// The live values the fold reads, keyed by Flag — so an assertion names the flag +// it varies rather than a position in makeParkPlan's op order. +struct FlagValues { + int showInTcp = 0, showInMixer = 0, mainSend = 0, fxEnable = 0; + + int operator()(Flag f) const { + switch (f) { + case Flag::ShowInTcp: return showInTcp; + case Flag::ShowInMixer: return showInMixer; + case Flag::MainSend: return mainSend; + case Flag::FxEnable: return fxEnable; + } + return -1; + } +}; + +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, FlagValues{0, 0, 0, 0})); +} + +static void testOneFlagStillAtTheUsersValueMeansNoParkReachedTheChain() { + const TrackPlan park = makeParkPlan("{A}", /*fxCount=*/0); + + // Each case varies exactly one flag, so together they also prove all four are + // in the plan: a missing op makes its case read parked and fail here. + CHECK(!parkFlagsAlreadyApplied(park.flags, FlagValues{1, 0, 0, 0})); + CHECK(!parkFlagsAlreadyApplied(park.flags, FlagValues{0, 1, 0, 0})); + CHECK(!parkFlagsAlreadyApplied(park.flags, FlagValues{0, 0, 1, 0})); + CHECK(!parkFlagsAlreadyApplied(park.flags, FlagValues{0, 0, 0, 1})); +} + +static void testAnEmptyPlanProvesNothing() { + CHECK(!parkFlagsAlreadyApplied(std::vector{}, FlagValues{0, 0, 0, 0})); +} + +static void testEitherHalfOfTheChainReadingParkedIsEnoughToRefuse() { + // Disjunction pinned; rationale at chainReadsParked (view_fx_park.h). + CHECK(!chainReadsParked(/*parkFlagsRead=*/false, /*anyFxOffline=*/false)); + CHECK(chainReadsParked(/*parkFlagsRead=*/true, /*anyFxOffline=*/false)); + CHECK(chainReadsParked(/*parkFlagsRead=*/false, /*anyFxOffline=*/true)); + CHECK(chainReadsParked(/*parkFlagsRead=*/true, /*anyFxOffline=*/true)); +} + +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 testRefusalNamesEveryRefusedTrackAndSaysNothingWhenNoneWere() { + CHECK(describeRefusedParks({}).empty()); + + const std::string one = describeRefusedParks({"Bass"}); + CHECK(one.find("1 track ") != std::string::npos); + CHECK(one.find("Bass") != std::string::npos); + + const std::string two = describeRefusedParks({"Bass", "Drum bus"}); + CHECK(two.find("2 tracks ") != std::string::npos); + CHECK(two.find("Bass") != std::string::npos); + CHECK(two.find("Drum bus") != std::string::npos); +} + +static void testRefusalRecoveryNamesThePerFxHalfAndAssertsNoCause() { + const std::string msg = describeRefusedParks({"Bass"}); + + // I_FXEN is the chain bypass: a user who restores only the four flags leaves + // every individually offlined FX offline and walks straight back into a + // refusal, so the recovery has to spell the per-FX step out. + CHECK(msg.find("every FX in its chain online") != std::string::npos); + // And it must not name a cause: the pair has several routes, and on a track + // the user themselves keeps hidden/bypassed there was no lost state at all. + CHECK(msg.find("undo") == std::string::npos); + CHECK(msg.find("lost") == std::string::npos); +} + +// -- the refusal memo's print-suppression gate -------------------------------- +// +// reportRefusedParks' own memo (owner + last-reported names, both REAPER-side +// statics) can't be driven from here, but the pure gate behind it can: prints +// (i.e. updates the memo) on any change, stays silent on an exact repeat, and +// treats a fresh empty set as a change too, so a later real refusal is never +// mistaken for a repeat of one that already healed. + +static void testUnchangedOwnerAndSetStaysSilent() { + CHECK(!shouldReport(/*sameOwnerAsLast=*/true, {"Bass"}, {"Bass"})); +} + +static void testADifferentNamedSetReports() { + CHECK(shouldReport(/*sameOwnerAsLast=*/true, {"Bass"}, {"Bass", "Drum bus"})); +} + +static void testADifferentOwnerReportsEvenWithTheSameNames() { + // Two alternating project tabs must not suppress each other's first refusal. + CHECK(shouldReport(/*sameOwnerAsLast=*/false, {"Bass"}, {"Bass"})); +} + +static void testAFreshEmptySetReportsAndResetsTheMemo() { + CHECK(shouldReport(/*sameOwnerAsLast=*/true, {"Bass"}, {})); + // Already empty, same owner: nothing changed, stays silent. + CHECK(!shouldReport(/*sameOwnerAsLast=*/true, {}, {})); +} + // -- re-entrancy ------------------------------------------------------------- static void testTakeDetachesEverythingAndLeavesTheQueueEmpty() { @@ -297,6 +423,21 @@ int main() { testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities(); testRestorePlanOpsRebuildTheIdentityKeyedSnapshotVerbatim(); testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim(); + testAChainSittingAtEveryValueTheParkWouldWriteReadsAsParked(); + testOneFlagStillAtTheUsersValueMeansNoParkReachedTheChain(); + testAnEmptyPlanProvesNothing(); + testEitherHalfOfTheChainReadingParkedIsEnoughToRefuse(); + testACleanChainIsSnapshottedThenParked(); + testAHeldSnapshotIsNeverOverwrittenWhateverTheChainReads(); + testAParkedChainWithNoSnapshotIsRefusedRatherThanResnapshotted(); + testRefusalNamesEveryRefusedTrackAndSaysNothingWhenNoneWere(); + testRefusalRecoveryNamesThePerFxHalfAndAssertsNoCause(); + + testUnchangedOwnerAndSetStaysSilent(); + testADifferentNamedSetReports(); + testADifferentOwnerReportsEvenWithTheSameNames(); + testAFreshEmptySetReportsAndResetsTheMemo(); + testTakeDetachesEverythingAndLeavesTheQueueEmpty(); testIntentsArrivingDuringADrainSurviveIt(); testAReEntrantParkCancelsOnlyWhatIsStillPending(); diff --git a/tests/test_view_mode_model.cpp b/tests/test_view_mode_model.cpp index dbdcc5f..a9e4427 100644 --- a/tests/test_view_mode_model.cpp +++ b/tests/test_view_mode_model.cpp @@ -756,9 +756,15 @@ namespace { using LiveFlags = std::map; // Runs one applyMode-equivalent toggle against `vm` + `live`. `guard` selects the -// fixed (snapshot-once) behavior vs. the original buggy (snapshot-every-park) one. +// 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 mirrors decidePark's three ways: a held snapshot parks +// without recapturing, a chain that already reads parked with no snapshot is +// REFUSED (left exactly as found), and only a clean chain is snapshotted. The one +// `live` flag stands in for the whole park-flag-plus-FX fold the shell reads — +// the model half of the defect is identical either way. void simulateApplyMode(ViewModeModel& vm, LiveFlags& live, const FolderTree& tree, const std::string& target, bool guard) { TogglePlan plan = vm.planToggle(tree, target); @@ -767,7 +773,14 @@ void simulateApplyMode(ViewModeModel& vm, LiveFlags& live, const FolderTree& tre for (const auto& tp : plan.park) { if (tp.flags.empty()) continue; const std::string& guid = tp.flags.front().guid; - if (!guard || vm.snapshot(guid) == nullptr) { + if (guard) { + if (vm.snapshot(guid) == nullptr) { + if (live[guid] == 0) continue; // reads parked, no snapshot — refuse + TrackSnapshot snap; + snap.showInTcp = live[guid]; + vm.storeSnapshot(guid, snap); + } + } else { TrackSnapshot snap; snap.showInTcp = live[guid]; // capture the LIVE visible flag vm.storeSnapshot(guid, snap); @@ -868,6 +881,74 @@ 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. + +static void testReloadedModelWithParkedTracksRefusesRatherThanResnapshotting() { + const FolderTree tree = nestedTree(); + + // GUARDED (fixed shell). + { + 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); + + // 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 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 + } +} + // =========================================================================== // D2 two-canvas lane extension tests // =========================================================================== @@ -2125,6 +2206,7 @@ int main() { testUntaggedLeavesManagedByModeSystem(); testTaggedLeafBehaviorUnchangedWithUntagged(); testNestedToggleSnapshotSurvivesRepark(); + testReloadedModelWithParkedTracksRefusesRatherThanResnapshotting(); testReconcilePrunesOrphanedSnapshots(); testReconcileFullLiveSetIsNoOp(); testReconcileThenReparkLifecycleIntact();