From ccf65415c94db3b774b065e58e80e63388e9ce1a Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sat, 25 Jul 2026 01:29:47 -0400 Subject: [PATCH] fix(banks): enforce unique bank display names in-model + B3 review minors --- CONTEXT.md | 12 +++++++-- docs/product/multi-bank.md | 13 ++++++--- src/actions.cpp | 54 +++++++++++++++++++++++++++++--------- src/bank_book.cpp | 46 +++++++++++++++++++++++++++++++- src/bank_book.h | 13 +++++++-- tests/test_bank_book.cpp | 53 +++++++++++++++++++++++++++++++++++++ 6 files changed, 169 insertions(+), 22 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 465d863..c97e120 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -547,7 +547,13 @@ arrange; the only change is *which* index the entry lands in. display name, ordinal, BankIndex }`. **`BankIndex` is untouched** — the multi-bank layer wraps it, it does not modify it (additive; no `bank-id` field on `Sample`). Bank id is the stable key (GUID-style, minted on bank create); display name and - ordinal are mutable (rename / reorder). The pool is the first, seeded, fixed-id + ordinal are mutable (rename / reorder). **Display names are unique**, enforced in the + pure model on create and rename: `createBank` / `renameBank` reject a name that + duplicates an existing bank's (renaming a bank to its own current name is a no-op + success). The comparison is **trimmed + case-insensitive (ASCII)**, so "Drums", + "drums", and " Drums " cannot coexist; the pool's reserved name "Pool" is protected + by the same check. Uniqueness makes by-name resolution in the action shell + unambiguous by construction. The pool is the first, seeded, fixed-id member. `bank_book` is the mirror of `bank_model` and `view_mode_model`: pure, no REAPER types, unit-tested outside the DAW, JSON round-trip. - **Active bank lives in the model, routes through the capture path.** `bank_book` @@ -637,7 +643,9 @@ Pure (no REAPER types, unit-tested — the mirror of `bank_model` / `view_mode_m - `bank_book` — ordered bank registry (`{ bank id, display name, ordinal, BankIndex }`); pool seeded with fixed id + name; create / rename / reorder / delete named banks (pool-privilege rules enforced here: reject delete/rename of - pool; delete drops member index entries); **evacuate** a bank (move every member to + pool; delete drops member index entries; **display names unique** — create/rename + reject a name that duplicates another bank's, trimmed + case-insensitive, "Pool" + protected); **evacuate** a bank (move every member to the pool, index-only, destination-collapse observed; pool cannot be evacuated); active-bank id (get/set, defaults to pool); **move** and **copy** a sample between banks (index-only, destination-collapse observed); query a bank's index; JSON diff --git a/docs/product/multi-bank.md b/docs/product/multi-bank.md index 9f7bc63..e8702da 100644 --- a/docs/product/multi-bank.md +++ b/docs/product/multi-bank.md @@ -90,9 +90,12 @@ reasons: So: `bank_book` is an ordered registry of `{ bank id, display name, ordinal, BankIndex }`, pool seeded as bank-zero. Bank id is the stable key (minted GUID-style -on create); name and ordinal are mutable. `BankIndex` is untouched. This is the -defer-the-feature, design-the-seam principle: the seam is a container above the -tested core, not a modification of it. +on create); name and ordinal are mutable. Display names are **unique** — two banks +cannot share a name (compared trimmed + case-insensitively, so "Drums" and "drums" +are the same name), enforced in the model on create and rename; the pool's "Pool" is +reserved by the same rule. `BankIndex` is untouched. This is the defer-the-feature, +design-the-seam principle: the seam is a container above the tested core, not a +modification of it. --- @@ -349,7 +352,9 @@ Mirrors the capture and Design View pillars exactly. - Ordered bank registry: `{ bank id, display name, ordinal, BankIndex }`; pool seeded with fixed id + fixed name. - Create / rename / reorder / delete named banks; pool-privilege rules enforced - here (reject delete-pool, reject rename-pool, never zero banks). + here (reject delete-pool, reject rename-pool, never zero banks). Display names are + unique — create/rename reject a name already used by another bank (trimmed + + case-insensitive; the pool's "Pool" is protected). - Active-bank id (get/set, defaults to pool); resolve the active bank's `BankIndex`. - Move / copy a sample between banks — index-only, destination collapse-by-hash observed, move removes the source entry. diff --git a/src/actions.cpp b/src/actions.cpp index 2a36c0e..24bf7ab 100644 --- a/src/actions.cpp +++ b/src/actions.cpp @@ -331,11 +331,14 @@ gaccel_register_t g_accelBankCopySel{}; gaccel_register_t g_accelBankPoolFull{}; gaccel_register_t g_accelBankBanksFull{}; -// Persists the book after a bank mutation. Mirrors the capture path (main.cpp -// RunCapture): a bank change is held in-session and written to the active project's -// ext state so it travels with the .rpp. No Save-As prompt here — saveToActiveProject -// no-ops on an unsaved project (the change stays valid for the session and persists -// on the user's next save), matching how capture persists. +// Persists the book after a bank mutation. Mirrors the CAPTURE path (main.cpp +// RunCapture), NOT the Design-View path: a bank change is held in-session and written +// to the active project's ext state so it travels with the .rpp. Deliberately no +// Save-As prompt — saveToActiveProject no-ops on an unsaved project (the change stays +// valid for the session and persists on the user's next save), exactly as capture +// 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(); } // Prompts the user for a single line of text via REAPER's stock input dialog. @@ -343,12 +346,21 @@ void persistBook() { g_session->saveToActiveProject(); } // cancel (SDK ~3808). `initial` pre-fills the field. Returns false (leaving `out` // untouched) on cancel or an empty entry. Self-contained bindable-action name entry; // B4's panel affordances supersede this with in-panel editing. +// +// COMMA GUARD: GetUserInputs splits the returned values on a separator that defaults +// to ',', so a bank name containing a comma would be truncated at the comma. We +// override the return separator to \x1f (ASCII unit separator, un-typeable in the +// dialog) via the documented `separator=X` extra caption field (SDK ~3806), so any +// printable name — commas included — round-trips whole. The captions_csv itself stays +// comma-joined: the single field caption, then the `separator=` directive as a +// trailing pseudo-caption (the directive redefines only the RETURN separator). bool promptText(const char* title, const char* caption, const std::string& initial, std::string& out) { std::vector buf(512, '\0'); // Pre-fill: GetUserInputs seeds the field from the retvals buffer's initial value. std::snprintf(buf.data(), buf.size(), "%s", initial.c_str()); - if (!GetUserInputs(title, 1, caption, buf.data(), static_cast(buf.size()))) + const std::string captions = std::string(caption) + ",separator=\x1f"; + if (!GetUserInputs(title, 1, captions.c_str(), buf.data(), static_cast(buf.size()))) return false; // user cancelled std::string s(buf.data()); if (s.empty()) return false; // an empty name is not a valid bank name @@ -368,9 +380,11 @@ std::string mintBankId() { } // Resolves a user-typed bank reference (a display name) to a bank id, scanning the -// book's banks in ordinal order. Case-sensitive exact match on displayName; "Pool" -// resolves the pool. Returns "" when no bank carries that name. Kept in the action -// layer (not the model) — it is UI name-resolution, not a model rule. +// book's banks in ordinal order. Exact match on displayName; "Pool" resolves the pool. +// Returns "" when no bank carries that name. Kept in the action layer (not the model) +// — it is UI name-resolution, not a model rule. First-match is unambiguous BY +// CONSTRUCTION: the model enforces unique display names (trimmed + case-insensitive), +// so at most one bank can carry a given name — no duplicate can shadow another here. std::string bankIdByDisplayName(const std::string& name) { for (const Bank& b : g_session->book().banks()) if (b.displayName == name) return b.id; @@ -381,14 +395,18 @@ std::string bankIdByDisplayName(const std::string& name) { // Create a named bank: prompt for a display name, mint a stable GUID id, create it in // the model, persist. The new bank is NOT auto-activated (create and activate are -// distinct acts — mirrors capture/placement separation). A duplicate-name is allowed -// (display names are not unique in the model); the fresh GUID keeps the id unique. +// distinct acts — mirrors capture/placement separation). The model rejects a display +// name that duplicates an existing bank's (trimmed + case-insensitive, incl. "Pool"); +// the create then fails and the user is told the name is taken. void doBankCreate() { std::string name; if (!promptText("ReaSampler: create bank", "Bank name:", "", name)) return; const std::string id = mintBankId(); if (!g_session->book().createBank(id, name)) { - ShowConsoleMsg("ReaSampler: could not create bank (id collision — try again).\n"); + ShowConsoleMsg( + ("ReaSampler: could not create bank \"" + name + + "\" (a bank with that name already exists).\n") + .c_str()); return; } persistBook(); @@ -411,7 +429,10 @@ void doBankRename() { std::string newName; if (!promptText("ReaSampler: rename bank", "New name:", which, newName)) return; if (!g_session->book().renameBank(id, newName)) { - ShowConsoleMsg("ReaSampler: cannot rename that bank (the pool is un-renamable).\n"); + // renameBank rejects the pool (un-renamable) or a name already used by another + // bank (unique display names, trimmed + case-insensitive). + ShowConsoleMsg("ReaSampler: cannot rename that bank (the pool is un-renamable, " + "or another bank already uses that name).\n"); return; } persistBook(); @@ -432,6 +453,13 @@ void doBankDelete() { ShowConsoleMsg(("ReaSampler: no bank named \"" + which + "\".\n").c_str()); return; } + // Pool early-out: the pool is un-deletable (the model rejects it). Catch it here, + // BEFORE the non-empty confirm, so typing "Pool" never shows a misleading + // "delete anyway?" prompt for an operation the model will refuse regardless. + if (id == kPoolBankId) { + ShowConsoleMsg("ReaSampler: the pool cannot be deleted.\n"); + return; + } // Read member count BEFORE deleting (the Bank* is invalidated by deleteBank; we do // not cache it — resolve size to an int up front). const Bank* b = g_session->book().bank(id); diff --git a/src/bank_book.cpp b/src/bank_book.cpp index 4fc4901..8f35d1d 100644 --- a/src/bank_book.cpp +++ b/src/bank_book.cpp @@ -77,6 +77,43 @@ void BankBook::normalizeOrdinals() { banks_[i].ordinal = static_cast(i); } +// --------------------------------------------------------------------------- +// Display-name uniqueness (trimmed + case-insensitive, ASCII) +// --------------------------------------------------------------------------- + +namespace { + +// Folds a display name to its uniqueness key: strip leading/trailing ASCII +// whitespace, lower-case ASCII letters. So "Drums", "drums", and " Drums " share one +// key and cannot coexist. ASCII-only by design — the pure core carries no locale +// facility and must not grow one; bank names are short user labels, not full Unicode +// case-folding candidates. +std::string nameKey(const std::string& s) { + std::size_t b = 0, e = s.size(); + auto isWs = [](char c) { return c == ' ' || c == '\t' || c == '\n' || c == '\r'; }; + while (b < e && isWs(s[b])) ++b; + while (e > b && isWs(s[e - 1])) --e; + std::string out; + out.reserve(e - b); + for (std::size_t i = b; i < e; ++i) { + char c = s[i]; + if (c >= 'A' && c <= 'Z') c = static_cast(c - 'A' + 'a'); + out += c; + } + return out; +} + +} // namespace + +// True if any bank OTHER than `exceptId` already carries `name`'s uniqueness key. The +// exception lets renameBank accept a bank keeping (or re-casing/-spacing) its own name. +bool BankBook::displayNameTaken(const std::string& name, const std::string& exceptId) const { + const std::string key = nameKey(name); + for (const auto& b : banks_) + if (b.id != exceptId && nameKey(b.displayName) == key) return true; + return false; +} + // --------------------------------------------------------------------------- // Bank lifecycle // --------------------------------------------------------------------------- @@ -84,7 +121,10 @@ void BankBook::normalizeOrdinals() { bool BankBook::createBank(const std::string& id, const std::string& displayName) { if (id.empty()) return false; // ids key the registry if (id == kPoolBankId) return false; // reserved pool id - if (bank(id) != nullptr) return false; // duplicate + if (bank(id) != nullptr) return false; // duplicate id + // Display names are unique (trimmed + case-insensitive); the pool's "Pool" is a + // reserved name and is caught here like any other collision. + if (displayNameTaken(displayName, /*exceptId=*/id)) return false; Bank b; b.id = id; @@ -99,6 +139,10 @@ bool BankBook::renameBank(const std::string& id, const std::string& displayName) if (id == kPoolBankId) return false; // pool is un-renamable Bank* b = bank(id); if (b == nullptr) return false; + // Reject a name already used by a DIFFERENT bank. Renaming a bank to its own + // current name (or a case/space variant of it) is a no-op success, not a + // rejection — exceptId=id excludes the bank itself from the collision scan. + if (displayNameTaken(displayName, /*exceptId=*/id)) return false; b->displayName = displayName; return true; } diff --git a/src/bank_book.h b/src/bank_book.h index 5b9b8e7..2a6084b 100644 --- a/src/bank_book.h +++ b/src/bank_book.h @@ -97,10 +97,14 @@ public: // Creates a named bank with the caller-supplied stable id and display name, // assigning the next ordinal. Rejects (returns false, no mutation) an empty id, - // a duplicate id, or the reserved pool id. Display name is not required unique. + // a duplicate id, the reserved pool id, or a display name that duplicates an + // existing bank's name (including the pool's "Pool"). Display-name uniqueness is + // trimmed + case-insensitive (ASCII): "Drums", "drums", and " Drums " collide. bool createBank(const std::string& id, const std::string& displayName); - // Renames a named bank. Rejects (false, no mutation) an unknown id or the pool. + // Renames a named bank. Rejects (false, no mutation) an unknown id, the pool, or a + // target name already used by a DIFFERENT bank (trimmed + case-insensitive, as + // createBank). Renaming a bank to its own current name is a no-op success. bool renameBank(const std::string& id, const std::string& displayName); // Deletes a NAMED bank, removing it (and its member index entries) from the @@ -210,6 +214,11 @@ private: std::vector banks_; // ordinal order; banks_[0] is always the pool std::string activeBankId_; // always names a live bank; defaults to pool + // True if a bank OTHER than `exceptId` already carries `name`'s uniqueness key + // (trimmed + case-insensitive, ASCII). Backs the create/rename uniqueness check; + // pass exceptId=id to let a bank keep (or re-case/-space) its own name. + bool displayNameTaken(const std::string& name, const std::string& exceptId) const; + // Re-sorts banks_ by ordinal (pool pinned first) and rewrites ordinals to a // contiguous 0..N-1 so the pool is 0 and named banks are 1..N. Called after any // structural change (create / delete / reorder). diff --git a/tests/test_bank_book.cpp b/tests/test_bank_book.cpp index 24e3788..5c96e22 100644 --- a/tests/test_bank_book.cpp +++ b/tests/test_bank_book.cpp @@ -92,6 +92,8 @@ static void testCreateRenameReorder() { CHECK(book.renameBank("b", "Beta-renamed")); CHECK(book.bank("b")->displayName == "Beta-renamed"); CHECK(!book.renameBank("missing", "x")); + // Renaming back to a non-colliding name keeps working. + CHECK(book.renameBank("b", "Beta")); // Reorder: move "c" to the front of the named region (ordinal 1). CHECK(book.reorderBank("c", 1)); @@ -113,6 +115,56 @@ static void testCreateRenameReorder() { CHECK(!book.reorderBank("missing", 1)); } +static void testDisplayNameUniqueness() { + BankBook book; + CHECK(book.createBank("a", "Drums")); + + // A unique name is accepted. + CHECK(book.createBank("b", "Bass")); + + // Exact duplicate rejected, no mutation (size unchanged, the collided id absent). + CHECK(!book.createBank("c", "Drums")); + CHECK(book.bank("c") == nullptr); + CHECK(book.size() == 3); // pool + a + b only + + // Trimmed + case-insensitive collisions: "drums", " Drums ", "DRUMS" all collide. + CHECK(!book.createBank("c", "drums")); + CHECK(!book.createBank("c", " Drums ")); + CHECK(!book.createBank("c", "DRUMS")); + CHECK(book.bank("c") == nullptr); + + // The pool's reserved name "Pool" (and its variants) cannot be taken by a new bank. + CHECK(!book.createBank("c", "Pool")); + CHECK(!book.createBank("c", " pool ")); + CHECK(book.bank("c") == nullptr); + + // -- renameBank uniqueness -------------------------------------------------- + // Rename to a name used by ANOTHER bank is rejected (no mutation). + CHECK(!book.renameBank("b", "Drums")); + CHECK(book.bank("b")->displayName == "Bass"); // unchanged + CHECK(!book.renameBank("b", "drums")); // case-insensitive collision too + CHECK(!book.renameBank("b", " Drums ")); // trimmed collision too + + // Renaming a bank to its OWN current name is a no-op success (not a rejection). + CHECK(book.renameBank("a", "Drums")); + CHECK(book.bank("a")->displayName == "Drums"); + // Re-casing/-spacing its own name is likewise allowed (it collides only with self). + CHECK(book.renameBank("a", " drums ")); + CHECK(book.bank("a")->displayName == " drums "); + + // Renaming to the pool's reserved name is rejected (pool is the "other" bank here). + CHECK(!book.renameBank("b", "Pool")); + CHECK(book.bank("b")->displayName == "Bass"); + + // A genuinely fresh unique name still renames fine. + CHECK(book.renameBank("b", "Low End")); + CHECK(book.bank("b")->displayName == "Low End"); + + // After the rejections, the previously-freed name is now reusable by a new bank. + CHECK(book.createBank("c", "Bass")); + CHECK(book.bank("c")->displayName == "Bass"); +} + static void testMoveSourceLosesDestGains() { BankBook book; CHECK(book.createBank("drums", "Drums")); @@ -473,6 +525,7 @@ int main() { testPoolSeededAndDefaults(); testPoolPrivileges(); testCreateRenameReorder(); + testDisplayNameUniqueness(); testMoveSourceLosesDestGains(); testCopySourceRetainedDestGains(); testMoveDestCollapse();