From 72b870459ceb33cbc32275bff909ef306255fb01 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sat, 1 Aug 2026 19:43:27 -0400 Subject: [PATCH 1/2] =?UTF-8?q?Publish=20the=20Media=20Explorer=20import?= =?UTF-8?q?=20into=20REAPER's=20Media=20Explorer=20action=20section=20as?= =?UTF-8?q?=20well=20as=20Main=20=E2=80=94=20custom=5Faction=20+=20hookcom?= =?UTF-8?q?mand2,=20second=20forever-stable=20id,=20one=20handler.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CLAUDE.md | 1 + src/app/main.cpp | 16 +++++++++-- src/shell/actions/CLAUDE.md | 2 +- src/shell/actions/ingest.cpp | 55 +++++++++++++++++++++++++++++------- src/shell/actions/ingest.h | 17 +++++++++-- tests/test_app_version.cpp | 42 +++++++++++++++++++++++++++ 6 files changed, 118 insertions(+), 15 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 985cf9b..e8be300 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -181,6 +181,7 @@ Comments carry *why*, and context where non-obvious — never *what* the code al 2. `rec->Register("gaccel", &accel)` — puts the action in the Actions list. 3. `rec->Register("hookcommand", ...)` — receives every action fired; claim only your own id, return `false` otherwise. 4. On unload (`rec == nullptr`), mirror-unregister everything with the same strings prefixed by `'-'`. +- **Non-main sections use a different mechanism.** `gaccel_register_t` carries no section field — `command_id` + `gaccel` can only ever produce a Main-section action. To publish into another section (Media Explorer = 32063, MIDI editor = 32060, MIDI event list = 32061, MIDI inline = 32062), register a `custom_action_register_t{uniqueSectionId, idStr, name, extra}` under `"custom_action"`; it returns the command id, or **0 on failure** (e.g. a duplicate `idStr`) — which the caller must tolerate rather than half-register. `idStr` must be unique **across all sections**, so an action published into both Main and a non-main section needs a SECOND id string; the FOREVER-STABLE contract binds it identically from the moment it ships. `custom_action_register_t` has no `ACCEL`, so a non-main entry ships no default keybinding. Dispatch for these ids arrives through `"hookcommand2"` (`bool(KbdSectionInfo*, int command, int val, int val2, int relmode, HWND)`) — `"hookcommand"` runs for the main section only. The two hooks must partition the ids between them: claiming one command in both double-fires it. On unload, mirror with `"-custom_action"` and `"-hookcommand2"`. ## Product design docs diff --git a/src/app/main.cpp b/src/app/main.cpp index 18929ec..398451e 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -216,8 +216,8 @@ static project_config_extension_t g_projectConfig{ nullptr, // userData }; -// REAPER calls this for EVERY action fired anywhere; claim only our own id, return -// false otherwise so REAPER keeps looking. This TU's own family dispatches through +// REAPER calls this for every action fired in the MAIN section; claim only our own id, +// return false otherwise so REAPER keeps looking. This TU's own family dispatches through // the registration table; the other families claim their own ids after it. static bool OnHookCommand(int command, int /*flag*/) { @@ -229,6 +229,16 @@ static bool OnHookCommand(int command, int /*flag*/) return false; } +// "hookcommand" covers the main section only, so actions we published into another +// section arrive here instead. Claim ONLY those — every main-section id stays with +// OnHookCommand, so no command can be claimed by both hooks. +static bool OnHookCommand2(KbdSectionInfo* /*sec*/, int command, int /*val*/, int /*val2*/, + int /*relmode*/, HWND /*hwnd*/) +{ + if (command == 0) return false; + return reasampler::ingestHandleSectionCommand(command); +} + // REAPER polls this to render each of OUR actions' checked state in menus/toolbars. // Return 1 (on) / 0 (off) for ids we own, -1 for everything else (per the contract). static int OnToggleAction(int command) @@ -255,6 +265,7 @@ extern "C" REAPER_PLUGIN_DLL_EXPORT int REAPER_PLUGIN_ENTRYPOINT( g_rec->Register("-projectconfig", (void*)&g_projectConfig); g_rec->Register("-toggleaction", (void*)&OnToggleAction); g_rec->Register("-hookcommand", (void*)&OnHookCommand); + g_rec->Register("-hookcommand2", (void*)&OnHookCommand2); reasampler::designViewUnregisterActions(g_rec); reasampler::bankUnregisterActions(g_rec); reasampler::ingestUnregisterActions(g_rec); @@ -307,6 +318,7 @@ extern "C" REAPER_PLUGIN_DLL_EXPORT int REAPER_PLUGIN_ENTRYPOINT( reasampler::ingestRegisterActions(rec, &g_session); rec->Register("hookcommand", (void*)&OnHookCommand); + rec->Register("hookcommand2", (void*)&OnHookCommand2); // Drives project-load / Save-As detection: the timer polls the active project // each tick; on a project load it reloads the bank from ext state, on a Save-As diff --git a/src/shell/actions/CLAUDE.md b/src/shell/actions/CLAUDE.md index 77c1a2d..42ce78f 100644 --- a/src/shell/actions/CLAUDE.md +++ b/src/shell/actions/CLAUDE.md @@ -34,7 +34,7 @@ is owned by other directories and only skinned here. - `shell/actions` (`action_registry` / `design_view_actions` / `bank_actions` / `prune_action`) — the bindable action families, all routed via the `command_id`/`gaccel`/`hookcommand` contract. `action_registry` owns the shared registration plumbing (interned channel-qualified id strings; register and mirror-unregister present the identical pointer) **and the Q-W6 registration TABLE**: `main.cpp`'s own family (capture scopes, panel toggle, insert, batch, realtime, recapture, version) is one `ActionTableRow` array — suffix, phrase, flat function-pointer handler — that registration, hookcommand dispatch, and the unload mirror-unregister all iterate, so adding an action touches the table only (OCP). Bank mutations flow through the promptless `shell/bank_ops` verbs (`bankOp*` + `persistBankOp`, taking `ReaSamplerSession&`), which the panel menus and `bank_actions` consume as thin UX skins. **Every bank index verb wraps its mutation in a batched REAPER undo point (`Undo_BeginBlock2`/`EndBlock2`, `UNDO_STATE_MISCCFG`) so one bank operation is one Ctrl-Z.** The prune action (`prune_action`, `BANK_PRUNE_FOLDER`) is **the ONLY file-deletion action in the system**; it opens no undo point (file deletion is not REAPER-undoable). `BANK_PRUNE_FOLDER` halts on `blockedByTracking` and prints each blocker that fired, with recovery instructions. - `drag_out_win` — OS drag-out shell: Windows OLE `DoDragDrop`/`CF_HDROP`, copy-only (`DROPEFFECT_MOVE` not offered); macOS/Linux via `SWELL_InitiateDragDropOfFileList`. - `instrument_drop_win` — FX-button drop shell: resolves a screen point to a track + FX-surface hotspot, then adds a ReaSampler 9000 instance and applies the dragged capture's state via a transient `.vstpreset` + `TrackFX_SetPreset` (the former `TrackFX_SetNamedConfigParm` "vst_chunk" write was silently unappliable for VST3). Exposes `loadInstrumentOntoTrack` (inner half, no own undo block) and `performInstrumentDrop` (wraps in its own undo block). **Never captures, never writes the bank, never inserts a timeline item.** -- `ingest` — ingest-through-the-bank shell on the EXTENSION side: three surfaces — (1) arrange capture→bank→assign (bindable action), (2) Media-Explorer import→bank→instrument on the selected track, (3) file drop onto the bank panel→bank only. Only surface (1) writes the `assignment_request` ext-state wire. **ingest NEVER inserts a timeline item.** +- `ingest` — ingest-through-the-bank shell on the EXTENSION side: three surfaces — (1) arrange capture→bank→assign (bindable action), (2) Media-Explorer import→bank→instrument on the selected track, (3) file drop onto the bank panel→bank only. Only surface (1) writes the `assignment_request` ext-state wire. **ingest NEVER inserts a timeline item.** Surface (2)'s action is the one in this directory published into a NON-main action section (Media Explorer) as well as Main — two ids, one handler, two dispatch hooks; see root `CLAUDE.md` §"REAPER extension contract" for the mechanism. ## Gotchas diff --git a/src/shell/actions/ingest.cpp b/src/shell/actions/ingest.cpp index e239c6e..dbc252e 100644 --- a/src/shell/actions/ingest.cpp +++ b/src/shell/actions/ingest.cpp @@ -62,18 +62,22 @@ namespace { // Not owned here (main.cpp owns g_session). ReaSamplerSession* g_session = nullptr; -// FOREVER-STABLE suffix — NEVER change after ship. Only the Media-Explorer import -// registers here — the arrange capture+assign action lives in the capture family in -// main.cpp, and the drop path is a panel callback (ingestDroppedFiles), not a -// bindable action. -constexpr const char* kIdImportMediaExplorer = "INGEST_IMPORT_MEDIA_EXPLORER"; +// Only the Media-Explorer import registers here — the arrange capture+assign action lives +// in the capture family in main.cpp, and the drop path is a panel callback +// (ingestDroppedFiles), not a bindable action. Its two FOREVER-STABLE suffixes are in +// ingest.h. +constexpr int kSectionMediaExplorer = 32063; -int g_cmdImportMediaExplorer = 0; -gaccel_register_t g_accelImportMediaExplorer{}; +int g_cmdImportMediaExplorer = 0; +int g_cmdImportMediaExplorerMx = 0; +gaccel_register_t g_accelImportMediaExplorer{}; +custom_action_register_t g_customImportMediaExplorer{}; // c_str() pointers are handed to REAPER at register and re-presented at unregister, -// so these strings must not be mutated after registration. +// so these strings must not be mutated after registration. The label backs BOTH the +// gaccel desc and the custom_action name. std::string g_idImportStr; +std::string g_idImportMxStr; std::string g_labelImportStr; // Forward-slashed, no trailing slash. Empty for an unsaved/no-active project, which @@ -457,14 +461,31 @@ void ingestDroppedFiles(const std::vector& absolutePaths) { void ingestRegisterActions(reaper_plugin_info_t* rec, ReaSamplerSession* session) { g_session = session; - g_idImportStr = channelCommandId(kIdImportMediaExplorer); + g_labelImportStr = channelActionName("import Media Explorer file into selected track"); + + g_idImportStr = channelCommandId(kIngestImportMediaExplorerId); g_cmdImportMediaExplorer = rec->Register("command_id", (void*)g_idImportStr.c_str()); if (g_cmdImportMediaExplorer) { - g_labelImportStr = channelActionName("import Media Explorer file into selected track"); g_accelImportMediaExplorer.accel.cmd = g_cmdImportMediaExplorer; g_accelImportMediaExplorer.desc = g_labelImportStr.c_str(); rec->Register("gaccel", (void*)&g_accelImportMediaExplorer); } + + // Second publication of the SAME action, into the Media Explorer section, so it can + // be put on that window's toolbar. gaccel cannot express this — it registers into the + // main keyboard section only — so custom_action is the one mechanism. It carries no + // ACCEL, hence no default keybinding for this entry; toolbar reach is the point. + g_idImportMxStr = channelCommandId(kIngestImportMediaExplorerMxId); + g_customImportMediaExplorer.uniqueSectionId = kSectionMediaExplorer; + g_customImportMediaExplorer.idStr = g_idImportMxStr.c_str(); + g_customImportMediaExplorer.name = g_labelImportStr.c_str(); + g_customImportMediaExplorer.extra = nullptr; // reserved + g_cmdImportMediaExplorerMx = + rec->Register("custom_action", (void*)&g_customImportMediaExplorer); + if (!g_cmdImportMediaExplorerMx) + ShowConsoleMsg("ReaSampler: could not publish the Media Explorer import action into " + "the Media Explorer action section -- it is still available in the " + "Main section.\n"); } bool ingestHandleCommand(int command) { @@ -473,10 +494,24 @@ bool ingestHandleCommand(int command) { return false; // not ours — caller's hookcommand keeps looking } +bool ingestHandleSectionCommand(int command) { + if (command == 0 || !g_session) return false; + // ONLY the Media Explorer id. The Main entry has its own distinct id and is claimed by + // ingestHandleCommand off "hookcommand"; claiming it here too would double-fire it. + if (command == g_cmdImportMediaExplorerMx) { doImportFromMediaExplorer(); return true; } + return false; +} + void ingestUnregisterActions(reaper_plugin_info_t* rec) { + // A 0 return from "custom_action" means REAPER holds no registration of ours (a dupe + // idStr is one documented cause) — mirroring it anyway could retire someone else's. + if (g_cmdImportMediaExplorerMx) + rec->Register("-custom_action", (void*)&g_customImportMediaExplorer); // '-command_id' re-presents the SAME interned id used at register (g_idImportStr). rec->Register("-gaccel", (void*)&g_accelImportMediaExplorer); rec->Register("-command_id", (void*)g_idImportStr.c_str()); + g_cmdImportMediaExplorer = 0; + g_cmdImportMediaExplorerMx = 0; g_session = nullptr; } diff --git a/src/shell/actions/ingest.h b/src/shell/actions/ingest.h index 3c4fa4b..1db5aa9 100644 --- a/src/shell/actions/ingest.h +++ b/src/shell/actions/ingest.h @@ -29,12 +29,25 @@ namespace reasampler { class ReaSamplerSession; -// `session` is shared with the capture / bank / Design-View families; the single -// hookcommand in main.cpp routes fired ids here via ingestHandleCommand. +// FOREVER-STABLE command-id suffixes — NEVER change after ship; user keybindings key off +// the composed ids. Two, because the Media-Explorer import publishes into two action +// sections and a custom_action idStr must be unique across all of them. Exposed here so +// the id-composition contract is assertable without linking this REAPER-facing TU. +inline constexpr const char* kIngestImportMediaExplorerId = "INGEST_IMPORT_MEDIA_EXPLORER"; +inline constexpr const char* kIngestImportMediaExplorerMxId = "INGEST_IMPORT_MEDIA_EXPLORER_MX"; + +// `session` is shared with the capture / bank / Design-View families; main.cpp's two +// dispatch hooks route fired ids back through the two handlers below. Both registration +// mechanisms are specified in root CLAUDE.md §"REAPER extension contract". void ingestRegisterActions(reaper_plugin_info_t* rec, ReaSamplerSession* session); +// Main-section dispatch ("hookcommand"). bool ingestHandleCommand(int command); +// Non-main-section dispatch ("hookcommand2"). Claims only ids this family published +// outside the Main section, so the two hooks never both claim one command. +bool ingestHandleSectionCommand(int command); + void ingestUnregisterActions(reaper_plugin_info_t* rec); // "The active sampler instance should now play (bankId, sampleId)." Called by EVERY diff --git a/tests/test_app_version.cpp b/tests/test_app_version.cpp index 2f8c1e3..808ad22 100644 --- a/tests/test_app_version.cpp +++ b/tests/test_app_version.cpp @@ -7,6 +7,10 @@ #include "../src/core/version/app_version.h" +// REAPER-free header; pulled in for the ingest action's FOREVER-STABLE id suffixes, so +// the composition assertions below check the strings that actually ship. +#include "../src/shell/actions/ingest.h" + #include #include @@ -151,6 +155,43 @@ static void testChannelQualifiedIdAndNameComposition() { } } +static void testMediaExplorerImportIdsAreDistinctAndChannelIsolated() { + // The Media-Explorer import publishes into TWO action sections, and a custom_action + // idStr must be unique across all sections — so the two entries carry two suffixes. + // Both are FOREVER-STABLE per channel. Composed from the SHIPPED constants and checked + // against spelled-out literals, so a suffix edit in ingest.cpp fails here. + const std::string mainId = channelCommandId(kIngestImportMediaExplorerId); + const std::string mxId = channelCommandId(kIngestImportMediaExplorerMxId); + + if (isBeta()) { + CHECK(mainId == "CEREBELLUM_REASAMPLER_BETA_INGEST_IMPORT_MEDIA_EXPLORER"); + CHECK(mxId == "CEREBELLUM_REASAMPLER_BETA_INGEST_IMPORT_MEDIA_EXPLORER_MX"); + CHECK(channelActionName("import Media Explorer file into selected track") == + "ReaSampler beta: import Media Explorer file into selected track"); + } else { + CHECK(mainId == "CEREBELLUM_REASAMPLER_INGEST_IMPORT_MEDIA_EXPLORER"); + CHECK(mxId == "CEREBELLUM_REASAMPLER_INGEST_IMPORT_MEDIA_EXPLORER_MX"); + CHECK(channelActionName("import Media Explorer file into selected track") == + "ReaSampler: import Media Explorer file into selected track"); + } + + // Distinctness is what keeps the two dispatch hooks from both claiming one command: + // hookcommand owns the Main id, hookcommand2 owns the Media Explorer id. A suffix + // collapse would silently double-fire the import. + CHECK(mainId != mxId); + + // Cross-channel isolation: this build's ids must NOT collide with the other channel's, + // or a beta install would rebind the stable keymap's entries. Asserted against the + // opposite channel's literals, which is the collision that would actually occur. + if (isBeta()) { + CHECK(mainId != "CEREBELLUM_REASAMPLER_INGEST_IMPORT_MEDIA_EXPLORER"); + CHECK(mxId != "CEREBELLUM_REASAMPLER_INGEST_IMPORT_MEDIA_EXPLORER_MX"); + } else { + CHECK(mainId != "CEREBELLUM_REASAMPLER_BETA_INGEST_IMPORT_MEDIA_EXPLORER"); + CHECK(mxId != "CEREBELLUM_REASAMPLER_BETA_INGEST_IMPORT_MEDIA_EXPLORER_MX"); + } +} + static void testStampClassifiesAsStampedOnOwnChannel() { // The V4 stamp-classifiability requirement: the value a channel WRITES (stampVersion()) // must classify as Stamped when that same channel reads it back — on BOTH channels. A @@ -277,6 +318,7 @@ int main() { testVstIdentityStringsForkByChannel(); testVstIdentityAndDataNamespaceShareOneChannel(); testChannelQualifiedIdAndNameComposition(); + testMediaExplorerImportIdsAreDistinctAndChannelIsolated(); testStampClassifiesAsStampedOnOwnChannel(); testParseWellFormed(); testParseRejectsMalformed(); From d589b99705f23794605febe9c88e338a9135056c Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sat, 1 Aug 2026 20:13:13 -0400 Subject: [PATCH 2/2] =?UTF-8?q?=CE=A8-W1-T3=20review=20remediation:=20soft?= =?UTF-8?q?en=20the=20double-fire=20claim=20to=20unspecified-by-SDK,=20mar?= =?UTF-8?q?k=20the=20unload=20mirror=20[verify=20=E2=80=94=20DAW],=20fix?= =?UTF-8?q?=20stale=20comment,=20drop=20redundant=20assertions,=20dedupe?= =?UTF-8?q?=20hook-partitioning=20comments,=20drop=20what-comment;=20file?= =?UTF-8?q?=20the=20action=5Fregistry=20test-seam=20deferral.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CLAUDE.md | 2 +- docs/PLAN.md | 5 ++++- docs/TODO.md | 37 ++++++++++++++++++++++++++++++++++++ src/app/main.cpp | 4 ++-- src/shell/actions/ingest.cpp | 6 +++--- tests/test_app_version.cpp | 18 +----------------- 6 files changed, 48 insertions(+), 24 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e8be300..5562a54 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -181,7 +181,7 @@ Comments carry *why*, and context where non-obvious — never *what* the code al 2. `rec->Register("gaccel", &accel)` — puts the action in the Actions list. 3. `rec->Register("hookcommand", ...)` — receives every action fired; claim only your own id, return `false` otherwise. 4. On unload (`rec == nullptr`), mirror-unregister everything with the same strings prefixed by `'-'`. -- **Non-main sections use a different mechanism.** `gaccel_register_t` carries no section field — `command_id` + `gaccel` can only ever produce a Main-section action. To publish into another section (Media Explorer = 32063, MIDI editor = 32060, MIDI event list = 32061, MIDI inline = 32062), register a `custom_action_register_t{uniqueSectionId, idStr, name, extra}` under `"custom_action"`; it returns the command id, or **0 on failure** (e.g. a duplicate `idStr`) — which the caller must tolerate rather than half-register. `idStr` must be unique **across all sections**, so an action published into both Main and a non-main section needs a SECOND id string; the FOREVER-STABLE contract binds it identically from the moment it ships. `custom_action_register_t` has no `ACCEL`, so a non-main entry ships no default keybinding. Dispatch for these ids arrives through `"hookcommand2"` (`bool(KbdSectionInfo*, int command, int val, int val2, int relmode, HWND)`) — `"hookcommand"` runs for the main section only. The two hooks must partition the ids between them: claiming one command in both double-fires it. On unload, mirror with `"-custom_action"` and `"-hookcommand2"`. +- **Non-main sections use a different mechanism.** `gaccel_register_t` carries no section field — `command_id` + `gaccel` can only ever produce a Main-section action. To publish into another section (Media Explorer = 32063, MIDI editor = 32060, MIDI event list = 32061, MIDI inline = 32062), register a `custom_action_register_t{uniqueSectionId, idStr, name, extra}` under `"custom_action"`; it returns the command id, or **0 on failure** (e.g. a duplicate `idStr`) — which the caller must tolerate rather than half-register. `idStr` must be unique **across all sections**, so an action published into both Main and a non-main section needs a SECOND id string; the FOREVER-STABLE contract binds it identically from the moment it ships. `custom_action_register_t` has no `ACCEL`, so a non-main entry ships no default keybinding. Dispatch for these ids arrives through `"hookcommand2"` (`bool(KbdSectionInfo*, int command, int val, int val2, int relmode, HWND)`) — `"hookcommand"` runs for the main section only. The two hooks must partition the ids between them; what happens when a command is claimed by both is unspecified by the SDK (`hookcommand2`'s doc says a `true` return prevents further hooks/actions from running, which is in tension with a clean double-fire either way), so nothing may rely on either outcome. On unload, mirror with `"-custom_action"` `[verify — DAW]` (the header confirms the `-` prefix for "most" registration types and spells out only `-pcmsrc` by name; `custom_action` itself is unconfirmed) and `"-hookcommand2"`. ## Product design docs diff --git a/docs/PLAN.md b/docs/PLAN.md index d0cf0e6..67eab47 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -2412,7 +2412,10 @@ must not touch `main.cpp`; this track is why. (`custom_action` / `hookcommand2` / `-custom_action` mirror) — a deliverable of this track. - **DAW-verification obligation:** Daniel adds the action to the Media Explorer toolbar - and imports from it; also confirms the Main-section binding still fires. + and imports from it; also confirms the Main-section binding still fires. Also: unload, + reload, confirm no duplicate Media Explorer entry — the `-custom_action` unload mirror + is unconfirmed against the SDK header (root `CLAUDE.md` §"REAPER extension contract", + `[verify — DAW]`), and if unsupported the entry leaks across a reload. **Open questions.** `[verify]` re-verify `custom_action_register_t` and `hookcommand2` argument order/types against the SDK headers at implementation time (root `CLAUDE.md` diff --git a/docs/TODO.md b/docs/TODO.md index a9c1fdf..0d72fdc 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -445,3 +445,40 @@ today. bitmap and confirmed to place the stroke correctly, or it is redesigned to rasterize at physical rather than logical resolution once `IPlugViewContentScaleSupport` (or equivalent) makes scaling real. + +## `ingestHandleSectionCommand` has no unit test + +**Context (what shipped — Ψ-W1-T3, media-explorer-section).** The Media-Explorer +import now dispatches through two hooks — `ingestHandleCommand` (Main, +`"hookcommand"`) and `ingestHandleSectionCommand` (Media Explorer, +`"hookcommand2"`). Both live in `ingest.cpp`, which compiles straight into the +`reaper_reasampler` MODULE target. + +**The wart.** No `shell/` translation unit in this repo has a test target — every +`_tests` executable is a `core/` pure-module target. `ingestHandleSectionCommand` +is a two-line command-id comparison; correctness here rests on code review, not CTest. +Review verified this constraint is real and the deferral correct. + +**Intended fix.** Make `action_registry` a linkable library and give it the repo's +first `shell/` test target, driven by a fake `reaper_plugin_info_t`. Its own header +(`reaper_plugin.h:153-172`) shows `Register` is a plain member-function pointer on the +struct, not a REAPER API pointer resolved through `REAPERAPI_LoadAPI` — a fake instance +needs no live REAPER process to exercise `rec->Register(...)` calls. Once +`action_registry` is test-covered, move the Media-Explorer section registration into it. + +**The constraint the fix MUST handle.** The extraction alone buys nothing: +`action_registry` has no test target today either, so lifting `ingestHandleSectionCommand` +into it without also standing up the test target just relocates the untested code. The +same follow-up could collapse `ingest.cpp:466-472`'s hand-rolled `command_id`+`gaccel` +pair onto `action_registry::registerAction`, which already does exactly that dance for +the Q-W6 table. + +**Priority / risk.** Low / deferred. `ingestHandleSectionCommand` is a two-branch +comparison, reviewed and correct at this scope; the gap is the missing test seam, not a +known defect. + +**Done looks like.** `action_registry` is a linkable library with its own `shell/`-first +CTest target driven by a fake `reaper_plugin_info_t`; the Media-Explorer section +registration and `ingestHandleSectionCommand` move into it and gain unit coverage; and +`ingest.cpp`'s own `command_id`+`gaccel` registration collapses onto +`action_registry::registerAction` where the shapes match. diff --git a/src/app/main.cpp b/src/app/main.cpp index 398451e..c6f8136 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -230,8 +230,8 @@ static bool OnHookCommand(int command, int /*flag*/) } // "hookcommand" covers the main section only, so actions we published into another -// section arrive here instead. Claim ONLY those — every main-section id stays with -// OnHookCommand, so no command can be claimed by both hooks. +// section arrive here instead. Partitioning contract: root `CLAUDE.md` §"REAPER +// extension contract". static bool OnHookCommand2(KbdSectionInfo* /*sec*/, int command, int /*val*/, int /*val2*/, int /*relmode*/, HWND /*hwnd*/) { diff --git a/src/shell/actions/ingest.cpp b/src/shell/actions/ingest.cpp index dbc252e..e60f15d 100644 --- a/src/shell/actions/ingest.cpp +++ b/src/shell/actions/ingest.cpp @@ -479,7 +479,7 @@ void ingestRegisterActions(reaper_plugin_info_t* rec, ReaSamplerSession* session g_customImportMediaExplorer.uniqueSectionId = kSectionMediaExplorer; g_customImportMediaExplorer.idStr = g_idImportMxStr.c_str(); g_customImportMediaExplorer.name = g_labelImportStr.c_str(); - g_customImportMediaExplorer.extra = nullptr; // reserved + g_customImportMediaExplorer.extra = nullptr; g_cmdImportMediaExplorerMx = rec->Register("custom_action", (void*)&g_customImportMediaExplorer); if (!g_cmdImportMediaExplorerMx) @@ -496,8 +496,8 @@ bool ingestHandleCommand(int command) { bool ingestHandleSectionCommand(int command) { if (command == 0 || !g_session) return false; - // ONLY the Media Explorer id. The Main entry has its own distinct id and is claimed by - // ingestHandleCommand off "hookcommand"; claiming it here too would double-fire it. + // ONLY the Media Explorer id — partitioning contract: root `CLAUDE.md` + // §"REAPER extension contract". if (command == g_cmdImportMediaExplorerMx) { doImportFromMediaExplorer(); return true; } return false; } diff --git a/tests/test_app_version.cpp b/tests/test_app_version.cpp index 808ad22..988025a 100644 --- a/tests/test_app_version.cpp +++ b/tests/test_app_version.cpp @@ -159,7 +159,7 @@ static void testMediaExplorerImportIdsAreDistinctAndChannelIsolated() { // The Media-Explorer import publishes into TWO action sections, and a custom_action // idStr must be unique across all sections — so the two entries carry two suffixes. // Both are FOREVER-STABLE per channel. Composed from the SHIPPED constants and checked - // against spelled-out literals, so a suffix edit in ingest.cpp fails here. + // against spelled-out literals, so a suffix edit in ingest.h fails here. const std::string mainId = channelCommandId(kIngestImportMediaExplorerId); const std::string mxId = channelCommandId(kIngestImportMediaExplorerMxId); @@ -174,22 +174,6 @@ static void testMediaExplorerImportIdsAreDistinctAndChannelIsolated() { CHECK(channelActionName("import Media Explorer file into selected track") == "ReaSampler: import Media Explorer file into selected track"); } - - // Distinctness is what keeps the two dispatch hooks from both claiming one command: - // hookcommand owns the Main id, hookcommand2 owns the Media Explorer id. A suffix - // collapse would silently double-fire the import. - CHECK(mainId != mxId); - - // Cross-channel isolation: this build's ids must NOT collide with the other channel's, - // or a beta install would rebind the stable keymap's entries. Asserted against the - // opposite channel's literals, which is the collision that would actually occur. - if (isBeta()) { - CHECK(mainId != "CEREBELLUM_REASAMPLER_INGEST_IMPORT_MEDIA_EXPLORER"); - CHECK(mxId != "CEREBELLUM_REASAMPLER_INGEST_IMPORT_MEDIA_EXPLORER_MX"); - } else { - CHECK(mainId != "CEREBELLUM_REASAMPLER_BETA_INGEST_IMPORT_MEDIA_EXPLORER"); - CHECK(mxId != "CEREBELLUM_REASAMPLER_BETA_INGEST_IMPORT_MEDIA_EXPLORER_MX"); - } } static void testStampClassifiesAsStampedOnOwnChannel() {