From 4b306dd436f5fe209b286c25ce301db59100ab6d Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Wed, 5 Aug 2026 17:34:49 -0400 Subject: [PATCH] Name refused tracks before Undo_EndBlock2, not after; tighten the route-list doc; pin the refusal memo's gate Fixes D (handle lifetime), C (ASCII dashes), G (comment compression), I (shouldReport extracted + tested); CLAUDE.md route list corrected per A/B/H. E pushed back -- cross-module GUID plumbing is architectural, not a targeted fix. --- src/shell/view/CLAUDE.md | 32 ++++++++++++++++-------- src/shell/view/view.cpp | 8 +++--- src/shell/view/view_fx_park.cpp | 32 +++++++++--------------- src/shell/view/view_fx_park.h | 39 +++++++++++++++++++++--------- tests/test_view_fx_park.cpp | 43 +++++++++++++++++++++++++++------ 5 files changed, 100 insertions(+), 54 deletions(-) diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index b3c619d..a165484 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -162,16 +162,28 @@ applies the resulting lane state to live tracks. 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 is NOT one of them: per the observed undo semantics above, the - flags, the FX and the ext-state snapshots all come back at the same step, and - stopping short of it leaves the snapshot in place (so the next park is a - `ParkOnly`, not a refusal). What remains: a model replaced without the project - rolling back with it (an unreadable or absent `view_state` against an `.rpp` - saved parked), a snapshot `reconcile` pruned while its track was out of the live - enumeration and then undo-restored with the track, and a hand or script edit - that leaves a chain park-shaped. 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. + 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 40e41ba..7bc57fd 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -442,7 +442,7 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject int undoMask = kApplyUndoMask; bool laneModeChanged = false; - std::vector refusedParks; + std::vector refusedParkNames; Undo_BeginBlock2(proj); { @@ -464,10 +464,10 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject MediaTrack* tr = resolve(trackByGuid, guid); if (!tr) continue; // stale GUID — prune - // Full contract at decidePark; view_fx_park owns the live reads. + // 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) { refusedParks.push_back(tr); continue; } + 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 @@ -539,7 +539,7 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject // 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, refusedParks); + reportRefusedParks(proj, refusedParkNames); return true; } diff --git a/src/shell/view/view_fx_park.cpp b/src/shell/view/view_fx_park.cpp index 21c13fe..fbe2e1f 100644 --- a/src/shell/view/view_fx_park.cpp +++ b/src/shell/view/view_fx_park.cpp @@ -104,11 +104,8 @@ std::string fxGuidString(MediaTrack* tr, int fx) { return std::string(buf); } -// ANY offline FX, not every one: a park offlines the whole chain, but a partial -// undo or a half-finished hand recovery leaves some back online, and there is no -// read that tells a park's leftover offline from one the user set. So the test -// refuses in the direction that commits nothing false — the cost is a chain -// carrying a hand-offlined plugin that the tool then declines to park. +// 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) @@ -116,11 +113,12 @@ bool anyFxOffline(MediaTrack* tr) { return false; } -// "Track N" for an unnamed track, matching REAPER's own convention (GetTrackName, -// not P_NAME, for the same reason capture::trackName picks it). 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. +} // 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()))) @@ -128,8 +126,6 @@ std::string trackDisplayName(MediaTrack* tr) { return std::string(buf.data(), strnlen(buf.data(), buf.size())); } -} // namespace - ParkAction decideParkForTrack(MediaTrack* tr, bool haveSnapshot, const std::vector& parkFlags) { if (haveSnapshot) return decidePark(true, false); // second term unread — skip the reads @@ -139,18 +135,14 @@ ParkAction decideParkForTrack(MediaTrack* tr, bool haveSnapshot, return decidePark(false, chainReadsParked(flagsRead, anyFxOffline(tr))); } -void reportRefusedParks(ReaProject* proj, const std::vector& refused) { +void reportRefusedParks(ReaProject* proj, const std::vector& refusedNames) { ReaProject* const owner = proj ? proj : currentProject(); - std::vector names; - names.reserve(refused.size()); - for (MediaTrack* tr : refused) names.push_back(trackDisplayName(tr)); - - if (owner == g_reportedOwner && names == g_reportedRefusals) return; + if (!shouldReport(owner == g_reportedOwner, g_reportedRefusals, refusedNames)) return; g_reportedOwner = owner; - g_reportedRefusals = names; + g_reportedRefusals = refusedNames; - const std::string msg = describeRefusedParks(names); + 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()); diff --git a/src/shell/view/view_fx_park.h b/src/shell/view/view_fx_park.h index 0ee33f1..be46868 100644 --- a/src/shell/view/view_fx_park.h +++ b/src/shell/view/view_fx_park.h @@ -147,11 +147,8 @@ bool parkFlagsAlreadyApplied(const std::vector& parkFlags, ReadFlag return true; } -// The two halves folded. DISJUNCTIVE, and that is the whole point: each half is -// the sole source of truth for its own half of the snapshot, so one of them -// reading parked poisons the snapshot whatever the other says. An AND would let a -// flags-clean/FX-offline chain — what the documented hand recovery produces after -// its first step — bake the parked FX states in as the user's. +// 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; } @@ -178,6 +175,12 @@ inline ParkAction decidePark(bool haveSnapshot, bool chainReadsParked) { 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 @@ -187,22 +190,34 @@ inline std::string describeRefusedParks(const std::vector& trackNam 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 " + " 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 " + "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; } -// Prints describeRefusedParks for `refused` (track handles, named here), 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. -void reportRefusedParks(ReaProject* proj, const std::vector& refused); +// 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 { diff --git a/tests/test_view_fx_park.cpp b/tests/test_view_fx_park.cpp index 8e66055..ba2112f 100644 --- a/tests/test_view_fx_park.cpp +++ b/tests/test_view_fx_park.cpp @@ -236,11 +236,8 @@ static void testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim() { // -- 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. +// 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. @@ -283,9 +280,7 @@ static void testAnEmptyPlanProvesNothing() { } static void testEitherHalfOfTheChainReadingParkedIsEnoughToRefuse() { - // The disjunction, pinned: a snapshot has a flag half and an FX half, and each - // is the only source of truth for its own. An AND here would re-open the - // defect one layer down — flags clean, FX still offline, snapshotted as truth. + // 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)); @@ -337,6 +332,33 @@ static void testRefusalRecoveryNamesThePerFxHalfAndAssertsNoCause() { 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() { @@ -411,6 +433,11 @@ int main() { testRefusalNamesEveryRefusedTrackAndSaysNothingWhenNoneWere(); testRefusalRecoveryNamesThePerFxHalfAndAssertsNoCause(); + testUnchangedOwnerAndSetStaysSilent(); + testADifferentNamedSetReports(); + testADifferentOwnerReportsEvenWithTheSameNames(); + testAFreshEmptySetReportsAndResetsTheMemo(); + testTakeDetachesEverythingAndLeavesTheQueueEmpty(); testIntentsArrivingDuringADrainSurviveIt(); testAReEntrantParkCancelsOnlyWhatIsStillPending();