Merge R-B: batched undo on bank ops + projectconfig reload-on-undo hook

src/actions.cpp

@@ -453,7 +453,41 @@ 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.
+//
+// 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
+}
 
 // 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
@@ -523,7 +557,7 @@ void doBankCreate() {
                 .c_str());
         return;
     }
-    persistBook();
+    persistBankOp("ReaSampler: create bank");
     ShowConsoleMsg(("ReaSampler: created bank \"" + name + "\".\n").c_str());
 }
 
@@ -549,7 +583,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 +627,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 +649,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 +664,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 +675,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 +726,19 @@ void doBankTransferSelected(bool copy) {
             case TransferResult::RejectedSameBank: break;
         }
     }
-    persistBook();
+    // 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());
+    }
     std::string log = std::string("ReaSampler: ") + verb + " -> \"" + destName +
                       "\": " + std::to_string(ok) + " " + verb + "d";
     if (collapsed) log += ", " + std::to_string(collapsed) + " collapsed on hash";

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 <EXTSTATE> 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 <EXTSTATE> 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

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
+//   projectconfig .rpp-line hook for STORAGE), and the timer composes cleanly with
-//   while covering both load and Save-As detection in one place. The
+//   ext-state while covering identity-transition load + Save-As detection in one
-//   `projectconfig` BeginLoadProjectState hook is a deterministic alternative for
+//   place.
-//   pure load detection but would still need the timer (or Main_SaveProject
+//
-//   post-hook) for Save-As path-change detection — surfaced in the handoff.
+//   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 <EXTSTATE> 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 (!proj) return false;           // no active project — nothing to persist
-    if (rppPath.empty()) return;       // unsaved project — no .rpp to store into
+    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<ReaProject*>(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
+    // <EXTSTATE> 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;
     }
 

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
+    // <EXTSTATE> 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