From ff4eb26eab364828ed09b8f495137a0ea4c065eb Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 26 Jul 2026 17:27:05 -0400 Subject: [PATCH] ux: drop last-reference confirm from sample remove; silent remove via undo (R-B) --- COMPLETED.md | 9 +++---- CONTEXT.md | 11 ++++----- docs/product/removal-and-prune.md | 17 ++++++------- src/actions.cpp | 41 +++++-------------------------- src/bank_panel.cpp | 38 ++++++---------------------- 5 files changed, 30 insertions(+), 86 deletions(-) diff --git a/COMPLETED.md b/COMPLETED.md index a6d3ae2..f10bc18 100644 --- a/COMPLETED.md +++ b/COMPLETED.md @@ -947,9 +947,8 @@ timeline item). stays a latent seam-only parameter, not shipped). - [x] `bank_panel` remove affordance on the current selection (reuse M5 selection model, as move/copy do). -- [x] Confirm-on-last-reference guardrail: remove that orphans a file (no other - bank references it) confirms, naming the orphaned-until-prune consequence; - remove of a still-referenced sample does not confirm. +- [x] Silent remove: no confirm dialog; recoverability via batched REAPER undo + (R-B) — one Ctrl-Z restores the index entry; files are never deleted by remove. - [x] Tests: remove drops the target entry; same-hash entry in another bank survives; remove-from-pool allowed; last-reference remove leaves an orphan (file untouched); non-destructive (no file/timeline mutation). @@ -958,8 +957,8 @@ timeline item). - **R-A — remove scope.** Settled: **this-bank**. Removes the entry from the bank in view only; the `scope: this-bank | all-banks` seam stays in the action signature but all-banks is a latent parameter, not a surfaced verb. -- `hashReferencedElsewhere` cross-bank reference query added to `bank_book` to - support the confirm-on-last-reference guardrail without coupling to the panel. +- `hashReferencedElsewhere` cross-bank reference query in `bank_book` retained as + a tested model API for Phase R prune; the remove shells no longer call it. - Both `bank_panel` context menu ("Remove selected sample(s)") and Delete key affordance wired; panel gesture is also one batched undo point (R-B applies). diff --git a/CONTEXT.md b/CONTEXT.md index 5efd279..0114392 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -797,12 +797,11 @@ model capability. ## Guardrails -- **Confirm on last-reference remove; don't confirm otherwise.** A remove that - drops the *last* index reference to a file orphans it (until prune) — confirm - that case, naming the consequence ("…its file remains on disk until pruned"). A - remove of a sample still referenced by another bank is cheap and re-derivable - (re-copy it back) and needs no confirmation. The confirmation is *earned by - actual orphan risk*, not fired on every remove. +- **Removes are silent — no confirm dialog.** Recoverability is provided by the + batched REAPER undo (R-B): one Ctrl-Z restores the index entry, whether or not + the sample was a last reference. Files are never deleted by remove + (orphaned-until-prune is unchanged). `hashReferencedElsewhere` is a tested + model API retained for Phase R prune; it has no shell caller in the remove path. - **Undo (fork R-B, SETTLED 2026-07-24 — batched REAPER undo points, Phase-B-wide).** Bank/index mutations integrate into REAPER's undo system as **batched undo points** (`Undo_BeginBlock` / `Undo_EndBlock`): the related index mutations of one bank diff --git a/docs/product/removal-and-prune.md b/docs/product/removal-and-prune.md index e5dc99b..7758d9f 100644 --- a/docs/product/removal-and-prune.md +++ b/docs/product/removal-and-prune.md @@ -95,14 +95,13 @@ the bytes survive on disk until an explicit prune. So the recovery story is: an accidental remove loses the *index entry*, not the audio. But there are two sharpnesses to guard: -- **Last-reference remove is the orphan-maker.** Removing a sample that exists in - only one bank orphans its file (until prune). This is the same footgun as - non-empty delete-bank, and it deserves the same treatment: **confirm when the - remove drops the last index reference** ("Remove 'kick_03'? It is in no other - bank — its file will remain on disk until pruned."). A remove of a sample that - still lives in another bank is cheap and reversible-in-spirit (re-copy it back) - and need not confirm. This makes the confirmation *earned* by actual risk rather - than fired on every remove. +- **Removes are silent — no confirm dialog.** Recoverability is provided by the + batched REAPER undo (R-B): one Ctrl-Z restores the index entry. A last-reference + remove does orphan the file (no other bank holds it), but the file stays on disk + until an explicit prune — remove never deletes bytes. The undo path makes the + confirm unnecessary; the file-safety guarantee (orphaned-until-prune) remains + unchanged. `hashReferencedElsewhere` is a tested model API retained for Phase R + prune; it has no shell caller in the remove path. - **Undo.** REAPER's own undo stack does not natively cover ext-state index mutations, so this is a Phase-B-wide decision (fork R-B, **settled**): bank/index mutations integrate into REAPER's undo system as **batched undo points** @@ -361,4 +360,4 @@ settled spec (explicitly out of scope). A periodic/background sweep remains reje | Deletes bytes? | no | no | **yes (only op that does)** | | Produces orphans? | yes (last-ref) | yes (non-empty) | — (it *reclaims* them) | | Reversible? | Ctrl-Z (batched undo, R-B) / re-capture | Ctrl-Z (batched undo, R-B) / re-create | **no in-app** (recoverable via OS trash, R-C) | -| Guardrail | confirm on last-ref | confirm on non-empty | dry-run + manifest confirm | +| Guardrail | silent (Ctrl-Z restores; files never deleted by remove) | confirm on non-empty | dry-run + manifest confirm | diff --git a/src/actions.cpp b/src/actions.cpp index f1d856e..ec998ab 100644 --- a/src/actions.cpp +++ b/src/actions.cpp @@ -758,12 +758,10 @@ void doBankTransferSelected(bool copy) { // 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). +// SILENT REMOVE: removes proceed without a confirm dialog. Recoverability is provided +// by the batched REAPER undo (R-B) — one Ctrl-Z restores the index entry. Files are +// never deleted by remove (orphaned-until-prune is unchanged). hashReferencedElsewhere +// is a tested model API retained for Phase R prune; it has no shell caller here. void doBankRemoveSelected() { const std::vector selected = bankPanelSelectedSampleIds(); if (selected.empty()) { @@ -772,41 +770,14 @@ void doBankRemoveSelected() { } const std::string srcId = bankPanelSelectedSourceBankId(); BankBook& book = g_session->book(); - const Bank* src = book.bank(srcId); - if (src == nullptr) { + if (book.bank(srcId) == 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). The per-outcome - // console summary was dropped (m11 chatter policy); only the "did anything change?" - // signal the undo guardrail needs is retained. + // undo point when nothing was removed (every id was already absent). int removed = 0; for (const std::string& sampleId : selected) { if (book.removeSample(sampleId, srcId, RemoveScope::ThisBank) == diff --git a/src/bank_panel.cpp b/src/bank_panel.cpp index 40acab0..2e798f9 100644 --- a/src/bank_panel.cpp +++ b/src/bank_panel.cpp @@ -1365,38 +1365,14 @@ void transferSamples(const std::vector& sampleIds, // 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. +// prune — remove NEVER deletes bytes (the manifest is untouched). Removes are silent +// (no confirm dialog); recoverability is provided by the batched REAPER undo (R-B) — +// one Ctrl-Z restores the index entry. 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; - } + if (!book()->bank(srcBankId)) return; int removed = 0; for (const std::string& sid : sampleIds) @@ -1771,8 +1747,8 @@ bool handleKey(int vk) { 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. + // Remove the focused-region selection (B5). Silent; 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));