Ψ-W1-T3 review remediation: soften the double-fire claim to unspecified-by-SDK, mark the unload mirror [verify — DAW], fix stale comment, drop redundant assertions, dedupe hook-partitioning comments, drop what-comment; file the action_registry test-seam deferral.
This commit is contained in:
+4
-1
@@ -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`
|
||||
|
||||
@@ -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
|
||||
`<module>_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.
|
||||
|
||||
Reference in New Issue
Block a user