From b7b93a7ac300bf5a8984de5386965af895393aea Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Fri, 7 Aug 2026 16:38:11 -0400 Subject: [PATCH] One undo-block opener for the whole tree: fourteen sites route through it, and the reclaim span can no longer swallow a user's act The drain refuses a tick inside a foreign block; applyMode wakes the reclaimer before it opens the span. --- CLAUDE.md | 5 +- src/app/CMakeLists.txt | 1 + src/shell/actions/CLAUDE.md | 12 +- src/shell/actions/arrange_drop_win.cpp | 10 +- src/shell/actions/design_view_actions.cpp | 66 +++------ src/shell/actions/ingest.cpp | 59 ++++---- src/shell/actions/instrument_drop_win.cpp | 7 +- src/shell/bank_ops/CLAUDE.md | 13 +- src/shell/bank_ops/bank_ops.cpp | 13 +- src/shell/capture/CLAUDE.md | 4 + src/shell/capture/bake_land.cpp | 30 +--- src/shell/capture/capture_batch.cpp | 41 +++--- src/shell/capture/capture_orchestrator.cpp | 29 ++-- src/shell/capture/insert.cpp | 17 +-- src/shell/capture/insert.h | 2 +- src/shell/capture/render_in_place.cpp | 157 ++++++++++----------- src/shell/undo/CLAUDE.md | 53 +++++++ src/shell/undo/undo_block.cpp | 51 +++++++ src/shell/undo/undo_block.h | 42 ++++++ src/shell/view/CLAUDE.md | 72 ++++++---- src/shell/view/fx_reclaim.cpp | 14 +- src/shell/view/fx_reclaim.h | 12 +- src/shell/view/view.cpp | 7 + src/shell/view/view_lanes.cpp | 68 +++++---- tests/test_fx_reclaim.cpp | 89 ++++++++++++ 25 files changed, 561 insertions(+), 313 deletions(-) create mode 100644 src/shell/undo/CLAUDE.md create mode 100644 src/shell/undo/undo_block.cpp create mode 100644 src/shell/undo/undo_block.h diff --git a/CLAUDE.md b/CLAUDE.md index 8b4f9bc..5af3c14 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 diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt index 217f589..b340903 100644 --- a/src/app/CMakeLists.txt +++ b/src/app/CMakeLists.txt @@ -38,6 +38,7 @@ 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/view/view.cpp ${REASAMPLER_SRC_DIR}/shell/view/fx_reclaim.cpp ${REASAMPLER_SRC_DIR}/shell/view/view_lanes.cpp diff --git a/src/shell/actions/CLAUDE.md b/src/shell/actions/CLAUDE.md index 56fc076..01bc6be 100644 --- a/src/shell/actions/CLAUDE.md +++ b/src/shell/actions/CLAUDE.md @@ -29,9 +29,13 @@ 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. - **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 +43,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 2147866..24bb279 100644 --- a/src/shell/actions/design_view_actions.cpp +++ b/src/shell/actions/design_view_actions.cpp @@ -25,7 +25,7 @@ #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/view/fx_reclaim.h" // fxReclaimEndUndoSpan — no block opens over the span +#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 @@ -41,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 { @@ -217,37 +215,13 @@ void doActivateMode(const std::string& modeId) { // of its own — nothing it writes mints a point unaided — so a user-fired tag stays one // Ctrl-Z only because the ACTION spans it, and the label names the act the user // actually fired rather than the reapply it happened to trigger. Every one of them -// closes the block before persistViewState, which can pop a modal Save-As. Stack RAII -// so a throw between the two calls cannot leave the block open in the user's project. +// closes the block before persistViewState, which can pop a modal Save-As. // -// ENDING THE RECLAIM SPAN IS PART OF OPENING, and every action here inherits it from -// this one constructor. A mode switch fired seconds ago has left the span open; a block -// opened over it makes each close pair with the other's open, collapsing the switch -// into this tag — mechanism and full rule in src/shell/view/CLAUDE.md. -// -// `markWrote` decides whether this block mints at all: re-tagging tracks that already -// sit in that mode writes nothing to the project, and the ("", 0) discard form is real -// from a hand-fired action (DAW-measured, same doc). The membership move itself is -// never in the point either way — persistViewState runs after the close. -class ActionUndoBlock { -public: - ActionUndoBlock(std::string label, int mask) : label_(std::move(label)), mask_(mask) { - fxReclaimEndUndoSpan(); - Undo_BeginBlock2(nullptr); - } - ~ActionUndoBlock() { - if (wrote_) Undo_EndBlock2(nullptr, label_.c_str(), mask_); - else Undo_EndBlock2(nullptr, "", 0); - } - void markWrote() { wrote_ = true; } - ActionUndoBlock(const ActionUndoBlock&) = delete; - ActionUndoBlock& operator=(const ActionUndoBlock&) = delete; - -private: - std::string label_; - int mask_; - bool wrote_ = false; -}; +// The point is recorded only where the reapply actually wrote: re-tagging tracks that +// already sit in that mode writes nothing to the project, and the ("", 0) discard form +// UndoBlock closes on by default is real from a hand-fired action (DAW-measured, +// src/shell/view/CLAUDE.md). The membership move itself is never in the point either +// way — persistViewState runs after the close. // tag() replaces any prior single-mode membership (a leaf lives in one mode; the // cross-mode case is show-both). @@ -256,9 +230,10 @@ void doTag(const std::string& modeId) { if (guids.empty()) return; // nothing selected — no act, no undo point (cf. doMoveItems) { - ActionUndoBlock undo("ReaSampler: tag selected tracks -> " + modeId, kViewUndoMask); + UndoBlock undo(nullptr); for (const std::string& g : guids) g_session->view().membership().tag(g, modeId); - if (reapplyActiveMode()) undo.markWrote(); + if (reapplyActiveMode()) + undo.recordPoint("ReaSampler: tag selected tracks -> " + modeId, kViewUndoMask); } persistViewState(PersistScope::Full); } @@ -270,9 +245,10 @@ void doUntag() { if (guids.empty()) return; { - ActionUndoBlock undo("ReaSampler: untag selected tracks", kViewUndoMask); + UndoBlock undo(nullptr); for (const std::string& g : guids) g_session->view().membership().untag(g); - if (reapplyActiveMode()) undo.markWrote(); + if (reapplyActiveMode()) + undo.recordPoint("ReaSampler: untag selected tracks", kViewUndoMask); } persistViewState(PersistScope::Full); } @@ -284,10 +260,11 @@ void doShowBoth() { if (guids.empty()) return; { - ActionUndoBlock undo("ReaSampler: toggle show-both on selected tracks", kViewUndoMask); + UndoBlock undo(nullptr); MembershipIndex& m = g_session->view().membership(); for (const std::string& g : guids) m.setShowBoth(g, !m.isShowBoth(g)); - if (reapplyActiveMode()) undo.markWrote(); + if (reapplyActiveMode()) + undo.recordPoint("ReaSampler: toggle show-both on selected tracks", kViewUndoMask); } persistViewState(PersistScope::Full); } @@ -307,10 +284,7 @@ void doMoveItems(const std::string& targetMode) { MembershipIndex& membership = g_session->view().membership(); { - ActionUndoBlock undo(targetMode.empty() - ? std::string("ReaSampler: untag selected items") - : "ReaSampler: move selected items -> " + targetMode, - -1); + UndoBlock undo(nullptr); for (const ItemRetagOp& op : ops) { if (op.untag) membership.untag(op.guid); else membership.tag(op.guid, op.modeId); @@ -321,7 +295,11 @@ void doMoveItems(const std::string& targetMode) { // no flag leaves nothing for a point to hold. const bool minted = mintManagedLanes(g_session->view(), nullptr); const bool reapplied = reapplyActiveMode(); - if (minted || reapplied) undo.markWrote(); + if (minted || reapplied) + undo.recordPoint(targetMode.empty() + ? std::string("ReaSampler: untag selected items") + : "ReaSampler: move selected items -> " + targetMode, + -1); } persistViewState(PersistScope::Full); 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..219229f 100644 --- a/src/shell/bank_ops/CLAUDE.md +++ b/src/shell/bank_ops/CLAUDE.md @@ -26,13 +26,18 @@ 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`. ## 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..5b4989b 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 { @@ -50,13 +49,11 @@ std::string mintBankId() { // 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 + // No ext-state write -> record no point; the block's default discard form is what + // keeps an unsaved project from collecting no-effect undo entries. + 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 cab8935..713dcfb 100644 --- a/src/shell/capture/render_in_place.cpp +++ b/src/shell/capture/render_in_place.cpp @@ -21,7 +21,7 @@ #include "shell/capture/track_guid.h" // guidString #include "shell/panel/panel_input.h" // bankPanelTailSetting #include "shell/persist/session.h" -#include "shell/view/fx_reclaim.h" // fxReclaimEndUndoSpan +#include "shell/undo/undo_block.h" // the ONE undo-block opener #include "shell/view/view.h" // applyMode #include "shell/view/view_lanes.h" // mintManagedLanes @@ -43,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 { @@ -134,87 +132,88 @@ void RunRenderTrackInPlace(ReaSamplerSession& session) { // visible — never to choose a tag. Both tags below are absolute. const std::string activeMode = session.view().activeModeId(); - // A mode switch fired seconds ago has left the FX reclaimer's undo span open, and - // no block may open over it — mechanism in src/shell/view/CLAUDE.md. Ending it here - // mints the switch's point, whole, before this block exists; idempotent otherwise. - fxReclaimEndUndoSpan(); + // Both read after the block closes, so both outlive the scope below. + bool placed = false; + std::string freshName; - Undo_BeginBlock2(nullptr); + // Scoped so the persist below lands OUTSIDE the block — see the note at the close. + { + UndoBlock undo(nullptr); - // 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; - } - - // 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..427c8c5 --- /dev/null +++ b/src/shell/undo/CLAUDE.md @@ -0,0 +1,53 @@ +# src/shell/undo — the one REAPER undo-block opener + +## Scope + +One type, `UndoBlock`, and one query, `undoBlockOpen()`. 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 no block ever +nests over it. + +## Invariants + +- **`Undo_BeginBlock2` is not callable outside this directory, 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` with no header in its include graph pulling the SDK in + unguarded, so a raw `Undo_BeginBlock2` there is an undeclared identifier. Adding + the WANT define back to a third TU is the one way to defeat this — 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`. +- **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_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 + +- 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..bf3922d --- /dev/null +++ b/src/shell/undo/undo_block.cpp @@ -0,0 +1,51 @@ +// 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 +#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. + if (g_depth++ == 0) fxReclaimEndUndoSpan(); + Undo_BeginBlock2(proj); +} + +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. + 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..7e89723 --- /dev/null +++ b/src/shell/undo/undo_block.h @@ -0,0 +1,42 @@ +#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. +// +// Enforced by the SDK's own opt-in: only this TU and fx_reclaim.cpp (which owns the span +// itself) define REAPERAPI_WANT_Undo_BeginBlock2, so `Undo_BeginBlock2` does not even +// name a declared symbol anywhere else in the tree. + +#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, matching Undo_BeginBlock2's own contract + // (reaper_plugin_functions.h:7753). + 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/view/CLAUDE.md b/src/shell/view/CLAUDE.md index 2c5abdb..1840215 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -122,9 +122,16 @@ decide membership or mode rules. 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`). -- **NOTHING MAY OPEN AN UNDO BLOCK WHILE THE SPAN IS OPEN — every opener ends it - first, as its first act.** REAPER's blocks are read as a DEPTH COUNTER, not a stack - of identities (`[verify — DAW]`, and the same undocumented nested-block behaviour the + 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 @@ -134,18 +141,31 @@ decide membership or mode rules. 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. Ending it first mints the switch's point whole, at - depth 0, and leaves the opener's block the only one in play. The - openers, all of them: `design_view_actions`' `ActionUndoBlock` (whose constructor - ends the span, so all four membership actions inherit it), `view_lanes`' - `mintManagedLanes`, and `shell/capture`'s `render_in_place`. The mirror half is - `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. RESIDUAL, out of this rule's reach: an extension undo block - ELSEWHERE (a capture, a bank verb, an ingest) that pumps the message loop can re-enter - the timer, and a drain tick that settles in there closes the span at depth 2->1 with - the same result. Closing that needs every extension-owned `Undo_BeginBlock2` to route - through one opener that ends the span — not attempted here. + 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. + 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` @@ -234,13 +254,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 on a REAL SWITCH ends any prior span and opens the ONE undo span all of that rides (`fx_reclaim` is what closes it; a reapply opens none and closes none). Reports what it wrote (`ModeApplyResult`) so a caller holding its own block can discard an empty 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 what it wrote (`ModeApplyResult`) so a caller holding its own block can discard an empty 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_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). It opens no undo block: it runs in the wake of a Ctrl-Z, and its writes are flags only. - `view_tracks` — the scaffolding every write pass sets up: `TrackHandles`/`TrackByGuid`, `readFolderEntries`/`indexHandles`, and `UiRefreshHold` (the `PreventUIRefresh` RAII pair every write phase runs under). - `view_solo` — the `I_SOLO` read/write pair behind the per-mode solo surface, plus `clearTrackSolos`/`restoreTrackSolos`, the outgoing-clear and incoming-replay entry points `view` drives them through. Holds no policy: what to cache, clear, or replay is `core/view/solo_cache`. -- `fx_reclaim` — the FX convergence loop, off the switch's synchronous path (`TrackFX_SetOffline` was 1897ms of a measured 1942ms switch). `drainTick` moves one track's FX per step against `core/view/fx_state`'s derived work list (Online before Offline; an Offline item waits out `kParkHoldMs` so a fast A→B→A flip moves nothing), budgeted per tick (`kOnlineBudgetMs`/`kOfflineBudgetMs`) and gated off the transport for the Offline direction only. `FxReclaimQueue` owns the derived queue's lifetime across re-entrancy (a plugin unload is assumed to pump). `UndoSpan` owns the ONE undo block the whole drain writes under — opened by `applyMode` on a real switch, else lazily by the first `writeOffline`, and closed through the single idempotent `fxReclaimEndUndoSpan` that every terminating path routes through (`spanEndsAfter` decides which ticks terminate; `fxReclaimAbandonForStateLoad` is the state-load terminator, which also stops the drain). `ReleaseFlush`/`fxReclaimFlushRelease` make a completed record's spend durable on the project's next save, never on the drain's own initiative. `fxReclaimActivity` reports WHICH DIRECTION the drain is moving for an indicator to paint — Idle whenever nothing may move, so a debounced or transport-held offline item never names a direction nothing is travelling in; undirected `fxReclaimPending` stays the timer's one idle test and must not drive a UI (it is true throughout the debounce, when only the OPPOSITE direction is eligible). +- `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 @@ -338,15 +358,15 @@ applies the resulting lane state to live tracks. into sync. - **"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 `Undo_BeginBlock2` pair; the reapply opens no block 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 (each caller ends the span first), 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`, both - properly paired inside a single outer block, so the SDK's undocumented nested-block - coalescing is load-bearing only for that shape. + 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 abe8077..9b3d4cc 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" @@ -233,12 +234,21 @@ void invalidateFxReclaim() { bool fxReclaimDrainInProgress() { return g_draining; } void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { - // Neither guard may close the span: both name a caller that is still on the stack and - // still owns it — the outer drain, or the applyMode that opened it. Both re-tick. + // 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. + if (undoBlockOpen()) return; DrainLatch latch; // beginTick owns what a pin/project mismatch means. What is local to here: the model diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index 9a4ae38..bff05c7 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -658,7 +658,9 @@ 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); @@ -667,7 +669,8 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj); // 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) 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 @@ -677,8 +680,9 @@ 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; a project save; extension -// unload. A leaked open block would swallow everything the user does afterwards, so -// nothing may return past a terminating condition without calling this. +// 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 diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index b9ddb6f..abedf5d 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -128,6 +128,13 @@ ModeApplyResult applyModeImpl(ViewModeModel& model, const std::string& targetMod // 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); } { diff --git a/src/shell/view/view_lanes.cpp b/src/shell/view/view_lanes.cpp index 982acce..ae3dbda 100644 --- a/src/shell/view/view_lanes.cpp +++ b/src/shell/view/view_lanes.cpp @@ -16,15 +16,13 @@ #include "core/view/lane_keys.h" #include "core/view/view_tree.h" #include "shell/capture/item_read.h" -#include "shell/view/fx_reclaim.h" // fxReclaimEndUndoSpan — no block opens over the span +#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 @@ -277,21 +275,46 @@ bool mintManagedLanes(ViewModeModel& model, ReaProject* proj) { const TrackByGuid trackByGuid = indexHandles(handleByGuid); - // No block may open over a still-open reclaim span — mechanism in this directory's - // CLAUDE.md. Placed past the empty-plan return above deliberately: a detect tick - // that mints nothing must not shorten a span it never competes with. - fxReclaimEndUndoSpan(); + 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); - // `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); + 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 @@ -299,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/tests/test_fx_reclaim.cpp b/tests/test_fx_reclaim.cpp index dfb2529..9eb7273 100644 --- a/tests/test_fx_reclaim.cpp +++ b/tests/test_fx_reclaim.cpp @@ -1433,6 +1433,93 @@ static void testTheDirectionHoldsThroughTheDrainsOwnMessagePump() { 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 — reverse the two and the loop is asleep under an open block. + { + 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. + for (ReaProject* arrived : {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); + + 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, so the + // wakeup is whatever it already was; the implication has to hold either way. + for (bool woken : {false, true}) { + FxReclaimQueue q; + UndoSpan span; + if (woken) q.invalidate(); + span.begin(kProjA, "ReaSampler: activate Design view", 9); + span.end(); + CHECK(!asleepWithTheSpanOpen(q, span)); + } +} + int main() { testFastFlipMovesZeroPluginsButAHeldParkStillReclaims(); @@ -1484,6 +1571,8 @@ int main() { testARollingTransportHidesOfflineWorkFromTheDirectionButNeverOnline(); testTheDirectionHoldsThroughTheDrainsOwnMessagePump(); + testNoTerminatorLeavesTheSpanOpenWithTheLoopAsleep(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; }