From 8c06383fe748e0e0df38750e9957cea01d377f02 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 04:32:26 -0400 Subject: [PATCH] Collapse a Design View mode switch to ONE undo point: FX writes run inline in applyMode's block, deferred park queue deleted --- docs/TODO.md | 21 +- docs/VERIFICATION.md | 11 +- src/app/main.cpp | 13 - src/shell/actions/design_view_actions.cpp | 8 - src/shell/capture/render_in_place.cpp | 2 - src/shell/view/CLAUDE.md | 170 ++++++----- src/shell/view/view.cpp | 138 ++++----- src/shell/view/view.h | 29 +- src/shell/view/view_fx_park.cpp | 305 +++++++------------ src/shell/view/view_fx_park.h | 277 ++++------------- tests/test_view_fx_park.cpp | 343 ++-------------------- 11 files changed, 392 insertions(+), 925 deletions(-) diff --git a/docs/TODO.md b/docs/TODO.md index 1585b3d..ffe79f7 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -867,23 +867,24 @@ public accessor surface. Doing that in the same commit that changed the byte for the golden test literals pin would roll a format change and a codec extraction together, which is the riskier order. -**Also over the bar, blocked differently.** `src/shell/view/view.cpp` measures -**625 lines** (re-measured after the deferred FX-park split took `fxGuidString`, -`liveFxGuids` and the park/restore FX writes out into `view_fx_park`). Its remaining -seam is blocked not by a private-state/friend question but by a build file another -team owns: `src/shell/view/` has no `CMakeLists.txt` of its own today — a new TU -there costs one `target_sources` line in `src/app/CMakeLists.txt` instead. +**`src/shell/view/view.cpp` is no longer over the bar.** It measures **594 lines** +(`wc -l`, re-measured after the undo collapse moved one track's whole park/restore — +snapshot, flags and per-FX offline — into `view_fx_park`'s `parkTrack`/ +`restoreTrack`). Recorded because the entry above used to name it as a second +over-ceiling file: if it grows again, note that a further seam there is blocked +not by a private-state/friend question but by a build file another team owns — +`src/shell/view/` has no `CMakeLists.txt` of its own today, so a new TU costs one +`target_sources` line in `src/app/CMakeLists.txt` instead. **Priority / risk.** Not stated. **Done looks like.** `view_mode_model.cpp`'s JSON codec is extracted into its own `view_state_codec` TU (with the friend/accessor question resolved deliberately, not -sidestepped), dropping the file under the ~600-line ceiling; `view.cpp`'s own path is -unblocked once the build-file ownership question is resolved. +sidestepped), dropping the file under the ~600-line ceiling. -## FX-GUID stability for `applyRestore` is unverified in the DAW +## FX-GUID stability for the park's restore is unverified in the DAW -**Context.** The Design View park/restore FX keying (`applyRestore`, +**Context.** The Design View park/restore FX keying (`restoreTrack`, `src/shell/view/view_fx_park.cpp`) rests on `TrackFX_GetFXGUID` returning an identity that survives a chain reorder while a track is parked. SWS issue #802 reports that after `SNM_MoveOrRemoveTrackFX` reorders a chain, the FXID lines do not follow the plugin diff --git a/docs/VERIFICATION.md b/docs/VERIFICATION.md index 8bb673e..51173c3 100644 --- a/docs/VERIFICATION.md +++ b/docs/VERIFICATION.md @@ -48,11 +48,12 @@ Checks for Θ, Ξ, Ψ, Ε, Ρ, Γ, and Ω work that no unit test can close. Buil - [ ] 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]` **The undo collapse.** In a project with several FX across several tracks, switch modes once and count the undo points REAPER shows (Ctrl-Z tooltip / undo history): expect exactly TWO — `ReaSampler: Design View FX state` above `ReaSampler: activate view` — never one per FX (`src/shell/view/view_fx_park.h`'s `fxParkUndoClose`) -- [ ] `[verify — DAW]` Then press Ctrl-Z once: every FX re-onlines in that one step and the flags stay parked. A second Ctrl-Z rolls the flags and the ext state back. The FX point must sit ABOVE the switch point, never below it and never folded into it. Verify the EFFECT, not merely that a point exists in the history — a mask narrower than `UNDO_STATE_FX` covers would still show an entry there while doing nothing to FX state on Ctrl-Z, which would look like a pass and isn't (`src/shell/view/CLAUDE.md` §Invariants — the undo-ordering premise, still `[verify — DAW]` until this item and the next close it) -- [ ] `[verify — DAW]` Open a project saved in Design mode with every inactive leaf's FX already offline, and check the undo history: the load-tick reapply then writes no FX state, so it must add NO undo point at all — the discard form. This only holds when the saved state was actually fully parked — a first open after tagging a new leaf, an FX added to a parked track while it was offline, or a plugin hand-onlined since the last save all have real FX state to write and legitimately mint one correct `ReaSampler: Design View FX state` point; don't record those as a false failure. An entry appearing on a project that WAS fully parked means the discard is not working (`src/shell/view/view_fx_park.h`'s `fxParkUndoClose`) -- [ ] `[verify — DAW]` Whether `Undo_EndBlock2` tolerates a `ReaProject*` that closed between it and its `Undo_BeginBlock2`, AND which of two undocumented block-nesting models REAPER implements — a GLOBAL counter (an unbalanced block swallows every later action, in ANY project, into one point) or a PER-PROJECT one (an unbalanced block on a now-dead project costs nothing). The drain closes its block unconditionally on the project-gone abort path on the assumption the global model applies; record which model is actually observed, not just whether the call survives. Reproduce by closing a project tab while a mode switch's FX drain is in flight (`src/shell/view/view_fx_park.cpp`'s `FxParkUndoBlock`) -- [ ] `[verify — DAW]` **Block-inside-drain.** The nesting audit at `drainDeferredFxParks` only covers the drain opening inside an already-open caller block; the pumping premise it documents (`FxParkQueue::take`) also makes the inverse possible — a re-entrant `applyMode` (an action fired mid-drain) or a re-entrant `OnTimer` -> `bankPanelRefresh` -> `mintManagedLanes` opening ITS OWN `Undo_BeginBlock2`/`Undo_EndBlock2` pair while the drain's block is still open. `g_draining` blocks only a second `drainDeferredFxParks` call, not this. Reproduce by triggering a mode-switch action or a lane-mint mid-drain (a heavy FX chain widens the window) and check the undo history for a split or misordered point (`src/shell/view/view_fx_park.h`'s `drainDeferredFxParks`) +- [ ] `[verify — DAW]` **ONE switch, ONE point.** In a project with several FX across several tracks, switch modes once and count the undo points REAPER shows (Ctrl-Z tooltip / undo history): expect exactly ONE, `ReaSampler: activate view` — never two, and never one per FX. Then press Ctrl-Z once: the flags unpark and EVERY FX comes back online, in that single step. Verify the EFFECT, not merely that one entry exists — a mask that failed to pick up `UNDO_STATE_FX` would still show one correct-looking entry while leaving every plugin offline, which reads as a pass and isn't. Then Ctrl-Y: the redo re-parks all of it, also in one step (`src/shell/view/CLAUDE.md` §Invariants — "ONE mode switch is ONE undo point") +- [ ] `[verify — DAW]` **Where the `view_state` lands on undo AND redo.** `persistViewState` writes the ext state AFTER `applyMode`'s block closes, so the point never saw the new `view_state` — the undo direction is expected to come out right by position, the redo direction is the open one. After the Ctrl-Z above, read the footer segment: it must show the mode the user came FROM, and a further switch must bank solos under that mode id. Then Ctrl-Y and read it again: if the tracks re-park while the footer still shows the pre-switch mode, the redo restored the stale ext state and model-vs-project is out of step. Do NOT fix by moving `persistViewState` inside the block — it can open a modal Save-As (`src/shell/view/CLAUDE.md` §Gotchas) +- [ ] `[verify — DAW]` **No point on open.** Open a project saved in Design mode with every inactive leaf already fully parked (flags AND FX), and check the undo history: the load-tick reapply writes nothing, so it must add NO undo point at all — the `("", 0)` discard form. This only holds when the saved state really was fully parked; a first open after tagging a new leaf, an FX added to a parked track, or a plugin hand-onlined since the last save all have real state to write and legitimately mint one correct point. An entry appearing on a project that WAS fully parked means the discard is not working — and since `mintManagedLanes`' no-op path rests on the same idiom, check that alongside it (`src/shell/view/view.h`'s `applyMintsUndoPoint`) +- [ ] `[verify — DAW]` **The hitch now sits on the switch.** On a project with heavy plugins (convolution, a loaded sampler) across several parked tracks, time the mode toggle: by design the action does not return until every plugin has unloaded/reloaded. Report roughly how long, and whether the UI recovers cleanly afterwards — the whole write phase runs under one `PreventUIRefresh(1)` hold (`src/shell/view/CLAUDE.md` §Invariants — the documented caveat) +- [ ] `[verify — DAW]` **Nested blocks around the FX writes.** Two directions, both undocumented in the SDK header. (a) `render_in_place` calls `applyMode` inside its OWN undo block, so its single `ReaSampler: render selected track to a new track` point should now absorb the reapply's FX moves — run that verb from Design mode over a track with FX and confirm ONE point, not two, and that Ctrl-Z reverses all of it. (b) The inverse: fire a mode-switch action, or let `OnTimer` -> `bankPanelRefresh` -> `mintManagedLanes` run, WHILE a switch's plugin loads are in flight (a heavy chain widens the window), and check the history for a split or misordered point. Nothing guards (b) (`src/shell/view/CLAUDE.md` §Gotchas) +- [ ] `[verify — DAW]` **A→B→A costs two full cycles.** Toggle Arrange→Design→Arrange quickly on a project with heavy plugins: every plugin unloads and reloads twice, where the retired intent queue made a fast double-toggle free. Confirm the END STATE is still correct (every FX online, every driven flag back at its captured value) and report whether the doubled cost is tolerable in practice (`src/shell/view/CLAUDE.md` §Invariants — the A→B→A bullet) - [ ] `[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/main.cpp b/src/app/main.cpp index 4dd9a05..8d792f8 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -39,7 +39,6 @@ #include "shell/panel/panel_window.h" // panel lifecycle (init/toggle/open-query/shutdown) #include "shell/persist/session.h" // ReaSamplerSession #include "shell/view/view.h" // reconcileManagedLanes / applyMode -#include "shell/view/view_fx_park.h" // the mode switch's deferred FX park namespace capture = reasampler::capture; @@ -191,24 +190,12 @@ 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/actions/design_view_actions.cpp b/src/shell/actions/design_view_actions.cpp index 77b24e7..7bf8ab9 100644 --- a/src/shell/actions/design_view_actions.cpp +++ b/src/shell/actions/design_view_actions.cpp @@ -26,7 +26,6 @@ #include "shell/capture/track_guid.h" // shared MediaTrack* -> canonical GUID key #include "shell/panel/panel_window.h" // bankPanelInvalidate — footer toggle repaint #include "shell/view/view.h" // applyMode + mintManagedLanes (D2 shell) -#include "shell/view/view_fx_park.h" // drainDeferredFxParks — see persistViewState #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_CountSelectedTracks @@ -145,13 +144,6 @@ enum class PersistScope { Full, ViewOnly }; // the project is unsaved, prompts Save-As first (mirrors the flow capture uses) — // DAW-ONLY: Main_SaveProject(proj, true) blocks until the dialog is dismissed. void persistViewState(PersistScope scope) { - // BEFORE the model is serialized, and before the Save-As below can write a - // .rpp: a deferred FX restore leaves the chain offline while the model has - // already dropped the snapshot that would replan it. Why that combination is - // unrecoverable on reopen is at drainDeferredFxParks. Runs for BOTH scopes — - // the narrowed save still writes the key the half-applied park contradicts. - drainDeferredFxParks(); - if (!g_session->view().membership().empty()) { ReaProject* proj = EnumProjects(-1, nullptr, 0); if (proj) { diff --git a/src/shell/capture/render_in_place.cpp b/src/shell/capture/render_in_place.cpp index 247e908..d6e343e 100644 --- a/src/shell/capture/render_in_place.cpp +++ b/src/shell/capture/render_in_place.cpp @@ -22,7 +22,6 @@ #include "shell/panel/panel_input.h" // bankPanelTailSetting #include "shell/persist/session.h" #include "shell/view/view.h" // applyMode / mintManagedLanes -#include "shell/view/view_fx_park.h" // drainDeferredFxParks #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_CountTrackMediaItems @@ -212,7 +211,6 @@ void RunRenderTrackInPlace(ReaSamplerSession& session) { // Persist outside the block. The offline render's own save gate already forced a // saved project, so the Save-As-guarded persist the Design View actions need // cannot have anything to prompt for here. - drainDeferredFxParks(); // the reapply above may have deferred a restore — see the contract there session.saveToActiveProject(); if (!placed) { diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index db63763..1a34e76 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -37,15 +37,15 @@ decide membership or mode rules. 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 + a saved project, a hand-edited chain and an undo/redo can each present one; the + Gotchas below enumerate the routes that still reach it. A park meeting that pair + REFUSES: no flag write, no FX write, 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 + symmetric with "hidden forever":** because it skips the FX half 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`. @@ -66,43 +66,51 @@ decide membership or mode rules. active mode — stateful plugins (convolution, loaded samplers, tail-holding effects) re-initialize on return (load hitch, un-persisted internal state lost). Accepted cost of the CPU reclaim; surfaced at the toggle affordance (tooltip). - **The hitch no longer sits on the switch's synchronous path:** per-FX - 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. What was MEASURED, before the fix - below: a `TrackFX_SetOffline` made outside any undo block mints ONE implicit + **The hitch sits on the switch's synchronous path, deliberately.** Per-FX + offline/online runs inside `applyMode`'s own undo block, between the flag + writes and `Undo_EndBlock2`, so an action-driven switch does not return until + every plugin has moved. That cost is the price of the invariant below, and it + was accepted as such. +- **ONE mode switch is ONE undo point.** Everything the switch wrote into the + project — the driven flags, every per-FX offline/online, the lane state — + rolls back in a single Ctrl-Z. (The `view_state` ext-state write is NOT among + them; it lands after the block, see Gotchas.) What was MEASURED (before any + fix): a `TrackFX_SetOffline` made outside any undo block mints ONE implicit undo point per call, and a small project showed 10 points for one switch — 1 - for the switch itself and 9 for its FX — which Ctrl-Z then walked one FX at a - time. **The drain now opens its own undo block** (`fxParkUndoClose`, - `UNDO_STATE_FX` only, no other domain) on the PREMISE that an explicit block - suppresses those per-call implicit points. That premise, and everything below - that follows from it — one drain being one point, a switch costing two, a - no-write drain minting nothing (`fxParkUndoClose` owns why), and ONE Ctrl-Z - re-onlining every FX at once with a second rolling the flags and the ext state - back — is INFERENCE from the code's design, not observed: `docs/VERIFICATION.md` - §"Mode switching" (items 51–53) is where this gets checked against REAPER. The - ordering property the pre-fix measurement established is DELIBERATELY - PRESERVED BY DESIGN regardless: the drain's block is its own and never the - switch's, so the FX point is meant to sit ABOVE the switch point, with flags, - FX and snapshot moving together on undo. Folding the FX writes into the - switch's own block, or making them undo-silent, would break that and is not an - option here. - **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 - carries the snapshot to replan them — unrecoverable on reopen. So an - action-driven switch does pay the FX hitch before it returns; the repaint and - the undo block have both closed by then, which is what the deferral was for. - Any new caller that reapplies a mode and then persists inherits this obligation. + for the switch itself and 9 for its FX. That measurement is the whole reason + the FX writes must sit inside a block. Which block is the design decision, and + it is settled: the SWITCH'S OWN. The undo surface + (`vendor/reaper-sdk/sdk/reaper_plugin_functions.h:7745-7822`) is + begin/end/state-change and nothing else — no append-to-point, no coalescing + flag, no undo-silent write — and a block cannot be held open across a return + to the message loop, so ANY deferral of the writes costs a second undo point. + Two points is a rejected design, not a lesser one: do not reintroduce a + deferred queue, an idle drain, or a second block. That an explicit block + suppresses the per-call implicit points is still the PREMISE the whole + arrangement rests on — INFERENCE from the measurement above, not observed; + `docs/VERIFICATION.md` §"Mode switching" is where it gets checked. +- **A reapply that wrote nothing leaves no undo point.** The load-tick reapply + re-plans a park for every inactive leaf, and over a project saved fully parked + it finds every flag and every FX already where the plan wants them. The block + then closes on the `("", 0)` discard form (`view.h`'s `applyMintsUndoPoint`; + `mintManagedLanes`' no-op path uses the same idiom) — opening a project must + not cost the user a Ctrl-Z. A REAL switch always mints, written state or not: + it is an explicitly fired action and has to stay undoable even when the plan + found nothing to write. - **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." + apply mask (`kApplyUndoMask`) drops `UNDO_STATE_FX` and ORs it back in when + `I_FXEN` or a per-FX offline state actually moved — which every park does. 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 that wrote nothing and on a no-op switch. Narrowing it further would + mean not carrying the FX domain in the undo record at all, which would break + "one switch is one Ctrl-Z." +- **A rapid A→B→A flip pays two full park/restore cycles.** The retired intent + queue let an intent annihilate its own pending inverse, so a fast double-toggle + moved no plugins at all. Synchronous writes have no such window: the first + switch offlines the chain, the second brings it back, and both hitches are + paid. Accepted — the flip was never free (the flag writes always ran twice), + and the alternative costs the one-point invariant above. - **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 @@ -140,8 +148,8 @@ 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 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 ONE undo point it collapses its writes into (`fxParkUndoClose` — it opens that block itself, so no caller may drain while holding one), 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` — Design View shell: runs the pure planner over the live folder tree and iterates its park/restore plans, applies managed-lane and parent-visibility writes, and owns the ONE undo block all of that rides (`applyMintsUndoPoint` decides whether it leaves a point). 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 park surface: ONE track's whole move, both halves together. `parkTrack` is the trust 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), the pre-park snapshot when it is owed, then flags before FX; `restoreTrack` mirrors it, FX before flags, and consumes the snapshot. Each reports back what the caller's block needs (`TrackApplyResult`: did anything move, does the mask owe `UNDO_STATE_FX`, was the park refused, what could a restore not place). Also the `TrackFX_GetFXGUID` identity read all three of snapshot/park/restore address FX through, `trackFlagParm` — the ONE `Flag` → REAPER-parameter mapping they likewise share — the `writeIfChanged` read-before-write every driven parameter goes through, `trackStillLive` (a per-track `ValidatePtr2`, because a plugin unloading is assumed to pump the message loop), and the two console reports (`reportRefusedParks`, which names the tracks and holds the last-reported set so a reapply does not reprint it; `reportFxRestoreDrops`). **The snapshot is consumed where the restore RUNS** — from that line the flags are back at their captured values, and a model still describing the track as parked would let a later persist or reapply replan a restore over whatever the user changed since. - `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 @@ -159,42 +167,60 @@ applies the resulting lane state to live tracks. under whatever mode id is active at that point, not the one the user undid back to. Pre-existing: `snapshots_` already carries this same model-vs-undo split; the solo cache inherits it rather than introducing it. Not fixed here. -- An undo/redo also DISCARDS every pending FX intent (`discardDeferredFxParks`), - which is not the pure loss it reads as: the same tick reapplies the active mode - 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, 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, ON THE UNDO-ORDERING PREMISE - above (`[verify — DAW]`, not yet observed — see Invariants): if the drain's - point does sit above the switch's, every FX is back online by the step that - rolls the flags and the ext state back — and the step in between (FX online, - flags still parked) still holds the model's snapshot, which decides - `ParkOnly`, not a refusal. 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 +- **Which routes reach the no-snapshot-plus-parked-chain pair, re-derived for the + one-point design.** Three of the routes that used to reach it were artifacts of + the retired deferral window and are GONE BY CONSTRUCTION: there is no longer a + moment in which a track's flags have been written but its FX have not, so no + save, no reload and no discard can land between the two halves, and there is no + queued intent for an undo to drop. What survives: + - 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. (Asserted by the + branch that introduced the refusal; not re-observed since.) + - 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. It reads identically to both of the above and is refused too — + no lost state there at all, which is why the report asserts no cause. + + An undo of a mode switch (Ctrl-Z) is NOT a route, and no longer needs an + ordering premise to say so: flags and FX moved inside ONE block, so they roll + back together and no intermediate state exists for a park to misread. +- **The `view_state` write is NOT inside `applyMode`'s block.** `persistViewState` + (`design_view_actions.cpp`) runs after `applyMode` has returned, so + `SetProjExtState` lands past `Undo_EndBlock2`. The UNDO direction still comes + out right — the point closed over the ext state as it stood BEFORE the switch, + which is what a Ctrl-Z should restore — but by position, not by design. The + REDO direction is what this leaves open: the point never saw the new + `view_state`, so a redo may re-park the tracks while restoring the model that + describes the mode the user undid back to. `[verify — DAW]`; if it does bite, + the result is the pre-existing model-vs-undo split the solo-cache gotcha above + already describes, not a new refusal route. Do not "fix" it by moving + `persistViewState` inside the block — it can open a modal Save-As. +- 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. +- **`applyMode`'s block can be nested, and now carries FX writes while it is.** + `render_in_place` calls `applyMode` inside its own `Undo_BeginBlock2` pair, so + that verb's single `UNDO_STATE_ALL` point now absorbs the reapply's FX moves + instead of a separate point landing after it — the better outcome, but it rests + on REAPER coalescing nested blocks, which the SDK header does not document. + The inverse is also open: a plugin unloading is ASSUMED to pump the message + loop, so an action fired mid-apply could open its own block inside this one. + Nothing guards against that (the retired queue's `g_draining` never did either + — it only stopped a second drain). Both are `[verify — DAW]`. + That same premise leaves a STATED RESIDUAL on track handles. The park and + restore loops re-`ValidatePtr2` per track (`trackStillLive`), because they are + the ones interleaved with the FX writes. The passes that run AFTER them — lane + ops, parent visibility, the solo replay — still use handles resolved before the + first FX write, so a track deleted mid-apply would reach them stale. Not closed: + the premise it depends on is itself unobserved, and a partial guard covering + three of those four passes would read as complete. - `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 7bc57fd..c5e565b 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -1,7 +1,8 @@ // See view.h. Compiled into the reaper_reasampler module; includes // reaper_plugin_functions.h without REAPERAPI_IMPLEMENT (main.cpp owns that). -// Tree arithmetic lives in view_tree (pure); this file owns REAPER reads/writes -// and the snapshot-before-park ordering. +// Tree arithmetic lives in view_tree (pure) and one track's park/restore in +// view_fx_park; this file owns the plan iteration, lanes, parents and the one +// undo block they all ride. #include "shell/view/view.h" @@ -65,8 +66,8 @@ constexpr int kTransportMoving = 1 | 4; // (UNDO_STATE_MISCCFG covers extension state). NOT UNDO_STATE_ALL, which // includes UNDO_STATE_FX and so makes every undo record carry every track's FX // 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. +// what the switch changed. FX is OR'd back in per apply, only when I_FXEN or a +// per-FX offline state really moved (TrackApplyResult::fxMoved). // [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 @@ -146,45 +147,6 @@ MediaTrack* resolve(const TrackByGuid& byGuid, const std::string& guid) { return it == byGuid.end() ? nullptr : it->second; // stale/deleted GUID — pruned by being skipped } -// Captures prior driven-flag state before parking. Never reads B_MUTE/I_SOLO; -// ints preserve whatever REAPER reported (TrackSnapshot's defensive contract). -// 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 = 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); - 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 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); - return true; -} - -void applyFlags(MediaTrack* tr, const std::vector& flags, int& undoMask) { - for (const TrackFlagOp& op : flags) { - 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; - } -} - // 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 @@ -216,25 +178,32 @@ std::map managedLaneOrdinals(MediaTrack* tr) { // item-side C_LANEPLAYS is read-only, so no per-item write exists or is // needed). B_FIXEDLANE_HIDDEN is also read-only — hide/show follows from // C_LANEPLAYS=0/1, never written directly. -void applyLanePlays(MediaTrack* tr, int laneIdx, int lanePlays) { +bool applyLanePlays(MediaTrack* tr, int laneIdx, int lanePlays) { char parm[32]; std::snprintf(parm, sizeof(parm), "C_LANEPLAYS:%d", laneIdx); - writeIfChanged(tr, parm, static_cast(lanePlays)); + return writeIfChanged(tr, parm, static_cast(lanePlays)); } +// `freeMode` alone decides whether UpdateTimeline is owed; `wrote` is any project +// write at all, which is what the apply's undo-point decision reads. +struct LaneApplyResult { + bool freeMode = false; + bool wrote = false; +}; + // Groups ops by track, reconciles each op's durable laneKey to the track's // current ordinal (a stale/renamed/deleted key is pruned, never mis-driven), // enables fixed-lane mode on any track carrying a managed lane, and drives -// C_LANEPLAYS. UpdateTimeline() is the caller's job when this returns true -// (SDK: required after an I_FREEMODE change). -bool applyLaneOps(const TrackByGuid& handleByGuid, - const std::vector& lanes) { - if (lanes.empty()) return false; +// C_LANEPLAYS. UpdateTimeline() is the caller's job when `freeMode` comes back +// true (SDK: required after an I_FREEMODE change). +LaneApplyResult applyLaneOps(const TrackByGuid& handleByGuid, + const std::vector& lanes) { + LaneApplyResult out; + if (lanes.empty()) return out; std::map> byTrack; for (const LanePlayOp& op : lanes) byTrack[op.trackGuid].push_back(&op); - bool touchedFreeMode = false; for (const auto& [guid, ops] : byTrack) { MediaTrack* tr = resolve(handleByGuid, guid); if (!tr) continue; // stale GUID — prune @@ -248,17 +217,18 @@ bool applyLaneOps(const TrackByGuid& handleByGuid, SetMediaTrackInfo_Value(tr, "I_FREEMODE", static_cast(kFreeModeFixedLanes)); applyTransparentLaneDisplay(tr); - touchedFreeMode = true; + out.freeMode = true; + out.wrote = true; } const std::map ordinals = managedLaneOrdinals(tr); for (const LanePlayOp* op : ops) { auto it = ordinals.find(op->laneKey); if (it == ordinals.end()) continue; // key not live on this track — prune - applyLanePlays(tr, it->second, op->lanePlays); + if (applyLanePlays(tr, it->second, op->lanePlays)) out.wrote = true; } } - return touchedFreeMode; + return out; } // Managed-lane minting: the DECISION (which tracks split, which lanes, which item @@ -442,7 +412,10 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject int undoMask = kApplyUndoMask; bool laneModeChanged = false; + bool wrote = false; // anything at all this apply changed — see applyMintsUndoPoint std::vector refusedParkNames; + FxRestoreDrops fxDrops; + int fxDropTracks = 0; Undo_BeginBlock2(proj); { @@ -455,48 +428,41 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject clearTrackSolos(handleByGuid, outgoingSolo); } - // 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. + // PARK / RESTORE: view_fx_park owns one track's whole move, flags and + // per-FX offline together. A refusal is named HERE, while the handle is + // fresh — never held past Undo_EndBlock2/TrackList_AdjustWindows. 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 + // Re-validated per track, not once at resolve: offlining a plugin is + // ASSUMED to pump the message loop, so a track deleted during an + // earlier iteration's FX writes would leave this handle dangling. + if (!tr || !trackStillLive(proj, tr)) continue; // stale/deleted 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. - const std::vector cancelled = deferFxPark(proj, guid); - if (action == ParkAction::SnapshotThenPark) - model.storeSnapshot(guid, snapshotTrack(tr, cancelled)); - applyFlags(tr, tp.flags, undoMask); + const TrackApplyResult r = parkTrack(model, guid, tr, tp.flags); + if (r.refused) { refusedParkNames.push_back(trackDisplayName(tr)); continue; } + if (r.wrote) wrote = true; + if (r.fxMoved) undoMask |= UNDO_STATE_FX; } - // 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; MediaTrack* tr = resolve(trackByGuid, guid); - if (!tr) continue; // stale GUID — prune + if (!tr || !trackStillLive(proj, tr)) continue; // stale/deleted GUID — prune - 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); + const TrackApplyResult r = restoreTrack(model, guid, tr, tp.fxOffline, tp.flags); + if (r.wrote) wrote = true; + if (r.fxMoved) undoMask |= UNDO_STATE_FX; + if (r.drops.total() > 0) { fxDrops.add(r.drops); ++fxDropTracks; } } // 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); + const LaneApplyResult lanes = applyLaneOps(trackByGuid, plan.lanes); + laneModeChanged = lanes.freeMode; + if (lanes.wrote) wrote = true; // PARENT VISIBILITY (never parked): recomputed every toggle, never // snapshotted. Only the two visibility flags — never mainSend/FX on a parent. @@ -506,8 +472,8 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject MediaTrack* tr = resolve(trackByGuid, node.guid); if (!tr) continue; // stale GUID — prune const double show = visible.count(node.guid) ? 1.0 : 0.0; - writeIfChanged(tr, "B_SHOWINTCP", show); - writeIfChanged(tr, "B_SHOWINMIXER", show); + if (writeIfChanged(tr, "B_SHOWINTCP", show)) wrote = true; + if (writeIfChanged(tr, "B_SHOWINMIXER", show)) wrote = true; } model.setActiveMode(targetModeId); @@ -535,11 +501,13 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject // fixed lanes this apply (SDK requirement for I_FREEMODE changes). if (laneModeChanged) UpdateTimeline(); - Undo_EndBlock2(proj, undoLabel.c_str(), undoMask); + const bool mint = applyMintsUndoPoint(realSwitch, wrote); + Undo_EndBlock2(proj, mint ? undoLabel.c_str() : "", mint ? undoMask : 0); - // 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. + // After the undo block and the UI hold: a report is not a project write, and + // it must not join what a Ctrl-Z rolls back. reportRefusedParks(proj, refusedParkNames); + reportFxRestoreDrops(fxDrops, fxDropTracks); return true; } diff --git a/src/shell/view/view.h b/src/shell/view/view.h index 94448ed..66ce82c 100644 --- a/src/shell/view/view.h +++ b/src/shell/view/view.h @@ -23,13 +23,32 @@ namespace reasampler { // `nullptr` meaning the current project here is inferred by analogy, not confirmed. bool transportBlocksModeSwitch(ReaProject* proj); +// Does one applyMode leave an undo point behind? +// +// A REAPPLY that wrote nothing must not: the load-tick reapply over a project +// saved fully parked re-plans a park for every inactive leaf and finds every +// flag and every FX already where it wants them, so a point there would mean +// opening a project costs an undo step. `Undo_EndBlock2(proj, "", 0)` is the +// discard form (mintManagedLanes' no-op path uses the same idiom). +// +// A real switch mints regardless, fail-safe: it is an explicitly fired action +// and has to stay undoable even in the degenerate case where the plan found +// nothing to write. Note what applyMode CANNOT see when it decides — the +// `view_state` ext-state write lands in persistViewState, after this block has +// closed (src/shell/view/CLAUDE.md's Gotchas). +inline bool applyMintsUndoPoint(bool realSwitch, bool wroteAnything) { + return realSwitch || wroteAnything; +} + // Snapshots each about-to-park track's flags into `model`, caches/clears the // outgoing mode's solo state and replays the incoming mode's, runs planToggle, -// applies park/restore writes plus parent visibility flags, then sets the active -// mode. Wrapped in one Undo block. Returns false (no mutation) if `targetModeId` -// isn't registered, or if this is a real switch (target != active) while the -// transport is running. A reapply (target == active) is never gated and never -// touches solo. `proj` == nullptr means the current project. +// applies park/restore writes — flags AND per-FX offline, synchronously — plus +// parent visibility flags, then sets the active mode. ONE switch is ONE undo +// point; everything it moved rolls back in a single Ctrl-Z. Returns false (no +// mutation) if `targetModeId` isn't registered, or if this is a real switch +// (target != active) while the transport is running. A reapply (target == +// active) is never gated and never touches solo. `proj` == nullptr means the +// current project. bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject* proj); // Splits any track visible in more than one mode while carrying its own media diff --git a/src/shell/view/view_fx_park.cpp b/src/shell/view/view_fx_park.cpp index a9c1080..1840adb 100644 --- a/src/shell/view/view_fx_park.cpp +++ b/src/shell/view/view_fx_park.cpp @@ -1,70 +1,40 @@ // See view_fx_park.h. Compiled into the reaper_reasampler module; includes // reaper_plugin_functions.h without REAPERAPI_IMPLEMENT (main.cpp owns that). -// The queue's rule is pure (header, test_view_fx_park.cpp); this file owns the -// REAPER reads/writes and the ordering against the live enumeration. +// The decisions are pure (header, test_view_fx_park.cpp); this file owns the +// REAPER reads/writes and their ordering against the live enumeration. #include "shell/view/view_fx_park.h" #include // strnlen — bounded read of GetTrackName's buffer #include -#include #include -#include "shell/capture/track_guid.h" - #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_SetMediaTrackInfo_Value #define REAPERAPI_WANT_ShowConsoleMsg #define REAPERAPI_WANT_TrackFX_GetCount #define REAPERAPI_WANT_TrackFX_GetFXGUID #define REAPERAPI_WANT_TrackFX_GetOffline #define REAPERAPI_WANT_TrackFX_SetOffline -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #define REAPERAPI_WANT_ValidatePtr2 #define REAPERAPI_WANT_guidToString #include "reaper_plugin_functions.h" -static_assert(reasampler::kFxParkUndoMask == UNDO_STATE_FX, - "the SDK-free copy of the drain's undo mask has drifted from UNDO_STATE_FX"); - namespace reasampler { namespace { -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. -struct DrainScope { - DrainScope() { g_draining = true; } - ~DrainScope() { g_draining = false; } - DrainScope(const DrainScope&) = delete; - DrainScope& operator=(const DrainScope&) = delete; -}; - ReaProject* currentProject() { return EnumProjects(-1, nullptr, 0); } -void adoptOwner(ReaProject* proj) { - ReaProject* p = proj ? proj : currentProject(); - if (p == g_owner) return; - g_queue.clear(); // intents planned against another project are never replayed here - g_owner = p; -} - -// Returns whether it actually wrote — the drain's undo point is discarded unless -// something did (fxParkUndoClose). +// Returns whether it actually wrote — the apply's undo point and its +// UNDO_STATE_FX bit both hang off that verdict. // // [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 @@ -77,62 +47,6 @@ bool setOfflineIfChanged(MediaTrack* tr, int fx, bool offline) { return true; } -bool applyPark(MediaTrack* tr) { - bool wrote = false; - const int fxCount = TrackFX_GetCount(tr); - for (int fx = 0; fx < fxCount; ++fx) - if (setOfflineIfChanged(tr, fx, true)) wrote = true; - return wrote; -} - -struct RestoreOutcome { - FxRestoreDrops drops; - bool wrote = false; -}; - -// Restores per-FX offline from the plan verbatim — never a blanket "online". -// Which live FX each captured state belongs to is resolveFxRestore's call, and -// what it could not place comes back for the caller to report. -RestoreOutcome applyRestore(MediaTrack* tr, const std::vector& ops) { - const FxRestoreResolution res = resolveFxRestore(ops, liveFxGuids(tr)); - RestoreOutcome out; - for (const FxOfflineWrite& w : res.writes) - if (setOfflineIfChanged(tr, w.fxIndex, w.offline)) out.wrote = true; - out.drops = res.drops; - return out; -} - -// ONE drain is ONE undo point (see src/shell/view/CLAUDE.md for the measured -// per-FX fragmentation this closes). The block is deliberately the drain's OWN -// and never the switch's — the FX point has to stay ABOVE the switch's. -// -// Closing is unconditional on every exit, the abort path included. Which of two -// undocumented models REAPER uses decides whether that is the safe call: -// under a GLOBAL block-nesting counter, leaving this one unbalanced would -// swallow every later action — in ANY project — into one point, far worse than -// calling Undo_EndBlock2 on a ReaProject* that closed since Undo_BeginBlock2 -// took it. Under a PER-PROJECT counter, an unbalanced block on a now-dead -// project costs nothing, and the closing call against a stale pointer is the -// riskier of the two. Closing unconditionally is the safer bet under the -// global model and merely unnecessary under the per-project one, hence the -// choice. [verify — DAW] which model REAPER actually implements. -class FxParkUndoBlock { -public: - explicit FxParkUndoBlock(ReaProject* proj) : proj_(proj) { Undo_BeginBlock2(proj_); } - ~FxParkUndoBlock() { - const FxParkUndoClose close = fxParkUndoClose(wrote_); - Undo_EndBlock2(proj_, close.label, close.mask); - } - FxParkUndoBlock(const FxParkUndoBlock&) = delete; - FxParkUndoBlock& operator=(const FxParkUndoBlock&) = delete; - - void noteWrite() { wrote_ = true; } - -private: - ReaProject* proj_; - bool wrote_ = false; -}; - // The FX's own durable identity, braced exactly like the track GUID keys. Empty // when REAPER reports none — an FX we cannot name is one we cannot restore, and // fx_offline treats it that way rather than guessing at its slot. Lifetime is @@ -155,6 +69,18 @@ std::string fxGuidString(MediaTrack* tr, int fx) { return std::string(buf); } +// 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 +// whatever it covers, all three cover identically. +std::vector liveFxGuids(MediaTrack* tr) { + const int fxCount = TrackFX_GetCount(tr); + std::vector guids; + guids.reserve(static_cast(fxCount)); + for (int fx = 0; fx < fxCount; ++fx) guids.push_back(fxGuidString(tr, fx)); + return guids; +} + // 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) { @@ -164,6 +90,41 @@ bool anyFxOffline(MediaTrack* tr) { return false; } +// Captures prior driven-flag state before parking. Never reads B_MUTE/I_SOLO; +// ints preserve whatever REAPER reported (TrackSnapshot's defensive contract). +// Must run before EITHER half of the park writes — both halves are read here. +// `fxKeying` stays Identity: a live read always knows the chain. +TrackSnapshot snapshotTrack(MediaTrack* tr) { + TrackSnapshot snap; + auto read = [tr](Flag f) { + return static_cast(GetMediaTrackInfo_Value(tr, trackFlagParm(f))); + }; + snap.showInTcp = read(Flag::ShowInTcp); + snap.showInMixer = read(Flag::ShowInMixer); + snap.mainSend = read(Flag::MainSend); + snap.fxEnable = read(Flag::FxEnable); + + 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; +} + +// Returns whether it wrote; raises `fxMoved` when a flag in the FX domain moved. +bool applyFlags(MediaTrack* tr, const std::vector& flags, bool& fxMoved) { + bool wrote = false; + for (const TrackFlagOp& op : flags) { + if (!writeIfChanged(tr, trackFlagParm(op.flag), static_cast(op.value))) continue; + wrote = true; + // 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) fxMoved = true; + } + return wrote; +} + } // namespace // GetTrackName, not P_NAME, matching capture::trackName's reason for the same @@ -195,121 +156,71 @@ void reportRefusedParks(ReaProject* proj, const std::vector& refuse 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. + // (reaper_plugin_functions.h:6536). if (!msg.empty()) ShowConsoleMsg(("!SHOW:" + msg).c_str()); } -std::vector liveFxGuids(MediaTrack* tr) { +bool writeIfChanged(MediaTrack* tr, const char* parm, double value) { + if (GetMediaTrackInfo_Value(tr, parm) == value) return false; + SetMediaTrackInfo_Value(tr, parm, value); + return true; +} + +bool trackStillLive(ReaProject* proj, MediaTrack* tr) { + return ValidatePtr2(proj ? proj : currentProject(), tr, "MediaTrack*"); +} + +TrackApplyResult parkTrack(ViewModeModel& model, const std::string& guid, MediaTrack* tr, + const std::vector& flags) { + TrackApplyResult out; + + const ParkAction action = decideParkForTrack(tr, model.snapshot(guid) != nullptr, flags); + if (action == ParkAction::Refuse) { + out.refused = true; + return out; + } + if (action == ParkAction::SnapshotThenPark) { + model.storeSnapshot(guid, snapshotTrack(tr)); + out.wrote = true; + } + + if (applyFlags(tr, flags, out.fxMoved)) out.wrote = true; + const int fxCount = TrackFX_GetCount(tr); - std::vector guids; - guids.reserve(static_cast(fxCount)); - for (int fx = 0; fx < fxCount; ++fx) guids.push_back(fxGuidString(tr, fx)); - return guids; + for (int fx = 0; fx < fxCount; ++fx) + if (setOfflineIfChanged(tr, fx, true)) out.fxMoved = out.wrote = true; + return out; } -PreParkFx snapshotFxOffline(MediaTrack* tr, const std::vector& cancelled) { - PreParkFx carried = preParkFxFromCancelledRestore(cancelled); - if (!carried.states.empty()) return carried; +TrackApplyResult restoreTrack(ViewModeModel& model, const std::string& guid, MediaTrack* tr, + const std::vector& fxOps, + const std::vector& flags) { + TrackApplyResult out; - // 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; + // Which live FX each captured state belongs to is resolveFxRestore's call — + // the plan is written VERBATIM, never a blanket "online". + const FxRestoreResolution res = resolveFxRestore(fxOps, liveFxGuids(tr)); + for (const FxOfflineWrite& w : res.writes) + if (setOfflineIfChanged(tr, w.fxIndex, w.offline)) out.fxMoved = out.wrote = true; + out.drops = res.drops; + + if (applyFlags(tr, flags, out.fxMoved)) out.wrote = true; + + // Unconditional, even when every write above was elided: the snapshot is + // gone, so this apply is not repeatable and must not close on the discard + // form. Model state, not project state — whether an undo actually puts the + // snapshot back is the ext-state question in src/shell/view/CLAUDE.md. + model.clearSnapshot(guid); + out.wrote = true; + return out; } -std::vector deferFxPark(ReaProject* proj, const std::string& guid) { - adoptOwner(proj); - return g_queue.park(guid); -} - -void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector ops) { - adoptOwner(proj); - g_queue.restore(guid, std::move(ops)); -} - -void discardDeferredFxParks() { - g_queue.clear(); - g_owner = nullptr; -} - -void drainDeferredFxParks() { - if (g_queue.empty()) return; - if (g_draining) return; // re-entered mid-apply: those intents are the outer drain's next pass - - if (g_owner != currentProject()) { - // The project the intents were planned against was closed or switched - // away from: its tracks are not ours to write and its handles may be - // gone. Discard rather than apply. This catches only a DIFFERENT live - // pointer; a REAPER-recycled address is caught upstream, by the caller - // discarding on the session's own load transition. - discardDeferredFxParks(); - return; - } - - ReaProject* const proj = g_owner; - const DrainScope scope; - - // Detached before the first write: [verify — DAW] applying is ASSUMED to pump - // the message loop (plugins load and unload), so a re-entrant switch can - // enqueue while this runs. Everything defensive below rests on that one - // assumption; each piece is correct regardless of whether it holds. - const std::vector draining = g_queue.take(); - - std::unordered_map byGuid; - const int count = CountTracks(proj); - byGuid.reserve(static_cast(count)); - for (int i = 0; i < count; ++i) { - MediaTrack* tr = GetTrack(proj, i); - if (!tr) continue; - std::string guid = guidString(tr); - if (!guid.empty()) byGuid.emplace(std::move(guid), tr); - } - - FxRestoreDrops drops; - int dropTracks = 0; - { - // Scoped so the block closes before the console report below: a report is - // not a project write and must not join what a Ctrl-Z rolls back, the same - // ordering applyMode gives reportRefusedParks. - FxParkUndoBlock undo(proj); - for (const FxParkIntent& intent : draining) { - auto it = byGuid.find(intent.guid); - if (it == byGuid.end()) continue; // track deleted since the switch — prune - - // Re-validated PER INTENT, not once above: under the same pumping - // assumption, a project closed or a track deleted between two applies - // leaves the handle resolved above dangling — a use-after-free, not a - // pruned intent. Same gate capture_realtime_shell's teardown uses; a null - // first argument validates the ReaProject* itself (SDK: proj is ignored - // when the pointer is a project). - if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return; - if (!ValidatePtr2(proj, it->second, "MediaTrack*")) continue; - - if (intent.park) { - if (applyPark(it->second)) undo.noteWrite(); - continue; - } - const RestoreOutcome r = applyRestore(it->second, intent.restoreOps); - if (r.wrote) undo.noteWrite(); - if (r.drops.total() > 0) { - drops.add(r.drops); - ++dropTracks; - } - } - } - - // 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 - // force-opening the console: this drain also runs behind an unattended - // project-load reapply, and cannot tell that case from an interactive toggle. - const std::string fxDropMsg = describeFxRestoreDrops(drops, dropTracks); - if (!fxDropMsg.empty()) ShowConsoleMsg(("!SHOW:" + fxDropMsg).c_str()); +void reportFxRestoreDrops(const FxRestoreDrops& drops, int trackCount) { + const std::string msg = describeFxRestoreDrops(drops, trackCount); + // "!SHOW:" (reaper_plugin_functions.h:6536) keeps it from force-opening the + // console: this also runs behind an unattended project-load reapply, and + // cannot tell that case from an interactive toggle. + if (!msg.empty()) ShowConsoleMsg(("!SHOW:" + msg).c_str()); } } // namespace reasampler diff --git a/src/shell/view/view_fx_park.h b/src/shell/view/view_fx_park.h index c864e9a..a4e7129 100644 --- a/src/shell/view/view_fx_park.h +++ b/src/shell/view/view_fx_park.h @@ -1,11 +1,10 @@ #pragma once -// 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. +// Design View's park surface: one track parked or restored — the trust test, +// the snapshot lifecycle, and the flag + per-FX writes both halves address +// through this one file. Every write declared here runs synchronously inside +// applyMode's own undo block, which is what makes one switch one undo point — +// see src/shell/view/CLAUDE.md's FX-parking caveat. -#include #include #include @@ -18,108 +17,19 @@ class ReaProject; namespace reasampler { -// 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 -// whatever it covers, all three cover identically. -std::vector liveFxGuids(MediaTrack* tr); - -// One deferred per-FX intent for one track. A park carries no ops (every live -// slot goes offline); a restore carries the planned ops verbatim. -struct FxParkIntent { - std::string guid; - bool park = false; - std::vector restoreOps; -}; - -// The queue's re-entrancy rule, pure so it can be asserted without a DAW: at -// most ONE intent per track GUID, and the latest one wins. Park and restore are -// inverses, so an intent landing on its own pending inverse CANCELS it rather -// than stacking — the queued work never ran, so the track already holds the -// state the newcomer asks for, and replaying both would be both slower and -// observably wrong. -class FxParkQueue { -public: - // 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). - // - // The empty return is a LOAD-BEARING sentinel that deliberately conflates - // "nothing was cancelled" with "cancelled a restore carrying no ops": a - // zero-op restore held no FX state to hand back, so falling through to a - // live chain read is the same answer, not a worse one. - std::vector park(const std::string& guid) { - FxParkIntent* held = find(guid); - if (!held) { - pending_.push_back(FxParkIntent{guid, true, {}}); - 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) { - pending_.push_back(FxParkIntent{guid, false, std::move(ops)}); - } else if (held->park) { - erase(held); - } else { - held->restoreOps = std::move(ops); - } - } - - // Enqueue order, which is apply order: park before restore within one - // switch, as the synchronous body already orders them. - const std::vector& pending() const { return pending_; } - 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. [verify — DAW] applying - // loads/unloads plugins, which is ASSUMED to pump 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. The detach is correct either way; only - // the need for it is unconfirmed. - std::vector take() { - std::vector taken; - taken.swap(pending_); - return taken; - } - -private: - FxParkIntent* find(const std::string& guid) { - for (FxParkIntent& i : pending_) - if (i.guid == guid) return &i; - return nullptr; - } - void erase(FxParkIntent* held) { - pending_.erase(pending_.begin() + (held - pending_.data())); - } - - 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. +// and silent. A model carrying no snapshot is NOT evidence the chain is clean; +// src/shell/view/CLAUDE.md lists the routes that still reach that pair. // // 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 flags, one hand-offlined plugin) 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 @@ -159,11 +69,10 @@ enum class ParkAction { 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. +// Refusing leaves the track exactly as found: no flag write, no FX write, 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; @@ -219,120 +128,58 @@ inline bool shouldReport(bool sameOwnerAsLast, const std::vector& l // 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; - FxKeying keying = FxKeying::Identity; +// -- parking and restoring one track ------------------------------------------ +// +// Both run INSIDE applyMode's undo block, on the switch's own synchronous path, +// so one switch is one undo point. + +// A track parameter written only when it is not already there. A reapply +// re-plans every flag it already applied, so most of what it writes is a value +// the track already holds. The read is ASSUMED cheaper than the write it elides +// (unmeasured); what it certainly does is keep those no-op writes out of the +// undo record's mask. Returns whether it wrote. +bool writeIfChanged(MediaTrack* tr, const char* parm, double value); + +// Is `tr` still a live track of `proj` (nullptr = current project)? The apply's +// loops re-ask per track: a plugin unloading is assumed to pump the message +// loop, so a handle resolved before the first FX write can be dead by the last. +// `proj` is resolved to a concrete pointer here because ValidatePtr2 documents +// no nullptr-means-current-project contract (reaper_plugin_functions.h:7899), +// and a false there would silently skip every track. +bool trackStillLive(ReaProject* proj, MediaTrack* tr); + +// What one track's park or restore did, folded into the apply's block state. +struct TrackApplyResult { + bool wrote = false; // a project write or a snapshot moved — the undo-point verdict + bool fxMoved = false; // ⇒ the block's mask owes UNDO_STATE_FX + bool refused = false; // park only: the chain was left exactly as found + + // Restore only: captured FX state resolveFxRestore could not place. Reported + // rather than swallowed — silence would read as "restore worked" while an FX + // sat at whatever state the park left it in. + FxRestoreDrops drops; }; -// 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. -// -// FAITHFUL TO THE SNAPSHOT, NOT THE CHAIN. The ops describe the chain as it was -// at the ORIGINAL park; applyPark enumerates it again at drain time. So an FX -// added while the track was parked (a floating FX-chain window, ReaScript) is -// absent from this reconstruction yet IS offlined by the cancelling park's -// drain — and so never comes back online. Rare, and recoverable by hand in the -// FX chain, but specific to the deferral. -// -// `offline` is already boolean by the time it arrives: makeRestorePlan narrowed -// FxOfflineState's defensive int to FxOfflineOp's bool, so this widening back to -// int restores the type, not lost information. -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; -} +// Parks one inactive leaf: the trust decision (decidePark), the pre-park +// snapshot when it is owed, then flags before FX so the track is out of the mix +// and bypassed before its plugins start unloading. On `refused` nothing at all +// was written — the caller names the track (trackDisplayName) while its handle +// is still fresh. +TrackApplyResult parkTrack(ViewModeModel& model, const std::string& guid, MediaTrack* tr, + const std::vector& flags); -// 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); +// Restores one returning leaf: FX before flags, park's order mirrored, so the +// chain comes back online while still bypassed and out of the mix and the track +// never sounds through a half-restored chain. Consumes the snapshot — from that +// point the flags are back at their captured values, and a later reapply must +// not replan a restore over whatever the user has changed since. +TrackApplyResult restoreTrack(ViewModeModel& model, const std::string& guid, MediaTrack* tr, + const std::vector& fxOps, + const std::vector& flags); -// Enqueue against `proj` (nullptr = current project). An enqueue naming a -// 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); - -// -- the drain's own undo point ----------------------------------------------- -// -// UNDO_STATE_FX and nothing else: per-FX offline is all the drain writes. The -// value is repeated here rather than included so this header stays SDK-free; -// view_fx_park.cpp static_asserts it against the macro (reaper_plugin.h:1542). -inline constexpr int kFxParkUndoMask = 2; - -// Undo_EndBlock2's two arguments for ONE drain. An empty label with a zero mask -// is REAPER's discard form — view.cpp's lane-mint path uses the same idiom — and -// discarding matters here rather than merely tidying: a project-load reapply -// plans a park for every inactive leaf, and a project saved parked already holds -// every one of those FX offline, so a block opened unconditionally would mint an -// empty undo point on each open. -struct FxParkUndoClose { - const char* label = ""; - int mask = 0; -}; - -inline FxParkUndoClose fxParkUndoClose(bool wroteAnyFx) { - if (!wroteAnyFx) return {}; - // "FX state", not "FX park": the drain restores as often as it parks, and a - // direction-coded label would misname the point that brings FX back online. - return FxParkUndoClose{"ReaSampler: Design View FX state", kFxParkUndoMask}; -} - -// 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. -// Re-entrant calls early-out: one drain at a time, the outer one owns the queue. -// -// Called on the idle tick AND synchronously before the view model is serialized -// (persistViewState, render_in_place). The second call is not an optimization: -// between a restore's synchronous flag writes and its drain the FX are still -// offline while the model has already dropped the snapshot that would replan -// them, so a save inside that window — deterministic under a custom action chain -// like "toggle mode; save project" — records offline FX beside a snapshot-free -// model, and nothing on reopen brings them back online. The cost is that an -// action-driven switch pays the FX hitch before it returns; its repaint and undo -// block have both closed by then, which is what the deferral was for. -// -// OPENS ITS OWN UNDO BLOCK, so a caller must not already hold one: the drain's -// point has to sit ABOVE the switch's, and nesting is undocumented in the SDK -// header either way. All three call sites drain outside any block they open -// (OnTimer holds none; persistViewState is reached only after applyMode's and -// doMoveItems' blocks have closed; render_in_place drains after its EndBlock2). -// -// That audit is ONE-DIRECTIONAL — it covers the drain opening inside a caller's -// block, not the inverse the pumping premise above (FxParkQueue::take) makes -// possible: a block opening INSIDE the drain's own open block. Two routes are -// live if that premise holds: a re-entrant applyMode (view.cpp) fired by an -// action mid-drain, and a re-entrant OnTimer -> bankPanelRefresh -> -// mintManagedLanes (view.cpp) — both open their OWN Undo_BeginBlock2/EndBlock2 -// pair. g_draining (view_fx_park.cpp) only blocks a SECOND drainDeferredFxParks -// call; it does nothing to stop a different subsystem's block opening while -// this one is still open. `[verify — DAW]` whether that nested pair (undocumented -// either way, as above) does anything worse than the ordering already assumed — -// see docs/VERIFICATION.md. -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. -// -// Not pure loss: the caller reapplies the active mode over the reloaded model -// immediately after, which re-plans a park for every inactive leaf, so parked FX -// converge on the following drain. The case that does NOT self-heal is a track -// whose reloaded model carries no snapshot — nothing plans a restore for it, so -// FX left offline stay offline. -void discardDeferredFxParks(); +// One console line for a whole apply's drops. Called after the undo block +// closes: a report is not a project write and must not join what a Ctrl-Z rolls +// back. +void reportFxRestoreDrops(const FxRestoreDrops& drops, int trackCount); } // namespace reasampler diff --git a/tests/test_view_fx_park.cpp b/tests/test_view_fx_park.cpp index 8137d2f..57e5e52 100644 --- a/tests/test_view_fx_park.cpp +++ b/tests/test_view_fx_park.cpp @@ -1,16 +1,15 @@ -// 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. +// Standalone tests for the Design View park surface's pure decisions — no +// REAPER, no test framework. // -// 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; -// a cancel must not strand the pre-park FX state it was the last record of; a -// pre-park snapshot is never taken from a chain a park has already touched; and -// one drain closes as at most ONE undo point, in the FX domain only. +// The properties under test: a pre-park snapshot is never taken from a chain a +// park has already touched, the refusal that follows names its tracks and is +// reported once per changed set, and one mode apply leaves at most ONE undo +// point — none at all when a reapply found everything already where it wanted it. #include "../src/shell/view/view_fx_park.h" +#include "../src/shell/view/view.h" // applyMintsUndoPoint — the apply's undo-point fold -#include "core/view/view_mode_model.h" // makeRestorePlan — the ops' only producer +#include "core/view/view_mode_model.h" // makeParkPlan — the park flags' only producer #include #include @@ -22,219 +21,6 @@ static int g_fail = 0; #define CHECK(cond) do { if(!(cond)) { \ std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) -// -- helpers ----------------------------------------------------------------- - -// The ops applyMode hands a restore: one per FX captured in the track's snapshot. -static std::vector ops(const std::string& fxGuid, bool offline) { - return {FxOfflineOp{"{TRACK}", FxKeying::Identity, fxGuid, 0, offline}}; -} - -static const FxParkIntent* intentFor(const FxParkQueue& q, const std::string& guid) { - for (const FxParkIntent& i : q.pending()) - if (i.guid == guid) return &i; - return nullptr; -} - -// -- tests ------------------------------------------------------------------- - -static void testParkEnqueuesOneIntentCarryingNoOps() { - FxParkQueue q; - q.park("{A}"); - - CHECK(q.pending().size() == 1); - const FxParkIntent* held = intentFor(q, "{A}"); - CHECK(held != nullptr); - CHECK(held && held->park); - 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 - // plugin only to reload it. - FxParkQueue q; - q.park("{A}"); - q.restore("{A}", ops("{FX}", false)); - - CHECK(q.empty()); -} - -static void testParkOnAnUndrainedRestoreCancelsRatherThanStacks() { - // The mirror case: the restore never ran, so the FX are still parked offline, - // which is where the new park wants them. - FxParkQueue q; - q.restore("{A}", ops("{FX}", false)); - q.park("{A}"); - - CHECK(q.empty()); -} - -static void testRepeatedParkStaysOneIntent() { - FxParkQueue q; - q.park("{A}"); - q.park("{A}"); - q.park("{A}"); - - CHECK(q.pending().size() == 1); - CHECK(intentFor(q, "{A}") && intentFor(q, "{A}")->park); -} - -static void testLaterRestoreReplacesTheEarlierOnesOps() { - FxParkQueue q; - q.restore("{A}", ops("{OLD}", false)); - q.restore("{A}", ops("{NEW}", true)); - - CHECK(q.pending().size() == 1); - const FxParkIntent* held = intentFor(q, "{A}"); - CHECK(held && !held->park); - CHECK(held && held->restoreOps.size() == 1); - CHECK(held && held->restoreOps.front().fxGuid == "{NEW}"); - CHECK(held && held->restoreOps.front().offline); -} - -static void testOneTracksCancelLeavesEveryOtherTrackAlone() { - FxParkQueue q; - q.park("{A}"); - q.park("{B}"); - q.park("{C}"); - q.restore("{B}", ops("{FX}", false)); // cancels B only - - CHECK(q.pending().size() == 2); - CHECK(intentFor(q, "{A}") != nullptr); - CHECK(intentFor(q, "{B}") == nullptr); - CHECK(intentFor(q, "{C}") != nullptr); - // Order survives the middle erase: the drain applies in enqueue order. - CHECK(q.pending()[0].guid == "{A}"); - CHECK(q.pending()[1].guid == "{C}"); -} - -static void testCancelledTrackCanBeQueuedAgain() { - // Two rapid switches then a third: the third is the one that must land. - FxParkQueue q; - q.park("{A}"); - q.restore("{A}", ops("{FX}", false)); - q.park("{A}"); - - CHECK(q.pending().size() == 1); - CHECK(intentFor(q, "{A}") && intentFor(q, "{A}")->park); -} - -static void testClearDropsEverythingPending() { - FxParkQueue q; - q.park("{A}"); - q.restore("{B}", ops("{FX}", true)); - q.clear(); - - CHECK(q.empty()); - 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); -} - -// -- the makeRestorePlan <-> preParkFxFromCancelledRestore round trip --------- -// -// The cancel path's whole premise is that a planned restore's ops are a LOSSLESS -// carrier of the snapshot's FX half. makeRestorePlan is their only producer, so -// the real claim is that the pair composes to the identity on (fxOffline, -// fxKeying). Asserting it against hand-built ops would let a change to -// makeRestorePlan's field mapping or op ordering pass with every test green. - -static void testRestorePlanOpsRebuildTheIdentityKeyedSnapshotVerbatim() { - TrackSnapshot snap; - snap.fxKeying = FxKeying::Identity; - snap.fxOffline = {FxOfflineState{"{ONE}", 1}, FxOfflineState{"{TWO}", 0}, - FxOfflineState{"{THREE}", 1}}; - - const TrackPlan plan = makeRestorePlan("{TRACK}", snap); - const PreParkFx rebuilt = preParkFxFromCancelledRestore(plan.fxOffline); - - CHECK(rebuilt.keying == FxKeying::Identity); - // Three DISTINCT entries, compared as a sequence: a dropped fxGuid, a flipped - // offline, or a reordering each fail here. - CHECK(rebuilt.states == snap.fxOffline); -} - -static void testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim() { - TrackSnapshot snap; - snap.fxKeying = FxKeying::Slot; - snap.fxOffline = {FxOfflineState{"", 0}, FxOfflineState{"", 1}, FxOfflineState{"", 1}}; - - const TrackPlan plan = makeRestorePlan("{TRACK}", snap); - - // Slot keying addresses by POSITION, so the identity holds only while the op - // at index i carries slot i. - CHECK(plan.fxOffline.size() == 3); - CHECK(plan.fxOffline.size() == 3 && plan.fxOffline[0].slot == 0 && - plan.fxOffline[1].slot == 1 && plan.fxOffline[2].slot == 2); - - const PreParkFx rebuilt = preParkFxFromCancelledRestore(plan.fxOffline); - - CHECK(rebuilt.keying == FxKeying::Slot); - CHECK(rebuilt.states == snap.fxOffline); -} - // -- may this chain be snapshotted? ------------------------------------------- // // Rationale: the snapshot-source invariant (view_fx_park.h, this directory's @@ -360,99 +146,33 @@ static void testAFreshEmptySetReportsAndResetsTheMemo() { CHECK(!shouldReport(/*sameOwnerAsLast=*/true, {}, {})); } -// -- 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() { - // [verify — DAW] applying an intent loads/unloads plugins, which is ASSUMED to - // pump 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); -} - -// -- the drain's undo point -------------------------------------------------- +// -- the apply's one undo point ---------------------------------------------- // -// These pin fxParkUndoClose's own fold only. The real wroteAnyFx verdict — -// setOfflineIfChanged -> applyPark/applyRestore -> FxParkUndoBlock::noteWrite -// -- runs against live REAPER calls and cannot be pinned here; docs/VERIFICATION.md -// items 51 and 53 are the sole cover for that plumbing being wired correctly. +// These pin applyMintsUndoPoint's fold only. The `wroteAnything` verdict it +// consumes — writeIfChanged / parkTrack / restoreTrack, all running against live +// REAPER calls — cannot be pinned here; docs/VERIFICATION.md §"Mode switching" +// is the sole cover for that plumbing being wired correctly. -static void testADrainThatWroteFxClosesItsBlockAsOneNamedFxPoint() { - const FxParkUndoClose close = fxParkUndoClose(true); - - // 2 is UNDO_STATE_FX (reaper_plugin.h:1542), pinned as a literal here and - // static_asserted against the macro in view_fx_park.cpp. The drain writes - // per-FX offline and nothing else, so any wider mask would make it marshal - // track config or items it never touched. - CHECK(close.mask == 2); - CHECK(close.label != nullptr && std::string(close.label) == - "ReaSampler: Design View FX state"); +static void testARealSwitchAlwaysLeavesAPointEvenWithNothingToWrite() { + // An explicitly fired action stays undoable even when the plan found nothing + // to write — the discard form is for reapplies only. + CHECK(applyMintsUndoPoint(/*realSwitch=*/true, /*wroteAnything=*/false)); + CHECK(applyMintsUndoPoint(/*realSwitch=*/true, /*wroteAnything=*/true)); } -static void testADrainThatWroteNothingClosesItsBlockAsADiscard() { - // The project-load reapply: a park is planned for every inactive leaf, and a - // project saved parked already holds every one of those FX offline. Nothing is - // written, so the block must leave no undo point behind at all. - const FxParkUndoClose close = fxParkUndoClose(false); +static void testAReapplyThatWroteSomethingLeavesAPoint() { + // A tag/untag reapply that actually parked a track: real project state moved. + CHECK(applyMintsUndoPoint(/*realSwitch=*/false, /*wroteAnything=*/true)); +} - CHECK(close.mask == 0); - CHECK(close.label != nullptr && std::string(close.label).empty()); +static void testAReapplyThatWroteNothingLeavesNoPointAtAll() { + // The project-load reapply over a project saved fully parked: every flag and + // every FX already sit where the plan wants them. Opening a project must not + // cost the user an undo step. + CHECK(!applyMintsUndoPoint(/*realSwitch=*/false, /*wroteAnything=*/false)); } int main() { - testParkEnqueuesOneIntentCarryingNoOps(); - testParkReportsNothingCancelledWhenNoIntentWasPending(); - testParkOnItsOwnPendingParkReportsNothingCancelled(); - testRestoreOnAnUndrainedParkCancelsRatherThanStacks(); - testParkOnAnUndrainedRestoreCancelsRatherThanStacks(); - testRepeatedParkStaysOneIntent(); - testLaterRestoreReplacesTheEarlierOnesOps(); - testOneTracksCancelLeavesEveryOtherTrackAlone(); - testCancelledTrackCanBeQueuedAgain(); - testClearDropsEverythingPending(); - testParkHandsBackTheOpsOfTheRestoreItCancelled(); - testCancelledRestoreOpsBecomeTheFreshSnapshotsFxHalf(); - testNothingCancelledLeavesTheFxHalfToTheCaller(); - testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities(); - testRestorePlanOpsRebuildTheIdentityKeyedSnapshotVerbatim(); - testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim(); testAChainSittingAtEveryValueTheParkWouldWriteReadsAsParked(); testOneFlagStillAtTheUsersValueMeansNoParkReachedTheChain(); testAnEmptyPlanProvesNothing(); @@ -468,12 +188,9 @@ int main() { testADifferentOwnerReportsEvenWithTheSameNames(); testAFreshEmptySetReportsAndResetsTheMemo(); - testTakeDetachesEverythingAndLeavesTheQueueEmpty(); - testIntentsArrivingDuringADrainSurviveIt(); - testAReEntrantParkCancelsOnlyWhatIsStillPending(); - - testADrainThatWroteFxClosesItsBlockAsOneNamedFxPoint(); - testADrainThatWroteNothingClosesItsBlockAsADiscard(); + testARealSwitchAlwaysLeavesAPointEvenWithNothingToWrite(); + testAReapplyThatWroteSomethingLeavesAPoint(); + testAReapplyThatWroteNothingLeavesNoPointAtAll(); if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0;