From 39529a5ac4b7b3bd75610fbe7a56c1e0e3f191f7 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Fri, 7 Aug 2026 14:41:17 -0400 Subject: [PATCH] Design View: drop the undo blocks around writes that mint nothing; the membership actions own theirs Flag writes are silent from a timer, so a block around a reapply or the reconcile repair could only manufacture a point. The real switch's span is unchanged. --- src/shell/actions/design_view_actions.cpp | 90 ++++++++++++++++------- src/shell/view/CLAUDE.md | 73 +++++++++--------- src/shell/view/fx_reclaim.h | 12 +-- src/shell/view/view.cpp | 34 ++++----- src/shell/view/view.h | 26 ++----- src/shell/view/view_reconcile.cpp | 8 +- src/shell/view/view_reconcile.h | 6 +- tests/test_view_park.cpp | 36 +-------- 8 files changed, 141 insertions(+), 144 deletions(-) diff --git a/src/shell/actions/design_view_actions.cpp b/src/shell/actions/design_view_actions.cpp index e83e5e1..c8176eb 100644 --- a/src/shell/actions/design_view_actions.cpp +++ b/src/shell/actions/design_view_actions.cpp @@ -27,6 +27,7 @@ #include "shell/panel/panel_window.h" // bankPanelInvalidate — footer toggle repaint #include "shell/view/view.h" // applyMode (D2 shell) #include "shell/view/view_lanes.h" // mintManagedLanes +#include "shell/view/view_park.h" // kViewUndoMask #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_CountSelectedTracks @@ -201,40 +202,76 @@ void doActivateMode(const std::string& modeId) { bankPanelInvalidate(); // repaint the footer [Arrange|Design] toggle immediately } +// The block each membership-edit action below owns. The reapply they drive opens none +// of its own — nothing it writes mints a point unaided — so a user-fired tag stays one +// Ctrl-Z only because the ACTION spans it, and the label names the act the user +// actually fired rather than the reapply it happened to trigger. Every one of them +// closes the block before persistViewState, which can pop a modal Save-As. Stack RAII +// so a throw between the two calls cannot leave the block open in the user's project. +class ActionUndoBlock { +public: + ActionUndoBlock(std::string label, int mask) : label_(std::move(label)), mask_(mask) { + Undo_BeginBlock2(nullptr); + } + ~ActionUndoBlock() { Undo_EndBlock2(nullptr, label_.c_str(), mask_); } + ActionUndoBlock(const ActionUndoBlock&) = delete; + ActionUndoBlock& operator=(const ActionUndoBlock&) = delete; + +private: + std::string label_; + int mask_; +}; + // tag() replaces any prior single-mode membership (a leaf lives in one mode; the // cross-mode case is show-both). void doTag(const std::string& modeId) { - for (const std::string& g : selectedTrackGuids()) - g_session->view().membership().tag(g, modeId); - reapplyActiveMode(); + const std::vector guids = selectedTrackGuids(); + if (guids.empty()) return; // nothing selected — no act, no undo point (cf. doMoveItems) + + { + ActionUndoBlock undo("ReaSampler: tag selected tracks -> " + modeId, kViewUndoMask); + for (const std::string& g : guids) g_session->view().membership().tag(g, modeId); + reapplyActiveMode(); + } persistViewState(PersistScope::Full); } // Shared body behind "Untag selected" and "Tag -> Arrange" — Arrange is the absence // of a tag, so the two actions are the same act. void doUntag() { - for (const std::string& g : selectedTrackGuids()) - g_session->view().membership().untag(g); - reapplyActiveMode(); + const std::vector guids = selectedTrackGuids(); + if (guids.empty()) return; + + { + ActionUndoBlock undo("ReaSampler: untag selected tracks", kViewUndoMask); + for (const std::string& g : guids) g_session->view().membership().untag(g); + reapplyActiveMode(); + } persistViewState(PersistScope::Full); } // Flips each track's pin independently — the honest semantics of a toggle on a // multi-selection (a mixed selection converges toward uniform only if it already was). void doShowBoth() { - MembershipIndex& m = g_session->view().membership(); - for (const std::string& g : selectedTrackGuids()) - m.setShowBoth(g, !m.isShowBoth(g)); - reapplyActiveMode(); + const std::vector guids = selectedTrackGuids(); + if (guids.empty()) return; + + { + ActionUndoBlock undo("ReaSampler: toggle show-both on selected tracks", kViewUndoMask); + MembershipIndex& m = g_session->view().membership(); + for (const std::string& g : guids) m.setShowBoth(g, !m.isShowBoth(g)); + reapplyActiveMode(); + } persistViewState(PersistScope::Full); } // Retag the current ITEM selection to `targetMode` (empty => untag -> Arrange // default). planItemRetag decides which items to retag (manual-lane items are -// EXEMPT), upholding the managed-lanes-only invariant. Wrapped in ONE Undo block. +// EXEMPT), upholding the managed-lanes-only invariant. // -// UNDO/SAVE ordering: persistViewState may pop a Save-As dialog, which must NOT sit -// inside the Undo block, so we close the block first, then persist. +// The mask stays UNDO_STATE_ALL rather than the view mask the three track actions +// close on: this one can mint lanes, and the fixed-lane properties are only ASSUMED to +// ride UNDO_STATE_ITEMS (see kViewUndoMask). void doMoveItems(const std::string& targetMode) { const std::vector selected = selectedRetagItems(); const std::vector ops = planItemRetag(selected, targetMode); @@ -242,21 +279,20 @@ void doMoveItems(const std::string& targetMode) { MembershipIndex& membership = g_session->view().membership(); - Undo_BeginBlock2(nullptr); - for (const ItemRetagOp& op : ops) { - if (op.untag) membership.untag(op.guid); - else membership.tag(op.guid, op.modeId); + { + ActionUndoBlock undo(targetMode.empty() + ? std::string("ReaSampler: untag selected items") + : "ReaSampler: move selected items -> " + targetMode, + -1); + for (const ItemRetagOp& op : ops) { + if (op.untag) membership.untag(op.guid); + else membership.tag(op.guid, op.modeId); + } + // Re-drive the same minting/apply path auto-tag uses: mint/split lanes for any + // track whose items now span modes, then reassert active-mode lane visibility. + mintManagedLanes(g_session->view(), nullptr); + reapplyActiveMode(); } - // Re-drive the same minting/apply path auto-tag uses: mint/split lanes for any - // track whose items now span modes, then reassert active-mode lane visibility. - mintManagedLanes(g_session->view(), nullptr); - reapplyActiveMode(); - - const std::string label = - targetMode.empty() - ? std::string("ReaSampler: untag selected items") - : std::string("ReaSampler: move selected items -> ") + targetMode; - Undo_EndBlock2(nullptr, label.c_str(), -1); persistViewState(PersistScope::Full); } diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index 08b40e4..03d8c10 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -64,22 +64,30 @@ decide membership or mode rules. offline/online runs on the background reclaimer (`fx_reclaim`), in ticks bounded by `kOnlineBudgetMs`/`kOfflineBudgetMs`, off `applyMode`'s block entirely (measured: `TrackFX_SetOffline` was 1897ms of a 1942ms switch). +- **`TrackFX_SetOffline` is the ONLY write in this feature that mints an undo point, + and a block around anything else can only manufacture one.** DAW-MEASURED, and the + premise every decision below rests on: ten writes of the four driven flags + (`B_SHOWINTCP`, `B_SHOWINMIXER`, `B_MAINSEND`, `I_FXEN`) from a timer callback, with + no block and no undo call, via `SetMediaTrackInfo_Value` and again via + `GetSetMediaTrackInfo`, added ZERO entries to the Undo History. Ten + `TrackFX_SetOffline` calls from a timer minted TEN points unblocked, TEN under a + block per write, and ONE under a single block spanning all ten — block WIDTH is the + entire effect there. The `("", 0)` discard form does NOT discard outside an action, + it coalesces; it discards only from a hand-fired action. A block opened on one timer + callback and closed on a LATER one holds. So: a path that writes only flags opens NO + block (a repair or a reapply must not cost the user a Ctrl-Z), and a path whose act + IS the user's opens one to make that act undoable at all — which is why the three + membership-edit actions own their own (`design_view_actions`' `ActionUndoBlock`) + rather than leaning on a reapply that now mints nothing. - **ONE mode switch is ONE undo point — the background FX parking included.** The driven flags, the lane state, AND the per-FX offline/online the reclaimer writes minutes-of-plugin-loads later all roll back in a single Ctrl-Z, because they share - ONE undo block: `applyMode` opens it (`fx_reclaim`'s `UndoSpan`) and a REAL switch - RETURNS WITH IT OPEN, for the reclaimer to close once the drain has converged. A - reapply closes its own before returning. This does not move an FX write back onto - the switch — the writes still run on the timer, off the 1897ms path. - DAW-MEASURED, and the reason the design is shaped this way: block WIDTH is the - entire effect. For ten `TrackFX_SetOffline` calls from a TIMER — unblocked, or - under a block per write, by every dispatch route — REAPER minted TEN points; under - ONE block spanning all ten it minted ONE. The `("", 0)` discard form does NOT - discard outside an action, it coalesces; it discards only from a hand-fired action. - A block opened on one timer callback and closed on a LATER one holds. Whether one - held from an ACTION into a later timer callback holds is the same property but is - `[verify — DAW]`; if it does not, the switch and its parking land as two points, - which is the intended degradation. + ONE undo block: `applyMode` opens it on a REAL SWITCH ONLY (`fx_reclaim`'s + `UndoSpan`) and RETURNS WITH IT OPEN, for the reclaimer to close once the drain has + converged. This does not move an FX write back onto the switch — the writes still + run on the timer, off the 1897ms path. Whether a block held from an ACTION into a + later timer callback holds is `[verify — DAW]`; if it does not, the switch and its + parking land as two points, which is the intended degradation. - **No path may return past a terminating condition with the span open.** A leaked block swallows every edit the user makes afterwards, indefinitely, so every terminating path routes through ONE idempotent close (`fxReclaimEndUndoSpan`): the @@ -129,16 +137,13 @@ decide membership or mode rules. rather than resuming on the next tick. (That a revert leaves the view model stale at all is the identity poll's defect, not the reclaimer's — this invariant only keeps the reclaimer from acting on it.) -- **A reapply that wrote nothing leaves no undo point** — and a reapply never holds - the span open, so a project open, a membership edit and `render_in_place`'s inner - reapply all keep the block shape they had. 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. +- **A reapply opens no undo block at all, so it can leave no point** — whether it + wrote or not, and whoever drove it: a project open, a membership edit, + `render_in_place`'s inner reapply. Nothing a reapply writes mints unaided (see the + measurement above), so a block there could only manufacture the Ctrl-Z opening a + project must not cost. A caller whose own act needs to stay undoable spans it + itself. A REAL switch still 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. - **A rapid A→B→A flip moves zero plugins.** The reclaimer only starts a park's OFFLINE work once a track has read continuously parked for `kParkHoldMs` (3000ms). A flip inside that window flips the flags twice but the debounce @@ -181,10 +186,10 @@ applies the resulting lane state to live tracks. ## Modules -- `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 OPENS the ONE undo span all of that rides (`applyMintsUndoPoint` decides whether it leaves a point; on a real switch `fx_reclaim` is what closes it). 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`.** FX are NOT written here — see `fx_reclaim`. +- `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 on a REAL SWITCH opens the ONE undo span all of that rides (`fx_reclaim` is what closes it; a reapply opens none). 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`.** FX are NOT written here — see `fx_reclaim`. - `view_park` — the park surface, FLAG DOMAIN ONLY. `parkTrack` captures the pre-park snapshot (flags AND the FX chain's identity-keyed offline state) when the model holds none, drives the four flags, and EXPOSEs a track that reads park-shaped with no record behind it rather than adopting it; `restoreTrack` writes the flags back from the snapshot and spends its flag half (`releaseSnapshotFlags`). Also `trackFlagParm` (the one `Flag` → REAPER-parameter mapping), `writeIfChanged` (the read-before-write every driven parameter goes through), `fxGuidString` (the one FX-identity read snapshot/reclaim both address FX through), `kViewUndoMask`, and `reportExposedTracks`/`describeExposedTracks` (named tracks, once per changed set per project). - `view_lanes` — managed fixed lanes: `mintManagedLanes` splits multi-mode content onto per-mode lanes, `applyLaneOps` drives `C_LANEPLAYS` for the active mode, `reconcileManagedLanes` recovers ownership from durable `P_LANENAME` on project open. Manual lanes are never minted, driven, renamed or reassigned. -- `view_reconcile` — the repair pass for a project state REAPER reloaded under us: `noteAppliedState` records what an apply left driven; `reconcileAppliedFlags` diffs that fingerprint against the reloaded model and hands back any track the reload no longer records but still reads driven — the fix for `UNDO_STATE_TRACKCFG` not carrying `B_SHOWINTCP`/`B_SHOWINMIXER` (DAW-confirmed). +- `view_reconcile` — the repair pass for a project state REAPER reloaded under us: `noteAppliedState` records what an apply left driven; `reconcileAppliedFlags` diffs that fingerprint against the reloaded model and hands back any track the reload no longer records but still reads driven — the fix for `UNDO_STATE_TRACKCFG` not carrying `B_SHOWINTCP`/`B_SHOWINMIXER` (DAW-confirmed). It opens no undo block: it runs in the wake of a Ctrl-Z, and its writes are flags only. - `view_tracks` — the scaffolding every write pass sets up: `TrackHandles`/`TrackByGuid`, `readFolderEntries`/`indexHandles`, and `UiRefreshHold` (the `PreventUIRefresh` RAII pair every write phase runs under). - `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`. - `fx_reclaim` — the FX convergence loop, off the switch's synchronous path (`TrackFX_SetOffline` was 1897ms of a measured 1942ms switch). `drainTick` moves one track's FX per step against `core/view/fx_state`'s derived work list (Online before Offline; an Offline item waits out `kParkHoldMs` so a fast A→B→A flip moves nothing), budgeted per tick (`kOnlineBudgetMs`/`kOfflineBudgetMs`) and gated off the transport for the Offline direction only. `FxReclaimQueue` owns the derived queue's lifetime across re-entrancy (a plugin unload is assumed to pump). `UndoSpan` owns the ONE undo block the whole drain writes under — opened by `applyMode` on a real switch, else lazily by the first `writeOffline`, and closed through the single idempotent `fxReclaimEndUndoSpan` that every terminating path routes through (`spanEndsAfter` decides which ticks terminate; `fxReclaimAbandonForStateLoad` is the state-load terminator, which also stops the drain). `ReleaseFlush`/`fxReclaimFlushRelease` make a completed record's spend durable on the project's next save, never on the drain's own initiative. `fxReclaimActivity` reports WHICH DIRECTION the drain is moving for an indicator to paint — Idle whenever nothing may move, so a debounced or transport-held offline item never names a direction nothing is travelling in; undirected `fxReclaimPending` stays the timer's one idle test and must not drive a UI (it is true throughout the debounce, when only the OPPOSITE direction is eligible). @@ -231,9 +236,10 @@ applies the resulting lane state to live tracks. unsaved project — land inside the block after all. That is a consequence of the span, not a decision made here; it does not resolve the redo hypothesis below, which turns on capture-time semantics the SDK does not document. `[verify — DAW]` - that a Save-As dialog opening under an open undo block is harmless. The rest of - this gotcha describes the reapply path, where nothing changed. The UNDO direction - still comes + that a Save-As dialog opening under an open undo block is harmless. On the reapply + path the write stays OUTSIDE every block, as it always did: the reapply opens none, + and a membership action's own `ActionUndoBlock` closes before `persistViewState` for + exactly this modal-Save-As reason. 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, and the cost is worse than the @@ -282,11 +288,12 @@ applies the resulting lane state to live tracks. refusal leaves the membership tags it already wrote in place, so the next reapply — not a user re-fire of render-in-place — is what brings the tracks into sync. -- `render_in_place` still calls `applyMode` inside its own `Undo_BeginBlock2` - pair for the reapply — one point rather than two for the flags and lane state - — but with no FX writes left inside `applyMode`'s block there is nothing left - to coalesce beyond that; the SDK's nested-block coalescing itself is still - `[verify — DAW]`. +- `render_in_place` calls `applyMode` inside its own `Undo_BeginBlock2` pair, and + that block is now the only one in play on that path — the reapply opens none of + its own, so nothing nests and the SDK's undocumented nested-block coalescing + stopped being load-bearing here. The same holds for the membership actions' + `ActionUndoBlock`; `doMoveItems` is the one that still nests, over + `mintManagedLanes`. - `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_park.cpp`) and SWS issue #802 is a diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index 71f2e7e..9a4ae38 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -586,7 +586,7 @@ public: return true; } - // A mode switch decides its label only after its writes (applyMintsUndoPoint). + // A mode switch names its point only after its writes, so the open cannot carry it. void relabel(std::string label, int mask) { if (!open_) return; label_ = std::move(label); @@ -664,14 +664,14 @@ bool fxReclaimDrainInProgress(); void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj); // Opens the undo span a mode switch and its background FX parking share, so the two are -// ONE Ctrl-Z. Called by applyMode in place of its own Undo_BeginBlock2; a REAL switch -// then returns with the span still open and the reclaimer closes it once the parking has -// drained. No-op if a span is already open, so callers END first (see applyMode) rather -// than nesting. +// ONE Ctrl-Z. Called by applyMode on a REAL SWITCH ONLY — a reapply opens no block at +// all — which then returns with the span still open, and the reclaimer closes it once +// the parking has drained. No-op if a span is already open, so callers END first (see +// applyMode) rather than nesting. void fxReclaimBeginUndoSpan(ReaProject* proj); // Sets what the eventual close names the point. Separate from the begin because the -// switch's own label is only decided after its writes (applyMintsUndoPoint). +// switch's own label is only decided after its writes. void fxReclaimLabelUndoSpan(const char* label, int mask); // The ONE close, idempotent: every terminating path routes through it — the drain diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index f33d5f6..7143d62 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -2,8 +2,8 @@ // reaper_plugin_functions.h without REAPERAPI_IMPLEMENT (main.cpp owns that). // Tree arithmetic lives in view_tree (pure), one track's park/restore in view_park // and the lane writes in view_lanes; this file owns the plan iteration, parent -// visibility, and opening the one undo span they all ride (fx_reclaim's UndoSpan, -// which a real switch hands to the reclaimer still open). +// visibility, and opening the undo span a REAL switch rides (fx_reclaim's UndoSpan, +// which it hands to the reclaimer still open). #include "shell/view/view.h" @@ -115,14 +115,17 @@ bool applyModeImpl(ViewModeModel& model, const std::string& targetModeId, ReaPro (targetMode ? targetMode->displayName : targetModeId) + " view"; bool laneModeChanged = false; - bool wrote = false; // anything at all this apply changed — see applyMintsUndoPoint std::vector exposedNames; // named while handles are fresh, reported after the writes // A prior switch's span, if the reclaimer has not closed it yet: this apply // supersedes it, so that parking lands as its own point rather than nesting under - // ours. Both calls are idempotent — the end reaches nothing when none is open. + // ours. Idempotent — it reaches nothing when none is open. fxReclaimEndUndoSpan(); - fxReclaimBeginUndoSpan(project); + + // ONLY a real switch opens one. Nothing a reapply writes mints on its own, so a + // block around it could only manufacture the undo point a project open must not + // cost — do not reintroduce one here (see this directory's CLAUDE.md). + if (realSwitch) fxReclaimBeginUndoSpan(project); { UiRefreshHold uiHold; // every write below lands with the TCP/MCP frozen @@ -141,7 +144,6 @@ bool applyModeImpl(ViewModeModel& model, const std::string& targetModeId, ReaPro MediaTrack* tr = resolve(trackByGuid, guid); if (!tr) continue; // stale/deleted GUID — prune const TrackParkResult r = parkTrack(model, guid, tr, tp.flags); - if (r.wrote) wrote = true; // Named here, not after the block: TrackList_AdjustWindows below can // invalidate the handle. if (r.exposed) exposedNames.push_back(trackDisplayName(tr)); @@ -152,14 +154,13 @@ bool applyModeImpl(ViewModeModel& model, const std::string& targetModeId, ReaPro const std::string& guid = tp.flags.front().guid; MediaTrack* tr = resolve(trackByGuid, guid); if (!tr) continue; // stale/deleted GUID — prune - if (restoreTrack(model, guid, tr, tp.flags)) wrote = true; + restoreTrack(model, guid, tr, tp.flags); } // 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. const LaneApplyResult lanes = applyLaneOps(trackByGuid, plan.lanes); laneModeChanged = lanes.freeMode; - if (lanes.wrote) wrote = true; // PARENT VISIBILITY (never parked): recomputed every toggle, never recorded. // Only the two visibility flags — never mainSend/FX on a parent. @@ -169,8 +170,8 @@ bool applyModeImpl(ViewModeModel& model, const std::string& targetModeId, ReaPro MediaTrack* tr = resolve(trackByGuid, node.guid); if (!tr) continue; // stale GUID — prune const double show = visible.count(node.guid) ? 1.0 : 0.0; - if (writeIfChanged(tr, "B_SHOWINTCP", show)) wrote = true; - if (writeIfChanged(tr, "B_SHOWINMIXER", show)) wrote = true; + writeIfChanged(tr, "B_SHOWINTCP", show); + writeIfChanged(tr, "B_SHOWINMIXER", show); } model.setActiveMode(targetModeId); @@ -197,14 +198,11 @@ bool applyModeImpl(ViewModeModel& model, const std::string& targetModeId, ReaPro // this apply (SDK requirement for I_FREEMODE changes). if (laneModeChanged) UpdateTimeline(); - const bool mint = applyMintsUndoPoint(realSwitch, wrote); - fxReclaimLabelUndoSpan(mint ? undoLabel.c_str() : "", mint ? kViewUndoMask : 0); - - // A REAL switch returns with the span STILL OPEN: the reclaimer closes it once the - // parking this apply is about to invalidate has drained, so the switch and its FX - // writes are ONE Ctrl-Z. Everything else closes here as it always did — a reapply is - // not a switch, and the load tick must not leave a block open across a project open. - if (!realSwitch) fxReclaimEndUndoSpan(); + // A real switch mints whether or not the plan found anything to write — it is an + // explicitly fired action and has to stay undoable — and RETURNS WITH THE SPAN STILL + // OPEN: the reclaimer closes it once the parking this apply is about to invalidate + // has drained, so the switch and its FX writes are ONE Ctrl-Z. + if (realSwitch) fxReclaimLabelUndoSpan(undoLabel.c_str(), kViewUndoMask); // Neither of the two below is a project write — a console message and an in-memory // fingerprint — so whether the span is still open over them is immaterial. diff --git a/src/shell/view/view.h b/src/shell/view/view.h index 643f7ba..8c3c973 100644 --- a/src/shell/view/view.h +++ b/src/shell/view/view.h @@ -1,9 +1,9 @@ #pragma once // REAPER-facing shell of Design View: reads the live folder tree, runs // ViewModeModel's pure planner, and applies the resulting flag / lane / parent -// writes under ONE undo block. See src/shell/view/CLAUDE.md for the enforced -// invariants (never touch master/mute, solo cached per mode and never lost, -// record-based restore). +// writes — under ONE undo block on a real switch, under none at all on a reapply. +// See src/shell/view/CLAUDE.md for the enforced invariants (never touch +// master/mute, solo cached per mode and never lost, record-based restore). #include @@ -22,20 +22,6 @@ 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 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 and the reconcile pass use 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. -inline bool applyMintsUndoPoint(bool realSwitch, bool wroteAnything) { - return realSwitch || wroteAnything; -} - // True while an applyMode call is on the stack. applyMode fails closed when // re-entered, and this lets a caller with no fallback test BEFORE it spends a // one-shot signal on a call that would be refused — see main.cpp's load glue. @@ -45,7 +31,8 @@ bool modeApplyInProgress(); // the outgoing mode's solo state and replays the incoming mode's, runs planToggle, // applies the park/restore flag writes plus managed-lane and parent-visibility // writes, then sets the active mode. ONE switch is ONE undo point; everything it -// moved rolls back in a single Ctrl-Z. +// moved rolls back in a single Ctrl-Z. A REAPPLY leaves no point at all — it opens no +// block, because none of what it writes mints one on its own. // // FX are NOT written here. Offlining a parked chain is the reclaimer's job, off this // path: TrackFX_SetOffline was 1897ms of a measured 1942ms switch, against 0.02ms for @@ -53,7 +40,8 @@ bool modeApplyInProgress(); // // A REAL switch therefore RETURNS WITH ITS UNDO BLOCK STILL OPEN (fx_reclaim's // UndoSpan): the reclaimer closes it once the parking has drained, which is what keeps -// the switch and its FX writes ONE point rather than two. A reapply closes its own. +// the switch and its FX writes ONE point rather than two. A reapply opens none, so a +// caller that wants its own act undoable opens its own (design_view_actions does). // // Returns false (no mutation) if `targetModeId` isn't registered, if this is a real // switch (target != active) while the transport is running, if it is re-entered diff --git a/src/shell/view/view_reconcile.cpp b/src/shell/view/view_reconcile.cpp index 6d605e8..0acb8a0 100644 --- a/src/shell/view/view_reconcile.cpp +++ b/src/shell/view/view_reconcile.cpp @@ -14,8 +14,6 @@ #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_EnumProjects #define REAPERAPI_WANT_TrackList_AdjustWindows -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #define REAPERAPI_WANT_UpdateArrange #define REAPERAPI_WANT_ValidatePtr2 #include "reaper_plugin_functions.h" @@ -58,8 +56,11 @@ void reconcileAppliedFlags(ViewModeModel& model, ReaProject* proj) { readFolderEntries(project, handleByGuid); const TrackByGuid trackByGuid = indexHandles(handleByGuid); + // NO undo block, deliberately: this runs in the wake of a Ctrl-Z, and the flag + // writes below mint nothing on their own — a block could only manufacture the point + // that would become the thing the user's next Ctrl-Z undoes. Do not reintroduce one + // (see this directory's CLAUDE.md). bool wrote = false; - Undo_BeginBlock2(project); { UiRefreshHold uiHold; for (const TrackFlagOp& op : result.ops) { @@ -69,7 +70,6 @@ void reconcileAppliedFlags(ViewModeModel& model, ReaProject* proj) { wrote = true; } } - Undo_EndBlock2(project, "", 0); // discard form — see the header if (wrote) { TrackList_AdjustWindows(false); // tracks reappearing owe the full relayout diff --git a/src/shell/view/view_reconcile.h b/src/shell/view/view_reconcile.h index 6c33328..07caa33 100644 --- a/src/shell/view/view_reconcile.h +++ b/src/shell/view/view_reconcile.h @@ -85,9 +85,9 @@ inline ProjectReconcileResult planReconcilePerProject( // once an undo has rolled the model back underneath it. void noteAppliedState(ReaProject* proj, const ViewModeModel& model); -// Runs planReconcile over the reloaded `model` and writes its ops. The undo block it -// opens is deliberately DISCARDED — this runs in the wake of a Ctrl-Z, and a repair -// that minted a point would become the thing the user's next Ctrl-Z undoes. +// Runs planReconcile over the reloaded `model` and writes its ops. It opens NO undo +// block — this runs in the wake of a Ctrl-Z, and a repair that minted a point would +// become the thing the user's next Ctrl-Z undoes. void reconcileAppliedFlags(ViewModeModel& model, ReaProject* proj); } // namespace reasampler diff --git a/tests/test_view_park.cpp b/tests/test_view_park.cpp index c8ef44a..5bed250 100644 --- a/tests/test_view_park.cpp +++ b/tests/test_view_park.cpp @@ -2,12 +2,10 @@ // test framework. // // The properties under test: the four flags a park drives address exactly the four -// REAPER parameters the tool owns (never mute, never solo), and one mode apply leaves -// at most ONE undo point — none at all when a reapply found everything already where -// it wanted it. +// REAPER parameters the tool owns (never mute, never solo), the re-park union, the +// park-shaped chain read, and the EXPOSE report's wording. #include "../src/shell/view/view_park.h" -#include "../src/shell/view/view.h" // applyMintsUndoPoint — the apply's undo-point fold #include "core/view/view_mode_model.h" // makeParkPlan — the park flags' only producer @@ -65,32 +63,6 @@ static void testAParkPlanDrivesEveryDrivenFlagToZero() { CHECK(parms.size() == 4); } -// -- the apply's one undo point ---------------------------------------------- -// -// 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 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 testAReapplyThatWroteSomethingLeavesAPoint() { - // A tag/untag reapply that actually parked a track: real project state moved. - CHECK(applyMintsUndoPoint(/*realSwitch=*/false, /*wroteAnything=*/true)); -} - -static void testAReapplyThatWroteNothingLeavesNoPointAtAll() { - // The project-load reapply over a project saved fully parked: every flag already - // sits where the plan wants it. Opening a project must not cost the user an undo - // step. - CHECK(!applyMintsUndoPoint(/*realSwitch=*/false, /*wroteAnything=*/false)); -} - // -- the re-park union: a plugin added between a restore and the next park -------- // // Sequence under test: park -> restore -> user adds plugin P -> re-park. Without the @@ -216,10 +188,6 @@ int main() { testNoDrivenFlagCanEverAddressMuteOrSolo(); testAParkPlanDrivesEveryDrivenFlagToZero(); - testARealSwitchAlwaysLeavesAPointEvenWithNothingToWrite(); - testAReapplyThatWroteSomethingLeavesAPoint(); - testAReapplyThatWroteNothingLeavesNoPointAtAll(); - testAPluginAddedWhileRestoredJoinsTheUnionRatherThanBeingDropped(); testAPluginRemovedWhileRestoredStaysInTheUnionForResolveFxRestoreToDrop();