diff --git a/CLAUDE.md b/CLAUDE.md index 8b4f9bc..88d0491 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,7 +6,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co **ReaSampler** is a per-project audio sample-bank capture tool that builds two artifacts: the REAPER extension (`reaper_reasampler`) and **ReaSampler 9000**, a Windows-only VST3 sampler instrument (`reasampler_9000.vst3`, `core/instrument/` + `shell/instrument/`, second CMake target `reasampler_vst`, gated on the vendored `vendor/vst3sdk` submodule slice). The pure-testable-core / REAPER-facing-shell discipline is preserved throughout: `core/` never includes REAPER or VST3 SDK types, `shell/` is where those hosts are actually touched, `app/` is the extension entry point. Every REAPER API name cited in project docs is correct-by-intent; verify argument order, types, and flag values against `vendor/reaper-sdk/sdk/reaper_plugin_functions.h` before use. -Per-module detail — what each file owns, its invariants — lives in the twenty-seven per-directory `src/**/CLAUDE.md` files; see the compact map in "Architecture: the load-bearing split" below to find the right one. Landed-phase history lives in `docs/ARCHIVE.md`; current work lives in `docs/COMPLETED.md`, `docs/TODO.md`, and `docs/TODO-1.0.md` — see "Project docs" below. +Per-module detail — what each file owns, its invariants — lives in the twenty-eight per-directory `src/**/CLAUDE.md` files; see the compact map in "Architecture: the load-bearing split" below to find the right one. Landed-phase history lives in `docs/ARCHIVE.md`; current work lives in `docs/COMPLETED.md`, `docs/TODO.md`, and `docs/TODO-1.0.md` — see "Project docs" below. ## Settled decisions @@ -84,7 +84,7 @@ There is no hot-reload. Copy the **Release** build's binary (`build/Release/` on ## Architecture: the load-bearing split -`core/` holds pure, unit-testable logic — no REAPER or VST3 SDK types, each with a corresponding `_tests` target that runs without a DAW. `shell/` holds the REAPER/host-facing shells — where those SDK types are actually touched. `app/` is the extension entry point. Each of the twenty-seven directories below carries its own `CLAUDE.md` with the full module list and that area's invariants — open the relevant one for detail; this file states only repo-wide truth. +`core/` holds pure, unit-testable logic — no REAPER or VST3 SDK types, each with a corresponding `_tests` target that runs without a DAW. `shell/` holds the REAPER/host-facing shells — where those SDK types are actually touched. `app/` is the extension entry point. Each of the twenty-eight directories below carries its own `CLAUDE.md` with the full module list and that area's invariants — open the relevant one for detail; this file states only repo-wide truth. | Directory | Scope | |---|---| @@ -114,6 +114,7 @@ There is no hot-reload. Copy the **Release** build's binary (`build/Release/` on | `src/shell/package/` | package filesystem I/O (streaming atomic read/write, exclusive-create landing), the rollback journal, and the REAPER file pickers | | `src/shell/panel/` | the docked bank-panel shell + the shared LICE draw kit | | `src/shell/persist/` | project ext-state persistence, prune filesystem I/O, usage scan | +| `src/shell/undo/` | the one REAPER undo-block opener every extension block goes through | | `src/shell/view/` | Design View mode application shell | ## Directory and namespace layout @@ -187,7 +188,7 @@ Comments carry *why*, and context where non-obvious — never *what* the code al 2. `rec->Register("gaccel", &accel)` — puts the action in the Actions list. 3. `rec->Register("hookcommand", ...)` — receives every action fired; claim only your own id, return `false` otherwise. 4. On unload (`rec == nullptr`), mirror-unregister everything with the same strings prefixed by `'-'`. -- **Non-main sections use a different mechanism.** `gaccel_register_t` carries no section field — `command_id` + `gaccel` can only ever produce a Main-section action. To publish into another section (Media Explorer = 32063, MIDI editor = 32060, MIDI event list = 32061, MIDI inline = 32062), register a `custom_action_register_t{uniqueSectionId, idStr, name, extra}` under `"custom_action"`; it returns the command id, or **0 on failure** (e.g. a duplicate `idStr`) — which the caller must tolerate rather than half-register. `idStr` must be unique **across all sections**, so an action published into both Main and a non-main section needs a SECOND id string; the FOREVER-STABLE contract binds it identically from the moment it ships. `custom_action_register_t` has no `ACCEL`, so a non-main entry ships no default keybinding. Dispatch for these ids arrives through `"hookcommand2"` (`bool(KbdSectionInfo*, int command, int val, int val2, int relmode, HWND)`) — `"hookcommand"` runs for the main section only. The two hooks must partition the ids between them; what happens when a command is claimed by both is unspecified by the SDK (`hookcommand2`'s doc says a `true` return prevents further hooks/actions from running, which is in tension with a clean double-fire either way), so nothing may rely on either outcome. On unload, mirror with `"-custom_action"` `[verify — DAW]` (the header confirms the `-` prefix for "most" registration types and spells out only `-pcmsrc` by name; `custom_action` itself is unconfirmed) and `"-hookcommand2"`. +- **Non-main sections use a different mechanism.** `gaccel_register_t` carries no section field — `command_id` + `gaccel` can only ever produce a Main-section action. To publish into another section (Media Explorer = 32063, MIDI editor = 32060, MIDI event list = 32061, MIDI inline = 32062), register a `custom_action_register_t{uniqueSectionId, idStr, name, extra}` under `"custom_action"`; it returns the command id, or **0 on failure** (e.g. a duplicate `idStr`) — which the caller must tolerate rather than half-register. `idStr` must be unique **across all sections**, so an action published into both Main and a non-main section needs a SECOND id string; the FOREVER-STABLE contract binds it identically from the moment it ships. `custom_action_register_t` has no `ACCEL`, so a non-main entry ships no default keybinding. Dispatch for these ids arrives through `"hookcommand2"` (`bool(KbdSectionInfo*, int command, int val, int val2, int relmode, HWND)`) — `"hookcommand"` runs for the main section only. `hookcommand2` is NOT the mirror-image "every other section" hook, and reading it that way would miss half the traffic: `reaper_plugin.h:214` says it runs prior to every action triggered by a KEY or MIDI event, in ANY section, main included — so a main-section keystroke can reach both hooks, in an order the SDK does not document. The two hooks must partition the ids between them; what happens when a command is claimed by both is unspecified by the SDK (`hookcommand2`'s doc says a `true` return prevents further hooks/actions from running, which is in tension with a clean double-fire either way), so nothing may rely on either outcome. On unload, mirror with `"-custom_action"` `[verify — DAW]` (the header confirms the `-` prefix for "most" registration types and spells out only `-pcmsrc` by name; `custom_action` itself is unconfirmed) and `"-hookcommand2"`. ## Product design docs diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt index 217f589..d07fb5e 100644 --- a/src/app/CMakeLists.txt +++ b/src/app/CMakeLists.txt @@ -38,6 +38,8 @@ add_library(reaper_reasampler MODULE ${REASAMPLER_SRC_DIR}/shell/panel/draw_kit.cpp ${LICE_SRC} ${REASAMPLER_SRC_DIR}/shell/capture/insert.cpp + ${REASAMPLER_SRC_DIR}/shell/undo/undo_block.cpp + ${REASAMPLER_SRC_DIR}/shell/undo/undo_intercept.cpp ${REASAMPLER_SRC_DIR}/shell/view/view.cpp ${REASAMPLER_SRC_DIR}/shell/view/fx_reclaim.cpp ${REASAMPLER_SRC_DIR}/shell/view/view_lanes.cpp @@ -76,6 +78,10 @@ reasampler_test(view_reconcile LINK applied_state view_mode_model) # The reclaimer's drain loop is templated on its host for exactly this reason: the # ordering, budget, debounce and resume bookkeeping run against a fake chain. reasampler_test(fx_reclaim LINK fx_state) +# Which dispatched action id ends the span. Header-only and SDK-free, so it links nothing; +# src/shell/undo has no CMakeLists of its own for the same reason src/shell/view has none. +add_executable(undo_intercept_tests ${REASAMPLER_TESTS_DIR}/test_undo_intercept.cpp) +add_test(NAME undo_intercept_tests COMMAND undo_intercept_tests) # Bank-package import: the promptless verb plus its action skin. Kept as its own # appended block rather than merged into the lists above, so the two package diff --git a/src/app/main.cpp b/src/app/main.cpp index 554568b..4d7ddb2 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -38,6 +38,7 @@ #include "shell/panel/panel_input.h" // bankPanelRefresh / bankPanelNotifyProjectLoaded #include "shell/panel/panel_window.h" // panel lifecycle (init/toggle/open-query/shutdown) #include "shell/persist/session.h" // ReaSamplerSession +#include "shell/undo/undo_intercept.h" // REAPER's own Undo/Redo, caught at dispatch #include "shell/view/fx_reclaim.h" // the FX reclaimer's tick #include "shell/view/view.h" // applyMode #include "shell/view/view_lanes.h" // reconcileManagedLanes @@ -107,7 +108,6 @@ static void RunShowVersion(int) { // the console window). ShowConsoleMsg(("ReaSampler " + reasampler::version::appVersion() + "\n").c_str()); } - // ONE row per bindable action this TU owns: FOREVER-STABLE id suffix, Actions-list // phrase, handler, per-row arg. Registration, hookcommand dispatch, and the unload // mirror-unregister all iterate this data. The capture scope rows come first, @@ -183,6 +183,17 @@ static void OnTimer() // the idle fast-path is a SINGLE POINTER TEST — drive only when a capture is live. if (capture::g_rtCapture) capture::DriveRealtimeCapture(g_session); + // A project load or save is IN FLIGHT, so this tick is running inside its own message + // pump — a restore brings plugins online and a plugin load pumps. Everything below + // would then act on a half-restored project: poll() reads the pre-load that + // OnBeginLoadProjectState defers past precisely because it is wrong, and the reclaim + // tick writes FX into a project REAPER is still rewriting. The extension's other + // re-entrancy guards cover OUR pumps; this one covers REAPER's. Consumes nothing, so + // the first tick after the load runs as if this one had never been due. Self-lowering + // (reaper_plugin_functions.h:1584-1587), and placed after the realtime drive so that + // fast path keeps its single-pointer-test shape. See src/shell/view/CLAUDE.md. + if (GetCurrentProjectInLoadSave()) return; + // poll()'s undo/redo-reload and project-switch Load branches call // loadFromProject, replacing g_session.view() wholesale — deferred here while // an applyMode is on the stack (reachable if a pumped message loop re-enters @@ -241,11 +252,17 @@ static void OnTimer() // TIMING: BeginLoadProjectState fires BEFORE any state restore, so reading // GetProjExtState here would return the PRE-undo value. Instead we raise a one-shot // reload request that OnTimer's poll() drains on the NEXT tick, once REAPER has -// finished restoring the block. A normal project open also fires this -// (isUndo=false); ignored here so a normal open flows solely through the timer's -// identity-transition Load path (no double load). +// finished restoring the block. Only the RELOAD REQUEST is undo-only: a +// normal open's reload belongs to the timer's identity-transition Load path, whose +// forked-sibling re-GUID branch requesting a reload here would bypass. static void OnBeginLoadProjectState(bool isUndo, project_config_extension_t* /*reg*/) { + // UNBRANCHED, and the reason the terminator is a hook at all: this is REAPER's one + // entry for a state load — "project load/undo", plus a new project + // (reaper_plugin.h:1203-1205) — and the last moment before the restore. Why that + // matters, and why isUndo does not narrow it: fxReclaimAbandonForStateLoad. + reasampler::fxReclaimAbandonForStateLoad(); + if (isUndo) g_session.requestReload(); } @@ -285,6 +302,10 @@ static project_config_extension_t g_projectConfig{ static bool OnHookCommand(int command, int /*flag*/) { if (command == 0) return false; + // FIRST, and ahead of every claim below: this hook runs "prior to every action in the + // main section" (reaper_plugin.h:202), so it is the last moment at which the mode + // switch's point can still be minted before REAPER's own undo runs. + reasampler::endReclaimSpanForUndoCommand(command, 0); if (reasampler::actionTableHandleCommand(command)) return true; if (reasampler::designViewHandleCommand(command)) return true; if (reasampler::bankHandleCommand(command)) return true; @@ -292,13 +313,20 @@ static bool OnHookCommand(int command, int /*flag*/) return false; } -// "hookcommand" covers the main section only, so actions we published into another -// section arrive here instead. Partitioning contract: root `CLAUDE.md` §"REAPER -// extension contract". -static bool OnHookCommand2(KbdSectionInfo* /*sec*/, int command, int /*val*/, int /*val2*/, +// Where the actions we published into a non-main section arrive — "hookcommand" runs for +// the main section only. Partitioning contract: root `CLAUDE.md` §"REAPER extension +// contract". +static bool OnHookCommand2(KbdSectionInfo* sec, int command, int /*val*/, int /*val2*/, int /*relmode*/, HWND /*hwnd*/) { if (command == 0) return false; + // The undo intercept runs from BOTH hooks, because this one is not the non-main + // sections' hook: it runs "prior to every action triggered by a key/MIDI event" + // (reaper_plugin.h:214) in ANY section, main included — and a Ctrl-Z is exactly a + // key event. Which of the two hooks REAPER calls first is undocumented and does not + // matter: the close is idempotent. A null section is left alone rather than assumed to + // be main; OnHookCommand covers the main section unconditionally either way. + reasampler::endReclaimSpanForUndoCommand(command, sec ? sec->uniqueID : -1); return reasampler::ingestHandleSectionCommand(command); } @@ -324,6 +352,10 @@ extern "C" REAPER_PLUGIN_DLL_EXPORT int REAPER_PLUGIN_ENTRYPOINT( // altered transport/cursor in the user's project on unload. capture::AbortRealtimeCaptureForUnload(g_session); + // Same reason, for the reclaimer's undo span: a block left open here has no + // owner left to close it and would swallow every edit the user makes after. + reasampler::fxReclaimEndUndoSpan(); + g_rec->Register("-timer", (void*)&OnTimer); g_rec->Register("-projectconfig", (void*)&g_projectConfig); g_rec->Register("-toggleaction", (void*)&OnToggleAction); diff --git a/src/shell/actions/CLAUDE.md b/src/shell/actions/CLAUDE.md index 56fc076..3e1e13e 100644 --- a/src/shell/actions/CLAUDE.md +++ b/src/shell/actions/CLAUDE.md @@ -29,9 +29,25 @@ is owned by other directories and only skinned here. separation is load-bearing here same as everywhere else. Only the arrange-capture surface writes the `assignment_request` wire; Media-Explorer import and file-drop onto the bank panel do not, and neither affects a live instance's selection. -- **Every bank index verb wraps its mutation in a batched REAPER undo point** - (`Undo_BeginBlock2`/`EndBlock2`, `UNDO_STATE_MISCCFG`) so one bank operation is - one Ctrl-Z. +- **Every undo block in this directory is opened through `shell/undo`'s + `UndoBlock`**, never `Undo_BeginBlock2` directly — the ingest surfaces, the two + drop shells and the Design View membership actions alike. That is what keeps a + user's act its own correctly-labelled Ctrl-Z while a Design View mode switch's FX + reclaim span is open; the mechanism and its compile-time enforcement are + `src/shell/undo/CLAUDE.md`'s. Bank index verbs additionally stay one batched + `UNDO_STATE_MISCCFG` point apiece, so one bank operation is one Ctrl-Z. +- **A Design View membership edit's undo point holds its `view_state` write — so the + persist runs INSIDE the block, and the point turns on a MODEL DIFF, not on "REAPER + was written to".** All four membership actions (tag, untag / tag→Arrange, show-both, + the item moves) share one body, `design_view_actions`' `runMembershipEdit`. + Everything the reapply they drive writes is the four driven track flags, which mint + no undo state at all (`src/shell/view/CLAUDE.md`), so a point closed BEFORE the + ext-state write covered no change: the edit was not undoable and rode whatever point + the user's next act minted. Two consequences bind. The persist's modal Save-As half + is split off and runs before the block opens — a modal may not hold a block open. + And a no-op edit records no label, because a recorded label re-titles the Undo button + whether or not REAPER appends an entry. Same shape as `shell/bank_ops`' + `persistBankOp`, for the same reason. - **The prune action is the ONLY file-deletion action in the system**; it opens no undo point (file deletion is not REAPER-undoable). It halts on `blockedByTracking` and prints whichever blockers fired — the malformed ledger, @@ -39,7 +55,7 @@ is owned by other directories and only skinned here. ## Modules -- `shell/actions` (`action_registry` / `design_view_actions` / `bank_actions` / `prune_action`) — the bindable action families, all routed via the `command_id`/`gaccel`/`hookcommand` contract. `action_registry` owns the shared registration plumbing (interned channel-qualified id strings; register and mirror-unregister present the identical pointer) **and the Q-W6 registration TABLE**: `main.cpp`'s own family (capture scopes, panel toggle, insert, batch, realtime, recapture, version) is one `ActionTableRow` array — suffix, phrase, flat function-pointer handler — that registration, hookcommand dispatch, and the unload mirror-unregister all iterate, so adding an action touches the table only (OCP). Bank mutations flow through the promptless `shell/bank_ops` verbs (`bankOp*` + `persistBankOp`, taking `ReaSamplerSession&`), which the panel menus and `bank_actions` consume as thin UX skins. **Every bank index verb wraps its mutation in a batched REAPER undo point (`Undo_BeginBlock2`/`EndBlock2`, `UNDO_STATE_MISCCFG`) so one bank operation is one Ctrl-Z.** The prune action (`prune_action`, `BANK_PRUNE_FOLDER`) is **the ONLY file-deletion action in the system**; it opens no undo point (file deletion is not REAPER-undoable). `BANK_PRUNE_FOLDER` halts on `blockedByTracking` and prints each blocker that fired, with recovery instructions. +- `shell/actions` (`action_registry` / `design_view_actions` / `bank_actions` / `prune_action`) — the bindable action families, all routed via the `command_id`/`gaccel`/`hookcommand` contract. `action_registry` owns the shared registration plumbing (interned channel-qualified id strings; register and mirror-unregister present the identical pointer) **and the Q-W6 registration TABLE**: `main.cpp`'s own family (capture scopes, panel toggle, insert, batch, realtime, recapture, version) is one `ActionTableRow` array — suffix, phrase, flat function-pointer handler — that registration, hookcommand dispatch, and the unload mirror-unregister all iterate, so adding an action touches the table only (OCP). Bank mutations flow through the promptless `shell/bank_ops` verbs (`bankOp*` + `persistBankOp`, taking `ReaSamplerSession&`), which the panel menus and `bank_actions` consume as thin UX skins; `src/shell/bank_ops/CLAUDE.md` owns the one-op-one-Ctrl-Z rule those verbs enforce. The prune action (`prune_action`, `BANK_PRUNE_FOLDER`) is **the ONLY file-deletion action in the system**; it opens no undo point (file deletion is not REAPER-undoable). `BANK_PRUNE_FOLDER` halts on `blockedByTracking` and prints each blocker that fired, with recovery instructions. - `package_export_action` — the "export bank as package" skin: survey and report first, confirm what is absent (and, separately, a destination being replaced), pick a destination, write. Every prompt in the flow lives here so `shell/package/export_bank` stays promptless. Read-only against the project — it holds the session by `const&`, so no ext-state write, generation bump or undo point is reachable. Registration rides `main.cpp`'s action table (`EXPORT_BANK_PACKAGE`); the panel's tab menu is the second skin over the same body. - `drag_out_win` — the host/OS boundary of drag-out: Windows OLE `DoDragDrop`/`CF_HDROP`, copy-only (`DROPEFFECT_MOVE` not offered); macOS/Linux via `SWELL_InitiateDragDropOfFileList`. Also owns `pointerOverHostWindow`, the window-ownership test behind `ui::DropContext::pointerOffHost` (`core/ui/CLAUDE.md` owns the hand-off rule it feeds) — process identity on Windows (`WindowFromPoint` + `GetWindowThreadProcessId`), non-null `WindowFromPoint` under SWELL, whose window list is already this-process-only. Its shipped exception: a plugin bridged into `reaper_host*.exe` reports off-host though the pointer never left REAPER. - `instrument_drop_win` — instrument-drop shell: `probeDropTarget` resolves a screen point to a track + a `ReaperSurface` (via the pure `wire::classifyReaperSurface`, whose token rules `core/wire/CLAUDE.md` owns) — it answers only what REAPER named, never whether the pointer is still in REAPER — and the drop half adds a ReaSampler 9000 instance and applies the dragged capture's state via a transient `.vstpreset` + `TrackFX_SetPreset` (the former `TrackFX_SetNamedConfigParm` "vst_chunk" write was silently unappliable for VST3). Exposes `loadInstrumentOntoTrack` (inner half, no own undo block) and `performInstrumentDrop` (wraps in its own undo block). **Never captures, never writes the bank, never inserts a timeline item.** diff --git a/src/shell/actions/arrange_drop_win.cpp b/src/shell/actions/arrange_drop_win.cpp index 8886413..41ee6b8 100644 --- a/src/shell/actions/arrange_drop_win.cpp +++ b/src/shell/actions/arrange_drop_win.cpp @@ -29,6 +29,7 @@ #include #include "core/capture/insert_plan.h" // computeInsertMode — the ONE InsertMedia bitfield owner +#include "shell/undo/undo_block.h" // the ONE undo-block opener #include "reaper_plugin.h" @@ -42,8 +43,6 @@ #define REAPERAPI_WANT_SetOnlyTrackSelected #define REAPERAPI_WANT_SetTrackSelected #define REAPERAPI_WANT_SnapToGrid -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #include "reaper_plugin_functions.h" namespace reasampler { @@ -98,7 +97,7 @@ int performArrangeDrop(MediaTrack* track, double time, // One undo block around the whole drop (every item plus the selection/cursor restore) so a // single Ctrl-Z returns the project to exactly its pre-drop state. - Undo_BeginBlock2(nullptr); + UndoBlock undo(nullptr); SetOnlyTrackSelected(track); SetEditCurPos(SnapToGrid(nullptr, time), /*moveview=*/false, /*seekplay=*/false); @@ -111,9 +110,6 @@ int performArrangeDrop(MediaTrack* track, double time, if (InsertMedia(path.c_str(), mode) != 0) ++inserted; } - // A fixed stack buffer, not std::string concatenation: the prior shape built the label with - // std::to_string + `+` between the restore below and Undo_EndBlock2, so a bad_alloc there - // would leave an unbalanced undo block open. snprintf here removes the allocation outright. char label[64]; std::snprintf(label, sizeof(label), "ReaSampler: drop %d %s onto arrange", inserted, inserted == 1 ? "capture" : "captures"); @@ -122,7 +118,7 @@ int performArrangeDrop(MediaTrack* track, double time, SetEditCurPos(priorCursor, /*moveview=*/false, /*seekplay=*/false); // extraflags -1 = UNDO_STATE_ALL, matching the insert action's own block. - Undo_EndBlock2(nullptr, label, -1); + undo.recordPoint(label, -1); return inserted; } diff --git a/src/shell/actions/design_view_actions.cpp b/src/shell/actions/design_view_actions.cpp index e83e5e1..5c947fd 100644 --- a/src/shell/actions/design_view_actions.cpp +++ b/src/shell/actions/design_view_actions.cpp @@ -25,8 +25,10 @@ #include "shell/capture/item_read.h" // shared MediaItem* -> GUID + fixed-lane-name reads (D2 W3-B) #include "shell/capture/track_guid.h" // shared MediaTrack* -> canonical GUID key #include "shell/panel/panel_window.h" // bankPanelInvalidate — footer toggle repaint +#include "shell/undo/undo_block.h" // the ONE undo-block opener #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 @@ -39,8 +41,6 @@ #define REAPERAPI_WANT_Main_SaveProject #define REAPERAPI_WANT_Help_Set #define REAPERAPI_WANT_ShowConsoleMsg -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #include "reaper_plugin_functions.h" namespace reasampler { @@ -104,10 +104,11 @@ std::vector selectedTrackGuids() { } // Reapplies the CURRENT active mode so a membership mutation takes visible effect -// immediately (park/unpark/re-derive parents). The return is deliberately discarded: a -// refused reapply leaves the membership edit standing and unapplied, and the user's next -// edit or mode switch reapplies it. What must NOT be dropped with it is the reclaimer's -// queue, which is why applyMode invalidates on its refusal path too (view.cpp). +// immediately (park/unpark/re-derive parents). `applied` is deliberately discarded: a +// refused reapply leaves the membership edit standing and unapplied, and the user's +// next edit or mode switch reapplies it. What must NOT be dropped with it is the +// reclaimer's queue, which is why applyMode invalidates on its refusal path too +// (view.cpp). void reapplyActiveMode() { applyMode(g_session->view(), g_session->view().activeModeId(), nullptr); } @@ -143,33 +144,45 @@ std::vector selectedRetagItems() { // serialization. Every other action keeps the full save. enum class PersistScope { Full, ViewOnly }; -// Persists the bank + Design-View model after every state-changing action so the -// view model is not lost across save/close/reopen. If membership is non-empty and -// 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) { - if (!g_session->view().membership().empty()) { - ReaProject* proj = EnumProjects(-1, nullptr, 0); - if (proj) { - auto readRppPath = [&]() -> std::string { - std::vector buf(4096, '\0'); - EnumProjects(-1, buf.data(), static_cast(buf.size())); - return std::string(buf.data()); - }; +// The persist's MODAL half, split from the write below so a caller can run the two on +// opposite sides of an undo block. If membership is non-empty and the project is +// unsaved, prompts Save-As (mirrors the flow capture uses) — DAW-ONLY: +// Main_SaveProject(proj, true) blocks until the dialog is dismissed. False means the +// user declined and nothing may be written. +bool ensurePersistTarget() { + if (g_session->view().membership().empty()) return true; - if (readRppPath().empty()) { - Main_SaveProject(proj, true); - if (readRppPath().empty()) { // still empty -> user cancelled - ShowConsoleMsg( - "ReaSampler: Design View state will not persist until " - "the project is saved.\n"); - return; - } - } - } - } - if (scope == PersistScope::ViewOnly) g_session->saveViewStateOnly(); - else g_session->saveToActiveProject(); + ReaProject* proj = EnumProjects(-1, nullptr, 0); + if (!proj) return true; // no active project — the write below no-ops on its own + + auto readRppPath = [&]() -> std::string { + std::vector buf(4096, '\0'); + EnumProjects(-1, buf.data(), static_cast(buf.size())); + return std::string(buf.data()); + }; + + if (!readRppPath().empty()) return true; + + Main_SaveProject(proj, true); + if (!readRppPath().empty()) return true; // still empty -> user cancelled + + ShowConsoleMsg("ReaSampler: Design View state will not persist until " + "the project is saved.\n"); + return false; +} + +// The persist's WRITE half: the bank + Design-View model, so neither is lost across +// save/close/reopen. Returns whether the write was ISSUED — false means no active +// project or an unsaved one, with NOTHING written (session.h owns that contract), which +// is the only reading a caller may turn into a discarded undo point. +bool writePersistedState(PersistScope scope) { + return scope == PersistScope::ViewOnly ? g_session->saveViewStateOnly() + : g_session->saveToActiveProject(); +} + +// The two halves as a caller holding no undo block runs them. +void persistViewState(PersistScope scope) { + if (ensurePersistTarget()) writePersistedState(scope); } // The footer segment already reads disabled while the transport runs, but an action can @@ -188,77 +201,137 @@ void doToggleMode() { const std::string target = nextModeId(g_session->view().modes(), g_session->view().activeModeId()); if (target.empty()) return; // no modes to cycle to (degenerate) - if (!applyMode(g_session->view(), target, nullptr)) { reportModeSwitchRefused(); return; } + if (!applyMode(g_session->view(), target, nullptr).applied) { + reportModeSwitchRefused(); + return; + } persistViewState(PersistScope::ViewOnly); bankPanelInvalidate(); // repaint the footer [Arrange|Design] toggle immediately } -// Direct jump to a named mode. applyMode is a no-op (returns false, no mutation) if +// Direct jump to a named mode. applyMode is a no-op (`applied` false, no mutation) if // the id is unregistered or the transport is running, so both fail safe. void doActivateMode(const std::string& modeId) { - if (!applyMode(g_session->view(), modeId, nullptr)) { reportModeSwitchRefused(); return; } + if (!applyMode(g_session->view(), modeId, nullptr).applied) { + reportModeSwitchRefused(); + return; + } persistViewState(PersistScope::ViewOnly); bankPanelInvalidate(); // repaint the footer [Arrange|Design] toggle immediately } +// The drive half of a membership edit: what runs inside the block, answering whether +// it made a project change of its own that a point must hold even with nothing +// persisted. The reapply alone never does — everything it writes is the four driven +// flags, which mint no undo state (src/shell/view/CLAUDE.md). +bool driveReapply() { + reapplyActiveMode(); + return false; +} + +// The item moves' drive: re-drive the same minting/apply path auto-tag uses — mint or +// split lanes for any track whose items now span modes, then reassert active-mode lane +// visibility. A mint moves items between lanes, which is a real project change whether +// or not the persist below lands. +bool driveMintLanesAndReapply() { + const bool minted = mintManagedLanes(g_session->view(), nullptr); + reapplyActiveMode(); + return minted; +} + +// The ONE body behind all four membership edits (this directory's CLAUDE.md owns the +// rule; what follows is why the ORDER is what it is). `mutate` is a pure model edit — +// nothing in it reaches REAPER. +// +// The prompt sits between the edit and the block because it is pinned on both sides: +// its condition reads the POST-edit membership (a first-ever tag on an unsaved project +// must still prompt), and a modal may not stand inside an open block. The persist then +// runs INSIDE, being the only thing here a point can hold; a re-tag of tracks already +// in that mode re-writes byte-identical ext state, which is why the label turns on the +// model diff and not on the write. +template +void runMembershipEdit(Mutate&& mutate, bool (*drive)(), const std::string& label, + int mask) { + const MembershipIndex before = g_session->view().membership(); + mutate(); + const bool changed = !(g_session->view().membership() == before); + + const bool persistable = ensurePersistTarget(); + + UndoBlock undo(nullptr); + const bool structural = drive(); + const bool persisted = persistable && writePersistedState(PersistScope::Full); + if (structural || (changed && persisted)) + undo.recordPoint(label, mask); + // Otherwise the block closes on its ("", 0) discard form — real from a hand-fired + // action (DAW-measured, src/shell/view/CLAUDE.md) — leaving history and the Undo + // button both untouched. +} + // 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(); - persistViewState(PersistScope::Full); + const std::vector guids = selectedTrackGuids(); + if (guids.empty()) return; // nothing selected — no act, no undo point (cf. doMoveItems) + + runMembershipEdit( + [&] { + for (const std::string& g : guids) g_session->view().membership().tag(g, modeId); + }, + &driveReapply, "ReaSampler: tag selected tracks -> " + modeId, kViewUndoMask); } // 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(); - persistViewState(PersistScope::Full); + const std::vector guids = selectedTrackGuids(); + if (guids.empty()) return; + + runMembershipEdit( + [&] { + for (const std::string& g : guids) g_session->view().membership().untag(g); + }, + &driveReapply, "ReaSampler: untag selected tracks", kViewUndoMask); } // 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(); - persistViewState(PersistScope::Full); + const std::vector guids = selectedTrackGuids(); + if (guids.empty()) return; + + runMembershipEdit( + [&] { + MembershipIndex& m = g_session->view().membership(); + for (const std::string& g : guids) m.setShowBoth(g, !m.isShowBoth(g)); + }, + &driveReapply, "ReaSampler: toggle show-both on selected tracks", kViewUndoMask); } // 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); if (ops.empty()) return; // nothing selected, or every selected item was exempt/empty - 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); - } - // 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); + runMembershipEdit( + [&] { + MembershipIndex& membership = g_session->view().membership(); + for (const ItemRetagOp& op : ops) { + if (op.untag) membership.untag(op.guid); + else membership.tag(op.guid, op.modeId); + } + }, + &driveMintLanesAndReapply, + targetMode.empty() ? std::string("ReaSampler: untag selected items") + : "ReaSampler: move selected items -> " + targetMode, + -1); } } // namespace diff --git a/src/shell/actions/ingest.cpp b/src/shell/actions/ingest.cpp index 65ce842..538494d 100644 --- a/src/shell/actions/ingest.cpp +++ b/src/shell/actions/ingest.cpp @@ -22,6 +22,7 @@ #include "core/wire/instrument_drop.h" // pure buildInstrumentDropPreset (.vstpreset image for a sampleId) #include "shell/actions/instrument_drop_win.h" // shell loadInstrumentOntoTrack (FX add+apply, no own undo block) #include "shell/persist/session.h" // ReaSamplerSession +#include "shell/undo/undo_block.h" // the ONE undo-block opener #include "core/capture/wav_codec.h" // parseWavLayout (32f fast-path validator), buildFloat32Wav, hashWavContent @@ -36,8 +37,6 @@ #define REAPERAPI_WANT_GetMediaSourceNumChannels #define REAPERAPI_WANT_GetMediaSourceSampleRate #define REAPERAPI_WANT_GetMediaSourceLength -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #define REAPERAPI_WANT_GetSelectedTrack #include "reaper_plugin_functions.h" @@ -362,14 +361,14 @@ void doImportFromMediaExplorer() { // so any open VST3 browser instances refresh to show the new sound. bool persisted = true; // true when nothing needed persisting (dedup) if (r.added) { - Undo_BeginBlock2(nullptr); - g_session->bumpBankGeneration(); - persisted = g_session->saveToActiveProject(); - if (persisted) - Undo_EndBlock2(nullptr, "ReaSampler: import Media Explorer file into bank", - UNDO_STATE_MISCCFG); - else - Undo_EndBlock2(nullptr, "", 0); + { + UndoBlock undo(nullptr); + g_session->bumpBankGeneration(); + persisted = g_session->saveToActiveProject(); + if (persisted) + undo.recordPoint("ReaSampler: import Media Explorer file into bank", + UNDO_STATE_MISCCFG); + } bankPanelRefresh(); } ShowConsoleMsg(("ReaSampler ingest: " + r.message + @@ -386,20 +385,21 @@ void doImportFromMediaExplorer() { // Persist happens INSIDE the block and BEFORE the FX add so the new instance's // setState -> reloadInstrument sees the just-persisted sample. - Undo_BeginBlock2(nullptr); bool persisted = true; // true when nothing needed persisting (dedup) - if (r.added) { - g_session->bumpBankGeneration(); - persisted = g_session->saveToActiveProject(); + bool placed = false; + { + UndoBlock undo(nullptr); + if (r.added) { + g_session->bumpBankGeneration(); + persisted = g_session->saveToActiveProject(); + } + placed = loadInstrumentOntoTrack(target, preset); + // Anything else — the FX add/inject failed (already rolled back, no orphan), or + // the project was unsaved (persist no-op) — records no point at all. + if (placed && persisted) + undo.recordPoint("ReaSampler: import from Media Explorer into selected track", + UNDO_STATE_MISCCFG); } - const bool placed = loadInstrumentOntoTrack(target, preset); - if (placed && persisted) - Undo_EndBlock2(nullptr, "ReaSampler: import from Media Explorer into selected track", - UNDO_STATE_MISCCFG); - else - // Either the FX add/inject failed (already rolled back, no orphan) or the - // project was unsaved (persist no-op): discard so no empty point is recorded. - Undo_EndBlock2(nullptr, "", 0); bankPanelRefresh(); if (placed) @@ -450,13 +450,14 @@ void ingestDroppedFiles(const std::vector& absolutePaths) { // One undo point for the whole drop, opened only if a NEW index entry was created. if (importedNew > 0) { - Undo_BeginBlock2(nullptr); - g_session->bumpBankGeneration(); - const bool persisted = g_session->saveToActiveProject(); - if (persisted) - Undo_EndBlock2(nullptr, "ReaSampler: import dropped file(s)", UNDO_STATE_MISCCFG); - else - Undo_EndBlock2(nullptr, "", 0); + bool persisted = false; + { + UndoBlock undo(nullptr); + g_session->bumpBankGeneration(); + persisted = g_session->saveToActiveProject(); + if (persisted) + undo.recordPoint("ReaSampler: import dropped file(s)", UNDO_STATE_MISCCFG); + } bankPanelRefresh(); const std::string msg = "ReaSampler ingest: imported " + std::to_string(importedTotal) + diff --git a/src/shell/actions/instrument_drop_win.cpp b/src/shell/actions/instrument_drop_win.cpp index d385c49..732cf9d 100644 --- a/src/shell/actions/instrument_drop_win.cpp +++ b/src/shell/actions/instrument_drop_win.cpp @@ -13,6 +13,7 @@ #include "core/version/app_version.h" // vstPluginName() — the CHANNEL-correct FX name (stable/beta pairing) #include "core/wire/instrument_drop.h" // classifyReaperSurface — the PURE, unit-tested classifier +#include "shell/undo/undo_block.h" // the ONE undo-block opener #include "reaper_plugin.h" @@ -22,8 +23,6 @@ #define REAPERAPI_WANT_TrackFX_Delete #define REAPERAPI_WANT_TrackFX_GetCount #define REAPERAPI_WANT_TrackFX_SetPreset -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #include "reaper_plugin_functions.h" namespace reasampler { @@ -138,9 +137,9 @@ bool loadInstrumentOntoTrack(MediaTrack* track, const std::vector& bool performInstrumentDrop(MediaTrack* track, const std::vector& presetBytes) { if (!track || presetBytes.empty()) return false; - Undo_BeginBlock2(nullptr); + UndoBlock undo(nullptr); const bool ok = loadInstrumentOntoTrack(track, presetBytes); - Undo_EndBlock2(nullptr, "ReaSampler: drop capture onto FX chain", -1); + undo.recordPoint("ReaSampler: drop capture onto FX chain", -1); return ok; } diff --git a/src/shell/bank_ops/CLAUDE.md b/src/shell/bank_ops/CLAUDE.md index 0e3abc5..6f67a05 100644 --- a/src/shell/bank_ops/CLAUDE.md +++ b/src/shell/bank_ops/CLAUDE.md @@ -26,13 +26,24 @@ calls into that model, it does not reimplement its rules. - **Movement is index-only.** Moving/copying a sample between banks removes/adds the index entry only; the underlying file never moves on disk. No bank operation writes, moves, or deletes a file. -- **One bank operation is one Ctrl-Z.** Every bank index verb wraps its mutation in - a batched REAPER undo point (`Undo_BeginBlock2`/`EndBlock2`, `UNDO_STATE_MISCCFG`) - via `persistBankOp` — this is enforced in this directory, not left to callers. +- **One bank operation is one Ctrl-Z, whatever else is in flight.** Every bank index + verb wraps its mutation in a batched REAPER undo point (`UNDO_STATE_MISCCFG`) via + `persistBankOp` — enforced in this directory, not left to callers. The block is + opened through `shell/undo`'s `UndoBlock`, which is what makes the claim hold while + a Design View mode switch's FX reclaim span is open: without it the verb's own + close landed at depth 2->1, minted nothing, and the rename or delete was absorbed + into the mode switch, un-undoable on its own. Reason and mechanism: + `src/shell/undo/CLAUDE.md`. ACCEPTED, same cost `view_lanes`' `mintManagedLanes` + already carries (`src/shell/view/view_lanes.cpp`'s note at its own `UndoBlock`): + opening `persistBankOp`'s block ends any pending reclaim span unconditionally, + including on the unsaved-project path above, which then records no point of its + own — the switch's point still mints early, and its remaining parking mints again + under `kReclaimUndoLabel`. Bounded at one extra undo point; the alternative + (nesting over the span) destroys the switch's point outright. ## Modules -- `bank_ops` (`shell/bank_ops`) — the promptless bank-mutation verb seam (Q-W6 lift out of `panel_bank_ops`): `bankOpCreate`/`Rename`/`Delete`/`Evacuate`/`Activate`/`Transfer`/`Remove` + `persistBankOp` (the undo-batched ext-state persist), each taking a `ReaSamplerSession&` and returning whether the model accepted the mutation — no prompts, no message boxes, no panel-state reads. `shell/panel/panel_bank_ops` (menu/prompt UX) and `shell/actions/bank_actions` (bindable-action UX) both consume these as thin skins, so the mutation logic has exactly one home. +- `bank_ops` (`shell/bank_ops`) — the promptless bank-mutation verb seam (Q-W6 lift out of `panel_bank_ops`): `bankOpCreate`/`Rename`/`Delete`/`Evacuate`/`Activate`/`Transfer`/`Remove` + `persistBankOp` (the `UndoBlock`-batched ext-state persist), each taking a `ReaSamplerSession&` and returning whether the model accepted the mutation — no prompts, no message boxes, no panel-state reads. `shell/panel/panel_bank_ops` (menu/prompt UX) and `shell/actions/bank_actions` (bindable-action UX) both consume these as thin skins, so the mutation logic has exactly one home. ## Gotchas diff --git a/src/shell/bank_ops/bank_ops.cpp b/src/shell/bank_ops/bank_ops.cpp index 134715e..1631621 100644 --- a/src/shell/bank_ops/bank_ops.cpp +++ b/src/shell/bank_ops/bank_ops.cpp @@ -14,12 +14,11 @@ #include "core/model/bank_book.h" // BankBook / TransferResult / RemoveScope #include "shell/persist/session.h" // ReaSamplerSession — the session the verbs mutate +#include "shell/undo/undo_block.h" // the ONE undo-block opener #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_genGuid #define REAPERAPI_WANT_guidToString -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #include "reaper_plugin_functions.h" namespace reasampler { @@ -45,18 +44,15 @@ std::string mintBankId() { // ext-state is the block's "after" image. // // UNSAVED-PROJECT GUARDRAIL: on an unsaved/no-active project saveToActiveProject() -// no-ops; we still CLOSE the block, but with an empty label + zero flag so REAPER -// discards the point instead of recording a no-effect undo entry. Quiet persist by -// design (mirrors capture, not Design-View) — deliberately no Save-As prompt. +// no-ops, so recordPoint below is skipped — the block closes on its own default +// discard form (empty label, zero flag) instead of recording a no-effect undo entry. +// Quiet persist by design (mirrors capture, not Design-View) — deliberately no +// Save-As prompt. void persistBankOp(ReaSamplerSession& session, const char* label, bool bumpGeneration) { - Undo_BeginBlock2(nullptr); + UndoBlock undo(nullptr); if (bumpGeneration) session.bumpBankGeneration(); - const bool persisted = session.saveToActiveProject(); - if (persisted) - Undo_EndBlock2(nullptr, label, UNDO_STATE_MISCCFG); - else - Undo_EndBlock2(nullptr, "", 0); // no ext-state write -> discard the empty point + if (session.saveToActiveProject()) undo.recordPoint(label, UNDO_STATE_MISCCFG); } // --- Promptless inner bank verbs (one home) ------------------------------------ diff --git a/src/shell/capture/CLAUDE.md b/src/shell/capture/CLAUDE.md index 064b826..705eecd 100644 --- a/src/shell/capture/CLAUDE.md +++ b/src/shell/capture/CLAUDE.md @@ -20,6 +20,10 @@ relative-paths-only) and the load-bearing capture/placement separation — see r `CLAUDE.md` §Precision invariants and §The load-bearing principle. Shell-specific detail not covered there: +- **Every undo block here is opened through `shell/undo`'s `UndoBlock`**, never + `Undo_BeginBlock2` directly — `insert`, `capture_orchestrator`, `capture_batch`, + `bake_land` and `render_in_place` alike. Rule and enforcement: + `src/shell/undo/CLAUDE.md`. - **The item scope's render source is window-dependent.** `ResolveScopeSource` is where that is decided — it measures the selected items' extent against the resolved range (`core/capture/render_window`) and hands the answer to diff --git a/src/shell/capture/bake_land.cpp b/src/shell/capture/bake_land.cpp index 6704755..add8848 100644 --- a/src/shell/capture/bake_land.cpp +++ b/src/shell/capture/bake_land.cpp @@ -1,7 +1,8 @@ // bake_land.cpp — see bake_land.h. main.cpp owns the API pointers; this TU gets them // extern. REAPER symbols used here (EnumProjects, EnumProjExtState, GetProjExtState, -// SetProjExtState, ShowConsoleMsg, Undo_BeginBlock2/EndBlock2) are verified against -// vendor/reaper-sdk/sdk/reaper_plugin_functions.h. Landing one request is bake_landing's. +// SetProjExtState, ShowConsoleMsg) are verified against +// vendor/reaper-sdk/sdk/reaper_plugin_functions.h. Landing one request is +// bake_landing's; the undo block is shell/undo's. #include "shell/capture/bake_land.h" @@ -19,6 +20,7 @@ #include "shell/capture/bake_landing.h" #include "shell/panel/panel_input.h" // bankPanelRefresh #include "shell/persist/session.h" +#include "shell/undo/undo_block.h" // the ONE undo-block opener #include "reaper_plugin.h" // UNDO_STATE_MISCCFG @@ -28,8 +30,6 @@ #define REAPERAPI_WANT_GetProjExtState #define REAPERAPI_WANT_SetProjExtState #define REAPERAPI_WANT_ShowConsoleMsg -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #include "reaper_plugin_functions.h" namespace reasampler::capture { @@ -107,26 +107,6 @@ struct ScannedKey { wire::BakeKeyOutcome report; }; -// Undo_BeginBlock2/EndBlock2 as stack RAII, the FxBypassGuard discipline: a landing reads a -// whole WAV and hashes it, so a bad_alloc between the two calls would otherwise leave an -// open undo block in the user's project. Closing with an empty description records no undo -// point, which is what every path that landed nothing wants anyway. -class UndoBlock { -public: - UndoBlock() { Undo_BeginBlock2(nullptr); } - ~UndoBlock() { Undo_EndBlock2(nullptr, description_.c_str(), flags_); } - void recordPoint(std::string description, int flags) { - description_ = std::move(description); - flags_ = flags; - } - UndoBlock(const UndoBlock&) = delete; - UndoBlock& operator=(const UndoBlock&) = delete; - -private: - std::string description_; - int flags_ = 0; -}; - // One entry's write-back, and the proof of whether it took. Guarded whole: it encodes into // a fresh string and readKey grows a buffer toward 16 MB, and a throw escaping here would // skip the ShowConsoleMsg for EVERY key — the silent console this action exists to make @@ -206,7 +186,7 @@ void RunResampleBake(ReaSamplerSession& session) { // the answers already buffered — the exact no-answer-with-a-silent-console this action // exists to make impossible. UndoBlock is inside the try, so unwinding still closes it. try { - UndoBlock undo; + UndoBlock undo(nullptr); for (const OpenProject& open : openProjects()) { ++tally.tabsScanned; const bool activeTab = static_cast(open.proj) == active; diff --git a/src/shell/capture/capture_batch.cpp b/src/shell/capture/capture_batch.cpp index 97f79fe..0e5ca41 100644 --- a/src/shell/capture/capture_batch.cpp +++ b/src/shell/capture/capture_batch.cpp @@ -25,6 +25,7 @@ #include "shell/capture/render_bounds_gate.h" // refusedRenderFolder #include "shell/capture/scope_resolve.h" // ResolvedSource #include "shell/capture/track_guid.h" // guidString +#include "shell/undo/undo_block.h" // the ONE undo-block opener #include "reaper_plugin.h" // UNDO_STATE_MISCCFG @@ -46,8 +47,6 @@ #define REAPERAPI_WANT_SetTrackSelected #define REAPERAPI_WANT_SetOnlyTrackSelected #define REAPERAPI_WANT_ShowConsoleMsg -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #include "reaper_plugin_functions.h" namespace reasampler::capture { @@ -493,26 +492,26 @@ void RunRecaptureFromSource(ReaSamplerSession& session) updated.provenance = prov; // One batched undo point around the in-place mutation; index-only ext-state, - // nothing placed on the timeline. - Undo_BeginBlock2(nullptr); - // Before the index outcome is even known: the render above already wrote the file, - // and makeUniqueTag guarantees a NEW path, so this is a second record carrying the - // same sampleId as the original. The superseded file becomes an orphan for prune. - session.recordCreated(updated, tracking::OriginKind::Recapture); - const bool changed = session.book().updateSampleInPlace(sampleId, updated); - if (changed) + // nothing placed on the timeline. A plan that mutated nothing records no point at + // all — the block's default discard form. { - // Regenerating the same id's audio is exactly why instances need the generation - // bump — they'd otherwise keep playing stale audio until reload. Bumped inside - // the undo block so undo rolls back the generation with the rest of the blob. - session.bumpBankGeneration(); - const bool persisted = session.saveToActiveProject(); // book + ledger + generation + MarkProjectDirty - Undo_EndBlock2(nullptr, persisted ? "ReaSampler: re-capture from source" : "", - persisted ? UNDO_STATE_MISCCFG : 0); - } - else - { - Undo_EndBlock2(nullptr, "", 0); // nothing mutated -> discard the empty point + UndoBlock undo(nullptr); + // Before the index outcome is even known: the render above already wrote the + // file, and makeUniqueTag guarantees a NEW path, so this is a second record + // carrying the same sampleId as the original. The superseded file becomes an + // orphan for prune. + session.recordCreated(updated, tracking::OriginKind::Recapture); + if (session.book().updateSampleInPlace(sampleId, updated)) + { + // Regenerating the same id's audio is exactly why instances need the + // generation bump — they'd otherwise keep playing stale audio until reload. + // Bumped inside the undo block so undo rolls back the generation with the + // rest of the blob. + session.bumpBankGeneration(); + // book + ledger + generation + MarkProjectDirty + if (session.saveToActiveProject()) + undo.recordPoint("ReaSampler: re-capture from source", UNDO_STATE_MISCCFG); + } } bankPanelRefresh(); // reflect the regenerated file in the docked grid diff --git a/src/shell/capture/capture_orchestrator.cpp b/src/shell/capture/capture_orchestrator.cpp index d0ef312..adaa30d 100644 --- a/src/shell/capture/capture_orchestrator.cpp +++ b/src/shell/capture/capture_orchestrator.cpp @@ -18,6 +18,7 @@ #include "shell/capture/realtime_lifecycle.h" // the in-flight realtime state #include "shell/capture/render_selection.h" // RenderTrackSelection #include "shell/capture/render_isolation.h" // UpstreamIsolation +#include "shell/undo/undo_block.h" // the ONE undo-block opener #include "reaper_plugin.h" // UNDO_STATE_MISCCFG @@ -28,8 +29,6 @@ #define REAPERAPI_WANT_GetMediaTrackInfo_Value #define REAPERAPI_WANT_SetMediaTrackInfo_Value #define REAPERAPI_WANT_ShowConsoleMsg -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #include "reaper_plugin_functions.h" namespace reasampler::capture { @@ -369,20 +368,22 @@ void RunCaptureItemAssign(ReaSamplerSession& session) { // Reuse the Item-scope def from the capture table (index 0) — same range logic, same // FX-scope neutralize, same bank/persist landing as the plain "capture item" action. - Undo_BeginBlock2(nullptr); - - const std::string sampleId = RunCapture(session, captureActionTable()[0]); - if (sampleId.empty()) { - // Capture failed or no-op'd — RunCapture already reported. Discard the empty point. - Undo_EndBlock2(nullptr, "", 0); - return; - } + UndoBlock undo(nullptr); - // Assign inside the same block so undo clears both keys together. - ingestAssignActiveInstance(session.book().activeBankId(), sampleId); - Undo_EndBlock2(nullptr, "ReaSampler: capture + assign to active instance", - UNDO_STATE_MISCCFG); + const std::string sampleId = RunCapture(session, captureActionTable()[0]); + if (sampleId.empty()) + { + // Capture failed or no-op'd — RunCapture already reported. The block closes + // on its default discard form, so the empty point is never recorded. + return; + } + + // Assign inside the same block so undo clears both keys together. + ingestAssignActiveInstance(session.book().activeBankId(), sampleId); + undo.recordPoint("ReaSampler: capture + assign to active instance", + UNDO_STATE_MISCCFG); + } bankPanelRefresh(); ShowConsoleMsg("ReaSampler ingest: captured into the bank and assigned to the active " diff --git a/src/shell/capture/insert.cpp b/src/shell/capture/insert.cpp index 412197f..3e0411d 100644 --- a/src/shell/capture/insert.cpp +++ b/src/shell/capture/insert.cpp @@ -33,6 +33,7 @@ #include "shell/panel/panel_bank_ops.h" // bankPanelSelectedSampleIds / SourceBankId #include "core/capture/capture_paths.h" #include "shell/persist/session.h" +#include "shell/undo/undo_block.h" #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_CountSelectedTracks @@ -44,8 +45,6 @@ #define REAPERAPI_WANT_SetOnlyTrackSelected #define REAPERAPI_WANT_SetTrackSelected #define REAPERAPI_WANT_ShowConsoleMsg -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #include "reaper_plugin_functions.h" namespace reasampler { @@ -139,9 +138,8 @@ InsertResult runInsert(ReaSamplerSession* session, const InsertRequest& request) const double cursorPos = GetCursorPosition(); // One undo block around the whole placement (all tracks + selection/cursor - // restore) so a single undo removes every item and restores prior state. Always - // balanced — opened before the first insert, closed after the restore. - Undo_BeginBlock2(nullptr); + // restore) so a single undo removes every item and restores prior state. + UndoBlock undo(nullptr); // Insert onto each selected track at the same cursor position (assumption B): per // track, isolate it as the only selection (A + E), reset the cursor (C is @@ -159,11 +157,10 @@ InsertResult runInsert(ReaSamplerSession* session, const InsertRequest& request) // Label reflects count + conform choice for a clear undo history. extraflags -1 = // UNDO_STATE_ALL (tracks, items, envelope points, project state). - const std::string label = - "ReaSampler: insert on " + std::to_string(result.inserted) + - (result.inserted == 1 ? " track" : " tracks") + - (request.options.conform == TempoConform::None ? "" : " (conform)"); - Undo_EndBlock2(nullptr, label.c_str(), -1); + undo.recordPoint("ReaSampler: insert on " + std::to_string(result.inserted) + + (result.inserted == 1 ? " track" : " tracks") + + (request.options.conform == TempoConform::None ? "" : " (conform)"), + -1); result.status = InsertStatus::Ok; return result; diff --git a/src/shell/capture/insert.h b/src/shell/capture/insert.h index 1255e80..782aead 100644 --- a/src/shell/capture/insert.h +++ b/src/shell/capture/insert.h @@ -44,7 +44,7 @@ struct InsertResult { // currently-selected track set, then inserts the sample onto EACH selected track // at the SAME edit-cursor position. Snapshot/restore ensures the user's track // selection and cursor position are unchanged after the action. The whole operation -// is wrapped in a single Undo_BeginBlock2 / Undo_EndBlock2. +// is wrapped in a single UndoBlock. // // No-op cases (with console messages): // - No track selected: prints "select a track first." diff --git a/src/shell/capture/render_in_place.cpp b/src/shell/capture/render_in_place.cpp index dfaf22c..713dcfb 100644 --- a/src/shell/capture/render_in_place.cpp +++ b/src/shell/capture/render_in_place.cpp @@ -21,6 +21,7 @@ #include "shell/capture/track_guid.h" // guidString #include "shell/panel/panel_input.h" // bankPanelTailSetting #include "shell/persist/session.h" +#include "shell/undo/undo_block.h" // the ONE undo-block opener #include "shell/view/view.h" // applyMode #include "shell/view/view_lanes.h" // mintManagedLanes @@ -42,8 +43,6 @@ #define REAPERAPI_WANT_SetOnlyTrackSelected #define REAPERAPI_WANT_ShowConsoleMsg #define REAPERAPI_WANT_TrackList_AdjustWindows -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #include "reaper_plugin_functions.h" namespace reasampler::capture { @@ -133,82 +132,88 @@ void RunRenderTrackInPlace(ReaSamplerSession& session) { // visible — never to choose a tag. Both tags below are absolute. const std::string activeMode = session.view().activeModeId(); - Undo_BeginBlock2(nullptr); + // Both read after the block closes, so both outlive the scope below. + bool placed = false; + std::string freshName; - // flags = 0, never 1: flags&1 adds default envelopes/FX, and a default chain - // would process a render that already carries the source's FX a second time. - InsertTrackInProject(proj, place.insertIndex, /*flags=*/0); - MediaTrack* fresh = GetTrack(proj, place.insertIndex); - if (!fresh) { - // InsertTrackInProject already mutated the project by this point, so the - // "no ext-state write -> discard" idiom does not apply here — a discard would - // leave the orphaned track un-undoable. - Undo_EndBlock2(nullptr, "ReaSampler: render in place (failed to create result track)", - -1); - refuse("could not create the result track; the render landed at " + - res.absolutePath + " but was not placed."); - return; - } + // Scoped so the persist below lands OUTSIDE the block — see the note at the close. + { + UndoBlock undo(nullptr); - // Both writes or none — one alone lands the new track at the wrong nesting level, - // which is audible in both directions (see siblingPlacement). - if (place.precedingIndex >= 0) { - if (MediaTrack* preceding = GetTrack(proj, place.precedingIndex)) - SetMediaTrackInfo_Value(preceding, "I_FOLDERDEPTH", - static_cast(place.precedingDepth)); - } - SetMediaTrackInfo_Value(fresh, "I_FOLDERDEPTH", - static_cast(place.newDepth)); - - // GetTrackColor returns the colour already OR'd with 0x1000000 and 0 for "no - // colour set", which I_CUSTOMCOLOR reads as unused — so one line clones a colour - // and the absence of one, with no branch. - SetMediaTrackInfo_Value(fresh, "I_CUSTOMCOLOR", - static_cast(GetTrackColor(source))); - const std::string freshName = captureTrackName(trackName(source)); - setTrackName(fresh, freshName); - - // After every attribute write, per the SDK header's manual-panel-update caveat. - TrackList_AdjustWindows(false); - - // Unsnapped and unrounded, deliberately: this placement IS the null test performed - // automatically, so snapping it to the grid would move the audio off the position - // it was rendered from. InsertOptions{} defaults give native length and no conform. - const double cursorPos = GetCursorPosition(); - SetOnlyTrackSelected(fresh); - SetEditCurPos(src.startSeconds, false, false); - // InsertMedia's int return isn't SDK-documented; treated conservatively as - // 0 = failure, matching performArrangeDrop — an empty result track would - // otherwise be a silent no-op, which is exactly what this verb must not produce. - const bool placed = - InsertMedia(res.absolutePath.c_str(), computeInsertMode(InsertOptions{})) != 0; - SetEditCurPos(cursorPos, false, false); - // The new track is left selected, alone — in the headline case the source is being - // parked out of sight in the same gesture, so restoring the selection would leave - // the user selecting an invisible track. - - // Absolute, not mode-following: the source parks on the bench, the result is an - // Arrange member whatever mode was active. Explicit records rather than untag(), - // because the record is what the panel's auto-tag detector defers to. - MembershipIndex& membership = session.view().membership(); - membership.tag(guidString(source), kDesignModeId); - membership.tag(guidString(fresh), kArrangeModeId); - - // The track is brand new, so its items are exactly the ones just placed. An - // untagged item would be handed to the detector, which tags to the active mode. - const int itemCount = CountTrackMediaItems(fresh); - for (int i = 0; i < itemCount; ++i) { - if (MediaItem* it = GetTrackMediaItem(fresh, i)) { - const std::string ig = itemGuid(it); - if (!ig.empty()) membership.tag(ig, kArrangeModeId); + // flags = 0, never 1: flags&1 adds default envelopes/FX, and a default chain + // would process a render that already carries the source's FX a second time. + InsertTrackInProject(proj, place.insertIndex, /*flags=*/0); + MediaTrack* fresh = GetTrack(proj, place.insertIndex); + if (!fresh) { + // InsertTrackInProject already mutated the project by this point, so the + // "no ext-state write -> discard" idiom does not apply here — a discard would + // leave the orphaned track un-undoable. + undo.recordPoint("ReaSampler: render in place (failed to create result track)", -1); + refuse("could not create the result track; the render landed at " + + res.absolutePath + " but was not placed."); + return; } + + // Both writes or none — one alone lands the new track at the wrong nesting level, + // which is audible in both directions (see siblingPlacement). + if (place.precedingIndex >= 0) { + if (MediaTrack* preceding = GetTrack(proj, place.precedingIndex)) + SetMediaTrackInfo_Value(preceding, "I_FOLDERDEPTH", + static_cast(place.precedingDepth)); + } + SetMediaTrackInfo_Value(fresh, "I_FOLDERDEPTH", + static_cast(place.newDepth)); + + // GetTrackColor returns the colour already OR'd with 0x1000000 and 0 for "no + // colour set", which I_CUSTOMCOLOR reads as unused — so one line clones a colour + // and the absence of one, with no branch. + SetMediaTrackInfo_Value(fresh, "I_CUSTOMCOLOR", + static_cast(GetTrackColor(source))); + freshName = captureTrackName(trackName(source)); + setTrackName(fresh, freshName); + + // After every attribute write, per the SDK header's manual-panel-update caveat. + TrackList_AdjustWindows(false); + + // Unsnapped and unrounded, deliberately: this placement IS the null test performed + // automatically, so snapping it to the grid would move the audio off the position + // it was rendered from. InsertOptions{} defaults give native length and no conform. + const double cursorPos = GetCursorPosition(); + SetOnlyTrackSelected(fresh); + SetEditCurPos(src.startSeconds, false, false); + // InsertMedia's int return isn't SDK-documented; treated conservatively as + // 0 = failure, matching performArrangeDrop — an empty result track would + // otherwise be a silent no-op, which is exactly what this verb must not produce. + placed = + InsertMedia(res.absolutePath.c_str(), computeInsertMode(InsertOptions{})) != 0; + SetEditCurPos(cursorPos, false, false); + // The new track is left selected, alone — in the headline case the source is being + // parked out of sight in the same gesture, so restoring the selection would leave + // the user selecting an invisible track. + + // Absolute, not mode-following: the source parks on the bench, the result is an + // Arrange member whatever mode was active. Explicit records rather than untag(), + // because the record is what the panel's auto-tag detector defers to. + MembershipIndex& membership = session.view().membership(); + membership.tag(guidString(source), kDesignModeId); + membership.tag(guidString(fresh), kArrangeModeId); + + // The track is brand new, so its items are exactly the ones just placed. An + // untagged item would be handed to the detector, which tags to the active mode. + const int itemCount = CountTrackMediaItems(fresh); + for (int i = 0; i < itemCount; ++i) { + if (MediaItem* it = GetTrackMediaItem(fresh, i)) { + const std::string ig = itemGuid(it); + if (!ig.empty()) membership.tag(ig, kArrangeModeId); + } + } + + mintManagedLanes(session.view(), nullptr); + applyMode(session.view(), activeMode, nullptr); // a reapply, never a switch + + undo.recordPoint("ReaSampler: render selected track to a new track", -1); } - mintManagedLanes(session.view(), nullptr); - applyMode(session.view(), activeMode, nullptr); // a reapply, never a switch - - Undo_EndBlock2(nullptr, "ReaSampler: render selected track to a new track", -1); - // 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. diff --git a/src/shell/undo/CLAUDE.md b/src/shell/undo/CLAUDE.md new file mode 100644 index 0000000..9bd952d --- /dev/null +++ b/src/shell/undo/CLAUDE.md @@ -0,0 +1,80 @@ +# src/shell/undo — the one REAPER undo-block opener + +## Scope + +One type, `UndoBlock`, and one query, `undoBlockOpen()`, plus the interception that ends +the FX reclaim span when REAPER's own Undo/Redo is dispatched. Every extension-owned undo +block in the tree is opened through this type; nothing here decides what a point is +called or whether one is worth minting — each call site still owns its own label, +mask, and discard-vs-mint condition. The FX reclaim span is NOT this directory's: +`shell/view/fx_reclaim` owns it, and this directory only guarantees that neither a block +nor REAPER's own undo ever runs nested over it. + +## Invariants + +- **`Undo_BeginBlock2` is not callable outside this directory in every TU but one, and + that is enforced by the compiler, not by review.** Only `undo_block.cpp` and + `shell/view`'s `fx_reclaim.cpp` (which owns the span's own begin/end) define + `REAPERAPI_WANT_Undo_BeginBlock2` / `..._Undo_EndBlock2`; every other TU compiles + under `REAPERAPI_MINIMAL`, so a raw `Undo_BeginBlock2` there is an undeclared + identifier. **`app/main.cpp` is the one exception** — as the sole + `REAPERAPI_IMPLEMENT` TU it never sets `REAPERAPI_MINIMAL` (it must allocate storage + for whatever ANY TU wants), so the whole API, `Undo_*` included, compiles clean + there too. That TU stays out of the undo business by discipline + (`src/app/CLAUDE.md`: pointers + entry + dispatch only), not because the compiler + stops it — do not add business logic there that would make this matter. Adding the + WANT define back to a third TU is a second, easier way to defeat the enforcement + that DOES exist — do not. +- **Ending the reclaim span is part of opening, and ONLY the outermost block does + it.** At depth 0 the span is the only block that can be open, so ending it mints + the pending mode switch's point whole before this block exists. At depth > 0 an + enclosing `UndoBlock` already ended it, and a close from the inner one would land + at depth 2->1 — minting nothing and hijacking the enclosing block's label. That + is the same criterion `applyMode`'s reapply is gated on; it lives in the + constructor so no call site has to make the judgment. Full reasoning for the rule + itself: `src/shell/view/CLAUDE.md`. +- **The constructor's body is one `try`.** `fxReclaimEndUndoSpan` copies a + `std::string` and can throw; a throwing constructor runs no destructor, so an + uncompensated depth increment would stick forever — `undoBlockOpen()` reads true + for the rest of the session, `fxReclaimTick` refuses every tick, and no later + `UndoBlock` is ever outermost. Any throw between the increment and the end of the + constructor rolls the depth back before propagating. +- **Closing is a destructor.** No throw between the open and the label decision can + leave a block open in the user's project, which is why several call sites can + allocate freely between the two. +- **No label recorded means no point.** `UndoBlock` closes on `("", 0)` unless + `recordPoint` was called — the discard form, real from a hand-fired action + (DAW-measured; `src/shell/view/CLAUDE.md`). A verb that mutated nothing therefore + costs no undo entry simply by saying nothing. + +## Modules + +- `undo_intercept` — the ids of REAPER's own `Edit: Undo`/`Edit: Redo`, the main-section + scoping of that pair, and `endReclaimSpanForUndoCommand`, which `app/main.cpp` calls + from BOTH command hooks before anything else claims the id. Consumes nothing. Why + dispatch and not the state-load hook, which ids are covered, and which undo routes are + NOT: `src/shell/view/CLAUDE.md`, which owns the span. +- `undo_block` — `UndoBlock` (RAII open/close plus `recordPoint`) and + `undoBlockOpen()`. The latter is read by `fxReclaimTick`, which refuses a tick + outright while any block is open: the drain's lazy `openSpan` would otherwise + begin at depth 2, and the enclosing block's close would then mint under the span's + label and destroy the user's own point. The refusal consumes nothing — no wakeup, + no queue, no project pin — so the first tick after the block closes drains as if + this one had never been due. + +## Gotchas + +- **`Undo_EndBlock2` is guarded by `ValidatePtr2` only when `project_` is a real, + non-null pointer.** `nullptr` means "the active project" — the same convention + `Undo_BeginBlock2` was handed — and `ValidatePtr2` does not accept `nullptr` as a + valid object of any type, so guarding it unconditionally would silently stop every + `UndoBlock undo(nullptr)` site (most of them) from ever closing. A real pointer, by + contrast, can go stale between open and close (several call sites allocate freely + in between), so only that case is validated — the same asymmetry `fx_reclaim.cpp`'s + `closeSpan` already has, for the same reason: its span always holds a real pinned + pointer, never `nullptr` for "active". +- The depth counter deliberately does NOT count the reclaim span. The span is the + thing this stands off, not a peer. +- The counter drops AFTER `Undo_EndBlock2`, not before: the close that reaches depth + 0 mints a point, and anything re-entered through that must still read a block as + open rather than let the drain open a span underneath it. diff --git a/src/shell/undo/undo_block.cpp b/src/shell/undo/undo_block.cpp new file mode 100644 index 0000000..9f10d76 --- /dev/null +++ b/src/shell/undo/undo_block.cpp @@ -0,0 +1,73 @@ +// See undo_block.h. Compiled into the reaper_reasampler module; includes +// reaper_plugin_functions.h without REAPERAPI_IMPLEMENT (main.cpp owns that). + +#include "shell/undo/undo_block.h" + +#include + +#include "shell/view/fx_reclaim.h" // fxReclaimEndUndoSpan — the span this must not nest over + +#define REAPERAPI_MINIMAL +#define REAPERAPI_WANT_Undo_BeginBlock2 +#define REAPERAPI_WANT_Undo_EndBlock2 +#define REAPERAPI_WANT_ValidatePtr2 +#include "reaper_plugin_functions.h" + +namespace reasampler { + +namespace { + +// Extension-owned nesting depth only — REAPER's own counter is not readable, and the +// reclaim span is deliberately NOT counted here (it is the thing this stands off). +int g_depth = 0; + +} // namespace + +bool undoBlockOpen() { return g_depth > 0; } + +UndoBlock::UndoBlock(ReaProject* proj) : project_(proj) { + // ONLY the outermost block ends the span, which mechanizes the whole nesting rule. + // At depth 0 the span is the only block that can be open, so ending it mints the + // switch's point whole, at depth 0, before this block exists. At depth > 0 the + // enclosing UndoBlock already did it, and a close from here would land at depth + // 2->1 — minting nothing and hijacking the enclosing block's label instead. That is + // the same criterion applyMode's reapply is gated on. + // + // The whole body is one try: fxReclaimEndUndoSpan's closeSpan copies a std::string + // and can throw. A throwing constructor runs no destructor, so an uncompensated + // ++g_depth would stick forever — undoBlockOpen() reads true for the rest of the + // session, fxReclaimTick refuses every tick, and no later UndoBlock is ever + // outermost. Roll the depth back on any throw so the type degrades to "this one + // block failed to open," not "every block after it is now poisoned." + try { + if (g_depth++ == 0) fxReclaimEndUndoSpan(); + Undo_BeginBlock2(proj); + } catch (...) { + --g_depth; + throw; + } +} + +UndoBlock::~UndoBlock() { + // Depth drops AFTER the SDK call, not before: the close that reaches depth 0 mints a + // point, and anything re-entered through that must still read a block as open rather + // than let the drain open a span underneath it. + // + // Guarded the same way fx_reclaim.cpp's closeSpan guards its own Undo_EndBlock2, but + // only for a REAL pinned pointer: `project_ == nullptr` means "the active project", + // the same convention Undo_BeginBlock2 was handed, and ValidatePtr2 does not accept + // nullptr as a valid object of any type — guarding it unconditionally would silently + // stop every `UndoBlock undo(nullptr)` site (most of them) from ever closing. A real + // pointer, by contrast, can go stale: several call sites allocate freely between open + // and close, and the project it named may have closed underneath by now. + if (!project_ || ValidatePtr2(nullptr, project_, "ReaProject*")) + Undo_EndBlock2(project_, label_.c_str(), mask_); + --g_depth; +} + +void UndoBlock::recordPoint(std::string label, int mask) { + label_ = std::move(label); + mask_ = mask; +} + +} // namespace reasampler diff --git a/src/shell/undo/undo_block.h b/src/shell/undo/undo_block.h new file mode 100644 index 0000000..ef99eb5 --- /dev/null +++ b/src/shell/undo/undo_block.h @@ -0,0 +1,47 @@ +#pragma once +// The ONE way an extension undo block is opened. Ending the FX reclaim span is part of +// opening, so no block can ever nest over it, and the close is a destructor, so no throw +// can leave one open in the user's project. +// +// The SDK's WANT opt-in stops a raw Undo_BeginBlock2/_EndBlock2 call in every OTHER TU — +// only this file and fx_reclaim.cpp (the span's own owner) request them, so the symbol is +// undeclared elsewhere. app/main.cpp is the one exception: the sole REAPERAPI_IMPLEMENT TU +// cannot set REAPERAPI_MINIMAL (it must allocate storage for whatever ANY TU wants), so a +// raw call compiles clean there too — kept out by discipline (src/app/CLAUDE.md), not by +// the compiler. + +#include + +// Forward-declared to keep this header SDK-free; the .cpp includes the real SDK header. +class ReaProject; + +namespace reasampler { + +class UndoBlock { +public: + // `proj` is nullptr for the active project — the general REAPER convention for a + // ReaProject* parameter. reaper_plugin_functions.h documents no proj semantics at all + // for Undo_BeginBlock2/_EndBlock2 (the line once cited here was the #if WANT guard, + // not a doc comment). `[verify — DAW]` + explicit UndoBlock(ReaProject* proj); + ~UndoBlock(); + + // What the close names the point. Never called, the block closes on the discard form + // (`""`, 0) — real from a hand-fired action, a coalesce from a timer (see UndoSpan), + // and what every site that ended up writing nothing already wanted. + void recordPoint(std::string label, int mask); + + UndoBlock(const UndoBlock&) = delete; + UndoBlock& operator=(const UndoBlock&) = delete; + +private: + ReaProject* project_ = nullptr; + std::string label_; + int mask_ = 0; +}; + +// True while any UndoBlock is alive, at any nesting depth. The FX reclaim drain reads it +// to stand off a tick it would otherwise open its span inside of — see fxReclaimTick. +bool undoBlockOpen(); + +} // namespace reasampler diff --git a/src/shell/undo/undo_intercept.cpp b/src/shell/undo/undo_intercept.cpp new file mode 100644 index 0000000..afea981 --- /dev/null +++ b/src/shell/undo/undo_intercept.cpp @@ -0,0 +1,17 @@ +// See undo_intercept.h. Touches no REAPER API of its own: the ids arrive from main.cpp's +// two command hooks, and the only thing done with one is the span's existing close. + +#include "shell/undo/undo_intercept.h" + +#include "shell/view/fx_reclaim.h" // fxReclaimEndUndoSpan — the ONE idempotent close + +namespace reasampler { + +void endReclaimSpanForUndoCommand(int command, int sectionId) { + if (!isMainUndoRedoCommand(command, sectionId)) return; + // Safe at any depth: a span is never open at depth > 0 (src/shell/view/CLAUDE.md), so + // a hook re-entered from inside an extension block finds nothing to close. + fxReclaimEndUndoSpan(); +} + +} // namespace reasampler diff --git a/src/shell/undo/undo_intercept.h b/src/shell/undo/undo_intercept.h new file mode 100644 index 0000000..47635de --- /dev/null +++ b/src/shell/undo/undo_intercept.h @@ -0,0 +1,30 @@ +#pragma once +// REAPER's own Undo/Redo, caught at action dispatch. The reclaim span has to close there +// — minting the mode switch's point — BEFORE REAPER runs the undo: while the span is open +// the switch has no point yet, so Ctrl-Z undoes whatever preceded it, with REAPER's undo +// machinery running nested inside our still-open block. + +namespace reasampler { + +// Main-section "Edit: Undo" / "Edit: Redo". Taken from the vendored SDK's own reference +// plugins, which send exactly these for undo/redo +// (vendor/reaper-sdk/reaper-plugins/reaper_csurf/csurf.h:312-313, used by csurf_01X, +// csurf_alphatrack, csurf_faderport, csurf_mcu) — not read off the action list by eye. +inline constexpr int kEditUndoCommand = 40029; +inline constexpr int kEditRedoCommand = 40030; + +// Command ids are per SECTION (reaper_plugin.h's custom_action_register_t: 0/100 = +// main/main alt, 32060-32063 = the MIDI and media-explorer sections), so the same integer +// arriving under another section is a different action entirely and must not be +// intercepted. What that leaves uncovered is enumerated in src/shell/view/CLAUDE.md. +inline bool isMainUndoRedoCommand(int command, int sectionId) { + if (sectionId != 0 && sectionId != 100) return false; + return command == kEditUndoCommand || command == kEditRedoCommand; +} + +// Ends the reclaim span when `command` is one of the two above, and does nothing +// otherwise. CONSUMES NOTHING: the caller must still return false from its hook and let +// REAPER run the undo, which then executes at depth 0 against a properly formed point. +void endReclaimSpanForUndoCommand(int command, int sectionId); + +} // namespace reasampler diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index d20eff5..ba2b982 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -64,29 +64,237 @@ 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). -- **ONE mode switch is ONE undo point.** The driven flags and the lane state - written inside `applyMode`'s block roll back in a single Ctrl-Z. (The - `view_state` ext-state write is NOT among them; it lands after the block, see - Gotchas.) Per-FX offline/online moved off this block entirely, onto the - reclaimer (`fx_reclaim`) — each write there runs inside its OWN discarded - block (`Undo_EndBlock2(proj, "", 0)`), so it mints nothing. DAW-VERIFIED: ten - unblocked `TrackFX_SetOffline` calls minted ten undo points; the same ten - calls wrapped in `Undo_BeginBlock2`/`Undo_EndBlock2(proj, "", 0)` minted zero - — confirming the discard-block premise the reclaimer's per-write block - (`fx_reclaim.cpp`'s `writeOffline`) rests on. -- **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. +- **`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. + **The four flags above are the whole of what was measured.** The lane parameters this + directory also writes with no block behind them — `C_LANEPLAYS`, `I_FREEMODE`, + `C_LANESCOLLAPSED`, `C_LANESETTINGS` (`view_lanes`' `applyLaneOps`) — are assumed + silent BY INFERENCE from the four, never observed: `[verify — DAW]`. If they turn out + to mint unaided, a reapply that repairs a lane leaves a stray point the criterion + below did not intend; that is the failure mode to look for. +- **ONE CRITERION decides who opens an undo block: the path whose own act has to + survive as a point, and no other.** Two things make an act the path's own — it was + explicitly fired by the user (a real mode switch, the membership actions, + `render_in_place`), or it makes a STRUCTURAL project change no re-derivation + reproduces (`view_lanes`' `mintManagedLanes`, which creates lanes and re-homes items; + the panel's detect tick fires it, so it is nobody's action, and it opens a block + anyway because nothing else would ever put those lanes back). Everything else + RE-ASSERTS driven state the view model recomputes from membership on every apply — + the park/restore flags, parent visibility, and `applyLaneOps`' `C_LANEPLAYS` plus its + `I_FREEMODE` re-assert on a track that already owns managed lanes — and opens none, + because a block there could only manufacture the point a project open or a membership + edit must not cost. This is why the same `I_FREEMODE` write is blocked at one site and + not the other: minting SPLITS a track, `applyLaneOps` only re-states which lane plays. + Second clause, binding every site that does open one: **a block with nothing REAPER + can hold closes on the discard form** (`Undo_EndBlock2(proj, "", 0)` — real from a + hand-fired action, per the measurement above), so re-tagging tracks already in that + mode leaves no empty point and no relabelled Undo button. The one exception is a REAL + SWITCH, which mints written state or not: it is an explicitly fired action and has to + stay undoable even where the plan found nothing to write. "Nothing REAPER can hold" is + NOT "no REAPER write", and reading it that way was a shipped defect: the four driven + flags ARE a REAPER write and mint nothing, so a site predicated on writes names points + REAPER then declines to append — history unchanged, Undo button relabelled. What a + membership edit's point holds is its `view_state` write, which is why that write is + INSIDE the block and the predicate is the model diff + (`shell/actions/design_view_actions`' `runMembershipEdit`). +- **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 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. **DAW-CONFIRMED, no longer inferred:** a block + opened in the `applyMode` ACTION and closed several timer callbacks later by the drain + holds — switch, then an undisturbed parking drained to completion, then one Ctrl-Z gave + exactly ONE undo entry and returned to the previous mode. (The two-point degradation + this once hedged against never materialised; the degradations that DO occur are the + transport deferral and `mintManagedLanes`' early close, both named below.) + **Opening the span any LATER than the switch is rejected**, and the ~3s of open block + before the drain's first FX write is not exposure buying nothing. The four driven flags + mint nothing unaided (measured above), so the block's CLOSE is the only thing that ever + makes the switch undoable; open the span at the drain's first write instead and the whole + park hold carries no point for a Ctrl-Z to reach — which is precisely the bug the dispatch + interception below fixes. The dead time is where the switch's point sits waiting to be + minted, which is exactly what the state-load terminator and the dispatch interception + reach into that window to do. +- **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 + drain settling, aborting on a generation bump, or deferring on the transport; the + project retargeting or closing; the next REAL SWITCH superseding it; ANY PROJECT-STATE + LOAD; REAPER's OWN Undo/Redo caught at action dispatch; a project save; extension + unload; and any path about to open a block of its own + (next invariant). The span may stay open only across a wait it + can BOUND — the 3s debounce, which is the dead time between the switch and its first + FX write, and the only reason the span exists. A TRANSPORT deferral is not bounded + (it lasts as long as the playback pass), so it ends the span and the parking that + follows mints its own point (`spanEndsAfter`). + Stated as ONE property over the pure state, which is how it is tested + (`tests/test_fx_reclaim.cpp`, at every terminator rather than incidentally in one + scenario): **`!pending()` implies `!span.open()`.** The timer gates every tick on + `pending()`, so a span left open with the wakeup down has nothing that could ever + close it. That is why `applyMode` raises the wakeup BEFORE it opens the span rather + than on its way out — a throw in between would otherwise leak the block with the + reclaimer asleep — and why the state-load terminator closes BEFORE it abandons. +- **NOTHING MAY OPEN AN UNDO BLOCK WHILE THE SPAN IS OPEN — and no site decides that + for itself any more.** REAPER's blocks are read as a DEPTH COUNTER, not a stack of + identities (`[verify — DAW]`, and the same undocumented nested-block behaviour the + Gotchas below name): `Undo_EndBlock2` at depth 2->1 mints nothing and drops the label + and mask it was handed, and the close that does reach depth 0 mints under whatever + label IT carries. So a block opened over the span leaves each close paired with the + other's open — the switch's point gone, its parking relabelled as the intruder's act, + and the `UNDO_STATE_FX` the drain widened the span to (`UndoSpan::widen`) dropped, + which is the exact loss the widen exists to prevent. The RULE does not rest on that + reading: ending the span first yields the switch's point and the opener's as two + correctly-labelled points under the counter reading AND under an identity-pairing one, + which is why it is stated as an unconditional discipline rather than a workaround for + a behaviour nobody has observed. **The mechanism is `shell/undo`'s `UndoBlock`, which + every extension-owned block in the tree now opens through — its constructor ends the + span when, and only when, it is the OUTERMOST block, so a nested opener cannot close + at depth 2->1 and hijack its enclosing block.** That directory's `CLAUDE.md` owns the + mechanism and the compile-time enforcement behind it; here it closes three things at + once. First, the openers no longer have to be enumerated by hand — the former list + (`ActionUndoBlock`, `mintManagedLanes`, `render_in_place`) was never the whole set and + omitted ten user-fired blocks in `shell/capture`, `shell/bank_ops` and + `shell/actions`, each of which absorbed the user's own act into the mode switch. + Second, `mintManagedLanes` gets the same nesting criterion as everything else: it is + called from inside `doMoveItems`' and `render_in_place`' blocks, where it used to end + the span unconditionally. Third, the RESIDUAL in the other direction is closed too — + a foreign block's message pump can re-enter the timer, and a drain tick in there would + open its span at depth 2; `fxReclaimTick` now refuses outright while `undoBlockOpen()` + and consumes nothing, so it simply drains on the first tick after the block closes. + The mirror half is still `applyMode`'s: a REAPPLY never closes a span, precisely + because it is reachable from inside one of those blocks — only a real switch + supersedes, and a real switch is always fired at depth 0. + **Consequently, a span is never open at depth > 0.** The only two openers are a real + switch (always fired straight from an action handler, never from inside another + block) and the reclaimer's own lazy `writeOffline`, which the residual above makes + unreachable at depth > 0. This is what lets `fxReclaimAbandonForStateLoad` and + `fxReclaimFlushRelease` close the span unconditionally with no depth check of their + own, even when the hook that calls them fires from inside an extension `UndoBlock` — + a capture's own save can (`capture_orchestrator`'s `RunCaptureItemAssign` wraps + `RunCapture`, which reaches `Main_SaveProject`). `closeSpan()` there just finds + nothing open. + ACCEPTED, and not fixed: `mintManagedLanes` opens its block once its plan is + non-empty, but a non-empty plan whose writes are all already satisfied still mints + nothing — so a background detect tick can end a switch's span having competed with it + for nothing. Telling the two apart means running the plan, which is the write. The + cost is bounded at ONE extra undo point (the switch mints early; its remaining parking + mints again under `kReclaimUndoLabel`), the same degradation a transport deferral + already carries, where the alternative destroys the switch's point outright. +- **EVERY project-state load terminates the span itself, at the hook, before REAPER + restores anything — `isUndo` does not narrow it.** + `fxReclaimAbandonForStateLoad` runs UNBRANCHED from `BeginLoadProjectState` + (`main.cpp`) — the only point at which the block still closes over the state it was + OPENED on, and the only entry REAPER gives for a state load at all, so undo, redo, a + project open, a `File > Revert` and a new project (`reaper_plugin.h:1203-1205`) all + reach the one terminator. The span is open for at least `kParkHoldMs` after every + real switch and a switch is exactly what a user undoes, so letting the close happen + where it used to — as a side effect of the reapply the reloaded model triggers, a + full timer tick later — closed the pre-undo block over POST-undo state, minting a + spurious point and destroying the redo branch. A NON-undo load left to the next + tick's project-retarget close is the identical defect one branch away: the same block + closes over the same post-load state, a tick late. `isUndo` decides one thing only, + in `main.cpp`: whether the SESSION owes a deferred ext-state reload (a normal open's + belongs to the identity poll's Load branch, whose forked-sibling re-GUID a reload + request here would bypass). The same call stops any drain on the stack where it + stands (`FxReclaimQueue::abandon`, refused at both host entry points), which is + stronger than an invalidation: an invalidation still finishes the track it is on, + because a step is atomic. A load that keeps the SAME `ReaProject` — a revert — is + what makes that stop load-bearing rather than tidy, for the reason stated at + `abandon`. Daniel's ruling, and the accepted cost: undoing a switch mid-drain leaves + a PARTIAL park — some FX offlined, some not — covered by the one point the + terminator closed. +- **REAPER's own Undo/Redo is intercepted one step EARLIER than that, at action dispatch — + the load hook is too late for an undo.** DAW-OBSERVED, and the whole reason this exists: + switch to Design view, press Ctrl-Z inside the park hold, and the user gets one undo point + per FX. While the span is open the switch HAS NO POINT YET, so Ctrl-Z does not undo the + switch — it undoes whatever preceded it, with REAPER's undo machinery running nested + inside our still-open block, and the drain then parks against a model that was undone + underneath it. So `main.cpp`'s two command hooks end the span BEFORE REAPER runs the + action (`shell/undo`'s `endReclaimSpanForUndoCommand`): the switch's point mints at that + instant and the undo then executes at depth 0 against a properly formed point. The + interception CONSUMES NOTHING — the hook returns false and REAPER proceeds normally. + The ids are `40029`/`40030`, taken from Cockos' own reference plugins in the vendored SDK + (`vendor/reaper-sdk/reaper-plugins/reaper_csurf/csurf.h:312-313`, sent as `WM_COMMAND` by + four of them) rather than read off the action list by eye; the pair and its section + scoping are pinned by `tests/test_undo_intercept.cpp`. BOTH hooks call it, because + `hookcommand2` is not the non-main sections' hook: it runs prior to every action triggered + by a KEY or MIDI event in ANY section, main included (`reaper_plugin.h:214`), and a + Ctrl-Z is a key event. Which of the two fires first is undocumented and immaterial — the + close is idempotent. + **What this does NOT cover — every one of these still lands inside an open span, exactly + as before:** an undo fired in a NON-MAIN section (the MIDI editor's own Ctrl-Z is a + project undo; its command id lives in that section's own id space and is not verifiable + from the SDK, so nothing is guessed here) — plausible in real use; the Undo History + window's list, which drives the undo directly rather than dispatching `40029` — plausible, + and the likeliest remaining route; and a ReaScript or another extension calling + `Undo_DoUndo2`/`Undo_DoRedo2` (`reaper_plugin_functions.h:7777-7790`) — plausible wherever + the user runs scripts. What IS covered beyond the keystroke: the Edit menu, the toolbar + button, a control surface (the csurf plugins above), an OSC/MIDI binding, and — by + inference from `reaper_plugin.h:206`'s recursion warning rather than by measurement — a + script's `Main_OnCommand(40029)`. +- **A state load puts the reclaimer to SLEEP; only a re-read of the model wakes it.** + `abandon` clears the wakeup where `invalidate` raises it, because the two say + different things: an invalidation says the MODEL moved and owes a re-derivation, a + load says the PROJECT moved under a model nobody has re-read yet. `applyMode` is + `invalidateFxReclaim`'s one caller and `fxReclaimPending` gates the tick that is the + one path to any FX write or park-record spend, so every load that re-reads the model + wakes the loop on the same tick, through the reapply the reload drives — and a load + that re-reads nothing wakes nothing. `File > Revert` is exactly that load: same + `ReaProject`, same GUID, same path, so the identity poll answers `NoOp` and the model + is never re-read. Waking there was the only thing that ran, and it converged the FX + chains toward the pre-revert model and spent the park records the reverted project + still needed. The accepted cost of the sleep: an outstanding reclaim in a project + whose interrupting load was some OTHER project's waits for that project's next apply + 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 opens no undo block and CLOSES NO SPAN, 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. The open half is the criterion above (a reapply + re-asserts derived state and is nobody's act); the close half is the nesting rule + above (a reapply runs inside its driver's block, so a supersede-close from there + would land at depth 2->1 and hijack it). It reports only whether it `applied`; a + driver holding a block decides its point from its OWN model diff, never from what the + reapply wrote, and `ModeApplyResult` no longer offers that answer to be misread. - **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 never matures, so the reclaimer finds every chain already where the record - wants it and writes nothing. + wants it and writes nothing. **Undoing a switch mid-park is that same flip**, so a + state load DROPS the holds where an invalidation keeps them (`FxReclaimQueue::abandon`, + and again at the end of `run` for an abandon that arrived through a write's own pump, + which found the clock moved into the drain's local). Keeping them fired the post-load + re-park on the FIRST tick after the load, with zero debounce, in the wake of REAPER's + own restore; re-earning them is what makes the undo cost nothing and keeps the writes + clear of the load. +- **NOTHING THE EXTENSION DOES PER TICK RUNS WHILE REAPER IS IN A PROJECT LOAD OR SAVE.** + `BeginLoadProjectState` fires before REAPER restores anything, and the restore itself + brings plugins online — a plugin load pumps the message loop, which is how a timer tick + lands INSIDE the load. `main.cpp`'s `OnTimer` therefore returns immediately while + `GetCurrentProjectInLoadSave()` names a project + (`reaper_plugin_functions.h:1584-1587`), self-lowering, with no latch to leak. Without + it, that tick's `poll()` reads the half-restored `` — the very pre-load value + the hook defers past because it is wrong — the reapply plans against it, and the reclaim + tick writes FX into a project REAPER is still rewriting; each such round is its own + settle-and-close, so a re-convergence that owes ONE point costs one per round. That last + step is reachable by inspection, not measured: the DAW report it answers is one undo + entry per FX after a Ctrl-Z mid-drain, and whether this is the whole of that cause is + `[verify — DAW]`. The extension's other re-entrancy guards (`modeApplyInProgress`, + `fxReclaimDrainInProgress`) cover OUR pumps only; this is the one that covers REAPER's. + It consumes nothing — reload request, load signal, wakeup, derived queue and pin all + stand. The realtime-capture drive deliberately sits AHEAD of it, keeping its + single-pointer-test idle path and its behaviour across a load unchanged. - **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 @@ -124,13 +332,13 @@ 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 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`.** 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 ends any prior span, raises the reclaimer's wakeup, and opens the ONE undo span all of that rides (`fx_reclaim` is what closes it; a reapply opens none and closes none). Reports whether it applied and nothing else (`ModeApplyResult`) — what it wrote is deliberately not an answer it gives, because none of it mints. 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); each `writeOffline` runs inside its own discarded undo block, DAW-verified to mint nothing. `ReleaseFlush`/`fxReclaimFlushRelease` make a completed record's spend durable on the project's next save, never on the drain's own initiative. +- `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`, never while an extension undo block is open (`fxReclaimTick` refuses the tick outright, consuming nothing), 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). ## Gotchas @@ -158,15 +366,33 @@ applies the resulting lane state to live tracks. reads identically and is EXPOSEd too — no lost state there at all, which is why the report never asserts a cause. A REDO of a mode switch is a further, hypothetical route — see the `view_state` gotcha below. -- **An UNDO of a mode switch no longer rolls back FX at all.** FX offline/online - moved off `applyMode`'s block entirely onto the reclaimer, so a Ctrl-Z restores - only the flags and lane state the block wrote — whatever the reclaimer had - converged (or had not yet converged) survives the undo untouched. Whether this - reads as correct in the DAW is `[verify — DAW]`, `docs/VERIFICATION.md` - §"Mode switching". -- **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 +- **An UNDO of a mode switch rolls back the FX the span covered, and only those.** + The span carries `UNDO_STATE_FX` only once the drain has actually written FX under + it (`UndoSpan::widen`), so a switch whose parking never matured — a fast A→B→A + flip — still closes on `kViewUndoMask` alone and costs no FX serialization. What a + Ctrl-Z cannot restore is anything the reclaimer converges AFTER the span closed: a + transport deferral, and a drain no switch opened a span for, each mint their own + point — and, since the terminator above, whatever the drain had NOT yet written when + the undo arrived, because the span closes at that instant. Whether that reads as + correct in the DAW is `[verify — DAW]`, `docs/VERIFICATION.md` §"Mode switching". +- **A NATIVE REAPER action fired during the span is absorbed into the switch's point.** + The nesting rule and `UndoBlock` cover every EXTENSION-owned block; REAPER's own actions + cannot be routed through it, so under the depth-counter reading their block closes at + depth 2->1, minting nothing and folding their state into ours. Not fixed here and not + measured — it rests on the same `[verify — DAW]` counter reading as the nesting rule, and + the window is the park hold plus the drain. +- **The `view_state` write is NOT inside `applyMode`'s block — but on a REAL switch + it is now inside the SPAN.** `persistViewState` (`design_view_actions.cpp`) runs + after `applyMode` has returned, and a real switch returns with the span open, so + the `SetProjExtState` — and the modal Save-As `persistViewState` can pop on an + 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. On the MEMBERSHIP + path the write is now deliberately inside the action's own `UndoBlock` — it is the + only thing there a point can hold — and the persist is split so its modal Save-As half + runs BEFORE that block opens (`shell/actions/design_view_actions`, which owns the + reasoning). 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 @@ -187,9 +413,13 @@ applies the resulting lane state to live tracks. semantics for the state mask; (2) that a redo fires `BeginLoadProjectState` with `isUndo == true` so the session really does reload the stale ext state (asserted in `main.cpp`, never observed). If (2) is false the in-memory model - keeps its snapshots and the redo is consistent. `[verify — DAW]`. Do not "fix" - it by moving `persistViewState` inside the block — it can open a modal Save-As; - whether to split the persist is a separate decision. + keeps its snapshots and the redo is consistent. `[verify — DAW]`. Link (1) is the + same capture-time reading the membership path's fix now rests on, and the nearest + thing to evidence for it is `reaper_plugin.h:1198-1201`: REAPER serializes an + extension's state for a point through `SaveExtensionConfig` with `isUndo` set — i.e. + at point creation, not later. Settling it by moving the SWITCH path's + `persistViewState` is still the wrong move: that one runs with the span open, so its + modal would stand inside a block nothing closes until the drain converges. - A DIFFERENT strand entirely, which the EXPOSE 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 @@ -215,11 +445,17 @@ 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]`. +- **"Nothing nests on the reapply paths" was FALSE, and cost the switch its undo + point.** `render_in_place` and the membership actions each call `applyMode` inside + their own block; the reapply opens none of its own, but a prior REAL SWITCH's span can + still be open — for `kParkHoldMs` plus the whole drain — and it is a block like any + other. Two things nest there, and both are now handled by the "nothing may open a + block while the span is open" invariant: the caller's block over the span (`UndoBlock` + ends it at the outermost open), and the reapply's former unconditional supersede-close + inside the caller's block (only a real switch closes now). What still nests + legitimately: `doMoveItems` and `render_in_place` over `mintManagedLanes`, all three + `UndoBlock`s, so the SDK's undocumented nested-block coalescing is load-bearing only + for that shape. - `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.cpp b/src/shell/view/fx_reclaim.cpp index 36ef36e..fc6979c 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -11,6 +11,7 @@ #include #include "shell/persist/session.h" // flushViewState — the release's durable half +#include "shell/undo/undo_block.h" // undoBlockOpen — the reverse of the nesting rule #include "shell/view/view.h" // modeApplyInProgress, transportBlocksModeSwitch #include "shell/view/view_park.h" // fxGuidString — the one FX-identity read #include "shell/view/view_tracks.h" @@ -38,6 +39,37 @@ FxReclaimQueue g_queue; // A release applied to the in-memory model and not yet written — see ReleaseFlush. ReleaseFlush g_releaseFlush; +// The one open undo block a mode switch and its parking share — see UndoSpan. +UndoSpan g_span; + +// The ONE open and the ONE close. Both idempotent, so every caller may say either +// unconditionally; the close reads the span's values before clearing it, because the +// clear has to precede the SDK call — nothing re-entered through Undo_EndBlock2 may +// find a span it could close a second time. +void openSpan(ReaProject* proj, const char* label, int mask) { + if (g_span.begin(proj, label, mask)) Undo_BeginBlock2(proj); +} + +void closeSpan() { + if (!g_span.open()) return; + ReaProject* const proj = g_span.project(); + const std::string label = g_span.label(); + const int mask = g_span.mask(); + g_span.end(); + // The span outlives its opener, so by here the project may be the one that just + // closed — its undo history went with it and there is no block left to end. + if (ValidatePtr2(nullptr, proj, "ReaProject*")) Undo_EndBlock2(proj, label.c_str(), mask); +} + +// Monotonic and sub-millisecond: the drain's budget discriminates at 30ms, which +// GetTickCount's ~16ms granularity cannot do. No wraparound to guard against, so +// util::elapsedAtLeast (whose whole reason for existing is that wrap) does not apply. +double monotonicMs() { + return std::chrono::duration( + std::chrono::steady_clock::now().time_since_epoch()) + .count(); +} + // TrackFX_SetOffline is ASSUMED to pump the message loop, so a plugin load can re-enter // the timer and land back here mid-drain. Fail closed: a re-entered drain would derive a // second queue for the same project and race the outer one's writes track for track. @@ -100,21 +132,18 @@ struct ReclaimHost { return cachedTrack_; } - // Monotonic and sub-millisecond: the budget below discriminates at 30ms, which - // GetTickCount's ~16ms granularity cannot do. No wraparound to guard against, so - // util::elapsedAtLeast (whose whole reason for existing is that wrap) does not apply. - double nowMs() const { - return std::chrono::duration( - std::chrono::steady_clock::now().time_since_epoch()) - .count(); - } + double nowMs() const { return monotonicMs(); } // The SAME predicate applyMode refuses a real mode switch on, deliberately shared // rather than re-read here: "the transport is rolling" is one fact with one home // (view.h). drainTick narrows what it gates with this answer to offline work only. bool transportRolling() const { return transportBlocksModeSwitch(project); } + // Refusing here ends the drain's REMAINING tracks after an abandon: drainTick treats a + // false as "this track is gone", so the rest of the queue costs one bool apiece and + // reaches no REAPER call at all. bool liveChain(const std::string& guid, std::vector& out) { + if (g_queue.abandoned()) return false; MediaTrack* tr = resolveForStep(guid); if (!tr) return false; const int count = TrackFX_GetCount(tr); @@ -126,22 +155,25 @@ struct ReclaimHost { return true; } + // ...and here for the track being written RIGHT NOW, whose own pump is how the state + // load arrived. FIRST, before the openSpan below: a write past the terminator would + // both land in the project the load is replacing and reopen the span it just closed. bool writeOffline(const std::string& guid, int fxIndex, bool offline) { + if (g_queue.abandoned()) return false; MediaTrack* tr = resolveForStep(guid); if (!tr) return false; if (fxIndex < 0 || fxIndex >= TrackFX_GetCount(tr)) return false; // chain shrank if (TrackFX_GetOffline(tr, fxIndex) == offline) return false; // already there - // DAW-VERIFIED: the same ten TrackFX_SetOffline calls mint ten undo points - // unblocked and ZERO inside this pair. One block per WRITE even though a step is - // now one whole track: the write itself is what pumps, so the block cannot avoid - // spanning a pump — it can only span as little of one as possible. Widening it to - // the track would multiply that exposure by the chain length while suppressing no - // extra undo point (the discard form mints none either way), and a user action - // landing inside a discarded block can lose its own Ctrl-Z. - Undo_BeginBlock2(project); + // Every write in the drain rides ONE span. Do not reintroduce a block per write: + // it minted one undo point per FX toggle, because the ("", 0) discard form + // coalesces rather than discards outside an action (see UndoSpan). The open is a + // no-op under a switch's span, which is why the FX bit is added separately — + // here, at the first real write, so a switch whose parking never matured closes + // on the flags-and-items mask it opened with and serializes no FX state. + openSpan(project, kReclaimUndoLabel, UNDO_STATE_FX); + g_span.widen(UNDO_STATE_FX); TrackFX_SetOffline(tr, fxIndex, offline); - Undo_EndBlock2(project, "", 0); return true; } @@ -178,6 +210,25 @@ void reportDrops(ReaProject* proj, const std::vector& drops) { bool fxReclaimPending() { return g_queue.pending(); } +FxReclaimActivity fxReclaimActivity() { + // The QUEUE's own pin, never the active tab: the direction is a fact about the project + // the drain is converging, and reading another tab's transport into it would gate the + // answer on a playhead that has nothing to do with this work. No pin means idle — and + // costs the caller no REAPER call at all, which is what makes this safe to poll per + // frame. + ReaProject* const proj = g_queue.project(); + if (!proj) return FxReclaimActivity::Idle; + // The pin is only re-tested at beginTick; a poll landing between two ticks can find the + // project already gone, and GetPlayStateEx is about to be handed this pointer. + if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return FxReclaimActivity::Idle; + // A FOURTH stand-off, alongside no-project/invalid-project/transport: fxReclaimTick + // refuses the tick outright while undoBlockOpen(), so nothing may move for as long as + // one is open — the same "Idle whenever nothing may move" promise this reports for the + // debounce hold and the transport gate. + if (undoBlockOpen()) return FxReclaimActivity::Idle; + return g_queue.activity(monotonicMs(), transportBlocksModeSwitch(proj)); +} + void invalidateFxReclaim() { // A generation bump and a flag, and deliberately nothing more. Discarding the queue // here is what let a re-entered apply free the element the outer drain was writing @@ -188,10 +239,24 @@ void invalidateFxReclaim() { bool fxReclaimDrainInProgress() { return g_draining; } void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { + // NO guard here may close the span: each names a caller that is still on the stack and + // still owns it — the outer drain, or the applyMode that opened it. All re-tick. if (g_draining) return; // The model is mid-rewrite and its records are being consumed out from under any // plan derived from it. The apply invalidates on its way out, so nothing is lost. if (modeApplyInProgress()) return; + // The MIRROR of the rule that no block may open over the span: an extension undo + // block is open — a capture, a bank verb, an ingest — and its own message pump is how + // this tick arrived. openSpan from in there would begin at depth 2, and that block's + // close would then mint under the SPAN's label and destroy the user's own point. + // Refused whole rather than written unspanned, which would instead absorb the FX + // toggles into the user's act. Starves nothing: the wakeup, the derived queue and the + // project pin are all untouched (this returns before beginTick), so the first tick + // after the block closes drains exactly as if this one had never been due. That last + // claim is this function's own control-flow order — this TU is REAPER-linked, off the + // pure fx_reclaim_tests target, so unlike the queue/span state machine above it, the + // ordering is DAW-verified, not unit-tested. + if (undoBlockOpen()) return; DrainLatch latch; // beginTick owns what a pin/project mismatch means. What is local to here: the model @@ -200,6 +265,12 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { // that gate that poll. ReaProject* project = proj ? proj : EnumProjects(-1, nullptr, 0); if (!ValidatePtr2(nullptr, project, "ReaProject*")) project = nullptr; + + // A span belongs to the project it was opened on. Keyed on the SPAN's own project + // rather than the queue's pin, which moves independently: a tab switch, a closed + // project, or no active project at all ends it where it stands rather than carrying + // it into another project's history — and the no-project arm below returns. + if (g_span.open() && g_span.project() != project) closeSpan(); if (!g_queue.beginTick(project)) return; // DERIVED, never persisted: recomputed from view state every time the queue was @@ -213,9 +284,32 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { ReclaimHost host{project, &session}; const ReclaimRunResult run = g_queue.run(host); reportDrops(project, run.drops); + if (spanEndsAfter(run)) closeSpan(); if (!run.aborted && run.settled) g_queue.goIdle(); } +void fxReclaimBeginUndoSpan(ReaProject* proj) { + openSpan(proj, "", 0); +} + +void fxReclaimLabelUndoSpan(const char* label, int mask) { + g_span.relabel(label ? label : "", mask); +} + +void fxReclaimEndUndoSpan() { closeSpan(); } + +void fxReclaimAbandonForStateLoad() { + // Close FIRST and unconditionally. The abandon below stops the drain and puts the loop + // to sleep; the block is what a Ctrl-Z would otherwise close over the wrong state, and + // it is open whether or not a drain is on the stack at all. Safe even if this hook were + // reachable from inside an extension UndoBlock (see fxReclaimFlushRelease below for a + // confirmed case of that shape on the save side) — the span can never be open at depth + // > 0 in the first place: src/shell/view/CLAUDE.md's "NOTHING MAY OPEN AN UNDO BLOCK + // WHILE THE SPAN IS OPEN" invariant. closeSpan() just finds nothing to close there. + closeSpan(); + g_queue.abandon(); +} + void fxReclaimFlushRelease(ReaSamplerSession& session) { // GetCurrentProjectInLoadSave() names the project THIS hook is being asked to save // (reaper_plugin_functions.h:1582-1587) -- not merely the active tab, which a @@ -224,6 +318,11 @@ void fxReclaimFlushRelease(ReaSamplerSession& session) { // the project actually being written, or match the arm against the active tab while a // different project saved and write into that wrong project's ext state. ReaProject* saving = GetCurrentProjectInLoadSave(); + // BEFORE the arm test, and unconditionally: a save is a user act the span must not + // absorb, and the arm is not what decides whether one is open. Same "depth > 0 never + // holds an open span" invariant as fxReclaimAbandonForStateLoad above makes this safe + // even when the save itself is issued from inside an extension UndoBlock. + closeSpan(); if (!g_releaseFlush.pendingFor(saving)) return; // Promptless, unlike design_view_actions' persistViewState, which can open a modal // Save-As. [verify — DAW] whether this write reaches the block of the diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index b56fa28..753a3fd 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -7,6 +7,7 @@ #include #include #include +#include #include #include "core/view/fx_state.h" @@ -121,6 +122,56 @@ inline int nextEligible(const std::vector& items, const ParkClock& return -1; } +// The item a step may move RIGHT NOW, under BOTH gates the loop applies: the debounce +// above, and the transport, which mirrors applyMode's refusal of a real mode switch +// (view.h, transportBlocksModeSwitch) narrowed to the OFFLINE direction only — +// onlining is the switch's own unfinished promise and the user is waiting on it, while +// offlining is opportunistic reclaim that can always wait for the transport to stop. +// -1 when nothing may move; `transportDeferred` is raised only when the transport, and +// not the debounce, is what held an otherwise-eligible item back. +// +// The ONE home of that pair: drainTick steps through it and nextActivity reports +// through it, so the direction an indicator paints can never disagree with the +// direction the next step actually takes. +inline int nextMovable(const std::vector& items, const ParkClock& clock, + double nowMs, bool transportRolling, bool& transportDeferred) { + const int i = nextEligible(items, clock, nowMs); + if (i < 0) return -1; + if (transportRolling && + items[static_cast(i)].work.kind == view::FxWorkKind::Offline) { + transportDeferred = true; + return -1; + } + return i; +} + +// Which way the drain is MOVING — the panel's indicator, and undirected `fxReclaimPending` +// is what it may not be driven off: for the whole ~3s after a switch the queue holds the +// park's offline work while the only thing eligible is the RETURNING chains' online work, +// so "pending" there names the opposite of what is happening. +// +// Idle means nothing may move on this tick, and deliberately does not distinguish why — +// an empty queue, an offline item inside its hold, and an offline item held by a rolling +// transport all read Idle, because none of them is work the user is currently waiting +// through. Online while the transport rolls is a real state and reads Online: that +// direction is not gated. +enum class FxReclaimActivity { + Idle, + Online, + Offline, +}; + +inline FxReclaimActivity nextActivity(const std::vector& items, + const ParkClock& clock, double nowMs, + bool transportRolling) { + bool deferred = false; + const int i = nextMovable(items, clock, nowMs, transportRolling, deferred); + if (i < 0) return FxReclaimActivity::Idle; + return items[static_cast(i)].work.kind == view::FxWorkKind::Online + ? FxReclaimActivity::Online + : FxReclaimActivity::Offline; +} + // --- the drain ---------------------------------------------------------------- // One ONLINE item's captured state the chain had no home for. An OFFLINE item can @@ -155,6 +206,12 @@ struct ReclaimTickResult { int writes = 0; bool settled = false; + // The transport gate — and only it — held an otherwise-eligible item back this + // tick. Distinguished from the debounce because the two waits differ in KIND: a + // hold expires at kParkHoldMs, a deferral lasts as long as the playback pass does, + // and the undo span may only stay open across a wait it can bound (spanEndsAfter). + bool transportDeferred = false; + // Tracks whose park record the tick drove fully online, so the record's FX half is // spent. REPORTED, not applied: releasing is a model write and the model is exactly // what a re-entered load can replace mid-drain, so only the owner may commit it — @@ -178,8 +235,9 @@ struct ReclaimTickResult { // may still online while it rolls // bool liveChain(guid, std::vector&) FX identity per live slot; // false when the track is gone -// bool writeOffline(guid, fxIndex, offline) compare-before-write inside its -// own discarded undo block; +// bool writeOffline(guid, fxIndex, offline) compare-before-write, opening +// the drain's undo span if no +// switch already did (UndoSpan); // true iff it actually wrote // void releaseFx(guid) release the record's FX half and // arm the save-time flush that @@ -203,19 +261,13 @@ ReclaimTickResult drainTick(std::vector& items, ParkClock& clock, H // its track genuinely stops owing offline work, matured or not. clock.observe(items, host.nowMs()); - // Mirrors applyMode's refusal (view.h, transportBlocksModeSwitch), narrowed to the - // OFFLINE direction only: onlining is the mode switch's own unfinished promise — the - // user is waiting on it — while offlining is opportunistic reclaim that can always - // wait for the transport to stop. Onlining while rolling still costs a plugin load - // mid-playback; that is an accepted trade for this gate's shape, not a hazard it - // introduces. - const bool offlineAllowed = !host.transportRolling(); + // Read once for the whole tick and applied through nextMovable, which owns the gate + // (and the reasoning for narrowing it to Offline). Onlining while rolling still costs + // a plugin load mid-playback; that is an accepted trade for this gate's shape, not a + // hazard it introduces. + const bool rolling = host.transportRolling(); auto nextAllowed = [&](double nowMs) { - const int i = nextEligible(items, clock, nowMs); - if (i >= 0 && !offlineAllowed && - items[static_cast(i)].work.kind == view::FxWorkKind::Offline) - return -1; - return i; + return nextMovable(items, clock, nowMs, rolling, result.transportDeferred); }; const int first = nextAllowed(host.nowMs()); @@ -274,6 +326,9 @@ struct ReclaimRunResult { int writes = 0; bool settled = false; + // Carried straight out of the tick — see ReclaimTickResult::transportDeferred. + bool transportDeferred = false; + // Something invalidated the queue while the drain was on the stack. The work and // the releases are DISCARDED, `settled` is never true, and the caller must not go // idle — the next tick re-derives against whatever changed. @@ -305,6 +360,50 @@ public: pending_ = true; } + // A project-state load — undo, redo, an open, a revert, a new project. The whole + // project is about to be replaced, so this is STRONGER than an invalidation: the + // queue's work is discarded as usual, AND any drain on the stack — reachable only + // through a write's own message pump — stops where it stands instead of finishing the + // track it is on. Accepted, and the reason a step's atomicity yields here: a park + // half-landed this way stays half-landed, covered by the one point the terminator + // closed. A load that keeps the SAME ReaProject (a revert) is what makes the stop + // load-bearing rather than tidy: the drain's pin and cached track handle both still + // validate afterwards, so nothing else would keep it out of the reloaded project. + // + // The wakeup goes DOWN, where an invalidation raises it — do not reintroduce the raise. + // An invalidation says the MODEL moved and owes a re-derivation; a load says the + // PROJECT moved under a model nobody has re-read yet, which is the opposite of a reason + // to converge. Every load that re-reads the model raises the wakeup itself, on that + // same tick, through the reapply the reload drives (applyMode is invalidateFxReclaim's + // one caller); File > Revert re-reads nothing, so waking here was the only thing that + // ever ran, and it wrote FX and spent park records against the pre-revert model. + // + // Safe to clear state under a drain, unlike invalidate(): the queue's own storage is + // empty for the drain's whole duration (run() moved it out), and run()'s abort path + // returns before it would read derived_ or commit anything. + void abandon() { + ++generation_; + abandoned_ = true; + discard(); + pending_ = false; + // The HOLDS GO, where an invalidation keeps them — the one place the two differ on + // the clock, and for the same reason they differ on the wakeup. A hold records "this + // track has read continuously parked since T", an observation about the project this + // load is replacing; after the load the chains may sit anywhere (an undo of a park + // restores them online) and no such continuity is in evidence. Keeping it fired the + // post-load re-park on the FIRST tick after the load, with zero debounce, right in + // the wake of REAPER's restore. Undoing a switch mid-park is precisely the A->B->A + // flip kParkHoldMs exists to make free: re-earning the hold is what makes it free + // again, and what keeps the re-convergence's writes clear of the load. + clock_.forget(); + // The pin STAYS — beginTick compares it to decide a retarget, and losing it there + // would read as "no project" rather than "same project, nothing derived yet". + } + + // Read by the shell's host on every step; false again from the next run(), because the + // stop belongs to the tick that was interrupted, not to the project that replaced it. + bool abandoned() const { return abandoned_; } + // The ONE test the timer makes per tick. False means settled: nothing to converge // until the next invalidation. bool pending() const { return pending_; } @@ -361,23 +460,51 @@ public: const std::vector& items() const { return items_; } + // Which way the drain is moving, for an indicator to paint — see nextActivity, which + // owns the rule. Const and cheap: the panel polls it per frame. + FxReclaimActivity activity(double nowMs, bool transportRolling) const { + // The queue's own storage is EMPTY for a drain's whole duration (run() moves it + // into locals), and a write pumps the message loop — so a repaint landing inside + // one would otherwise read the drain as idle at exactly the moment it is working. + if (draining_ != FxReclaimActivity::Idle) return draining_; + return nextActivity(items_, clock_, nowMs, transportRolling); + } + template ReclaimRunResult run(Host& host) { const unsigned long long entered = generation_; + abandoned_ = false; std::vector work; ParkClock clock; work.swap(items_); clock.swap(clock_); + draining_ = nextActivity(work, clock, host.nowMs(), host.transportRolling()); const ReclaimTickResult tick = drainTick(work, clock, host); + draining_ = FxReclaimActivity::Idle; // The hold is keyed on the track and outlives any re-derivation, so it comes // back whether or not the drain's own work survives. clock_.swap(clock); + // ...but not across a state load. An abandon reaching us through a write's own + // message pump found the clock already moved out into the drain's local, so its + // forget cleared an empty member; redo it now the holds are back. `abandoned_` is + // cleared at this call's entry, so it can only mean an abandon landed inside it. + if (abandoned_) clock_.forget(); - if (generation_ != entered) - return ReclaimRunResult{tick.writes, false, true, tick.drops}; + // Assembled field by field rather than positionally: the aggregate is read by + // the undo span's close decision now, and a silently mis-slotted bool there + // leaks an open block. + ReclaimRunResult run; + run.writes = tick.writes; + run.transportDeferred = tick.transportDeferred; + run.drops = tick.drops; + + if (generation_ != entered) { + run.aborted = true; + return run; + } items_.swap(work); // A release dropped by an abort is re-owed, not lost: the next derivation plans @@ -385,7 +512,8 @@ public: // release fires then. Applied in MEMORY only — the spend reaches disk on the // project's next save (ReleaseFlush), never on the drain's own initiative. for (const std::string& guid : tick.released) host.releaseFx(guid); - return ReclaimRunResult{tick.writes, tick.settled, false, tick.drops}; + run.settled = tick.settled; + return run; } private: @@ -396,6 +524,8 @@ private: unsigned long long derivedAt_ = 0; bool derived_ = false; bool pending_ = false; + bool abandoned_ = false; + FxReclaimActivity draining_ = FxReclaimActivity::Idle; }; // --- the release's durable half -------------------------------------------------- @@ -433,12 +563,100 @@ private: ReaProject* project_ = nullptr; // COMPARE-ONLY, never dereferenced }; +// --- the undo span --------------------------------------------------------------- + +// The label a drain that no mode switch opened a span for closes under. Names the +// feature the user knows — Design View, and its CPU reclaim (the toggle's own tooltip +// wording) — rather than the loop behind it, and covers both directions: one act, +// converging the parked/returning chains to what the view state says. +inline constexpr const char* kReclaimUndoLabel = "ReaSampler: reclaim Design View FX"; + +// One open REAPER undo block, held across the timer callbacks the drain runs on. +// +// DAW-measured this phase, and the whole reason this type exists: block WIDTH is the +// entire effect. From a timer, ten TrackFX_SetOffline calls mint ten undo points +// unblocked, ten with a block PER WRITE, and ONE under a single block spanning all +// ten — the ("", 0) discard form does not discard outside an action, it coalesces. A +// block opened on one timer callback and closed on a LATER one holds. +// +// Pure: the Undo_BeginBlock2/EndBlock2 calls are fx_reclaim.cpp's, which owns the ONE +// instance and routes every path through one open and one close. Both moves report +// whether they actually moved, so a caller may say either unconditionally and still +// make no second BeginBlock2 and no unbalanced EndBlock2. +class UndoSpan { +public: + bool open() const { return open_; } + ReaProject* project() const { return project_; } + const std::string& label() const { return label_; } + int mask() const { return mask_; } + + // True iff this call opened it. `label`/`mask` are adopted only then, so a drain + // writing under a switch's span cannot displace the switch's own label. + bool begin(ReaProject* proj, std::string label, int mask) { + if (open_) return false; + project_ = proj; + label_ = std::move(label); + mask_ = mask; + open_ = true; + return true; + } + + // 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); + mask_ = mask; + } + + // Add state the span did not originally promise to serialize — the FX bit, once + // this drain has actually written FX under a switch's flags-and-items mask. + void widen(int extraMask) { + if (open_) mask_ |= extraMask; + } + + // True iff this call closed it; the caller owes Undo_EndBlock2 with the values it + // read BEFORE calling, since this clears them. + bool end() { + if (!open_) return false; + project_ = nullptr; + label_.clear(); + mask_ = 0; + open_ = false; + return true; + } + +private: + ReaProject* project_ = nullptr; // passed back to Undo_EndBlock2, never dereferenced here + std::string label_; + int mask_ = 0; + bool open_ = false; +}; + +// Must the tick that produced `run` end the span? +// +// The span may stay open only across a wait whose length it can bound. Waiting out the +// 3s debounce qualifies and is the wait the whole design turns on — that dead time sits +// between the switch and its first FX write. A TRANSPORT deferral does not: it lasts as +// long as the user plays, and a block held open for a playback pass swallows every edit +// made during it. Ending there costs a second undo point for the parking that follows, +// which is the documented degradation, not a failure. +inline bool spanEndsAfter(const ReclaimRunResult& run) { + return run.settled || run.aborted || run.transportDeferred; +} + // --- shell entry points -------------------------------------------------------- // The ONE test the timer makes per tick, mirroring the realtime capture's single // pointer test. False means the queue is settled and there is nothing to converge. bool fxReclaimPending(); +// Which direction the drain is moving, answered against the project the queue was derived +// against rather than the active tab. `fxReclaimPending` above is deliberately left +// undirected and unchanged; this is the surface an indicator must be driven off instead, +// because "pending" is true throughout the debounce, when the only thing moving is the +// OPPOSITE direction. Idle whenever nothing may move — see FxReclaimActivity. +FxReclaimActivity fxReclaimActivity(); + // Raises the wakeup and marks the derived queue for re-derivation by the NEXT tick; // discards nothing here, because this is reachable from inside a drain. Called from // applyMode and nothing else @@ -455,11 +673,49 @@ bool fxReclaimDrainInProgress(); // Drains one tick against `proj` (nullptr = current project), re-deriving the queue from // the session's view model first if it was invalidated or if the active project moved // (FxReclaimQueue::beginTick). Returns without touching the project while an applyMode is -// on the stack, while already draining, or when there is no valid active project at all. +// on the stack, while already draining, while any extension undo block is open +// (shell/undo's undoBlockOpen — a refusal that consumes nothing, so the next tick drains), +// or when there is no valid active project at all. // Takes the SESSION rather than the model alone because completing an online spends the // park record's FX half — see FxReclaimQueue::run. 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 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. applyMode also raises the wakeup BEFORE this call: the +// tick it guarantees is the only thing that can ever close the span again. +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. +void fxReclaimLabelUndoSpan(const char* label, int mask); + +// The ONE close, idempotent: every terminating path routes through it — the drain +// settling, aborting, or deferring on the transport; the project retargeting or closing; +// the next apply superseding it; ANY project-state load; REAPER's own Undo/Redo at action +// dispatch (shell/undo's endReclaimSpanForUndoCommand, which mints the switch's point +// before REAPER runs the undo); a project save; extension unload; and shell/undo's +// UndoBlock, at its OUTERMOST open. A leaked open block would swallow everything the user +// does afterwards, so nothing may return past a terminating condition without calling this. +void fxReclaimEndUndoSpan(); + +// EVERY project-state load, undo-driven or not — Ctrl-Z, Ctrl-Shift-Z, a project open, a +// File > Revert, a new project. Ends the span, stops any drain on the stack, drops the +// debounce holds, and leaves the loop ASLEEP until something that re-read the model wakes +// it — FxReclaimQueue::abandon owns all four and the reasoning behind each. +// +// Called from the projectconfig BeginLoadProjectState hook, which fires BEFORE REAPER +// restores anything: that is the ONLY point at which the block can still close over the +// state it was opened on. The span is open for at least kParkHoldMs after every real +// switch and a switch is exactly what a user undoes, so closing it as a side effect of +// the load — several calls later, over the restored project — would mint the point over +// post-load state and, for an undo, destroy the redo branch. That defect does not turn on +// isUndo: a non-undo load left to the next tick's project-retarget close lands the same +// point over the same wrong state, one hook branch away, so this hook does not branch. +void fxReclaimAbandonForStateLoad(); + // Writes an in-memory release into the SAVING project's view state (identified via // GetCurrentProjectInLoadSave(), not the active tab). Called from the project-save // hook and nowhere else: this is the ONE path that makes a spent record durable, and diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index 685f48a..7a2fe9a 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -2,7 +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 the one undo block they all ride. +// 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" @@ -23,8 +24,6 @@ #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_EnumProjects #define REAPERAPI_WANT_GetPlayStateEx -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #define REAPERAPI_WANT_TrackList_AdjustWindows #define REAPERAPI_WANT_UpdateArrange #define REAPERAPI_WANT_UpdateTimeline @@ -64,19 +63,20 @@ bool modeApplyInProgress() { return g_applying; } namespace { -bool applyModeImpl(ViewModeModel& model, const std::string& targetModeId, ReaProject* proj) { - if (g_applying) return false; // see ApplyLatch; same fail-closed shape as the guards below +ModeApplyResult applyModeImpl(ViewModeModel& model, const std::string& targetModeId, + ReaProject* proj) { + if (g_applying) return {}; // see ApplyLatch; same fail-closed shape as the guards below ApplyLatch latch; if (!model.modes().contains(targetModeId)) { - return false; // reject before touching the project — no partial apply + return {}; // reject before touching the project — no partial apply } // Pinned ONCE: resolving "current project" independently at the begin and the end // would let a tab switch mid-apply open the block on one project and close it on // another. ReaProject* const project = proj ? proj : EnumProjects(-1, nullptr, 0); - if (!ValidatePtr2(nullptr, project, "ReaProject*")) return false; + if (!ValidatePtr2(nullptr, project, "ReaProject*")) return {}; // The discriminator behind both halves of the contract in view.h. A gate placed // unconditionally here would break tagging and the project-load reapply during @@ -86,7 +86,7 @@ bool applyModeImpl(ViewModeModel& model, const std::string& targetModeId, ReaPro const bool realSwitch = targetModeId != outgoingModeId; if (realSwitch && transportBlocksModeSwitch(project)) { - return false; // same fail-closed shape as the mode-exists guard above + return {}; // same fail-closed shape as the mode-exists guard above } TrackHandles handleByGuid; @@ -116,10 +116,26 @@ 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 block + std::vector exposedNames; // named while handles are fresh, reported after the writes - Undo_BeginBlock2(project); + // ONLY A REAL SWITCH, both halves — end a prior switch's still-open span, then open + // this one's. A REAPPLY must do neither: it opens none because nothing it writes + // mints unaided, and it CLOSES none because it is reachable from inside a caller's + // own block (a membership action, render_in_place), where a close would land at + // depth 2->1 and hijack the switch's point instead of minting it. A caller that + // opens a block ends the span itself, first, at depth 0 — see this directory's + // CLAUDE.md. Both calls are idempotent; do not reintroduce either unconditionally. + if (realSwitch) { + fxReclaimEndUndoSpan(); + // BEFORE the open, never after this function returns. The wakeup is what + // guarantees a tick, and a tick is the only thing that can close this span; a + // throw anywhere below would otherwise leave the block open with the reclaimer + // asleep, swallowing every later edit until a save, a state load, the next switch + // or unload. The wrapper's own invalidate stays where it is — it covers the + // refusal paths above, which never reach this line. + invalidateFxReclaim(); + fxReclaimBeginUndoSpan(project); + } { UiRefreshHold uiHold; // every write below lands with the TCP/MCP frozen @@ -138,7 +154,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)); @@ -149,14 +164,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. @@ -166,8 +180,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); @@ -194,24 +208,28 @@ 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); - Undo_EndBlock2(project, mint ? undoLabel.c_str() : "", mint ? kViewUndoMask : 0); + // 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); - // After the undo block: an EXPOSE report is a console message, not a project - // write, and must not join what a Ctrl-Z rolls back. + // 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. reportExposedTracks(project, exposedNames); // The tool's own account of what it just left driven — the only one that survives - // an undo rolling the model back underneath it. + // an undo rolling the model back underneath it. In memory, never on disk. noteAppliedState(project, model); - return true; + return ModeApplyResult{true}; } } // namespace -bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject* proj) { - const bool applied = applyModeImpl(model, targetModeId, proj); +ModeApplyResult applyMode(ViewModeModel& model, const std::string& targetModeId, + ReaProject* proj) { + const ModeApplyResult result = applyModeImpl(model, targetModeId, proj); // Every membership, snapshot and active-mode mutation in the system reaches the // project through this call, so this is the whole invalidation surface: the @@ -224,7 +242,7 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject // else would drop a queue that now converges toward the mode the user just left. // The cost when the refusal followed no mutation is one re-derivation on the timer. invalidateFxReclaim(); - return applied; + return result; } } // namespace reasampler diff --git a/src/shell/view/view.h b/src/shell/view/view.h index d91e5c4..933d377 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,46 +22,53 @@ 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. -// 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; -} - // 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. bool modeApplyInProgress(); +// What one applyMode did. +// +// Deliberately does NOT report whether it wrote project state. It once did, and a +// caller holding its own block used that as its mint-or-discard predicate — which is +// wrong twice over: everything a reapply writes is the four driven flags, which mint +// no undo state at all, so the answer was true for writes REAPER will not hold and the +// resulting label re-titled the Undo button over an entry that was never appended. What +// a membership edit's point actually holds is its `view_state` write; the caller's own +// model diff is what says whether there was one (design_view_actions' +// runMembershipEdit). Do not reintroduce a wrote-project-state return here. +struct ModeApplyResult { + bool applied = false; +}; + // Captures each about-to-park track's prior flag values into `model`, caches/clears // 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 // all the flag work below. Do not reintroduce a synchronous FX write. // -// 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 +// 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 opens none, so a +// caller that wants its own act undoable opens its own (design_view_actions does) — +// and, because REAPER's blocks are a depth counter, ENDS THE SPAN BEFORE IT OPENS +// THAT BLOCK. A reapply never closes a span, precisely because it can be running +// inside such a caller's block. +// +// `applied` is 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 // (modeApplyInProgress), or if the project does not validate. A reapply (target == // active) is never gated and never touches solo. `proj` == nullptr means the current // project, resolved ONCE and used for every call this apply makes. // // Either way — applied or refused — this drops the FX reclaimer's derived queue, which // is why callers may safely discard the return after mutating the model. -bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject* proj); +ModeApplyResult applyMode(ViewModeModel& model, const std::string& targetModeId, + ReaProject* proj); } // namespace reasampler diff --git a/src/shell/view/view_lanes.cpp b/src/shell/view/view_lanes.cpp index 087589c..ae3dbda 100644 --- a/src/shell/view/view_lanes.cpp +++ b/src/shell/view/view_lanes.cpp @@ -16,14 +16,13 @@ #include "core/view/lane_keys.h" #include "core/view/view_tree.h" #include "shell/capture/item_read.h" +#include "shell/undo/undo_block.h" // the ONE undo-block opener #include "shell/view/view_park.h" // writeIfChanged — the read-before-write every driven parm takes #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_GetMediaTrackInfo_Value #define REAPERAPI_WANT_SetMediaTrackInfo_Value #define REAPERAPI_WANT_GetSetMediaTrackInfo_String -#define REAPERAPI_WANT_Undo_BeginBlock2 -#define REAPERAPI_WANT_Undo_EndBlock2 #define REAPERAPI_WANT_UpdateArrange #define REAPERAPI_WANT_UpdateTimeline #define REAPERAPI_WANT_CountTrackMediaItems @@ -276,16 +275,46 @@ bool mintManagedLanes(ViewModeModel& model, ReaProject* proj) { const TrackByGuid trackByGuid = indexHandles(handleByGuid); - // `proj` deliberately NOT pinned the way applyMode pins it: nothing between this - // begin and its end pumps the message loop, so no tab switch can land between them. - Undo_BeginBlock2(proj); - const bool changed = applyMintPlan(model, plan, trackByGuid); + bool changed = false; + { + // Opening ends any reclaim span — but only when this is the OUTERMOST block, so + // the two callers that already hold one (doMoveItems, render_in_place) are not + // hijacked. UndoBlock owns that criterion; do not re-end the span here. + // + // ACCEPTED, not fixed: the plan being non-empty is not the same as its writes + // being needed, so the already-satisfied case below still ends a switch's span + // having minted nothing of its own. Telling the two apart means running the plan + // first, which is the write. The cost is bounded at ONE extra undo point — the + // switch mints early and its remaining parking mints again under + // kReclaimUndoLabel, the same degradation a transport deferral already carries — + // where the alternative, opening over the span, destroys the switch's point + // outright. + // + // `proj` deliberately NOT pinned the way applyMode pins it: nothing between the + // open and the close pumps the message loop, so no tab switch can land between. + UndoBlock undo(proj); + changed = applyMintPlan(model, plan, trackByGuid); + + if (changed) { + // Reapply the active mode's lane visibility so freshly-minted lanes take + // their play/show state immediately. applyMode is deliberately NOT reused + // here — it would re-park/restore whole tracks and recompute parent + // visibility, which a lane-only mint must not touch. + const TogglePlan togglePlan = + model.planToggle(FolderTree{}, model.activeModeId()); + applyLaneOps(trackByGuid, togglePlan.lanes); + + UpdateTimeline(); // a split happened this call — refresh is owed + UpdateArrange(); + + undo.recordPoint("ReaSampler: separate cross-mode content into lanes", + kViewUndoMask); + } + // Otherwise every write was already satisfied: the block's default discard form + // keeps the detect tick from flooding history with empty points. + } if (!changed) { - // Plan was non-empty but every write was already satisfied — discard the empty - // undo point rather than flooding history every detect tick. - Undo_EndBlock2(proj, "", 0); - // The arrange still needs a redraw: this no-op path is reached when a // freshly-inserted item already landed on the active mode's playing lane // (REAPER places new items on the playing lane), so assignItemToLane wrote @@ -293,21 +322,8 @@ bool mintManagedLanes(ViewModeModel& model, ReaProject* proj) { // alone (no I_FREEMODE change happened, so UpdateTimeline isn't owed) is a // repaint, not a mutation — stays outside the undo block. UpdateArrange(); - return false; } - - // Reapply the active mode's lane visibility so freshly-minted lanes take their - // play/show state immediately. applyMode is deliberately NOT reused here — it - // would re-park/restore whole tracks and recompute parent visibility, which a - // lane-only mint must not touch. - const TogglePlan togglePlan = model.planToggle(FolderTree{}, model.activeModeId()); - applyLaneOps(trackByGuid, togglePlan.lanes); - - UpdateTimeline(); // a split happened this call — refresh is owed - UpdateArrange(); - - Undo_EndBlock2(proj, "ReaSampler: separate cross-mode content into lanes", kViewUndoMask); - return true; + return changed; } void reconcileManagedLanes(ViewModeModel& model, ReaProject* proj) { diff --git a/src/shell/view/view_lanes.h b/src/shell/view/view_lanes.h index 6fd1311..ca0bdd5 100644 --- a/src/shell/view/view_lanes.h +++ b/src/shell/view/view_lanes.h @@ -15,8 +15,8 @@ class ReaProject; namespace reasampler { // `freeMode` alone decides whether UpdateTimeline is owed (SDK: required after an -// I_FREEMODE change); `wrote` is any project write at all, which is what the apply's -// undo-point decision reads. +// I_FREEMODE change); `wrote` is a REAPER write and nothing more — not an undo-point +// predicate (these writes mint no undo state), and read by nothing today. struct LaneApplyResult { bool freeMode = false; bool wrote = false; @@ -24,14 +24,22 @@ struct LaneApplyResult { // Reconciles each op's durable laneKey to the track's current ordinal (a // stale/renamed/deleted key is pruned, never mis-driven) and drives C_LANEPLAYS. +// +// Opens NO undo block, where mintManagedLanes below opens one over the same +// parameters — the one criterion this directory's CLAUDE.md states, applied: this +// re-asserts derived state the view model recomputes from membership on every apply, +// so nothing here needs to survive as a point of its own; minting creates lanes and +// re-homes items, which no re-derivation reproduces. LaneApplyResult applyLaneOps(const TrackByGuid& handleByGuid, const std::vector& lanes); // Splits any track visible in more than one mode while carrying its own media into // fixed lanes (one managed lane per involved mode), assigns items, and records -// ownership in `model`. Wraps the mutation in one Undo block when non-empty. Returns -// true if any lane was minted (repaint hint). `proj` == nullptr means the current -// project. +// ownership in `model`. Wraps the mutation in one Undo block when non-empty — see +// applyLaneOps above for why this site opens one and that one does not — and ENDS THE +// RECLAIM SPAN first, since REAPER's blocks are a depth counter and this is reachable +// from a timer tick with a prior switch's span still open. Returns true if any lane +// was minted (repaint hint). `proj` == nullptr means the current project. bool mintManagedLanes(ViewModeModel& model, ReaProject* proj); // Recovers managed-lane ownership from durable P_LANENAME on project open — diff --git a/src/shell/view/view_park.cpp b/src/shell/view/view_park.cpp index 58f4785..a65da1b 100644 --- a/src/shell/view/view_park.cpp +++ b/src/shell/view/view_park.cpp @@ -144,7 +144,6 @@ TrackParkResult parkTrack(ViewModeModel& model, const std::string& guid, MediaTr snap.fxKeying = prior->fxKeying; } model.storeSnapshot(guid, snap); - result.wrote = true; } if (applyFlags(tr, flags)) result.wrote = true; @@ -153,9 +152,9 @@ TrackParkResult parkTrack(ViewModeModel& model, const std::string& guid, MediaTr bool restoreTrack(ViewModeModel& model, const std::string& guid, MediaTrack* tr, const std::vector& flags) { - applyFlags(tr, flags); + const bool wrote = applyFlags(tr, flags); model.releaseSnapshotFlags(guid); - return true; + return wrote; } std::string trackDisplayName(MediaTrack* tr) { diff --git a/src/shell/view/view_park.h b/src/shell/view/view_park.h index 5150db2..78d0106 100644 --- a/src/shell/view/view_park.h +++ b/src/shell/view/view_park.h @@ -93,11 +93,14 @@ extern const int kViewUndoMask; // Returns whether it wrote. bool writeIfChanged(MediaTrack* tr, const char* parm, double value); -// What one parkTrack call did. `wrote` is any project write at all — a flag write -// or a record write — and feeds the apply's undo-point fold. `exposed` is true iff -// this call handed the track back visible instead of parking or adopting it; the -// caller (view.cpp) collects exposed tracks across the whole apply for the one -// batched console report. +// What one parkTrack call did. `wrote` is a REAPER write — a driven flag moved — and +// nothing else; storing the snapshot is a MODEL move and does not count. It is NOT an +// undo-point predicate and never was a sound one: the driven flags mint no undo state, +// so no point can hold what this reports (view.h's ModeApplyResult, which used to carry +// it up, no longer does). Nothing reads it today. +// `exposed` is true iff this call handed the track back visible instead of parking or +// adopting it; the caller (view.cpp) collects exposed tracks across the whole apply for +// the one batched console report. struct TrackParkResult { bool wrote = false; bool exposed = false; @@ -170,8 +173,9 @@ inline bool shouldReport(bool sameOwnerAsLast, const std::vector& l void reportExposedTracks(ReaProject* proj, const std::vector& trackNames); // Restores one returning leaf from its record and spends the record's flag half. -// Always true: the record is consumed here, so this apply changed state and is not -// repeatable, which is exactly what its one reader (the undo-point decision) asks. +// Returns whether a FLAG actually moved — the same REAPER-writes-only reading +// TrackParkResult::wrote carries, and for the same reason: spending the record is a +// model move no undo point can hold. bool restoreTrack(ViewModeModel& model, const std::string& guid, MediaTrack* tr, const std::vector& flags); 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_fx_reclaim.cpp b/tests/test_fx_reclaim.cpp index f28a070..685078f 100644 --- a/tests/test_fx_reclaim.cpp +++ b/tests/test_fx_reclaim.cpp @@ -6,7 +6,9 @@ // the user's save rather than on the drain, and — via a host that invalidates from inside // a write — that a re-entered invalidation reaches nothing the drain is using. The queue // owns the project pin and the timer's wakeup for the same reason: a compare-only pointer -// makes the tab switch assertable. +// makes the tab switch assertable. The undo span is here on the same terms: its state +// machine is pure, so open-once/close-once and the wait it may or may not outlast are +// assertable; only the Undo_BeginBlock2/EndBlock2 calls themselves need a DAW. #include "../src/shell/view/fx_reclaim.h" @@ -57,12 +59,29 @@ struct FakeHost { int invalidateAfterWrite = -1; int itemsSeenMidDrain = -1; // queue size as the re-entrant caller finds it + // A Ctrl-Z arriving through the same pump: what fxReclaimAbandonForStateLoad does, + // fired on the Nth successful write. The direction probe rides the same hook because + // the property it checks is the same one — what a caller re-entering mid-drain sees. + int abandonAfterWrite = -1; + int probeActivityAfterWrite = -1; + int spanClosesMidDrain = 0; + FxReclaimActivity activityMidDrain = FxReclaimActivity::Idle; + bool rolling = false; // the transport gate's input + // Stands in for the shell's one span instance: fx_reclaim.cpp opens it on the first + // real write of a drain, exactly here, and widens it to carry the FX bit. Only the + // Undo_BeginBlock2/EndBlock2 calls themselves are out of reach without a DAW. + UndoSpan* span = nullptr; + int spanOpens = 0; + double nowMs() const { return clock; } bool transportRolling() const { return rolling; } + // Both host entry points refuse once abandoned, exactly as fx_reclaim.cpp's do: this + // one ends every REMAINING track, writeOffline below ends the one being written. bool liveChain(const std::string& guid, std::vector& out) { + if (reentrant && reentrant->abandoned()) return false; auto it = chains.find(guid); if (it == chains.end()) return false; out.clear(); @@ -71,11 +90,16 @@ struct FakeHost { } bool writeOffline(const std::string& guid, int fxIndex, bool offline) { + if (reentrant && reentrant->abandoned()) return false; auto it = chains.find(guid); if (it == chains.end()) return false; if (fxIndex < 0 || fxIndex >= static_cast(it->second.size())) return false; if (it->second[static_cast(fxIndex)].offline == offline) return false; + if (span) { + if (span->begin(project, kReclaimUndoLabel, 2)) ++spanOpens; + span->widen(2); + } it->second[static_cast(fxIndex)].offline = offline; clock += stepCostMs; writeLog.push_back(guid + ":" + std::to_string(fxIndex) + (offline ? "=off" : "=on")); @@ -85,6 +109,12 @@ struct FakeHost { reentrant->discard(); reentrant->invalidate(); } + if (reentrant && static_cast(writeLog.size()) == probeActivityAfterWrite) + activityMidDrain = reentrant->activity(clock, rolling); + if (reentrant && static_cast(writeLog.size()) == abandonAfterWrite) { + if (span && span->end()) ++spanClosesMidDrain; // closeSpan, then the stop + reentrant->abandon(); + } return true; } @@ -192,6 +222,18 @@ FxReclaimQueue queueOf(const ViewModeModel& vm, const std::set& liv return q; } +// One whole timer tick as the shell makes it: main.cpp's ONE pending() test, then +// fxReclaimTick's pin, its re-derivation from the session's model whenever the queue is +// stale, and the drain. The re-derivation is what makes the wakeup the load-bearing bit — +// it reads whatever model is live right now, whether or not anything re-read it since. +void timerTick(FxReclaimQueue& q, const ViewModeModel& vm, const std::set& live, + FakeHost& host) { + if (!q.pending()) return; + if (!q.beginTick(host.project)) return; + if (q.stale()) q.adopt(derive(vm, live)); + q.run(host); +} + } // namespace // -- 1. debounce --------------------------------------------------------------- @@ -1011,6 +1053,546 @@ static void testAGatedTickStillGoesIdleWithNothingLeftToConverge() { CHECK(q.run(host).settled); } +// -- 10. the undo span ---------------------------------------------------------------- + +static void testTheSpanOpensOnceAndClosesOnceHoweverManyTimesItIsAsked() { + // The whole safety of the design is in these two answers: a caller may say begin or + // end unconditionally and still make no second Undo_BeginBlock2 and no unbalanced + // Undo_EndBlock2. A leaked open block swallows everything the user does afterwards. + UndoSpan span; + CHECK(!span.open()); + CHECK(!span.end()); // nothing open — the caller owes REAPER nothing + + CHECK(span.begin(kProjA, "ReaSampler: activate Design view", 9)); + CHECK(span.open()); + CHECK(span.project() == kProjA); + CHECK(span.label() == "ReaSampler: activate Design view"); + CHECK(span.mask() == 9); + + // The drain's own begin, arriving under a switch's span: it must adopt nothing — + // neither the other project, nor a label that would rename the user's switch. + CHECK(!span.begin(kProjB, kReclaimUndoLabel, 2)); + CHECK(span.project() == kProjA); + CHECK(span.label() == "ReaSampler: activate Design view"); + CHECK(span.mask() == 9); + + span.widen(2); // ...but the FX bit it DID write under still has to reach the point + CHECK(span.mask() == 11); + + span.relabel("ReaSampler: activate Arrange view", 1); + CHECK(span.label() == "ReaSampler: activate Arrange view"); + CHECK(span.mask() == 1); + + CHECK(span.end()); + CHECK(!span.open()); + CHECK(!span.end()); // the second close reaches nothing + CHECK(span.project() == nullptr); + + // And nothing a closed span carries can be revived out from under the next opener. + span.widen(4); + span.relabel("stale", 8); + CHECK(span.mask() == 0); + CHECK(span.label().empty()); +} + +static void testTheSpanOutlastsADebounceWaitButNeverATransportDeferral() { + // The one policy call: the span may stay open across a wait it can BOUND. The 3s + // debounce is the dead time between the switch and its first FX write, and spanning + // it is the whole point. A transport deferral lasts as long as the user plays. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + const ReclaimRunResult holding = q.run(host); // the hold starts at t=0 + CHECK(!holding.settled); + CHECK(!holding.transportDeferred); + CHECK(!spanEndsAfter(holding)); // bounded by kParkHoldMs — the span holds + + host.clock += kParkHoldMs; + host.rolling = true; + const ReclaimRunResult gated = q.run(host); + CHECK(gated.writes == 0); + CHECK(!gated.settled); + CHECK(gated.transportDeferred); + CHECK(spanEndsAfter(gated)); // unbounded — the switch's point closes without it + + host.rolling = false; + const ReclaimRunResult done = q.run(host); + CHECK(done.writes == 1); + CHECK(done.settled); + CHECK(!done.transportDeferred); + CHECK(spanEndsAfter(done)); +} + +static void testAnAbortEndsTheSpanSoNoLaterSwitchNestsUnderIt() { + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + q.run(host); + host.clock += kParkHoldMs; + + host.reentrant = &q; + host.invalidateAfterWrite = 1; + const ReclaimRunResult aborted = q.run(host); + CHECK(aborted.aborted); + CHECK(!aborted.settled); + CHECK(spanEndsAfter(aborted)); // the model moved; this span's work is superseded +} + +static void testOneSwitchAndItsMultiTickParkingAreOneSpan() { + // Three tracks at a cost that admits one per tick, so the parking really does span + // several timer callbacks — the case a per-write block turned into one undo point per + // FX toggle. The span must open exactly once and close exactly once, on the tick that + // settles, under the SWITCH's label rather than the reclaimer's. + ViewModeModel vm = parkedDesignLeaves({{"{T1}", {"{FX-A}"}}, + {"{T2}", {"{FX-B}"}}, + {"{T3}", {"{FX-C}"}}}); + FakeHost host; + host.chains["{T1}"] = onlineChain({"{FX-A}"}); + host.chains["{T2}"] = onlineChain({"{FX-B}"}); + host.chains["{T3}"] = onlineChain({"{FX-C}"}); + host.stepCostMs = 50.0; // dearer than kOfflineBudgetMs ⇒ one track per tick + + // What applyMode leaves behind on a real switch: an open span, its label set, and no + // close — the reclaimer's to finish. + UndoSpan span; + CHECK(span.begin(kProjA, "", 0)); + span.relabel("ReaSampler: activate Design view", 9); + host.span = &span; + + int closes = 0; + FxReclaimQueue q = queueOf(vm, {"{T1}", "{T2}", "{T3}"}); + auto tick = [&]() { + const ReclaimRunResult r = q.run(host); + if (spanEndsAfter(r) && span.end()) ++closes; + return r; + }; + + CHECK(!tick().settled); // hold starts + host.clock += kParkHoldMs; + CHECK(!tick().settled); // {T1} + CHECK(!tick().settled); // {T2} + CHECK(tick().settled); // {T3}, and the queue is exhausted in the same pass + + CHECK(host.writeLog.size() == 3); + CHECK(host.spanOpens == 0); // the switch's span was already open — no second begin + CHECK(closes == 1); // ...and exactly one close, on the settling tick + CHECK(!span.open()); + + // Extra ticks after the settle must not produce a second close. + tick(); + CHECK(closes == 1); +} + +static void testADrainNoSwitchOpenedMintsOnePointOfItsOwn() { + // The non-switch origins — a membership edit's reapply, the load tick, a drain + // resuming after the transport stopped. Nobody opened a span, so the drain opens its + // own: one point for the whole convergence, never one per FX toggle. + ViewModeModel vm = parkedDesignLeaves({{"{T1}", {"{FX-A}", "{FX-B}"}}, + {"{T2}", {"{FX-C}"}}}); + FakeHost host; + host.chains["{T1}"] = onlineChain({"{FX-A}", "{FX-B}"}); + host.chains["{T2}"] = onlineChain({"{FX-C}"}); + host.stepCostMs = 50.0; + + UndoSpan span; + host.span = &span; + + int closes = 0; + FxReclaimQueue q = queueOf(vm, {"{T1}", "{T2}"}); + auto tick = [&]() { + const ReclaimRunResult r = q.run(host); + if (spanEndsAfter(r) && span.end()) ++closes; + return r; + }; + + CHECK(!tick().settled); // hold starts; nothing written, so nothing opened yet + CHECK(!span.open()); + CHECK(host.spanOpens == 0); + + host.clock += kParkHoldMs; + CHECK(!tick().settled); // {T1}'s chain: the first write opens the span + CHECK(host.spanOpens == 1); + CHECK(span.open()); + CHECK(span.label() == kReclaimUndoLabel); + CHECK(span.project() == kProjA); + + CHECK(tick().settled); // {T2}, still under the SAME span + CHECK(host.spanOpens == 1); + CHECK(host.writeLog.size() == 3); + CHECK(closes == 1); + CHECK(!span.open()); +} + +// -- 11. the state-load terminator ------------------------------------------------------ + +static void testAStateLoadStopsTheDrainWhereItStandsRatherThanFinishingTheTrack() { + // The contrast with testAReentrantInvalidationReachesNothingTheDrainIsUsing above, + // which finishes its track's remaining three writes: an ordinary invalidation only + // says the model moved, while a state load says the PROJECT is being replaced under + // us and the block covering these writes has already closed. So a step is atomic only + // up to the abandon — the half-parked chain that leaves is the accepted cost. + ViewModeModel vm = parkedDesignLeaves({{"{T1}", {"{FX-A}", "{FX-B}"}}, + {"{T2}", {"{FX-C}"}}}); + FakeHost host; + host.chains["{T1}"] = onlineChain({"{FX-A}", "{FX-B}"}); + host.chains["{T2}"] = onlineChain({"{FX-C}"}); + host.stepCostMs = 1.0; // cheap enough that only the abandon can stop the tick + + UndoSpan span; // what a real switch left open + CHECK(span.begin(kProjA, "ReaSampler: activate Design view", 9)); + host.span = &span; + + FxReclaimQueue q = queueOf(vm, {"{T1}", "{T2}"}); + q.run(host); // observes both parked at t=0 + host.clock += kParkHoldMs; + + host.reentrant = &q; + host.abandonAfterWrite = 1; + const ReclaimRunResult r = q.run(host); + + CHECK(host.writeLog == std::vector{"{T1}:0=off"}); + CHECK(host.offlineAt("{T1}", 0)); + CHECK(!host.offlineAt("{T1}", 1)); // the same track, mid-chain — the partial park + CHECK(!host.offlineAt("{T2}", 0)); // and every track after it, untouched + + CHECK(host.spanClosesMidDrain == 1); + CHECK(!span.open()); + CHECK(r.aborted); + CHECK(!r.settled); + CHECK(spanEndsAfter(r)); // the tick's own close still fires... + CHECK(!span.end()); // ...and finds nothing: the terminator got there first + + // The work is owed again, and the loop is ASLEEP until something re-reads the model — + // the reapply a reload drives is what raises the wakeup, and a load that drives none + // must raise nothing (testARevertBetweenTicksLeavesTheLoopAsleepOnAStaleModel). + CHECK(q.stale()); + CHECK(!q.pending()); + + // ...and the stop belonged to that tick alone. The reloaded project's own drain writes — + // but not before its holds are earned again: the abandon dropped them (it reached the + // clock through the drain's own local, see FxReclaimQueue::run), so the re-park starts + // its 3s from the first post-load tick rather than inheriting the pre-load observation. + host.abandonAfterWrite = -1; + q.adopt(derive(vm, {"{T1}", "{T2}"})); + CHECK(q.run(host).writes == 0); // first post-load tick: the holds start here + CHECK(!host.offlineAt("{T1}", 1)); + host.clock += kParkHoldMs; + CHECK(q.run(host).writes == 2); // {T1}'s remaining slot, and {T2}'s whole chain + CHECK(host.offlineAt("{T1}", 1)); + CHECK(host.offlineAt("{T2}", 0)); +} + +static void testTheWholePostLoadReconvergenceIsOneSpanHoweverFarTheParkingGot() { + // The acceptance property behind a Ctrl-Z landing anywhere in the drain: whatever + // fraction of the parking had completed, everything the reloaded project owes rides + // ONE span and costs ONE point. Two things carry it — the abandon dropping the holds, + // so nothing writes on the tick that re-derives (the load's own wake), and the lazy + // open being the drain's, so the many ticks that follow share one block. + ViewModeModel vm = parkedDesignLeaves({{"{T1}", {"{FX-A}", "{FX-B}"}}, + {"{T2}", {"{FX-C}"}}, + {"{T3}", {"{FX-D}"}}}); + FakeHost host; + host.chains["{T1}"] = onlineChain({"{FX-A}", "{FX-B}"}); + host.chains["{T2}"] = onlineChain({"{FX-C}"}); + host.chains["{T3}"] = onlineChain({"{FX-D}"}); + host.stepCostMs = 40.0; // over the offline budget: one track per tick, many ticks + + UndoSpan span; // the switch's, still open when the undo arrives + CHECK(span.begin(kProjA, "ReaSampler: activate Design view", 9)); + host.span = &span; + + FxReclaimQueue q = queueOf(vm, {"{T1}", "{T2}", "{T3}"}); + q.run(host); + host.clock += kParkHoldMs; + host.reentrant = &q; + host.abandonAfterWrite = 1; // Ctrl-Z one write in — the parking is part-done + q.run(host); + CHECK(!span.open()); // the terminator closed the switch's point over pre-load state + host.reentrant = nullptr; + host.abandonAfterWrite = -1; + + // The reloaded model's reapply raises the wakeup and the queue re-derives. + q.invalidate(); + q.adopt(derive(vm, {"{T1}", "{T2}", "{T3}"})); + + int closes = 0; + auto tick = [&]() { + const ReclaimRunResult r = q.run(host); + if (spanEndsAfter(r) && span.end()) ++closes; + return r; + }; + + CHECK(!tick().settled); // the holds start here, so this tick writes nothing at all + CHECK(host.spanOpens == 0); + CHECK(!span.open()); + CHECK(closes == 0); + + host.clock += kParkHoldMs; + int ticks = 0; + while (!tick().settled && ++ticks < 20) {} + CHECK(ticks >= 2); // it really did take several ticks to converge... + CHECK(host.spanOpens == 1); // ...under ONE span, opened by the drain's first write + CHECK(closes == 1); // ...closed exactly once, on the tick that settled + CHECK(!span.open()); + CHECK(host.offlineAt("{T1}", 1) && host.offlineAt("{T2}", 0) && host.offlineAt("{T3}", 0)); +} + +static void testARevertBetweenTicksLeavesTheLoopAsleepOnAStaleModel() { + // File > Revert is the load that re-reads NOTHING: it keeps the same ReaProject, so the + // session's identity poll answers NoOp, no reload follows and therefore no reapply + // does either. The terminator is the only thing that runs, so whether it raises the + // wakeup decides the whole outcome. Online work is the sharp case — undebounced, so ONE + // woken tick both writes FX and spends the record's FX half, against the very model the + // user reverted away from. The contrast with the mid-drain terminator test above: there + // the load arrives through a write's own message pump, here between two ticks entirely. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}"}); + vm.setActiveMode(kDesignModeId); + vm.releaseSnapshotFlags("{T}"); + + FakeHost host; + host.model = &vm; + host.chains["{T}"] = offlineChain({"{FX-A}", "{FX-B}"}); + ReleaseFlush flush; + host.flush = &flush; + + FxReclaimQueue q; + CHECK(q.beginTick(kProjA)); // where an earlier tick left it: pinned, nothing owed + q.invalidate(); // applyMode, leaving the switch that brought {T} back + CHECK(q.pending()); + + q.abandon(); + CHECK(!q.pending()); + CHECK(q.stale()); + CHECK(q.items().empty()); + CHECK(q.project() == kProjA); // the pin stays: it is what guards the debounce clock + + // Timer ticks keep arriving. The queue is empty AND stale, so it is the wakeup alone + // that keeps the drain out — timerTick re-derives from the stale model exactly as the + // shell would, which is what a raise here would have handed the drain. + for (int i = 0; i < 3; ++i) timerTick(q, vm, {"{T}"}, host); + CHECK(host.writeLog.empty()); + CHECK(host.released.empty()); + CHECK(!flush.pendingFor(kProjA)); + // ...and the chain is untouched — still parked offline, as the revert left it. + CHECK(host.offlineAt("{T}", 0) && host.offlineAt("{T}", 1)); + + // The contrast, so the assertions above are about the wakeup and not about this queue + // being unable to converge: a load that DOES re-read the model reaches the reclaimer + // through its reapply's invalidate, and the very same tick over the very same model + // both writes and spends. + q.invalidate(); + timerTick(q, vm, {"{T}"}, host); + CHECK(host.writeLog.size() == 2); + CHECK(!host.offlineAt("{T}", 0) && !host.offlineAt("{T}", 1)); + CHECK(host.released.count("{T}") == 1); + CHECK(flush.pendingFor(kProjA)); +} + +// -- 12. the direction the drain is moving ---------------------------------------------- + +static void testTheDirectionReadsOnlineWhileTheParksOwnOfflineWorkWaitsOutItsHold() { + // The panel bug this surface exists for: `pending` is true from the instant of the + // switch, but for the whole kParkHoldMs the only thing eligible is the RETURNING + // chains' online work — an indicator driven off the queue's mere contents would name + // parking while the drain is doing the opposite. + ViewModeModel vm; + vm.membership().tag("{Z-back}", kArrangeModeId); // active; owed an online + vm.membership().tag("{A-park}", kDesignModeId); // parked; owed an offline + vm.storeSnapshot("{Z-back}", recordWithFx({{"{FX-1}", 0}})); + vm.storeSnapshot("{A-park}", recordWithFx({{"{FX-2}", 0}})); + + FakeHost host; + host.chains["{Z-back}"] = offlineChain({"{FX-1}"}); + host.chains["{A-park}"] = onlineChain({"{FX-2}"}); + host.stepCostMs = 1.0; + + FxReclaimQueue q = queueOf(vm, {"{Z-back}", "{A-park}"}); + CHECK(q.activity(host.clock, host.rolling) == FxReclaimActivity::Online); + + CHECK(q.run(host).writes == 1); // the online item only — the offline is still held + CHECK(q.activity(host.clock, host.rolling) == FxReclaimActivity::Idle); + + host.clock += kParkHoldMs; + CHECK(q.activity(host.clock, host.rolling) == FxReclaimActivity::Offline); + CHECK(q.run(host).settled); + CHECK(q.activity(host.clock, host.rolling) == FxReclaimActivity::Idle); +} + +static void testAnOfflineOnlyQueueReadsIdleUntilItsHoldMatures() { + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + // Idle, not Offline: nothing may move, and the wait is the reason the span is allowed + // to stay open across it. Reporting Offline here is what would light the indicator for + // 3s before a single plugin moves. + CHECK(q.activity(host.clock, host.rolling) == FxReclaimActivity::Idle); + CHECK(!q.run(host).settled); // the hold's origin is set by this tick's observe + CHECK(host.writeLog.empty()); + CHECK(q.activity(host.clock, host.rolling) == FxReclaimActivity::Idle); + + host.clock += kParkHoldMs; + CHECK(q.activity(host.clock, host.rolling) == FxReclaimActivity::Offline); + CHECK(q.run(host).writes == 1); +} + +static void testARollingTransportHidesOfflineWorkFromTheDirectionButNeverOnline() { + ViewModeModel vm; + vm.membership().tag("{Z-back}", kArrangeModeId); + vm.membership().tag("{A-park}", kDesignModeId); + vm.storeSnapshot("{Z-back}", recordWithFx({{"{FX-1}", 0}})); + vm.storeSnapshot("{A-park}", recordWithFx({{"{FX-2}", 0}})); + + FakeHost host; + host.chains["{Z-back}"] = offlineChain({"{FX-1}"}); + host.chains["{A-park}"] = onlineChain({"{FX-2}"}); + host.stepCostMs = 1.0; + host.rolling = true; + + FxReclaimQueue q = queueOf(vm, {"{Z-back}", "{A-park}"}); + // Online is not transport-gated, so it both drains and reads through a rolling + // transport — the second half of the panel bug: this work had no indicator at all. + CHECK(q.activity(host.clock, host.rolling) == FxReclaimActivity::Online); + CHECK(q.run(host).writes == 1); + host.clock += kParkHoldMs; + + // The offline half is eligible now and still may not move, so it reads Idle rather + // than naming a direction nothing is travelling in. + CHECK(q.activity(host.clock, host.rolling) == FxReclaimActivity::Idle); + CHECK(q.run(host).transportDeferred); + CHECK(host.writeLog.size() == 1); + + host.rolling = false; + CHECK(q.activity(host.clock, host.rolling) == FxReclaimActivity::Offline); + CHECK(q.run(host).writes == 1); +} + +static void testTheDirectionHoldsThroughTheDrainsOwnMessagePump() { + // A write pumps, and a repaint landing in there polls this. The queue object holds + // NOTHING for the drain's whole duration (itemsSeenMidDrain == 0, asserted above), so + // without the latch the indicator would read Idle at exactly the moment the plugin + // loads the user is waiting through are happening. + ViewModeModel vm = parkedDesignLeaves({{"{T1}", {"{FX-A}", "{FX-B}"}}}); + FakeHost host; + host.chains["{T1}"] = onlineChain({"{FX-A}", "{FX-B}"}); + host.stepCostMs = 1.0; + + FxReclaimQueue q = queueOf(vm, {"{T1}"}); + q.run(host); + host.clock += kParkHoldMs; + + host.reentrant = &q; + host.probeActivityAfterWrite = 1; + CHECK(q.run(host).settled); + + CHECK(host.activityMidDrain == FxReclaimActivity::Offline); + CHECK(q.activity(host.clock, host.rolling) == FxReclaimActivity::Idle); // latch released +} + +// -- 13. the sleep invariant ------------------------------------------------------------ + +// !pending() => !span.open(), at EVERY terminator. A span left open with the wakeup down +// has nothing left that could ever close it: the timer gates every tick on pending(), and +// the block then swallows the user's edits until a save, a state load, the next switch or +// unload. The check below is written as that implication and applied to each pure move +// that can put the loop to sleep, rather than falling out of one scenario. +static bool asleepWithTheSpanOpen(const FxReclaimQueue& q, const UndoSpan& span) { + return !q.pending() && span.open(); +} + +static void testNoTerminatorLeavesTheSpanOpenWithTheLoopAsleep() { + // FIRST, the predicate's own teeth: the violation is expressible, so a CHECK that it + // does not occur is not vacuously true. + { + FxReclaimQueue q; + UndoSpan span; + span.begin(kProjA, "ReaSampler: activate Design view", 9); + q.goIdle(); // the sleep WITHOUT the close — exactly the leak this test forbids + CHECK(asleepWithTheSpanOpen(q, span)); + } + + // 1. Every verdict a tick can come back with, swept whole. The shell closes on + // spanEndsAfter and sleeps on settled, in that order (fxReclaimTick); the property + // is that the second can never fire where the first did not. + for (int bits = 0; bits < 8; ++bits) { + ReclaimRunResult run; + run.settled = (bits & 1) != 0; + run.aborted = (bits & 2) != 0; + run.transportDeferred = (bits & 4) != 0; + + FxReclaimQueue q; + UndoSpan span; + q.invalidate(); + CHECK(q.beginTick(kProjA)); + span.begin(kProjA, "ReaSampler: activate Design view", 9); + + if (spanEndsAfter(run)) span.end(); + if (!run.aborted && run.settled) q.goIdle(); + + CHECK(!asleepWithTheSpanOpen(q, span)); + } + + // 2. The state-load terminator. `abandon` is the one move that clears the wakeup with + // work still owed, which is what makes the close-FIRST ordering load-bearing rather + // than tidy. Proved both ways, not asserted by construction: the WRONG order below + // (abandon, then close) is caught mid-sequence by the same predicate the RIGHT order + // is checked against — swapping the two lines here would make the first CHECK below + // fail, which is the teeth `fxReclaimAbandonForStateLoad` closing FIRST relies on. + { + FxReclaimQueue q; + UndoSpan span; + q.invalidate(); + span.begin(kProjA, "ReaSampler: activate Design view", 9); + q.abandon(); // WRONG order: wakeup cleared before the span closes + CHECK(asleepWithTheSpanOpen(q, span)); // the exact leak the real order avoids + span.end(); + } + { + FxReclaimQueue q; + UndoSpan span; + q.invalidate(); + span.begin(kProjA, "ReaSampler: activate Design view", 9); + span.end(); // fxReclaimAbandonForStateLoad closes before it abandons + q.abandon(); + CHECK(!q.pending()); + CHECK(!asleepWithTheSpanOpen(q, span)); + } + + // 3. The project moving out from under the span. No active project sleeps the loop + // (beginTick's nullptr arm), a tab switch does not — but both close, because the + // span is keyed on its OWN project and neither one is it. kProjA is the control: a + // tick landing back on the SAME project must NOT close the span, or a live switch's + // block would drop the flags-and-items mask it opened with mid-drain. Sweeping all + // three, with a branch-dependent CHECK, is what gives the guard real teeth — the + // original two-value sweep always took the close branch, so the no-close branch was + // never exercised. + for (ReaProject* arrived : {kProjA, static_cast(nullptr), kProjB}) { + FxReclaimQueue q; + UndoSpan span; + q.invalidate(); + q.beginTick(kProjA); + span.begin(kProjA, "ReaSampler: activate Design view", 9); + + if (span.project() != arrived) span.end(); + q.beginTick(arrived); + + if (arrived == kProjA) CHECK(span.open()); // same project: nothing to close + else CHECK(!span.open()); + CHECK(!asleepWithTheSpanOpen(q, span)); + } + + // 4. The terminators with no queue move at all — a project save, an extension unload, + // the next real switch superseding. Each is the idempotent close alone, with no + // guard and nothing else to sweep, so it reduces to end()'s own idempotency + // (testTheSpanOpensOnceAndClosesOnceHoweverManyTimesItIsAsked) rather than adding a + // separate claim here. +} + int main() { testFastFlipMovesZeroPluginsButAHeldParkStillReclaims(); @@ -1048,6 +1630,23 @@ int main() { testAHoldDropsWhenItsTrackStopsOwingOfflineWorkWhileRolling(); testAGatedTickStillGoesIdleWithNothingLeftToConverge(); + testTheSpanOpensOnceAndClosesOnceHoweverManyTimesItIsAsked(); + testTheSpanOutlastsADebounceWaitButNeverATransportDeferral(); + testAnAbortEndsTheSpanSoNoLaterSwitchNestsUnderIt(); + testOneSwitchAndItsMultiTickParkingAreOneSpan(); + testADrainNoSwitchOpenedMintsOnePointOfItsOwn(); + + testAStateLoadStopsTheDrainWhereItStandsRatherThanFinishingTheTrack(); + testTheWholePostLoadReconvergenceIsOneSpanHoweverFarTheParkingGot(); + testARevertBetweenTicksLeavesTheLoopAsleepOnAStaleModel(); + + testTheDirectionReadsOnlineWhileTheParksOwnOfflineWorkWaitsOutItsHold(); + testAnOfflineOnlyQueueReadsIdleUntilItsHoldMatures(); + testARollingTransportHidesOfflineWorkFromTheDirectionButNeverOnline(); + testTheDirectionHoldsThroughTheDrainsOwnMessagePump(); + + testNoTerminatorLeavesTheSpanOpenWithTheLoopAsleep(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; } diff --git a/tests/test_undo_intercept.cpp b/tests/test_undo_intercept.cpp new file mode 100644 index 0000000..add5cf5 --- /dev/null +++ b/tests/test_undo_intercept.cpp @@ -0,0 +1,65 @@ +// Standalone tests for which dispatched action ids end the FX reclaim span — no REAPER, +// no test framework. The predicate is the whole of the interception's decision; the close +// it drives needs a DAW. What is assertable here: the id pair is exact, and it is scoped +// to the main sections, because command ids are per-section and the same integer under +// another section is an unrelated action. + +#include "../src/shell/undo/undo_intercept.h" + +#include + +using namespace reasampler; + +static int g_fail = 0; +#define CHECK(cond) do { if(!(cond)) { \ + std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) + +namespace { + +constexpr int kMainSection = 0; +constexpr int kMainAltSection = 100; +constexpr int kMidiEditor = 32060; +constexpr int kMediaExplorer = 32063; + +void undoAndRedoInTheMainSectionAreIntercepted() { + CHECK(isMainUndoRedoCommand(kEditUndoCommand, kMainSection)); + CHECK(isMainUndoRedoCommand(kEditRedoCommand, kMainSection)); + // Main alt shares the main id space (reaper_plugin.h: "0/100=main/main alt"), so a + // Ctrl-Z fired while the alt section is active is the same action. + CHECK(isMainUndoRedoCommand(kEditUndoCommand, kMainAltSection)); + CHECK(isMainUndoRedoCommand(kEditRedoCommand, kMainAltSection)); +} + +void everyOtherMainSectionActionIsLeftAlone() { + // Every action in the tree passes through this predicate; only these two ids may end + // the span, or an ordinary edit made mid-drain would split the switch's undo point. + CHECK(!isMainUndoRedoCommand(kEditUndoCommand - 1, kMainSection)); + CHECK(!isMainUndoRedoCommand(kEditRedoCommand + 1, kMainSection)); + CHECK(!isMainUndoRedoCommand(40012, kMainSection)); + CHECK(!isMainUndoRedoCommand(0, kMainSection)); + CHECK(!isMainUndoRedoCommand(-1, kMainSection)); +} + +void theSameIdsUnderAnotherSectionAreNotOurUndo() { + CHECK(!isMainUndoRedoCommand(kEditUndoCommand, kMidiEditor)); + CHECK(!isMainUndoRedoCommand(kEditRedoCommand, kMidiEditor)); + CHECK(!isMainUndoRedoCommand(kEditUndoCommand, kMediaExplorer)); + // A hook that could not name its section (null KbdSectionInfo) is not assumed to be + // main — hookcommand covers the main section unconditionally anyway. + CHECK(!isMainUndoRedoCommand(kEditUndoCommand, -1)); +} + +} // namespace + +int main() { + undoAndRedoInTheMainSectionAreIntercepted(); + everyOtherMainSectionActionIsLeftAlone(); + theSameIdsUnderAnotherSectionAreNotOurUndo(); + + if (g_fail) { + std::printf("%d check(s) failed\n", g_fail); + return 1; + } + std::printf("undo_intercept: all checks passed\n"); + return 0; +} 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();