fix(banks): enforce unique bank display names in-model + B3 review minors

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

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.

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<char> 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<int>(buf.size())))
+    const std::string captions = std::string(caption) + ",separator=\x1f";
+    if (!GetUserInputs(title, 1, captions.c_str(), buf.data(), static_cast<int>(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);

src/bank_book.cpp

@@ -77,6 +77,43 @@ void BankBook::normalizeOrdinals() {
         banks_[i].ordinal = static_cast<int>(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<char>(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;
 }

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<Bank> 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).

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();