From 6d372794f53cdfc1d83530d699667cc549889efd Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 26 Jul 2026 05:49:38 -0400 Subject: [PATCH 1/6] feat(actions): batch bank index verbs into single REAPER undo points (R-B) Wrap each bank verb's persist in Undo_BeginBlock2/EndBlock2 with UNDO_STATE_MISCCFG so one bank op is one Ctrl-Z; ext-state participates in undo per SDK. Rejected/no-op ops open no block. --- src/actions.cpp | 43 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/src/actions.cpp b/src/actions.cpp index 0754cd8..bf3bd07 100644 --- a/src/actions.cpp +++ b/src/actions.cpp @@ -455,6 +455,27 @@ gaccel_register_t g_accelBankBanksFull{}; // follows capture's quiet-persist idiom, a Design-View mutation follows the prompt idiom. void persistBook() { g_session->saveToActiveProject(); } +// Persists a completed bank index verb (create/rename/reorder/delete/evacuate/ +// move/copy) as a SINGLE batched REAPER undo point (R-B) — one bank op = one Ctrl-Z. +// +// WHY THIS WRAPS AND persistBook() DOES NOT: a bank verb mutates ONLY our project +// ext-state (SetProjExtState under "reasampler"), which REAPER's undo system captures +// iff UNDO_STATE_MISCCFG is set in the Undo_EndBlock2 flags — the SDK documents +// MISCCFG as covering "extensions!" project ext-state (reaper_plugin.h ~1544, ~1199). +// We pass exactly UNDO_STATE_MISCCFG (not -1 / UNDO_STATE_ALL as the item-move family +// does): a bank verb touches no tracks, FX, items, or envelopes, so snapshotting them +// would be both heavier and semantically wrong. persistBook() (= SetProjExtState) runs +// INSIDE the block so the post-mutation ext-state is the block's "after" image. +// +// NO-OP GUARDRAIL: callers invoke this ONLY after the model mutation succeeded — a +// rejected op (duplicate name, un-deletable pool, etc.) returns before reaching here, +// so no dangling/empty undo point is ever opened. +void persistBankOp(const char* label) { + Undo_BeginBlock2(nullptr); + persistBook(); + Undo_EndBlock2(nullptr, label, UNDO_STATE_MISCCFG); +} + // Prompts the user for a single line of text via REAPER's stock input dialog. // GetUserInputs(title, num_inputs=1, captions_csv, retvals_csv, sz) -> false on // cancel (SDK ~3808). `initial` pre-fills the field. Returns false (leaving `out` @@ -523,7 +544,7 @@ void doBankCreate() { .c_str()); return; } - persistBook(); + persistBankOp("ReaSampler: create bank"); ShowConsoleMsg(("ReaSampler: created bank \"" + name + "\".\n").c_str()); } @@ -549,7 +570,7 @@ void doBankRename() { "or another bank already uses that name).\n"); return; } - persistBook(); + persistBankOp("ReaSampler: rename bank"); ShowConsoleMsg(("ReaSampler: renamed \"" + which + "\" -> \"" + newName + "\".\n") .c_str()); } @@ -593,7 +614,7 @@ void doBankDelete() { ShowConsoleMsg("ReaSampler: cannot delete that bank (the pool is un-deletable).\n"); return; } - persistBook(); + persistBankOp("ReaSampler: delete bank"); ShowConsoleMsg(("ReaSampler: deleted bank \"" + which + "\".\n").c_str()); } @@ -615,7 +636,7 @@ void doBankEvacuate() { "destination, not a source).\n"); return; } - persistBook(); + persistBankOp("ReaSampler: evacuate bank"); ShowConsoleMsg(("ReaSampler: evacuated \"" + which + "\" to the pool.\n").c_str()); } @@ -630,7 +651,7 @@ void doBankActivateNext() { const std::string target = nextBankId(ids, g_session->book().activeBankId()); if (target.empty()) return; // degenerate (no banks) — cannot happen (pool seeded) if (!g_session->book().setActiveBank(target)) return; - persistBook(); + persistBankOp("ReaSampler: activate bank"); const Bank* b = g_session->book().bank(target); ShowConsoleMsg(("ReaSampler: active bank -> \"" + (b ? b->displayName : target) + "\".\n") @@ -641,7 +662,7 @@ void doBankActivateNext() { // direct-by-id form; a general activate-bank-by-name/menu is a B4 affordance. void doBankActivatePool() { if (!g_session->book().setActiveBank(kPoolBankId)) return; - persistBook(); + persistBankOp("ReaSampler: activate bank"); ShowConsoleMsg("ReaSampler: active bank -> \"Pool\".\n"); } @@ -692,7 +713,15 @@ void doBankTransferSelected(bool copy) { case TransferResult::RejectedSameBank: break; } } - persistBook(); + // No-op guardrail: if nothing actually changed the index (every selected sample was + // absent, or all were pre-checked rejects), don't open an undo point. `collapsed` + // counts a hash-collapse — that DID mutate the index (source entry removed on a + // move, or dest already held the hash), so it belongs inside the undo point. + if (ok > 0 || collapsed > 0) { + const std::string label = + std::string("ReaSampler: ") + verb + " sample(s)"; + persistBankOp(label.c_str()); + } std::string log = std::string("ReaSampler: ") + verb + " -> \"" + destName + "\": " + std::to_string(ok) + " " + verb + "d"; if (collapsed) log += ", " + std::to_string(collapsed) + " collapsed on hash"; From a07b28fd658fcdd08004b7f234c57096e30e170d Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 26 Jul 2026 06:59:26 -0400 Subject: [PATCH 2/6] feat(persist): reload book+view on undo/redo via projectconfig hook (R-B) BeginLoadProjectState(isUndo) requests a deferred reload the timer drains next tick, once REAPER has restored ext-state. Hook owns undo/redo; identity poll still owns open/tab-switch/save-as. Fixes copy-collapse over-count (verb-aware guard) and dangling undo point on unsaved-project bank ops. --- src/actions.cpp | 35 +++++++++++++++++++++-------- src/main.cpp | 57 +++++++++++++++++++++++++++++++++++++++++++++++ src/persist.cpp | 59 ++++++++++++++++++++++++++++++++++++++++++------- src/persist.h | 25 ++++++++++++++++++++- 4 files changed, 158 insertions(+), 18 deletions(-) diff --git a/src/actions.cpp b/src/actions.cpp index bf3bd07..f180011 100644 --- a/src/actions.cpp +++ b/src/actions.cpp @@ -453,7 +453,9 @@ gaccel_register_t g_accelBankBanksFull{}; // persists. This is an intentional divergence from persistViewState (above), which // DOES prompt Save-As on an unsaved project; do not "align" the two — a bank mutation // follows capture's quiet-persist idiom, a Design-View mutation follows the prompt idiom. -void persistBook() { g_session->saveToActiveProject(); } +// Returns whether a persist actually happened (false on an unsaved/no-active project), +// so persistBankOp can skip its undo block when nothing was written. +bool persistBook() { return g_session->saveToActiveProject(); } // Persists a completed bank index verb (create/rename/reorder/delete/evacuate/ // move/copy) as a SINGLE batched REAPER undo point (R-B) — one bank op = one Ctrl-Z. @@ -469,11 +471,22 @@ void persistBook() { g_session->saveToActiveProject(); } // // NO-OP GUARDRAIL: callers invoke this ONLY after the model mutation succeeded — a // rejected op (duplicate name, un-deletable pool, etc.) returns before reaching here, -// so no dangling/empty undo point is ever opened. +// so no dangling/empty undo point is ever opened for a rejected op. +// +// UNSAVED-PROJECT GUARDRAIL: on an unsaved / no-active project persistBook() no-ops +// (nothing is written to ext state). We must still CLOSE the block we opened, but with +// an EMPTY label and a zero flag so REAPER DISCARDS the point instead of recording a +// no-effect undo entry — mirroring view.cpp's empty-plan close. The in-session model +// change stands and persists on the user's next save; it just earns no undo point until +// there is a project to persist into (undo of an unsaved bank op has nothing to roll +// back to anyway). The Begin/End must still be balanced, hence the close-either-way. void persistBankOp(const char* label) { Undo_BeginBlock2(nullptr); - persistBook(); - Undo_EndBlock2(nullptr, label, UNDO_STATE_MISCCFG); + const bool persisted = persistBook(); + if (persisted) + Undo_EndBlock2(nullptr, label, UNDO_STATE_MISCCFG); + else + Undo_EndBlock2(nullptr, "", 0); // no ext-state write -> discard the empty point } // Prompts the user for a single line of text via REAPER's stock input dialog. @@ -713,11 +726,15 @@ void doBankTransferSelected(bool copy) { case TransferResult::RejectedSameBank: break; } } - // No-op guardrail: if nothing actually changed the index (every selected sample was - // absent, or all were pre-checked rejects), don't open an undo point. `collapsed` - // counts a hash-collapse — that DID mutate the index (source entry removed on a - // move, or dest already held the hash), so it belongs inside the undo point. - if (ok > 0 || collapsed > 0) { + // No-op guardrail — VERB-AWARE (a collapse means different things per verb): + // * MOVE collapse: the source entry WAS removed (bank_book moveSample removes + // unconditionally before the dest add collapses on hash), so the index DID + // mutate — it counts toward opening an undo point. + // * COPY collapse: the source is left intact AND the dest already held the hash, + // so NOTHING changed — a true index no-op. It must NOT open an undo point. + // Hence: copy counts only real gains (ok); move counts gains OR collapses. + const bool mutated = copy ? (ok > 0) : (ok > 0 || collapsed > 0); + if (mutated) { const std::string label = std::string("ReaSampler: ") + verb + " sample(s)"; persistBankOp(label.c_str()); diff --git a/src/main.cpp b/src/main.cpp index 2223f07..04a4974 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -241,6 +241,56 @@ static void OnTimer() reasampler::bankPanelRefresh(); } +// --- projectconfig hook: reload the session on undo/redo (R-B) --------------- +// A Ctrl-Z / Ctrl-Shift-Z rolls back / forward the "reasampler" project ext state on +// disk but keeps the SAME project identity (ReaProject*/GUID/.rpp path), so the timer's +// identity poll reads it as NoOp and never re-reads ext state — the in-memory book/view +// would stay stale until close+reopen. REAPER's projectconfig extension fires +// BeginLoadProjectState on every project-state (re)load, INCLUDING an undo/redo restore +// (isUndo == true for both). We hook it to drive a session reload. +// +// TIMING (the crux): BeginLoadProjectState is documented (reaper_plugin.h ~1203) as +// firing BEFORE any state restore. Reading GetProjExtState synchronously here would +// return the PRE-undo value. So we do NOT read here — we raise a one-shot reload request +// (g_session.requestReload()) that OnTimer's poll() drains on the NEXT tick, by which +// point REAPER has finished restoring the block and GetProjExtState returns +// the POST-undo value. Deterministic, event-driven — NOT ext-state content polling. +// +// GATED ON isUndo: a normal project open also fires BeginLoadProjectState (isUndo=false); +// we ignore that here so a normal open flows solely through the timer's identity-transition +// Load path (no double load). Only undo/redo (isUndo=true) requests the reload. +static void OnBeginLoadProjectState(bool isUndo, project_config_extension_t* /*reg*/) +{ + if (isUndo) + g_session.requestReload(); +} + +// ProcessExtensionLine / SaveExtensionConfig are intentional no-ops: ReaSampler stores +// its state via project EXT STATE (SetProjExtState/GetProjExtState under "reasampler"), +// which REAPER persists in its own RPP block — NOT via this extension's own +// project lines. We register the struct ONLY for the BeginLoadProjectState undo/redo +// notification. Returning false from ProcessExtensionLine means "not our line" so REAPER +// keeps dispatching (we claim none). SaveExtensionConfig writes nothing. +static bool OnProcessExtensionLine(const char* /*line*/, ProjectStateContext* /*ctx*/, + bool /*isUndo*/, project_config_extension_t* /*reg*/) +{ + return false; // we own no project lines — ext state carries our data +} + +static void OnSaveExtensionConfig(ProjectStateContext* /*ctx*/, bool /*isUndo*/, + project_config_extension_t* /*reg*/) +{ + // Nothing to write: our data rides in ext state, not project lines. +} + +// Storage must outlive registration — REAPER holds this pointer until we unregister it. +static project_config_extension_t g_projectConfig{ + &OnProcessExtensionLine, + &OnSaveExtensionConfig, + &OnBeginLoadProjectState, + nullptr, // userData +}; + // --- Scope-action source resolution ----------------------------------------- // The three scope actions (item / track / master) each resolve to (1) an exact // render range in project seconds — razor-else-time, inferred here — and (2) the @@ -786,6 +836,7 @@ extern "C" REAPER_PLUGIN_DLL_EXPORT int REAPER_PLUGIN_ENTRYPOINT( } g_rec->Register("-timer", (void*)&OnTimer); + g_rec->Register("-projectconfig", (void*)&g_projectConfig); g_rec->Register("-toggleaction", (void*)&OnToggleAction); g_rec->Register("-hookcommand", (void*)&OnHookCommand); // Tear down the Design View action family (D4) — mirror-unregisters each @@ -957,6 +1008,12 @@ extern "C" REAPER_PLUGIN_DLL_EXPORT int REAPER_PLUGIN_ENTRYPOINT( // state, on a Save-As it relocates the bank folder under the new .rpp. rec->Register("timer", (void*)&OnTimer); + // Register the projectconfig hook so an UNDO/REDO state restore reloads the + // session's book + view from the restored ext state (R-B). The timer's identity + // poll cannot see an undo (same project identity), so this hook owns undo/redo; it + // requests a deferred reload that the next timer tick drains (see the hook comment). + rec->Register("projectconfig", (void*)&g_projectConfig); + ShowConsoleMsg("ReaSampler loaded.\n"); return 1; // success — REAPER keeps us loaded diff --git a/src/persist.cpp b/src/persist.cpp index d9c755f..2b43baa 100644 --- a/src/persist.cpp +++ b/src/persist.cpp @@ -51,11 +51,23 @@ // (after relocating, or on the forked-sibling Load branch) so identities diverge. // // Rationale for the timer: the brief mandates ext-state storage (rules out the -// projectconfig .rpp-line hook), and the timer composes cleanly with ext-state -// while covering both load and Save-As detection in one place. The -// `projectconfig` BeginLoadProjectState hook is a deterministic alternative for -// pure load detection but would still need the timer (or Main_SaveProject -// post-hook) for Save-As path-change detection — surfaced in the handoff. +// projectconfig .rpp-line hook for STORAGE), and the timer composes cleanly with +// ext-state while covering identity-transition load + Save-As detection in one +// place. +// +// DIVISION OF LABOUR (R-B undo): +// * Identity-transition poll (this file, classifyProjectTransition) owns +// open / tab-switch / new / forked-sibling / Save-As-relocation — every case +// where the project OF RECORD changes. +// * The `projectconfig` hook (main.cpp registers project_config_extension_t; +// BeginLoadProjectState with isUndo) owns UNDO/REDO — where the project +// identity is unchanged but its ext state rolled back/forward on disk. The +// identity poll sees NoOp there and would never re-read ext state, so the hook +// requests a reload (requestReload) that poll() drains on the next tick, once +// REAPER has restored the block. See requestReload / the poll drain. +// The hook fires on undo AND redo (isUndo true for both), and on normal open +// (isUndo false) — but we set the reload flag ONLY for isUndo, so a normal open +// flows solely through the identity-transition Load path and never double-loads. // // NON-DESTRUCTIVE: this module writes ONLY our own ext-state key and moves ONLY // our own reasampler_bank/ folder. It never touches the user's media, items, or @@ -172,11 +184,11 @@ bool relocateBankFolder(const std::string& oldBankDir, } // namespace -void ReaSamplerSession::saveToActiveProject() { +bool ReaSamplerSession::saveToActiveProject() { std::string rppPath; void* proj = readActiveProject(rppPath); - if (!proj) return; // no active project — nothing to persist - if (rppPath.empty()) return; // unsaved project — no .rpp to store into + if (!proj) return false; // no active project — nothing to persist + if (rppPath.empty()) return false; // unsaved project — no .rpp to store into // Phase B: the whole book (pool as bank-zero + named banks) is authoritative and // rides in the `banks` key. @@ -206,6 +218,7 @@ void ReaSamplerSession::saveToActiveProject() { kProjExtTailKey, tailJson.c_str()); MarkProjectDirty(static_cast(proj)); + return true; } namespace { @@ -339,6 +352,13 @@ bool ReaSamplerSession::consumeLoadSignal() { return pending; } +void ReaSamplerSession::requestReload() { + // Set-only; poll() drains it on the next tick (see the poll() drain block for why + // the read is deferred past the projectconfig callback). Cheap and idempotent — + // multiple undo/redo callbacks before the next tick collapse to one reload. + reloadRequested_ = true; +} + void ReaSamplerSession::poll() { std::string rppPath; void* proj = readActiveProject(rppPath); @@ -355,6 +375,29 @@ void ReaSamplerSession::poll() { lastProject_ = proj; lastGuid_ = ensureProjectGuid(proj, rppPath, currentGuid); lastRppPath_ = rppPath; + reloadRequested_ = false; // priming already loaded — a co-tick request is moot + return; + } + + // Undo/redo reload (owner: the projectconfig hook, NOT the identity classifier + // below). An undo/redo keeps the SAME project identity — same ReaProject*, GUID, + // and .rpp path — so classifyProjectTransition would return NoOp and never re-read + // ext state, leaving book_/view_ stale after the on-disk ext state rolled back. + // The projectconfig BeginLoadProjectState callback (isUndo) raised reloadRequested_ + // one or more ticks ago; by NOW REAPER has finished restoring the project's + // block, so GetProjExtState returns the POST-undo value. Reload from the + // current active project and identity-adopt it (no relocation — the path is + // unchanged), then return. loadFromProject raises loadPending_, so the existing + // consumeLoadSignal() glue re-baselines the panel detector and reapplies the active + // mode; bankPanelRefresh's fingerprint pass then repaints the restored book. This is + // the ONLY undo/redo reload path — the timer never polls ext-state CONTENT to detect + // an undo (Daniel's directive: the hook drives it, not a poll heuristic). + if (reloadRequested_) { + reloadRequested_ = false; + loadFromProject(proj, projectDirOf(rppPath)); + lastProject_ = proj; + lastGuid_ = ensureProjectGuid(proj, rppPath, currentGuid); + lastRppPath_ = rppPath; return; } diff --git a/src/persist.h b/src/persist.h index de3743d..c74c8df 100644 --- a/src/persist.h +++ b/src/persist.h @@ -126,13 +126,35 @@ public: // to the active project's ext state (namespace "reasampler"), and clear the retired // legacy `bank_index` key. Non-destructive beyond writing our own ext-state keys. // Safe to call when there is no active/saved project (it no-ops). - void saveToActiveProject(); + // + // Returns true iff a persist actually happened (an active, SAVED project existed); + // false when it no-op'd (no active project, or an unsaved one with no .rpp). Lets a + // caller wrapping this in an undo block skip the block when nothing was written, so + // no dangling no-effect undo entry is opened on an unsaved project. + bool saveToActiveProject(); // Poll the active project. Detects a project load (active project changed) // and a Save-As (active project's .rpp path changed) and reacts accordingly. // Intended to be driven by REAPER's "timer" register. Idempotent per tick. + // + // Also drains a pending undo/redo reload (requestReload): a Ctrl-Z / Ctrl-Shift-Z + // keeps the SAME project identity (same ReaProject*/GUID/.rpp path), so the + // identity classifier below reads it as NoOp and would never re-read ext state. + // The projectconfig hook (main.cpp) raises the reload flag on an undo/redo state + // restore; poll() honours it FIRST — reloading book_ + view_ + tail_ from the + // (now-restored) ext state of the current project — before the identity check, so + // the undo is reflected in-session without any content polling. void poll(); + // Request a reload of book_ + view_ + tail_ from the CURRENT active project's ext + // state on the next poll() tick. Raised by the projectconfig hook (main.cpp) ONLY + // on an undo/redo state restore (isUndo). Deferred (a flag, not an immediate read) + // because the projectconfig callback fires BEFORE REAPER has restored the project's + // block — reading GetProjExtState synchronously there would return the + // PRE-undo value. Draining it on the next timer tick reads the restored value. This + // is REAPER-facing shell state; the request itself carries no REAPER types. + void requestReload(); + // Load signal for the D4 reapply-on-open glue. poll() raises this whenever it // (re)loads the view model from a project — prime, a project switch/open, or a // forked-sibling load. consumeLoadSignal() returns true ONCE per load and clears @@ -171,6 +193,7 @@ private: std::string lastRppPath_; // .rpp path last seen for lastProject_ bool primed_ = false; // false until the first poll() observes state bool loadPending_ = false; // raised by loadFromProject; drained by consumeLoadSignal + bool reloadRequested_ = false; // raised by requestReload (projectconfig undo/redo); drained by poll // Load the book from the given project's ext state (the `banks` key, else the // legacy `bank_index` key migrated into the pool) and resolve bank paths against From 63f35fa58dab6a606926f63bdbf883ba12d97129 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 26 Jul 2026 14:50:18 -0400 Subject: [PATCH 3/6] =?UTF-8?q?feat(persist):=20owned-file=20manifest=20se?= =?UTF-8?q?am=20=E2=80=94=20capture=20records=20created=20files=20(B-cap)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pure OwnedFileManifest (relative paths, dedup, JSON round-trip) persisted under sibling owned_files ext-state key; both capture commit paths record; joins the R-B undo-reload set. Phase R prune consumes it later. --- CMakeLists.txt | 19 +- src/main.cpp | 20 ++- src/owned_manifest.cpp | 315 ++++++++++++++++++++++++++++++++++ src/owned_manifest.h | 91 ++++++++++ src/persist.cpp | 34 ++++ src/persist.h | 31 +++- tests/test_owned_manifest.cpp | 171 ++++++++++++++++++ 7 files changed, 674 insertions(+), 7 deletions(-) create mode 100644 src/owned_manifest.cpp create mode 100644 src/owned_manifest.h create mode 100644 tests/test_owned_manifest.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index c8158f2..c565887 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -157,6 +157,18 @@ add_library(bank_book STATIC src/bank_book.cpp) target_include_directories(bank_book PUBLIC src) target_link_libraries(bank_book PUBLIC bank_model) +# --------------------------------------------------------------------------- +# 2g'') Pure owned_manifest library — NO REAPER, NO SWELL. The owned-file manifest +# seam (Phase B B-cap): the set of project-relative files the capture path +# itself created, so Phase R prune can tell the bank system's own orphans from +# hand-dropped files. Deliberately DECOUPLED from bank_book — it tracks files +# CREATED, not index membership (sample-remove is not manifest-remove). Small +# pure type + JSON round-trip; mirror of wav_trim / tab_strip. B-cap writes + +# persists it; Phase R (R1/R2) consumes it — no prune logic here. +# --------------------------------------------------------------------------- +add_library(owned_manifest STATIC src/owned_manifest.cpp) +target_include_directories(owned_manifest PUBLIC src) + # --------------------------------------------------------------------------- # 2g) Pure realtime_record library — NO REAPER, NO SWELL. The M8 realtime-record # logic: capture scope + FX-tap point -> I_RECMODE / I_RECMODE_FLAGS values, @@ -251,6 +263,10 @@ add_executable(wav_trim_tests tests/test_wav_trim.cpp) target_link_libraries(wav_trim_tests PRIVATE wav_trim) add_test(NAME wav_trim_tests COMMAND wav_trim_tests) +add_executable(owned_manifest_tests tests/test_owned_manifest.cpp) +target_link_libraries(owned_manifest_tests PRIVATE owned_manifest) +add_test(NAME owned_manifest_tests COMMAND owned_manifest_tests) + # --------------------------------------------------------------------------- # 4) The REAPER extension — a loadable module (dlopen'd by REAPER, not linked). # --------------------------------------------------------------------------- @@ -286,8 +302,9 @@ add_library(reaper_reasampler MODULE src/item_read.cpp src/actions.cpp src/bank_book.cpp + src/owned_manifest.cpp ) -target_link_libraries(reaper_reasampler PRIVATE bank_model capture_paths peaks bank_grid mode_switch tab_strip view_mode_model insert_plan render_settings tail_control realtime_record bank_book wav_trim) +target_link_libraries(reaper_reasampler PRIVATE bank_model capture_paths peaks bank_grid mode_switch tab_strip view_mode_model insert_plan render_settings tail_control realtime_record bank_book wav_trim owned_manifest) target_include_directories(reaper_reasampler PRIVATE ${SDK_INC} ${WDL_INC}) set_target_properties(reaper_reasampler PROPERTIES PREFIX "" OUTPUT_NAME "reaper_reasampler") diff --git a/src/main.cpp b/src/main.cpp index 04a4974..49ec89e 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -145,7 +145,12 @@ static void CommitRealtimeResult(const reasampler::CaptureResult& res) return; } reasampler::AddResult added = g_session.bank().add(res.sample); - g_session.saveToActiveProject(); // persist + MarkProjectDirty (travels with .rpp) + // B-cap: record the file the capture created in the owned-file manifest, at the same + // point the Sample is added and before the same persist. Recorded regardless of the + // index AddResult — even a hash-collapse still WROTE a file the tool owns, and the + // manifest dedups a repeat path itself (Phase R prune reconciles manifest vs index). + g_session.owned().add(res.sample.relativePath); + g_session.saveToActiveProject(); // persist book + manifest + MarkProjectDirty (travels with .rpp) std::string log = "ReaSampler: " + res.message + "\n"; log += " bank size now " + std::to_string(g_session.bank().size()) + @@ -615,10 +620,15 @@ static void RunCapture(const reasampler::CaptureActionDef& def) // Add to the ACTIVE bank: g_session.bank() resolves to book.activeIndex() (B2). reasampler::AddResult added = g_session.bank().add(res.sample); - // Persist the updated book into the active project's ext state (the `banks` key) - // so the capture survives Save / close+reopen (M4) and travels with the .rpp. - // saveToActiveProject also clears the retired legacy key and calls MarkProjectDirty. - // Non-destructive: writes only our own ext-state keys. + // B-cap: record the created file in the owned-file manifest, at the same point the + // Sample is added and before the same persist. Recorded regardless of the index + // AddResult — even a hash-collapse still WROTE a file the tool owns, and the manifest + // dedups a repeat path itself (Phase R prune reconciles manifest vs index later). + g_session.owned().add(res.sample.relativePath); + // Persist the updated book AND manifest into the active project's ext state (the + // `banks` + `owned_files` keys) so the capture survives Save / close+reopen (M4) and + // travels with the .rpp. saveToActiveProject also clears the retired legacy key and + // calls MarkProjectDirty. Non-destructive: writes only our own ext-state keys. g_session.saveToActiveProject(); std::string log = "ReaSampler: " + res.message + "\n"; diff --git a/src/owned_manifest.cpp b/src/owned_manifest.cpp new file mode 100644 index 0000000..08c4798 --- /dev/null +++ b/src/owned_manifest.cpp @@ -0,0 +1,315 @@ +#include "owned_manifest.h" + +#include +#include + +// owned_manifest implementation. +// +// JSON is hand-rolled and self-contained (project convention: the pure core is +// dependency-free — no third-party JSON lib, mirror of bank_model / bank_book / +// tail_control). The shape is a single object with one string array: +// +// {"owned":["reasampler_bank/a.wav","reasampler_bank/b.wav"]} +// +// so a compact writer + a focused string-array parser is all it needs — far smaller +// than bank_model's full recursive-descent parser, because there is exactly one key +// and one value kind. + +namespace reasampler { + +// --------------------------------------------------------------------------- +// path invariant (mirror of bank_model's isAbsolutePath) +// --------------------------------------------------------------------------- + +namespace { + +// Any leading '/' or '\' (POSIX root / UNC), or a leading : (Windows drive, +// incl. drive-relative "C:foo") is absolute. Same rejection bank_model applies to +// Sample.relativePath — the manifest holds the SAME kind of path, so the invariant +// must match exactly (a path the index accepts must be recordable, and vice versa). +bool isAbsolutePath(const std::string& p) { + if (p.empty()) return false; + if (p[0] == '/' || p[0] == '\\') return true; + if (p.size() >= 2 && std::isalpha(static_cast(p[0])) && p[1] == ':') + return true; + return false; +} + +} // namespace + +// --------------------------------------------------------------------------- +// mutation / query +// --------------------------------------------------------------------------- + +ManifestAddResult OwnedFileManifest::add(const std::string& relativePath) { + if (relativePath.empty()) return ManifestAddResult::RejectedEmptyPath; + if (isAbsolutePath(relativePath)) return ManifestAddResult::RejectedAbsolutePath; + if (contains(relativePath)) return ManifestAddResult::AlreadyPresent; + paths_.push_back(relativePath); + return ManifestAddResult::Added; +} + +bool OwnedFileManifest::contains(const std::string& relativePath) const { + for (const auto& p : paths_) + if (p == relativePath) return true; + return false; +} + +// --------------------------------------------------------------------------- +// JSON writer +// --------------------------------------------------------------------------- + +namespace { + +void writeEscaped(std::string& out, const std::string& s) { + out += '"'; + for (char c : s) { + switch (c) { + case '"': out += "\\\""; break; + case '\\': out += "\\\\"; break; + case '\b': out += "\\b"; break; + case '\f': out += "\\f"; break; + case '\n': out += "\\n"; break; + case '\r': out += "\\r"; break; + case '\t': out += "\\t"; break; + default: + if (static_cast(c) < 0x20) { + char buf[8]; + std::snprintf(buf, sizeof(buf), "\\u%04x", + static_cast(c)); + out += buf; + } else { + out += c; + } + } + } + out += '"'; +} + +} // namespace + +std::string OwnedFileManifest::serialize() const { + std::string out = "{\"owned\":["; + for (std::size_t i = 0; i < paths_.size(); ++i) { + if (i) out += ','; + writeEscaped(out, paths_[i]); + } + out += "]}"; + return out; +} + +// --------------------------------------------------------------------------- +// JSON parser (string-array only) +// --------------------------------------------------------------------------- + +namespace { + +class Parser { +public: + explicit Parser(const std::string& s) : s_(s) {} + + // Parse the manifest object into `out`. Tolerates unknown keys (forward-compat) + // and requires the "owned" value to be an array of strings. + bool parseManifest(OwnedFileManifest& out); + +private: + const std::string& s_; + std::size_t pos_ = 0; + + bool eof() const { return pos_ >= s_.size(); } + + void skipWs() { + while (!eof()) { + char c = s_[pos_]; + if (c == ' ' || c == '\t' || c == '\n' || c == '\r') ++pos_; + else break; + } + } + + bool consume(char c) { + skipWs(); + if (eof() || s_[pos_] != c) return false; + ++pos_; + return true; + } + + bool parseString(std::string& out); + bool parseStringArray(std::vector& out); + bool skipValue(); // for forward-compat unknown keys +}; + +// Parses a JSON string literal (with the escapes our writer emits, plus \uXXXX for +// control chars). Positioned before the opening quote (skips leading whitespace). +bool Parser::parseString(std::string& out) { + skipWs(); + if (eof() || s_[pos_] != '"') return false; + ++pos_; + out.clear(); + while (!eof()) { + char c = s_[pos_++]; + if (c == '"') return true; + if (c == '\\') { + if (eof()) return false; + char e = s_[pos_++]; + switch (e) { + case '"': out += '"'; break; + case '\\': out += '\\'; break; + case '/': out += '/'; break; + case 'b': out += '\b'; break; + case 'f': out += '\f'; break; + case 'n': out += '\n'; break; + case 'r': out += '\r'; break; + case 't': out += '\t'; break; + case 'u': { + auto readHex4 = [&](unsigned int& cp) -> bool { + if (pos_ + 4 > s_.size()) return false; + cp = 0; + for (int i = 0; i < 4; ++i) { + char h = s_[pos_++]; + cp <<= 4; + if (h >= '0' && h <= '9') cp |= static_cast(h - '0'); + else if (h >= 'a' && h <= 'f') cp |= static_cast(h - 'a' + 10); + else if (h >= 'A' && h <= 'F') cp |= static_cast(h - 'A' + 10); + else return false; + } + return true; + }; + + unsigned int hi = 0; + if (!readHex4(hi)) return false; + + unsigned int codePoint = hi; + if (hi >= 0xD800 && hi <= 0xDBFF) { + if (pos_ + 6 > s_.size()) return false; + if (s_[pos_] != '\\' || s_[pos_ + 1] != 'u') return false; + pos_ += 2; + unsigned int lo = 0; + if (!readHex4(lo)) return false; + if (lo < 0xDC00 || lo > 0xDFFF) return false; + codePoint = 0x10000 + ((hi - 0xD800) << 10) + (lo - 0xDC00); + } else if (hi >= 0xDC00 && hi <= 0xDFFF) { + return false; // unpaired low surrogate + } + + if (codePoint <= 0x7F) { + out += static_cast(codePoint); + } else if (codePoint <= 0x7FF) { + out += static_cast(0xC0 | (codePoint >> 6)); + out += static_cast(0x80 | (codePoint & 0x3F)); + } else if (codePoint <= 0xFFFF) { + out += static_cast(0xE0 | (codePoint >> 12)); + out += static_cast(0x80 | ((codePoint >> 6) & 0x3F)); + out += static_cast(0x80 | (codePoint & 0x3F)); + } else { + out += static_cast(0xF0 | (codePoint >> 18)); + out += static_cast(0x80 | ((codePoint >> 12) & 0x3F)); + out += static_cast(0x80 | ((codePoint >> 6) & 0x3F)); + out += static_cast(0x80 | (codePoint & 0x3F)); + } + break; + } + default: return false; + } + } else { + out += c; + } + } + return false; // unterminated string +} + +bool Parser::parseStringArray(std::vector& out) { + if (!consume('[')) return false; + skipWs(); + if (consume(']')) return true; // empty array + for (;;) { + std::string s; + if (!parseString(s)) return false; + out.push_back(std::move(s)); + skipWs(); + if (consume(',')) continue; + if (consume(']')) return true; + return false; // neither separator nor terminator — malformed + } +} + +// Skip a single JSON value (string / array / object / bare scalar) so an unknown key +// does not abort the parse. Minimal: enough for forward-compat siblings we don't know. +bool Parser::skipValue() { + skipWs(); + if (eof()) return false; + char c = s_[pos_]; + if (c == '"') { + std::string tmp; + return parseString(tmp); + } + if (c == '[' || c == '{') { + // Balance nested brackets of either kind, ignoring bracket chars inside + // strings. Enough to step over an unknown nested value; not a full validator. + int depth = 0; + bool inStr = false; + while (!eof()) { + char d = s_[pos_]; + if (inStr) { + if (d == '\\') { pos_ += 2; continue; } + if (d == '"') inStr = false; + ++pos_; + continue; + } + if (d == '"') { inStr = true; ++pos_; continue; } + if (d == '[' || d == '{') ++depth; + else if (d == ']' || d == '}') { + --depth; + if (depth == 0) { ++pos_; return true; } + } + ++pos_; + } + return false; + } + // bare scalar (number / true / false / null) — read to the next structural char + while (!eof()) { + char d = s_[pos_]; + if (d == ',' || d == '}' || d == ']' || d == ' ' || d == '\t' || + d == '\n' || d == '\r') + break; + ++pos_; + } + return true; +} + +bool Parser::parseManifest(OwnedFileManifest& out) { + if (!consume('{')) return false; + skipWs(); + if (consume('}')) return true; // empty object -> empty manifest + for (;;) { + std::string key; + if (!parseString(key)) return false; + if (!consume(':')) return false; + if (key == "owned") { + std::vector paths; + if (!parseStringArray(paths)) return false; + for (auto& p : paths) { + // Feed through add() so the persisted invariants (dedup, reject + // empty/absolute) are re-asserted on load — a hand-edited or corrupt + // blob cannot smuggle an absolute or duplicate path into the manifest. + out.add(p); + } + } else { + if (!skipValue()) return false; // forward-compat: tolerate unknown keys + } + skipWs(); + if (consume(',')) continue; + if (consume('}')) return true; + return false; + } +} + +} // namespace + +std::optional OwnedFileManifest::deserialize(const std::string& json) { + OwnedFileManifest m; + Parser p(json); + if (!p.parseManifest(m)) return std::nullopt; + return m; +} + +} // namespace reasampler diff --git a/src/owned_manifest.h b/src/owned_manifest.h new file mode 100644 index 0000000..fe3a3d7 --- /dev/null +++ b/src/owned_manifest.h @@ -0,0 +1,91 @@ +#pragma once +// owned_manifest — the pure core of the owned-file manifest seam (Phase B, B-cap). +// +// PURE MODULE (CLAUDE.md §load-bearing split): NO REAPER types, NO SWELL, NO +// vendor/ includes. Standard library only. Unit-tested outside the DAW — the same +// "small pure type + JSON round-trip" pattern as wav_trim / tab_strip. +// +// -- What it is -------------------------------------------------------------- +// +// The set of files the bank system ITSELF created — every file the capture path +// writes gets recorded here. Phase R prune consumes it to tell the system's own +// orphans (owned ∩ present − referenced) apart from hand-dropped files. B-cap only +// WRITES and PERSISTS the manifest; no prune logic lives here (fork R-D, settled +// 2026-07-24: "defer the feature, design the seam"). +// +// -- What it is NOT ---------------------------------------------------------- +// +// It is NOT a mirror of the bank index. Removing or moving an index entry does NOT +// remove the file's manifest record: the manifest tracks files *created*, and prune +// (Phase R) reconciles manifest-vs-index later. The ONLY thing that adds to it is +// the capture add-path. There is deliberately no remove verb here. +// +// -- The relative-paths-only invariant --------------------------------------- +// +// A manifest path is ALWAYS project-relative (same invariant as Sample.relativePath +// and the persisted BankIndex). add() rejects an absolute path rather than guess a +// relativization — the pure model has no project root, so a "normalization" would be +// a guess that could point at the wrong file (mirror of BankIndex::add's rejection). + +#include +#include +#include + +namespace reasampler { + +// Outcome of an add(). Mirrors BankIndex::AddResult's honesty — the op reports what +// happened rather than silently mutating on a bad request. +// - Added: the path was new and recorded. +// - AlreadyPresent: the path was already in the manifest (dedup no-op). +// - RejectedEmptyPath: the path was empty. +// - RejectedAbsolutePath: the path was absolute (relative-paths-only invariant). +enum class ManifestAddResult { + Added, + AlreadyPresent, + RejectedEmptyPath, + RejectedAbsolutePath, +}; + +// The owned-file manifest: an insertion-ordered, deduplicated set of project-relative +// paths the capture path has created. Insertion order is preserved so serialize() +// round-trips byte-identically (deterministic ext-state, mirror of the index). +class OwnedFileManifest { +public: + OwnedFileManifest() = default; + + // Record a project-relative path as owned. Rejects an empty or absolute path (no + // mutation). A path already present is a dedup no-op (AlreadyPresent), so a repeat + // capture of an identical request does not double-record. + ManifestAddResult add(const std::string& relativePath); + + // True iff the exact path string is recorded. Phase R uses this to attribute a + // present file to the bank system. Exact string match — path normalization (if any) + // is the caller's concern, consistent across add and query. + bool contains(const std::string& relativePath) const; + + // The owned paths in insertion order. Phase R unions this with the on-disk file + // set; here it is the round-trip + query surface. + const std::vector& paths() const { return paths_; } + + std::size_t size() const { return paths_.size(); } + bool empty() const { return paths_.empty(); } + + bool operator==(const OwnedFileManifest& o) const { return paths_ == o.paths_; } + + // -- Persistence --------------------------------------------------------- + + // Serialize to a JSON string (lossless round-trip): deserialize(serialize(x)) == x. + // An empty manifest serializes to a well-formed empty shape (round-trips to empty). + std::string serialize() const; + + // Parse a manifest JSON produced by serialize(). std::nullopt on malformed input + // (the persist shell warns + falls back to an empty manifest, mirroring the bank / + // view malformed handling). An empty/absent stored value is the caller's concern + // (an empty string is not valid JSON) — the shell maps absence to a fresh manifest. + static std::optional deserialize(const std::string& json); + +private: + std::vector paths_; // insertion order; deduplicated +}; + +} // namespace reasampler diff --git a/src/persist.cpp b/src/persist.cpp index 2b43baa..fe303f1 100644 --- a/src/persist.cpp +++ b/src/persist.cpp @@ -217,6 +217,15 @@ bool ReaSamplerSession::saveToActiveProject() { SetProjExtState(static_cast(proj), kProjExtNamespace, kProjExtTailKey, tailJson.c_str()); + // Additive: the owned-file manifest (Phase B B-cap) rides alongside in its own + // `owned_files` key. Independent write — does not disturb the blobs above. Written + // on EVERY save so a capture's manifest record survives Save / Save-As / reopen, + // and so the manifest and the bank stay in lockstep on disk (both persisted by the + // same saveToActiveProject the capture add-path calls). + const std::string ownedJson = owned_.serialize(); + SetProjExtState(static_cast(proj), kProjExtNamespace, + kProjExtOwnedKey, ownedJson.c_str()); + MarkProjectDirty(static_cast(proj)); return true; } @@ -259,6 +268,25 @@ TailSetting loadTailSetting(ReaProject* proj) { return *loaded; } +// Load the owned-file manifest from a project's owned_files key, or return an empty +// manifest. An absent/empty key (older / never-captured project) yields an empty +// manifest — graceful, never a crash. Malformed JSON is warned and also falls back to +// empty, mirroring the bank's / view's / tail's malformed handling. Phase R prune then +// sees an empty ownership record and (safely) attributes nothing until the next capture +// rebuilds it — losing the record degrades safety, never correctness. +OwnedFileManifest loadOwnedManifest(ReaProject* proj) { + if (!proj) return OwnedFileManifest{}; + const std::string ownedJson = + getProjExtStateString(proj, kProjExtNamespace, kProjExtOwnedKey); + if (ownedJson.empty()) return OwnedFileManifest{}; // no stored manifest -> empty + std::optional loaded = OwnedFileManifest::deserialize(ownedJson); + if (!loaded) { + ShowConsoleMsg("ReaSampler: stored owned-file manifest is malformed — ignoring.\n"); + return OwnedFileManifest{}; + } + return std::move(*loaded); +} + } // namespace void ReaSamplerSession::loadFromProject(void* proj, const std::string& projectDir) { @@ -281,6 +309,12 @@ void ReaSamplerSession::loadFromProject(void* proj, const std::string& projectDi // the previous project's choice (this REPLACES the old session-carry behavior). tail_ = loadTailSetting(static_cast(proj)); + // The owned-file manifest is restored on EVERY load path too (peer-symmetry with the + // bank/view/tail resets): switching to a project with no stored manifest must reset + // to empty, not inherit the previous project's ownership record; an undo/redo reload + // (R-B) must re-read the restored manifest so it matches the rolled-back bank state. + owned_ = loadOwnedManifest(static_cast(proj)); + if (!proj) { book_ = BankBook{}; return; diff --git a/src/persist.h b/src/persist.h index c74c8df..75252fa 100644 --- a/src/persist.h +++ b/src/persist.h @@ -21,6 +21,7 @@ #include "bank_book.h" #include "bank_model.h" +#include "owned_manifest.h" #include "tail_control.h" #include "view_mode_model.h" @@ -58,6 +59,17 @@ inline constexpr const char* kProjExtViewKey = "view_state"; // default — graceful, but the user's saved choice would be lost). inline constexpr const char* kProjExtTailKey = "tail_setting"; +// The ext-state key holding the owned-file manifest JSON (the set of project-relative +// files the capture path itself created — Phase B B-cap seam, consumed by Phase R +// prune to distinguish the bank system's own orphans from hand-dropped files). A +// SIBLING key alongside banks/view_state/tail_setting — NOT folded into the `banks` +// blob, so it stays decoupled from bank membership (removing an index entry is not a +// manifest removal). One namespace, four content keys. FOREVER-STABLE: changing it +// strands every already-saved project's ownership record, so Phase R prune could no +// longer tell the tool's own files apart (it would fall back to an empty manifest — +// graceful, but the attribution safety net is lost until the next capture rebuilds it). +inline constexpr const char* kProjExtOwnedKey = "owned_files"; + // The ext-state key holding a GUID we mint per project to establish CONTENT-BASED // project identity (REAPER exposes no stable per-project GUID). poll() uses it to // tell a genuine Save-As (same GUID, new .rpp path) apart from a project switch @@ -122,6 +134,15 @@ public: TailSetting& tail() { return tail_; } const TailSetting& tail() const { return tail_; } + // The owned-file manifest (Phase B B-cap): the set of project-relative files the + // capture path itself created. The capture add-path records each created file here + // (main.cpp, alongside the bank add), exactly as it adds the Sample to the active + // bank; persist serializes it under the `owned_files` key on save and replaces it on + // project load / undo-reload — peer to book_/view_/tail_. Phase R prune CONSUMES it; + // B-cap only writes and persists it (no prune logic here). + OwnedFileManifest& owned() { return owned_; } + const OwnedFileManifest& owned() const { return owned_; } + // Serialize the current book (under the `banks` key), view model, and tail setting // to the active project's ext state (namespace "reasampler"), and clear the retired // legacy `bank_index` key. Non-destructive beyond writing our own ext-state keys. @@ -180,6 +201,13 @@ private: // adjusted project), so an absent key is graceful. Peer to bank_/view_. TailSetting tail_; + // The owned-file manifest. Default empty; loadFromProject resets it to empty (or the + // stored set) on EVERY load path (peer-symmetry with book_/view_/tail_): switching to + // a project with no stored manifest must not inherit the previous project's ownership + // record, and an undo that rolled back a capture must re-read the restored manifest so + // the in-memory set matches disk. Absent key -> empty is graceful (older project). + OwnedFileManifest owned_; + // The project identity last observed by poll(), used to detect load/Save-As. // The GUID is the PRIMARY signal (a different stored GUID = a different project // of record = Load, immune to pointer recycling). The pointer disambiguates the @@ -197,7 +225,8 @@ private: // Load the book from the given project's ext state (the `banks` key, else the // legacy `bank_index` key migrated into the pool) and resolve bank paths against - // projectDir at read time. Replaces the in-memory book. projectDir empty -> the + // projectDir at read time. Replaces the in-memory book. Also restores view_, tail_, + // and owned_ from their sibling keys on every load path. projectDir empty -> the // book is reset to empty (unsaved project has no resolvable banks). void loadFromProject(void* proj, const std::string& projectDir); }; diff --git a/tests/test_owned_manifest.cpp b/tests/test_owned_manifest.cpp new file mode 100644 index 0000000..77ecf17 --- /dev/null +++ b/tests/test_owned_manifest.cpp @@ -0,0 +1,171 @@ +// Standalone tests for reasampler::OwnedFileManifest — no REAPER, no framework. +// The owned-file manifest seam (Phase B B-cap): a deduplicated, insertion-ordered +// set of project-relative files the capture path created, with JSON round-trip. +// +// Covers (brief-named): JSON round-trip, dedup of repeated adds, the empty manifest. +// Plus: the relative-paths-only invariant (reject empty / absolute), contains() +// semantics, insertion-order preservation, malformed-parse -> nullopt (the persist +// shell's warn+fallback hinges on it), and round-trip of paths with JSON metacharacters. + +#include "../src/owned_manifest.h" + +#include +#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) + +// --- empty manifest ---------------------------------------------------------- + +static void testEmptyManifest() { + OwnedFileManifest m; + CHECK(m.empty()); + CHECK(m.size() == 0); + CHECK(m.paths().empty()); + CHECK(!m.contains("anything.wav")); + + // An empty manifest serializes to a well-formed shape and round-trips to empty. + const std::string json = m.serialize(); + auto back = OwnedFileManifest::deserialize(json); + CHECK(back.has_value()); + CHECK(*back == m); + CHECK(back->empty()); +} + +// --- add / contains / order -------------------------------------------------- + +static void testAddAndContains() { + OwnedFileManifest m; + CHECK(m.add("reasampler_bank/a.wav") == ManifestAddResult::Added); + CHECK(m.add("reasampler_bank/b.wav") == ManifestAddResult::Added); + + CHECK(m.size() == 2); + CHECK(m.contains("reasampler_bank/a.wav")); + CHECK(m.contains("reasampler_bank/b.wav")); + CHECK(!m.contains("reasampler_bank/c.wav")); + // Exact-string match — not a prefix / substring match. + CHECK(!m.contains("reasampler_bank/a")); + CHECK(!m.contains("a.wav")); + + // Insertion order is preserved (deterministic ext-state). + CHECK(m.paths().size() == 2); + CHECK(m.paths()[0] == "reasampler_bank/a.wav"); + CHECK(m.paths()[1] == "reasampler_bank/b.wav"); +} + +// --- dedup of repeated adds -------------------------------------------------- + +static void testDedupRepeatedAdds() { + OwnedFileManifest m; + CHECK(m.add("reasampler_bank/take.wav") == ManifestAddResult::Added); + // A repeat capture of an identical request must not double-record the file. + CHECK(m.add("reasampler_bank/take.wav") == ManifestAddResult::AlreadyPresent); + CHECK(m.add("reasampler_bank/take.wav") == ManifestAddResult::AlreadyPresent); + CHECK(m.size() == 1); + CHECK(m.paths().size() == 1); +} + +// --- relative-paths-only invariant ------------------------------------------- + +static void testRejectsEmptyAndAbsolute() { + OwnedFileManifest m; + CHECK(m.add("") == ManifestAddResult::RejectedEmptyPath); + // Every absolute form bank_model rejects, the manifest rejects too. + CHECK(m.add("/abs/take.wav") == ManifestAddResult::RejectedAbsolutePath); // POSIX root + CHECK(m.add("\\\\host\\share\\x.wav") == ManifestAddResult::RejectedAbsolutePath); /* UNC */ + CHECK(m.add("C:/bank/x.wav") == ManifestAddResult::RejectedAbsolutePath); // Win drive / + CHECK(m.add("C:\\bank\\x.wav") == ManifestAddResult::RejectedAbsolutePath); /* Win drive backslash */ + CHECK(m.add("C:x.wav") == ManifestAddResult::RejectedAbsolutePath); // drive-relative + // A rejected add never mutates. + CHECK(m.empty()); + CHECK(!m.contains("/abs/take.wav")); +} + +// --- JSON round-trip --------------------------------------------------------- + +static void testRoundTrip() { + OwnedFileManifest m; + m.add("reasampler_bank/one.wav"); + m.add("reasampler_bank/two.wav"); + m.add("reasampler_bank/three.wav"); + + const std::string json = m.serialize(); + auto back = OwnedFileManifest::deserialize(json); + CHECK(back.has_value()); + CHECK(*back == m); + // Order + membership survive. + CHECK(back->paths().size() == 3); + CHECK(back->paths()[0] == "reasampler_bank/one.wav"); + CHECK(back->paths()[2] == "reasampler_bank/three.wav"); + + // serialize(deserialize(serialize(x))) is stable. + CHECK(back->serialize() == json); +} + +// A path carrying JSON metacharacters must survive the escape/unescape round-trip. +static void testRoundTripEscaping() { + OwnedFileManifest m; + m.add("reasampler_bank/od\"d name.wav"); // embedded quote + m.add("reasampler_bank/back\\slash.wav"); // embedded backslash + m.add("reasampler_bank/tab\tafter.wav"); // control char + + auto back = OwnedFileManifest::deserialize(m.serialize()); + CHECK(back.has_value()); + CHECK(*back == m); + CHECK(back->contains("reasampler_bank/od\"d name.wav")); + CHECK(back->contains("reasampler_bank/back\\slash.wav")); + CHECK(back->contains("reasampler_bank/tab\tafter.wav")); +} + +// --- malformed / tolerant parse ---------------------------------------------- + +static void testMalformedParse() { + // The persist shell's warn+fallback hinges on nullopt for a corrupt blob. + CHECK(!OwnedFileManifest::deserialize("").has_value()); // empty string + CHECK(!OwnedFileManifest::deserialize("not json").has_value()); + CHECK(!OwnedFileManifest::deserialize("{\"owned\":[").has_value()); // unterminated array + CHECK(!OwnedFileManifest::deserialize("{\"owned\":[1,2]}").has_value()); // non-string element + CHECK(!OwnedFileManifest::deserialize("{\"owned\":\"x\"}").has_value()); // wrong value type + + // An explicit empty array parses to an empty manifest. + auto empty = OwnedFileManifest::deserialize("{\"owned\":[]}"); + CHECK(empty.has_value()); + CHECK(empty->empty()); + + // An unknown sibling key is tolerated (forward-compat) — the owned array still loads. + auto fwd = OwnedFileManifest::deserialize( + "{\"future\":{\"nested\":[1,2]},\"owned\":[\"reasampler_bank/x.wav\"]}"); + CHECK(fwd.has_value()); + CHECK(fwd->size() == 1); + CHECK(fwd->contains("reasampler_bank/x.wav")); + + // A stored blob cannot smuggle a duplicate or absolute path past the load-time + // invariant re-assertion (deserialize routes each element through add()). + auto dupe = OwnedFileManifest::deserialize( + "{\"owned\":[\"reasampler_bank/x.wav\",\"reasampler_bank/x.wav\"]}"); + CHECK(dupe.has_value()); + CHECK(dupe->size() == 1); + + auto absolute = OwnedFileManifest::deserialize( + "{\"owned\":[\"reasampler_bank/ok.wav\",\"/etc/evil.wav\"]}"); + CHECK(absolute.has_value()); + CHECK(absolute->size() == 1); + CHECK(absolute->contains("reasampler_bank/ok.wav")); + CHECK(!absolute->contains("/etc/evil.wav")); +} + +int main() { + testEmptyManifest(); + testAddAndContains(); + testDedupRepeatedAdds(); + testRejectsEmptyAndAbsolute(); + testRoundTrip(); + testRoundTripEscaping(); + testMalformedParse(); + + if (g_fail == 0) std::printf("All tests passed.\n"); + return g_fail ? 1 : 0; +} From 6c1efa0f259792e258526f3f009ac31e1331a485 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 26 Jul 2026 15:07:41 -0400 Subject: [PATCH 4/6] =?UTF-8?q?B5:=20sample-remove=20verb=20=E2=80=94=20in?= =?UTF-8?q?dex-only=20drop,=20this-bank=20scope,=20confirm-on-last-referen?= =?UTF-8?q?ce?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/actions.cpp | 83 ++++++++++++++++++++++++++ src/bank_book.cpp | 33 +++++++++++ src/bank_book.h | 55 +++++++++++++++++ src/bank_panel.cpp | 96 ++++++++++++++++++++++++------ tests/test_bank_book.cpp | 124 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 373 insertions(+), 18 deletions(-) diff --git a/src/actions.cpp b/src/actions.cpp index f180011..f639bf5 100644 --- a/src/actions.cpp +++ b/src/actions.cpp @@ -420,6 +420,7 @@ constexpr const char* kIdBankActivateNext = "CEREBELLUM_REASAMPLER_BANK_ACTIVATE constexpr const char* kIdBankActivatePool = "CEREBELLUM_REASAMPLER_BANK_ACTIVATE_POOL"; constexpr const char* kIdBankMoveSel = "CEREBELLUM_REASAMPLER_BANK_MOVE_SELECTED"; constexpr const char* kIdBankCopySel = "CEREBELLUM_REASAMPLER_BANK_COPY_SELECTED"; +constexpr const char* kIdBankRemoveSel = "CEREBELLUM_REASAMPLER_BANK_REMOVE_SELECTED"; constexpr const char* kIdBankPoolFull = "CEREBELLUM_REASAMPLER_BANK_POOL_FULLHEIGHT"; constexpr const char* kIdBankBanksFull = "CEREBELLUM_REASAMPLER_BANK_BANKS_FULLHEIGHT"; @@ -431,6 +432,7 @@ int g_cmdBankActivateNext = 0; int g_cmdBankActivatePool = 0; int g_cmdBankMoveSel = 0; int g_cmdBankCopySel = 0; +int g_cmdBankRemoveSel = 0; int g_cmdBankPoolFull = 0; int g_cmdBankBanksFull = 0; @@ -442,6 +444,7 @@ gaccel_register_t g_accelBankActivateNext{}; gaccel_register_t g_accelBankActivatePool{}; gaccel_register_t g_accelBankMoveSel{}; gaccel_register_t g_accelBankCopySel{}; +gaccel_register_t g_accelBankRemoveSel{}; gaccel_register_t g_accelBankPoolFull{}; gaccel_register_t g_accelBankBanksFull{}; @@ -747,6 +750,81 @@ void doBankTransferSelected(bool copy) { ShowConsoleMsg(log.c_str()); } +// Remove the panel's selected samples from the SOURCE bank (the focused region's +// displayed bank — bankPanelSelectedSourceBankId, same source as move/copy). Index-only +// and non-destructive to the file: a last-reference remove leaves the file on disk, +// orphaned until Phase R prune (remove NEVER deletes bytes — the manifest is untouched). +// +// SCOPE (fork R-A): this-bank only — the sole surfaced verb. The RemoveScope::AllBanks +// seam stays latent in the model; nothing here reaches for it. +// +// CONFIRM-ON-LAST-REFERENCE (guardrail): a remove that would orphan a file (no OTHER +// bank references its content hash after the remove) earns a confirm; a remove of a +// still-referenced sample does not. BATCH UX: for a multi-select we compute the +// last-reference set BEFORE mutating (removal changes the reference graph), then fire a +// SINGLE confirm summarizing the N that would orphan — not one dialog per sample. If +// none would orphan, no confirm fires at all (the confirm is earned by actual risk). +void doBankRemoveSelected() { + const std::vector selected = bankPanelSelectedSampleIds(); + if (selected.empty()) { + ShowConsoleMsg("ReaSampler: nothing selected in the bank panel to remove.\n"); + return; + } + const std::string srcId = bankPanelSelectedSourceBankId(); + BankBook& book = g_session->book(); + const Bank* src = book.bank(srcId); + if (src == nullptr) { + ShowConsoleMsg("ReaSampler: the selection's bank no longer exists.\n"); + return; + } + + // Count the samples whose file this remove would orphan — computed on the CURRENT + // (pre-mutation) reference graph so a same-hash sibling in another bank counts as a + // surviving reference. Resolve by id against the live source index (ids, not cached + // refs); an id no longer present is skipped (it removes to a no-op below). + int orphanCount = 0; + for (const std::string& sampleId : selected) { + const Sample* s = src->index.query(sampleId); + if (s == nullptr) continue; // already gone; not a last-reference orphan + if (!book.hashReferencedElsewhere(s->contentHash, srcId)) ++orphanCount; + } + + if (orphanCount > 0) { + const std::string msg = + std::to_string(orphanCount) + + (orphanCount == 1 ? " selected sample is" : " selected samples are") + + " in no other bank.\n\nRemoving " + + (orphanCount == 1 ? "it" : "them") + + " drops the index entry only — the file stays on disk until you prune " + "(it is never deleted by remove).\n\nRemove anyway?"; + const int r = ShowMessageBox(msg.c_str(), + "ReaSampler: remove last-reference sample(s)", 4); + if (r != 6) return; // 6 == YES; anything else cancels (SDK ~6544) + } + + // Perform the removes (this-bank scope). Pass ids by value — no BankIndex& is cached + // across the loop's mutations. Count real drops so the no-op guardrail can skip the + // undo point when nothing was removed (every id was already absent). + int removed = 0, absent = 0; + for (const std::string& sampleId : selected) { + switch (book.removeSample(sampleId, srcId, RemoveScope::ThisBank)) { + case RemoveResult::Removed: ++removed; break; + case RemoveResult::RejectedSampleAbsent: ++absent; break; + // Unknown bank cannot occur — srcId was resolved to a live bank above. + case RemoveResult::RejectedUnknownBank: break; + } + } + + // No-op guardrail (R-B): open an undo point only if the index actually mutated. + if (removed > 0) persistBankOp("ReaSampler: remove sample(s)"); + + std::string log = "ReaSampler: removed " + std::to_string(removed) + + (removed == 1 ? " sample" : " samples"); + if (absent) log += ", " + std::to_string(absent) + " no longer present"; + log += ".\n"; + ShowConsoleMsg(log.c_str()); +} + } // namespace void bankRegisterActions(reaper_plugin_info_t* rec, ReaSamplerSession* session) { @@ -768,6 +846,8 @@ void bankRegisterActions(reaper_plugin_info_t* rec, ReaSamplerSession* session) "ReaSampler: move selected samples to bank"); g_cmdBankCopySel = registerAction(rec, kIdBankCopySel, g_accelBankCopySel, "ReaSampler: copy selected samples to bank"); + g_cmdBankRemoveSel = registerAction(rec, kIdBankRemoveSel, g_accelBankRemoveSel, + "ReaSampler: remove selected samples"); g_cmdBankPoolFull = registerAction(rec, kIdBankPoolFull, g_accelBankPoolFull, "ReaSampler: toggle pool full-height"); g_cmdBankBanksFull = registerAction(rec, kIdBankBanksFull, g_accelBankBanksFull, @@ -785,6 +865,7 @@ bool bankHandleCommand(int command) { if (command == g_cmdBankActivatePool) { doBankActivatePool(); return true; } if (command == g_cmdBankMoveSel) { doBankTransferSelected(false); return true; } if (command == g_cmdBankCopySel) { doBankTransferSelected(true); return true; } + if (command == g_cmdBankRemoveSel) { doBankRemoveSelected(); return true; } if (command == g_cmdBankPoolFull) { bankPanelToggledPoolFullHeight(); return true; } if (command == g_cmdBankBanksFull) { bankPanelToggledBanksFullHeight(); return true; } @@ -797,6 +878,8 @@ void bankUnregisterActions(reaper_plugin_info_t* rec) { rec->Register("-command_id", (void*)kIdBankBanksFull); rec->Register("-gaccel", (void*)&g_accelBankPoolFull); rec->Register("-command_id", (void*)kIdBankPoolFull); + rec->Register("-gaccel", (void*)&g_accelBankRemoveSel); + rec->Register("-command_id", (void*)kIdBankRemoveSel); rec->Register("-gaccel", (void*)&g_accelBankCopySel); rec->Register("-command_id", (void*)kIdBankCopySel); rec->Register("-gaccel", (void*)&g_accelBankMoveSel); diff --git a/src/bank_book.cpp b/src/bank_book.cpp index 9daebd3..44544f6 100644 --- a/src/bank_book.cpp +++ b/src/bank_book.cpp @@ -281,6 +281,39 @@ TransferResult BankBook::copySample(const std::string& sampleId, return applyDestAdd(to->index, copy, TransferResult::Copied); } +// --------------------------------------------------------------------------- +// Sample removal (index-only) + the last-reference query +// --------------------------------------------------------------------------- + +RemoveResult BankBook::removeSample(const std::string& sampleId, + const std::string& fromBankId, + RemoveScope scope) { + if (scope == RemoveScope::AllBanks) { + // Latent seam: purge the id from every bank that holds it. fromBankId is + // ignored (the id is dropped book-wide). Removed iff at least one drop landed. + bool any = false; + for (auto& b : banks_) + if (b.index.remove(sampleId)) any = true; + return any ? RemoveResult::Removed : RemoveResult::RejectedSampleAbsent; + } + + // ThisBank (default, the only surfaced verb): drop from the one named source bank. + Bank* from = bank(fromBankId); + if (from == nullptr) return RemoveResult::RejectedUnknownBank; + return from->index.remove(sampleId) ? RemoveResult::Removed + : RemoveResult::RejectedSampleAbsent; +} + +bool BankBook::hashReferencedElsewhere(const std::string& hash, + const std::string& exceptBankId) const { + if (hash.empty()) return false; // empty hashes never dedup (mirror findByHash) + for (const auto& b : banks_) { + if (b.id == exceptBankId) continue; // the removed-from bank is excluded + if (b.index.findByHash(hash) != nullptr) return true; + } + return false; +} + // =========================================================================== // JSON — writer // =========================================================================== diff --git a/src/bank_book.h b/src/bank_book.h index 2a6084b..6cc7cd8 100644 --- a/src/bank_book.h +++ b/src/bank_book.h @@ -85,6 +85,30 @@ enum class TransferResult { RejectedSameBank, }; +// Scope of a sample-remove (fork R-A, settled 2026-07-24). ThisBank is the default +// and the ONLY behavior surfaced in the UI/action layer; AllBanks is a latent seam — +// live and tested at the model level, promotable later behind this parameter without +// a rewrite, but never wired to an affordance in B5. +// - ThisBank: drop the entry from the one named source bank only. A same-hash entry +// in another bank survives (no cross-bank cascade — dedup is per-bank). +// - AllBanks: drop the sample's entry from EVERY bank that holds the source id +// ("purge from the library"). Latent; unsurfaced. +enum class RemoveScope { + ThisBank, + AllBanks, +}; + +// Outcome of BankBook::removeSample. Mirrors TransferResult's honesty: the op reports +// what happened rather than silently mutating on a bad request. +// - Removed: at least one index entry was dropped. +// - RejectedUnknownBank: the source bank id named no bank (ThisBank scope only). +// - RejectedSampleAbsent: the sample id was in no bank in scope (nothing removed). +enum class RemoveResult { + Removed, + RejectedUnknownBank, + RejectedSampleAbsent, +}; + // An ordered registry of banks with the pool seeded as bank-zero, per-bank sample // indices, an active-bank pointer, and lossless JSON round-trip. The heart of the // multi-bank phase — mirror of bank_model / view_mode_model. @@ -155,6 +179,37 @@ public: const std::string& fromBankId, const std::string& toBankId); + // -- Sample removal (index-only; the file is NEVER touched — orphaned until prune) -- + + // Drops a sample's index entry (the sample-level sibling of move/copy/evacuate). + // Index-only and non-destructive to the file: a last-reference remove leaves the + // file on disk, orphaned until Phase R prune — remove NEVER deletes bytes. + // + // Scope (fork R-A): ThisBank (default, the only surfaced verb) drops the entry from + // `fromBankId` alone; AllBanks (latent seam) drops the sample id from every bank + // that holds it. See RemoveResult for the outcome set. + // * ThisBank: RejectedUnknownBank if `fromBankId` names no bank; RejectedSampleAbsent + // if that bank does not hold the id; Removed on a drop. + // * AllBanks: `fromBankId` is ignored (the id is purged book-wide); + // RejectedSampleAbsent if NO bank held the id; Removed otherwise. + // No mutation occurs on any Rejected outcome (no-op guardrail for the undo layer). + RemoveResult removeSample(const std::string& sampleId, + const std::string& fromBankId, + RemoveScope scope = RemoveScope::ThisBank); + + // Reference-count query backing the confirm-on-last-reference guardrail: does any + // bank OTHER than `exceptBankId` still hold an entry whose contentHash == `hash`? + // + // Identity is the CONTENT HASH, not the file path: hash is the canonical dedup key + // the whole model already reasons in (findByHash / collapse-by-hash), and two + // entries that share content share one file — so "some other bank still references + // this hash" is exactly "removing here does not orphan the file." An EMPTY hash is + // never matched (it does not participate in dedup, mirroring findByHash), so an + // empty-hash sample reads as referenced-nowhere-else — the safe, confirm-eliciting + // direction (we cannot prove another bank shares an unhashed file). + bool hashReferencedElsewhere(const std::string& hash, + const std::string& exceptBankId) const; + // -- Query --------------------------------------------------------------- // The bank with `id`, or nullptr. Pointer invalidated by any mutating call. diff --git a/src/bank_panel.cpp b/src/bank_panel.cpp index 4595151..73f4761 100644 --- a/src/bank_panel.cpp +++ b/src/bank_panel.cpp @@ -1315,6 +1315,55 @@ void transferSamples(const std::vector& sampleIds, invalidatePanel(); } +// Remove `sampleIds` from `srcBankId` (index-only, this-bank scope). Non-destructive to +// the file: a last-reference remove leaves the file on disk, orphaned until Phase R +// prune — remove NEVER deletes bytes (the manifest is untouched). Confirm-on-last- +// reference guardrail: a single confirm summarizing the N whose files this would orphan +// (computed on the PRE-mutation reference graph), fired only when at least one would +// orphan. Ids passed by value — no BankIndex& cached across the loop's mutations. +void removeSamples(const std::vector& sampleIds, + const std::string& srcBankId) { + if (!book() || sampleIds.empty()) return; + const Bank* src = book()->bank(srcBankId); + if (!src) return; + + // Count files this remove would orphan — computed BEFORE mutating, so a same-hash + // sibling in another bank counts as a surviving reference. + int orphanCount = 0; + for (const std::string& sid : sampleIds) { + const Sample* s = src->index.query(sid); + if (!s) continue; // already gone; not a last-reference orphan + if (!book()->hashReferencedElsewhere(s->contentHash, srcBankId)) ++orphanCount; + } + + if (orphanCount > 0) { + const std::string msg = + std::to_string(orphanCount) + + (orphanCount == 1 ? " selected sample is" : " selected samples are") + + " in no other bank.\n\nRemoving " + + (orphanCount == 1 ? "it" : "them") + + " drops the index entry only — the file stays on disk until you prune " + "(it is never deleted by remove).\n\nRemove anyway?"; + // 4 == MB_YESNO. 6=Yes (SDK); anything else cancels. + const int r = ShowMessageBox(msg.c_str(), + "ReaSampler: remove last-reference sample(s)", 4); + if (r != 6) return; + } + + int removed = 0; + for (const std::string& sid : sampleIds) + if (book()->removeSample(sid, srcBankId, RemoveScope::ThisBank) == + RemoveResult::Removed) + ++removed; + if (removed == 0) return; // nothing changed — no persist, no undo point + + persistBook(); + // The selection indexed into the source; after a remove those indices are stale, so + // clear it (the fingerprint pass will also clear, but do it now for immediacy). + g_panel.selection = Selection{}; + invalidatePanel(); +} + // The selection's sample ids resolved against the FOCUSED region's bank (source of a // move/copy). Returns ids in bank order; empty when nothing selected. std::vector focusedSelectionIds() { @@ -1356,6 +1405,7 @@ enum : unsigned int { kMenuDelete, kMenuEvacuate, kMenuCreate, + kMenuRemove, // remove selected sample(s) from the source bank (B5) kMenuMoveBase = 1000, // move-to-bank: kMenuMoveBase + destination index kMenuCopyBase = 2000, // copy-to-bank: kMenuCopyBase + destination index }; @@ -1411,30 +1461,32 @@ void showSelectionMenu(int screenX, int screenY) { for (const Bank* bk : namedBanks()) if (bk->id != srcId) dests.push_back({bk->id, bk->displayName}); - HMENU menu = CreatePopupMenu(); - if (dests.empty()) { - menuAppend(menu, kMenuNone, "No other bank to move to", /*grayed=*/true); - TrackPopupMenu(menu, TPM_RETURNCMD, screenX, screenY, 0, g_panel.hwnd, nullptr); - DestroyMenu(menu); - return; - } - const std::string label = std::to_string(sel.size()) + (sel.size() == 1 ? " sample" : " samples"); - menuAppend(menu, kMenuNone, ("Move " + label + " to:").c_str(), /*grayed=*/true); - for (std::size_t i = 0; i < dests.size(); ++i) - menuAppend(menu, kMenuMoveBase + static_cast(i), - (" " + dests[i].name).c_str()); - menuSeparator(menu); - menuAppend(menu, kMenuNone, ("Copy " + label + " to:").c_str(), /*grayed=*/true); - for (std::size_t i = 0; i < dests.size(); ++i) - menuAppend(menu, kMenuCopyBase + static_cast(i), - (" " + dests[i].name).c_str()); + + HMENU menu = CreatePopupMenu(); + // Move/copy blocks appear only when there is another bank to transfer to; Remove is + // always offered (it needs no destination — it drops the entry from the source). + if (!dests.empty()) { + menuAppend(menu, kMenuNone, ("Move " + label + " to:").c_str(), /*grayed=*/true); + for (std::size_t i = 0; i < dests.size(); ++i) + menuAppend(menu, kMenuMoveBase + static_cast(i), + (" " + dests[i].name).c_str()); + menuSeparator(menu); + menuAppend(menu, kMenuNone, ("Copy " + label + " to:").c_str(), /*grayed=*/true); + for (std::size_t i = 0; i < dests.size(); ++i) + menuAppend(menu, kMenuCopyBase + static_cast(i), + (" " + dests[i].name).c_str()); + menuSeparator(menu); + } + menuAppend(menu, kMenuRemove, ("Remove " + label + "\xE2\x80\xA6").c_str()); const int cmd = TrackPopupMenu(menu, TPM_RETURNCMD, screenX, screenY, 0, g_panel.hwnd, nullptr); DestroyMenu(menu); - if (cmd >= static_cast(kMenuMoveBase) && + if (cmd == static_cast(kMenuRemove)) { + removeSamples(sel, srcId); + } else if (cmd >= static_cast(kMenuMoveBase) && cmd < static_cast(kMenuMoveBase + dests.size())) { transferSamples(sel, srcId, dests[cmd - kMenuMoveBase].id, /*copy=*/false); } else if (cmd >= static_cast(kMenuCopyBase) && @@ -1670,6 +1722,14 @@ bool handleKey(int vk) { case VK_ESCAPE: stopAudition(); return true; + case VK_DELETE: { + // Remove the focused-region selection (B5). Same confirm-on-last-reference + // path the context-menu "Remove" uses; a no-op when nothing is selected. + const std::vector sel = focusedSelectionIds(); + if (sel.empty()) return false; // nothing selected — let the key fall through + removeSamples(sel, bankIdForRegion(g_panel.focusedRegion)); + return true; + } default: return false; } diff --git a/tests/test_bank_book.cpp b/tests/test_bank_book.cpp index 1058056..479550e 100644 --- a/tests/test_bank_book.cpp +++ b/tests/test_bank_book.cpp @@ -638,6 +638,124 @@ static void testDeserializeNamedBankCollidingWithPoolIsDisambiguated() { if (back2) CHECK(back2->serialize() == back->serialize()); } +// --- B5: sample-remove (this-bank + latent all-banks) + last-reference query ------ +// +// removeSample drops a Sample's index entry (index-only, non-destructive to the file). +// ThisBank (default, surfaced) drops from one named source bank; AllBanks (latent seam) +// purges the id book-wide. hashReferencedElsewhere backs the confirm-on-last-reference +// guardrail: does any OTHER bank still hold the content hash? + +static void testRemoveDropsTargetEntry() { + BankBook book; + CHECK(book.createBank("drums", "Drums")); + CHECK(book.bank("drums")->index.add(sampleWith("kick")) == AddResult::Added); + CHECK(book.bank("drums")->index.add(sampleWith("snare")) == AddResult::Added); + + // Remove the kick from drums: dropped from the target bank, the sibling survives. + CHECK(book.removeSample("id-kick", "drums") == RemoveResult::Removed); + CHECK(book.bank("drums")->index.query("id-kick") == nullptr); // dropped + CHECK(book.bank("drums")->index.query("id-snare") != nullptr); // sibling kept + CHECK(book.bank("drums")->index.size() == 1); +} + +static void testRemoveThisBankLeavesSameHashInAnotherBank() { + // Copy a sample into two banks (same hash in both), then remove from one under the + // default this-bank scope: the OTHER bank's entry survives — no cross-bank cascade. + BankBook book; + CHECK(book.createBank("drums", "Drums")); + CHECK(book.pool().index.add(sampleWith("clap", "clap-hash")) == AddResult::Added); + CHECK(book.copySample("id-clap", kPoolBankId, "drums") == TransferResult::Copied); + + CHECK(book.removeSample("id-clap", kPoolBankId, RemoveScope::ThisBank) == + RemoveResult::Removed); + CHECK(book.pool().index.query("id-clap") == nullptr); // removed from pool + CHECK(book.bank("drums")->index.query("id-clap") != nullptr); // drums copy survives + CHECK(book.bank("drums")->index.findByHash("clap-hash") != nullptr); +} + +static void testRemoveFromPoolAllowedContainerPrivilegesHold() { + // Pool CONTENTS are removable (the pool must not be a roach-motel); the pool + // CONTAINER privileges (un-deletable / un-renamable / un-evacuable) are untouched. + BankBook book; + CHECK(book.pool().index.add(sampleWith("loop")) == AddResult::Added); + + CHECK(book.removeSample("id-loop", kPoolBankId) == RemoveResult::Removed); + CHECK(book.pool().index.empty()); // content removed + + // Container privileges still enforced. + CHECK(!book.deleteBank(kPoolBankId)); + CHECK(!book.renameBank(kPoolBankId, "NotPool")); + CHECK(!book.evacuate(kPoolBankId)); + CHECK(book.size() == 1); + CHECK(book.pool().displayName == std::string(kPoolBankName)); +} + +static void testRemoveRejectionsNoMutation() { + BankBook book; + CHECK(book.createBank("drums", "Drums")); + CHECK(book.bank("drums")->index.add(sampleWith("kick")) == AddResult::Added); + + // Unknown bank → honest rejection, no mutation. + CHECK(book.removeSample("id-kick", "ghost") == RemoveResult::RejectedUnknownBank); + CHECK(book.bank("drums")->index.query("id-kick") != nullptr); // untouched + + // Absent sample (right bank, wrong id) → honest rejection, no mutation. + CHECK(book.removeSample("id-missing", "drums") == RemoveResult::RejectedSampleAbsent); + CHECK(book.bank("drums")->index.size() == 1); +} + +static void testHashReferencedElsewhere() { + BankBook book; + CHECK(book.createBank("drums", "Drums")); + CHECK(book.createBank("hits", "Hits")); + + // Same-bank-only: the hash lives ONLY in drums → not referenced elsewhere (true + // last-reference; removing from drums would orphan the file). + CHECK(book.bank("drums")->index.add(sampleWith("solo", "solo-hash")) == AddResult::Added); + CHECK(!book.hashReferencedElsewhere("solo-hash", "drums")); + + // Copied-to-two-banks: the hash lives in drums AND hits → referenced elsewhere from + // either vantage (removing from one leaves the other's reference intact). + CHECK(book.bank("drums")->index.add(sampleWith("dup-d", "dup-hash")) == AddResult::Added); + CHECK(book.bank("hits")->index.add(sampleWith("dup-h", "dup-hash")) == AddResult::Added); + CHECK(book.hashReferencedElsewhere("dup-hash", "drums")); // hits still holds it + CHECK(book.hashReferencedElsewhere("dup-hash", "hits")); // drums still holds it + + // A hash present in NO bank is referenced nowhere. + CHECK(!book.hashReferencedElsewhere("absent-hash", "drums")); + + // An empty hash never matches (mirrors findByHash) → reads as not-referenced-else, + // the safe confirm-eliciting direction for an unhashed sample. + CHECK(book.pool().index.add(sampleWith("nohash", "")) == AddResult::Added); + CHECK(!book.hashReferencedElsewhere("", "drums")); +} + +static void testRemoveAllBanksLatentScope() { + // The latent all-banks seam (fork R-A): unsurfaced in the UI but live at the model + // level. Purges the id from EVERY bank that holds it in one act; fromBankId ignored. + BankBook book; + CHECK(book.createBank("drums", "Drums")); + CHECK(book.createBank("hits", "Hits")); + CHECK(book.pool().index.add(sampleWith("clap", "clap-hash")) == AddResult::Added); + CHECK(book.copySample("id-clap", kPoolBankId, "drums") == TransferResult::Copied); + CHECK(book.copySample("id-clap", kPoolBankId, "hits") == TransferResult::Copied); + // The id now lives in all three banks. + CHECK(book.pool().index.query("id-clap") != nullptr); + CHECK(book.bank("drums")->index.query("id-clap") != nullptr); + CHECK(book.bank("hits")->index.query("id-clap") != nullptr); + + // AllBanks purge — fromBankId is ignored (pass a nonexistent bank to prove it). + CHECK(book.removeSample("id-clap", "ignored-bank", RemoveScope::AllBanks) == + RemoveResult::Removed); + CHECK(book.pool().index.query("id-clap") == nullptr); + CHECK(book.bank("drums")->index.query("id-clap") == nullptr); + CHECK(book.bank("hits")->index.query("id-clap") == nullptr); + + // A second all-banks purge of the now-absent id is an honest no-op rejection. + CHECK(book.removeSample("id-clap", "ignored-bank", RemoveScope::AllBanks) == + RemoveResult::RejectedSampleAbsent); +} + int main() { testPoolSeededAndDefaults(); testPoolPrivileges(); @@ -667,6 +785,12 @@ int main() { testDeserializeCoalescesDuplicateFoldedNames(); testDeserializeCoalescesMultipleCollisions(); testDeserializeNamedBankCollidingWithPoolIsDisambiguated(); + testRemoveDropsTargetEntry(); + testRemoveThisBankLeavesSameHashInAnotherBank(); + testRemoveFromPoolAllowedContainerPrivilegesHold(); + testRemoveRejectionsNoMutation(); + testHashReferencedElsewhere(); + testRemoveAllBanksLatentScope(); if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; From 3514bb0b6de56f859033053a33cd7e5f379960de Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 26 Jul 2026 15:25:15 -0400 Subject: [PATCH 5/6] route bank_panel bank-op persists through persistBankOp so panel gestures get undo points like the bindable actions --- src/actions.cpp | 49 +++++++++++++++++++++++++++++++--------------- src/actions.h | 9 +++++++++ src/bank_panel.cpp | 38 ++++++++++++++++++++++++++--------- 3 files changed, 71 insertions(+), 25 deletions(-) diff --git a/src/actions.cpp b/src/actions.cpp index f639bf5..1beb0d1 100644 --- a/src/actions.cpp +++ b/src/actions.cpp @@ -476,22 +476,6 @@ bool persistBook() { return g_session->saveToActiveProject(); } // rejected op (duplicate name, un-deletable pool, etc.) returns before reaching here, // so no dangling/empty undo point is ever opened for a rejected op. // -// UNSAVED-PROJECT GUARDRAIL: on an unsaved / no-active project persistBook() no-ops -// (nothing is written to ext state). We must still CLOSE the block we opened, but with -// an EMPTY label and a zero flag so REAPER DISCARDS the point instead of recording a -// no-effect undo entry — mirroring view.cpp's empty-plan close. The in-session model -// change stands and persists on the user's next save; it just earns no undo point until -// there is a project to persist into (undo of an unsaved bank op has nothing to roll -// back to anyway). The Begin/End must still be balanced, hence the close-either-way. -void persistBankOp(const char* label) { - Undo_BeginBlock2(nullptr); - const bool persisted = persistBook(); - if (persisted) - Undo_EndBlock2(nullptr, label, UNDO_STATE_MISCCFG); - else - Undo_EndBlock2(nullptr, "", 0); // no ext-state write -> discard the empty point -} - // Prompts the user for a single line of text via REAPER's stock input dialog. // GetUserInputs(title, num_inputs=1, captions_csv, retvals_csv, sz) -> false on // cancel (SDK ~3808). `initial` pre-fills the field. Returns false (leaving `out` @@ -827,6 +811,39 @@ void doBankRemoveSelected() { } // namespace +// Persists a completed bank-index verb as a SINGLE batched REAPER undo point (R-B) — +// one bank op = one Ctrl-Z. Declared in actions.h so bank_panel.cpp can call it +// without duplicating the undo logic. +// +// WHY THIS WRAPS AND persistBook() DOES NOT: a bank verb mutates ONLY our project +// ext-state (SetProjExtState under "reasampler"), which REAPER's undo system captures +// iff UNDO_STATE_MISCCFG is set in the Undo_EndBlock2 flags — the SDK documents +// MISCCFG as covering "extensions!" project ext-state (reaper_plugin.h ~1544, ~1199). +// We pass exactly UNDO_STATE_MISCCFG (not -1 / UNDO_STATE_ALL as the item-move family +// does): a bank verb touches no tracks, FX, items, or envelopes, so snapshotting them +// would be both heavier and semantically wrong. persistBook() (= SetProjExtState) runs +// INSIDE the block so the post-mutation ext-state is the block's "after" image. +// +// NO-OP GUARDRAIL: callers invoke this ONLY after the model mutation succeeded — a +// rejected op (duplicate name, un-deletable pool, etc.) returns before reaching here, +// so no dangling/empty undo point is ever opened for a rejected op. +// +// UNSAVED-PROJECT GUARDRAIL: on an unsaved / no-active project persistBook() no-ops +// (nothing is written to ext state). We must still CLOSE the block we opened, but with +// an EMPTY label and a zero flag so REAPER DISCARDS the point instead of recording a +// no-effect undo entry — mirroring view.cpp's empty-plan close. The in-session model +// change stands and persists on the user's next save; it just earns no undo point until +// there is a project to persist into (undo of an unsaved bank op has nothing to roll +// back to anyway). The Begin/End must still be balanced, hence the close-either-way. +void persistBankOp(const char* label) { + Undo_BeginBlock2(nullptr); + const bool persisted = persistBook(); + if (persisted) + Undo_EndBlock2(nullptr, label, UNDO_STATE_MISCCFG); + else + Undo_EndBlock2(nullptr, "", 0); // no ext-state write -> discard the empty point +} + void bankRegisterActions(reaper_plugin_info_t* rec, ReaSamplerSession* session) { g_session = session; // shared with the Design View family; same live session diff --git a/src/actions.h b/src/actions.h index 44bf442..ebe0eab 100644 --- a/src/actions.h +++ b/src/actions.h @@ -66,4 +66,13 @@ bool bankHandleCommand(int command); // rec==nullptr (before g_session is torn down). void bankUnregisterActions(reaper_plugin_info_t* rec); +// Persists a completed bank-index verb as a single REAPER undo point (R-B). +// Wraps persistBook() (= SetProjExtState) in a Begin/End block with UNDO_STATE_MISCCFG +// so the bank op is one Ctrl-Z. On an unsaved / no-active project persistBook() no-ops +// and the block is closed with an empty label + zero flag (REAPER discards it). Callers +// must invoke this ONLY after a successful/effective mutation — rejected ops (duplicate +// name, un-deletable pool, etc.) must return before reaching here so no empty undo +// point is ever opened for a no-op. Defined in actions.cpp alongside persistBook(). +void persistBankOp(const char* label); + } // namespace reasampler diff --git a/src/bank_panel.cpp b/src/bank_panel.cpp index 73f4761..c7327c8 100644 --- a/src/bank_panel.cpp +++ b/src/bank_panel.cpp @@ -46,6 +46,7 @@ #include #include +#include "actions.h" // persistBankOp — shared undo-block wrapper (R-B panel path) #include "bank_book.h" #include "bank_grid.h" #include "bank_model.h" @@ -1224,7 +1225,7 @@ void doCreateBank() { } g_panel.shownBankId = id; // show the freshly-created bank g_panel.focusedRegion = Region::Banks; - persistBook(); + persistBankOp("ReaSampler: create bank"); invalidatePanel(); } @@ -1240,7 +1241,7 @@ void doRenameBank(const std::string& bankId) { "ReaSampler: rename bank", 0); return; } - persistBook(); + persistBankOp("ReaSampler: rename bank"); invalidatePanel(); } @@ -1273,7 +1274,7 @@ void doDeleteBank(const std::string& bankId) { // r == 6 (Yes) falls through to a plain delete (drops members). } if (!book()->deleteBank(bankId)) return; - persistBook(); + persistBankOp("ReaSampler: delete bank"); // shownBankId is reconciled by the next fingerprint pass. If no named banks remain, // nudge focus to the pool so the selection has a valid home. if (namedBanks().empty()) g_panel.focusedRegion = Region::Pool; @@ -1285,30 +1286,49 @@ void doEvacuateBank(const std::string& bankId) { const Bank* bk = book()->bank(bankId); if (!bk || bk->isPool()) return; if (!book()->evacuate(bankId)) return; - persistBook(); + persistBankOp("ReaSampler: evacuate bank"); invalidatePanel(); } void doActivateBank(const std::string& bankId) { if (!book()) return; if (!book()->setActiveBank(bankId)) return; // rejects an unknown id - persistBook(); + persistBankOp("ReaSampler: activate bank"); invalidatePanel(); } // Move or copy `sampleIds` from `srcBankId` to `destBankId` (index-only). Both pass // ids straight to the model op (no BankIndex& cached across the loop's mutations). +// +// NO-OP GUARDRAIL — VERB-AWARE (matches the action layer's doBankTransferSelected): +// * MOVE collapse: the source entry WAS removed (bank_book removes unconditionally +// before the dest add collapses on hash), so the index DID mutate — counts. +// * COPY collapse: the source is left intact AND the dest already held the hash, +// so NOTHING changed — a true index no-op. Must NOT open an undo point. +// Hence: copy counts only real gains (Copied); move counts gains OR collapses. void transferSamples(const std::vector& sampleIds, const std::string& srcBankId, const std::string& destBankId, bool copy) { if (!book()) return; if (sampleIds.empty() || srcBankId == destBankId) return; if (!book()->bank(srcBankId) || !book()->bank(destBankId)) return; + int ok = 0, collapsed = 0; for (const std::string& sid : sampleIds) { - if (copy) book()->copySample(sid, srcBankId, destBankId); - else book()->moveSample(sid, srcBankId, destBankId); + const TransferResult r = + copy ? book()->copySample(sid, srcBankId, destBankId) + : book()->moveSample(sid, srcBankId, destBankId); + switch (r) { + case TransferResult::Moved: + case TransferResult::Copied: ++ok; break; + case TransferResult::Collapsed: ++collapsed; break; + default: break; + } } - persistBook(); + const bool mutated = copy ? (ok > 0) : (ok > 0 || collapsed > 0); + if (!mutated) return; // nothing changed — no persist, no undo point + + const char* label = copy ? "ReaSampler: copy sample(s)" : "ReaSampler: move sample(s)"; + persistBankOp(label); // The selection indexed into the source; after a move those indices are stale, so // clear it (the fingerprint pass will also clear, but do it now for immediacy). g_panel.selection = Selection{}; @@ -1357,7 +1377,7 @@ void removeSamples(const std::vector& sampleIds, ++removed; if (removed == 0) return; // nothing changed — no persist, no undo point - persistBook(); + persistBankOp("ReaSampler: remove sample(s)"); // The selection indexed into the source; after a remove those indices are stale, so // clear it (the fingerprint pass will also clear, but do it now for immediacy). g_panel.selection = Selection{}; From 704a7b5ffcbba552f0e02668a57228d224f87fd2 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 26 Jul 2026 15:32:22 -0400 Subject: [PATCH 6/6] chore(bank_panel): clean up persistBook dead code and stale comment; align transfer switch to explicit enum cases --- src/bank_panel.cpp | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/bank_panel.cpp b/src/bank_panel.cpp index c7327c8..37219d0 100644 --- a/src/bank_panel.cpp +++ b/src/bank_panel.cpp @@ -1180,14 +1180,11 @@ bool regionAt(int x, int y, Region& out) { // --- Bank management ops (id-keyed; drive the B1 model + persist) -------------- // -// Each op mutates g_session.book() then persists via saveToActiveProject(). After a +// Each op mutates g_session.book() then persists via persistBankOp(). After a // STRUCTURAL mutation (create/delete/evacuate) any Bank*/BankIndex& is invalid — we -// resolve fresh, pass ids, and let the next refreshFingerprint repaint. persistBook -// no-ops on an unsaved project (matches the capture/B3 quiet-persist idiom). - -void persistBook() { - if (g_panel.session) g_panel.session->saveToActiveProject(); -} +// resolve fresh, pass ids, and let the next refreshFingerprint repaint. On an +// unsaved project the empty-close discard in persistBankOp ensures no stale state +// survives (matches the capture/B3 quiet-persist idiom). // REAPER's stock single-line input (comma-safe via the \x1f return separator, as B3). bool promptText(const char* title, const char* caption, const std::string& initial, @@ -1319,9 +1316,11 @@ void transferSamples(const std::vector& sampleIds, : book()->moveSample(sid, srcBankId, destBankId); switch (r) { case TransferResult::Moved: - case TransferResult::Copied: ++ok; break; - case TransferResult::Collapsed: ++collapsed; break; - default: break; + case TransferResult::Copied: ++ok; break; + case TransferResult::Collapsed: ++collapsed; break; + case TransferResult::RejectedUnknownBank: + case TransferResult::RejectedSampleAbsent: + case TransferResult::RejectedSameBank: break; } } const bool mutated = copy ? (ok > 0) : (ok > 0 || collapsed > 0);