From 9599e96e0c017722006c096ae237704d1f0920b3 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 15:50:25 -0400 Subject: [PATCH 1/2] Fix drag-out giving live drags to OLE: prove the pointer left REAPER OffReaper leaves the surface vocabulary; OsHandoff now gates on a window-ownership proof resolved in the shell, so no in-REAPER surface can reach DoDragDrop. --- src/core/ui/CLAUDE.md | 6 +- src/core/ui/drag_out.cpp | 8 +- src/core/ui/drag_out.h | 25 +++-- src/core/wire/CLAUDE.md | 2 +- src/core/wire/instrument_drop.cpp | 3 +- src/core/wire/instrument_drop.h | 17 ++-- src/shell/actions/CLAUDE.md | 4 +- src/shell/actions/drag_out_win.cpp | 26 +++++ src/shell/actions/drag_out_win.h | 15 ++- src/shell/actions/instrument_drop_win.cpp | 2 +- src/shell/actions/instrument_drop_win.h | 6 +- src/shell/panel/panel_drag.cpp | 11 +- tests/test_drag_out.cpp | 118 ++++++++++++++++------ tests/test_instrument_drop.cpp | 87 ++++++++-------- 14 files changed, 221 insertions(+), 109 deletions(-) diff --git a/src/core/ui/CLAUDE.md b/src/core/ui/CLAUDE.md index ac439e6..422db3c 100644 --- a/src/core/ui/CLAUDE.md +++ b/src/core/ui/CLAUDE.md @@ -88,7 +88,9 @@ L7 sub-pass, 2026-07-27): between evaluations. That is what makes every transition reversible and what makes drag speed (WM_MOUSEMOVE coalescing) unable to change an outcome. Leaving REAPER entirely is the one irreversible transition, because the OS hand-off goes - modal. A first-move class lock and a drag-lifetime "cannot hand off" latch both + modal — so it must be PROVEN (a window-ownership test in the shell), never inferred + from a hit-test token REAPER could not name; an unrecognised surface *inside* REAPER + refuses. A first-move class lock and a drag-lifetime "cannot hand off" latch both existed here and were removed — do not reintroduce either. ## Modules @@ -97,7 +99,7 @@ L7 sub-pass, 2026-07-27): - `bank_grid` — REAPER-free grid layout, selection, keyboard-nav, and thumbnail-cache-key logic for the docked bank panel. - `tab_strip` — REAPER-free scrollable tab-strip layout + hit-test for the named-banks strip. - `prune_button` — pure layout/hit-test for the `bank_panel` footer Prune button. -- `drag_out` — the pure drag-out gesture law plus path-list assembly. Owns the `ReaperSurface` vocabulary (OffReaper / TrackPanel / FxSurface / FxEmbed / Arrange / Other — `core/wire/instrument_drop` classifies REAPER's info token INTO it), `decideDropClass` (surface × single-vs-multi payload → Internal / InstrumentDrop / ArrangeInsert / Refuse / OsHandoff / None), and `cueForDropClass`. **No `DropClass` means "nothing happens"**: a surface with no defined outcome for the payload resolves to `Refuse`, which the shell shows as a cursor, so "no silent no-op release" is a property of the enumeration rather than of any call site. +- `drag_out` — the pure drag-out gesture law plus path-list assembly. Owns the `ReaperSurface` vocabulary (TrackPanel / FxSurface / FxEmbed / Arrange / Other — `core/wire/instrument_drop` classifies REAPER's info token INTO it; there is deliberately no off-REAPER member, since no token can carry that fact), `decideDropClass` (surface × single-vs-multi payload → Internal / InstrumentDrop / ArrangeInsert / Refuse / OsHandoff / None), and `cueForDropClass`. `OsHandoff` is reachable ONLY through `DropContext::pointerOffHost`, a positive window-ownership proof the shell owes the law (`drag_out_win::pointerOverHostWindow`), gated ahead of the surface switch — so no reading of REAPER's hit-test can give a live drag away to OLE. **No `DropClass` means "nothing happens"**: a surface with no defined outcome for the payload resolves to `Refuse`, which the shell shows as a cursor, so "no silent no-op release" is a property of the enumeration rather than of any call site. - `theme` — pure palette module: role→color mapping, REAPER-grey neutral ladder + the pastel accent system, the keyboard strip's spectral ramp, WCAG contrast-floor helpers + `compositeOver` (the effective color of a translucent fill, so alpha overlays are testable). Only the ramp's MID stop is its own constant; lo/hi are still aliases of `accent/primary`/`accent/tertiary`, so a categorical accent move CAN still reorder the ramp — `testSpectralRampLuminanceIsMonotonic` is the build-time catch, not the structure. - `component_geometry` — pure button/slider/list-row geometry + hover hit-test helpers. - `action_bar` — pure task-grouped action-bar layout/hit-test: clusters (Capture / Placement / Maintenance / Tagging / Switching). diff --git a/src/core/ui/drag_out.cpp b/src/core/ui/drag_out.cpp index 725782d..39d7184 100644 --- a/src/core/ui/drag_out.cpp +++ b/src/core/ui/drag_out.cpp @@ -24,10 +24,12 @@ DropClass decideDropClass(int px, int py, const PanelClientRect& client, if (!ctx.drag.dragging || !ctx.drag.hasArmedSamples) return DropClass::None; if (insideClient(px, py, client)) return DropClass::Internal; - switch (ctx.surface) { - case ReaperSurface::OffReaper: - return DropClass::OsHandoff; + // The hand-off gate sits AHEAD of the surface switch and is the only route to OsHandoff, so + // no reading of REAPER's hit-test — recognised, unrecognised, or empty — can reach the modal + // OLE loop while the pointer is still over a host window. + if (ctx.pointerOffHost) return DropClass::OsHandoff; + switch (ctx.surface) { case ReaperSurface::TrackPanel: case ReaperSurface::FxSurface: // One instance holds one capture, so a multi payload names no instrument to build — diff --git a/src/core/ui/drag_out.h b/src/core/ui/drag_out.h index 4c3336f..3c30c6a 100644 --- a/src/core/ui/drag_out.h +++ b/src/core/ui/drag_out.h @@ -6,8 +6,10 @@ // // THE LAW: the class is resolved from what is under the cursor on EVERY move; every transition // is reversible until release or until the pointer leaves REAPER entirely; the OS hand-off is -// reserved for leaving REAPER, and every REAPER-internal target executes natively on release. -// Do not reintroduce a first-move class lock or a drag-lifetime "blocked" latch. +// reserved for leaving REAPER — which the shell must PROVE (DropContext::pointerOffHost), never +// infer from a hit-test token it could not name — and every REAPER-internal target executes +// natively on release. Do not reintroduce a first-move class lock or a drag-lifetime "blocked" +// latch. // // Path-list assembly: turns armed sample ids into the absolute path list an OS drop carries // (Windows CF_HDROP / macOS file-list pasteboard) — set algebra only; the shell resolves each id @@ -32,16 +34,18 @@ struct DragState { }; // What REAPER reports under the pointer, reduced to the surfaces the law distinguishes. -// Produced from GetThingFromPoint's info token by wire::classifyReaperSurface. +// Produced from GetThingFromPoint's info token by wire::classifyReaperSurface. There is +// deliberately NO off-REAPER member: this vocabulary describes what REAPER's own hit-test +// named, and "the pointer left REAPER" is a window-ownership fact no token can carry — +// it enters through DropContext::pointerOffHost instead. enum class ReaperSurface { - OffReaper, // not over REAPER at all — the one irreversible exit TrackPanel, // TCP/MCP, ANY sub-element: the WHOLE panel is the instrument hotspot FxSurface, // fx_* — the FX chain and floating-FX windows FxEmbed, // tcp.fxembed / mcp.fxembed — an instance already draws there Arrange, // the timeline - Other, // ruler, transport, spacers, docker chrome, unknown future tokens + Other, // ruler, transport, spacers, docker chrome, unknown/empty future tokens Count, // sentinel, NOT a real surface — tests/test_drag_out.cpp's kAllSurfaces is - // pinned against this via static_assert so a 7th surface can't silently skip + // pinned against this via static_assert so a 6th surface can't silently skip // the exhaustiveness matrix }; @@ -62,8 +66,15 @@ enum class DropClass { struct DropContext { DragState drag; bool singlePayload = false; // payload holds EXACTLY one capture (arms InstrumentDrop) - ReaperSurface surface = ReaperSurface::OffReaper; // only read outside the client rect + ReaperSurface surface = ReaperSurface::Other; // only read outside the client rect bool haveTrack = false; // GetThingFromPoint returned a non-null MediaTrack* + // The ONE gate on the irreversible OS hand-off, and the only member whose true value can + // produce it. It is a POSITIVE proof the shell owes the law — a window-ownership test + // (drag_out_win::pointerOverHostWindow), not "the hit-test named nothing I recognise", + // which is equally true of the transport, the toolbar and the docker chrome. Defaults + // false so an unset context stays inside REAPER: the failure mode of forgetting to + // resolve it must be a refusal, never a drag given away to OLE. + bool pointerOffHost = false; }; // Resolves the class for a drag at pointer (px, py) over `client`. Position-and-context only, diff --git a/src/core/wire/CLAUDE.md b/src/core/wire/CLAUDE.md index e12f482..a6ef1da 100644 --- a/src/core/wire/CLAUDE.md +++ b/src/core/wire/CLAUDE.md @@ -81,7 +81,7 @@ This directory owns two cross-artifact contracts specifically: - `reasampler_uid.h` — SDK-free header owning the FOREVER-FROZEN VST3 class-UID integer macros (stable + beta pairs, `REASAMPLER_PROC_UID_*` / `REASAMPLER_PROC_UID_BETA_*`) and the `REASAMPLER_ACTIVE_UID_*` channel-selector macros. Split out of `reasampler_vst.h` so the pure extension side (`instrument_drop`) can derive the `.vstpreset` class-ID hex string without pulling in the VST3 SDK. Both `reasampler_vst.h` (runtime `FUID`) and `instrument_drop` (preset hex string) source from this single header — the binary identity and the preset-file identity cannot diverge. - `assignment_request` — pure ingest-assign wire: typed request record carrying the drop payload from the `ingest` shell through to the VST3 bridge. - `bake_wire` — the resample bake's request/outcome pair on ONE per-instance key (`rsbake_`): the instrument writes a `BakeRequest`, invokes the extension's action synchronously, and reads the extension's `BakeOutcome` back over the same key inside that one call. Not a handshake — a call and a return, and it must not grow a claim protocol. Also the ONE home of the bake action's command-id suffix and of the leading underscore `NamedCommandLookup` needs but `rec->Register("command_id", …)` does not, so both artifacts name one action. `BakeStatus` values are WIRE INTEGERS: never renumber, only append, and an unrecognized value decodes as `Failed` rather than as the numeric default `Ok`. It owns BOTH ends' reading of that key, since the key's contents are the only evidence either side gets: `classifyBakeAnswer` (instrument side — six kinds, of which `Unanswered`, the request still sitting there untouched, separates "nothing wrote an outcome over our key" from a refusal — it does NOT identify a landing that never ran, since a skipped key and a rejected answer-write look identical from here) and `classifyBakeScan` + `kMaxRequestAgeSeconds` (extension side — the per-key Land / RefuseWrongProject / ClearStale / IgnoreUnreadable / IgnoreNotARequest verdict over every open tab, stated without a REAPER type so the multi-tab matrix is unit-provable). `BakeScanTally` + `describeBakeScan` + `describeBakeKey` are that same reading counted and spoken — the rationale lives at the types. **The report's absence is NOT evidence the landing never ran**, and no sentence either artifact prints may say it is: `answered` is pass-wide and counts a QUEUED write, so a pass can answer some other key while skipping ours, or have our own answer's `SetProjExtState` rejected. The summary is therefore silent only when the pass answered somebody, left no key unanswered, and every answer was READ BACK from its own key; `describeBakeKey` prints one line per enumerated key regardless, which is the only thing that names WHICH key — the counts cannot. The read-back verdict behind `BakeWriteProof` is `ext_state_read.h`'s `extStateWriteLanded`, above. `bakeLandingAfterPersist` is the ONE route to a `Banked` landing: every `Land` verdict is assigned `Unpersisted` and passed through it, so no shell path — a dedup hit least of all, since its target may be an entry the same pass just added — can claim the word without the pass's persist having reported success. What `Banked` claims is exactly that `persisted` input — a saved project was active and the bank write was issued — and the enum comment is that claim's one home. -- `instrument_drop` — pure FX-drop payload builder: constructs a Steinberg-format `.vstpreset` image (channel-active class ID + the instrument's own component state, capture pre-selected) the shell applies via `TrackFX_SetPreset`; owns `classifyReaperSurface`, the prefix classifier mapping a `GetThingFromPoint` (info token, track-present) pair onto `core/ui/drag_out`'s `ReaperSurface`. Classifier ordering is load-bearing: the embed strip is matched before the `tcp`/`mcp` panel family, which now claims the WHOLE track panel rather than just its FX sub-elements. All-or-nothing contract — caller rolls back via `TrackFX_Delete` on any failure. +- `instrument_drop` — pure FX-drop payload builder: constructs a Steinberg-format `.vstpreset` image (channel-active class ID + the instrument's own component state, capture pre-selected) the shell applies via `TrackFX_SetPreset`; owns `classifyReaperSurface`, the prefix classifier mapping a `GetThingFromPoint` info token onto `core/ui/drag_out`'s `ReaperSurface`. Classifier ordering is load-bearing: the embed strip is matched before the `tcp`/`mcp` panel family, which claims the WHOLE track panel rather than just its FX sub-elements. An empty token is `Other`, never an off-REAPER verdict — the SDK documents no off-REAPER return, so the toolbar/transport/docker chrome all report nothing REAPER names. All-or-nothing contract — caller rolls back via `TrackFX_Delete` on any failure. - `sample_usage` — instance-usage wire: `UsageRecord`, `planUsagePublish` (fresh/heal/clean-replace/union/remint publish plan), `foldUsageRecords`/`usageHeldPaths` (liveness fold — protect-all when records exist but no instance is live; abort→protect-all on unreadable record; `counted` carries key-attributed live records), `identityMatches` (ReaSampler 9000 FX identity). REAPER-free, unit-tested. The mirror of `assignment_request` on the instrument→extension direction: the wire format and the two safety-critical decisions (what to write on publish, which records count at prune time) are pure so they are provable without a DAW. It lives here because it is a *wire format* with an instrument-side writer; the fold's output is consumed by `core/tracking`'s authority, which owns every consumer-facing decision built on it. ## Gotchas diff --git a/src/core/wire/instrument_drop.cpp b/src/core/wire/instrument_drop.cpp index 73f1a79..64737dd 100644 --- a/src/core/wire/instrument_drop.cpp +++ b/src/core/wire/instrument_drop.cpp @@ -87,11 +87,10 @@ DropOutcome decideDropOutcome(const DropAttempt& attempt) { return out; } -ui::ReaperSurface classifyReaperSurface(const std::string& info, bool haveTrack) { +ui::ReaperSurface classifyReaperSurface(const std::string& info) { // See the header contract for the prefix rule and the ordering it depends on. auto startsWith = [&info](const char* p) { return info.rfind(p, 0) == 0; }; - if (info.empty()) return haveTrack ? ui::ReaperSurface::Other : ui::ReaperSurface::OffReaper; if (startsWith("tcp.fxembed") || startsWith("mcp.fxembed")) return ui::ReaperSurface::FxEmbed; if (startsWith("fx_")) return ui::ReaperSurface::FxSurface; if (startsWith("tcp") || startsWith("mcp")) return ui::ReaperSurface::TrackPanel; diff --git a/src/core/wire/instrument_drop.h b/src/core/wire/instrument_drop.h index 845512c..371033b 100644 --- a/src/core/wire/instrument_drop.h +++ b/src/core/wire/instrument_drop.h @@ -55,9 +55,9 @@ std::vector buildVstPresetBytes(const std::string& classIdHex32, // preset. Deterministic. std::vector buildInstrumentDropPreset(const std::string& sampleId); -// Pure classifier for GetThingFromPoint's (info string, track-was-returned) pair -// into the surfaces the drag-out gesture law distinguishes. The SDK warns future -// versions may append information, so every rule is PREFIX-based: +// Pure classifier for GetThingFromPoint's info string into the surfaces the drag-out +// gesture law distinguishes. The SDK warns future versions may append information, so +// every rule is PREFIX-based: // "tcp.fxembed*" / "mcp.fxembed*" -> FxEmbed. Checked FIRST: it is the surface // an existing instance already draws on, and a drop must not stack a second. // "fx_*" -> FxSurface (FX chain + floating FX windows). @@ -66,12 +66,15 @@ std::vector buildInstrumentDropPreset(const std::string& sampleId) // still means "sampler on this track", and the old glyph-only rule is exactly // why such a drop landed nowhere. // "arrange*" -> Arrange. -// "" with no track -> OffReaper (the pointer left REAPER). -// anything else, incl. "" WITH a track -> Other. Over REAPER on a surface we -// cannot name: refuse visibly, never guess an outcome. +// anything else, "" INCLUDED -> Other. A surface we cannot name: refuse +// visibly, never guess an outcome. An empty token is NOT evidence the pointer +// left REAPER — the SDK documents no off-REAPER return at all, and the transport, +// toolbar, menu bar and docker chrome all report nothing REAPER names. Reading it +// as "off REAPER" is what handed live drags to OLE mid-gesture; whether the +// pointer left is a window-ownership fact, resolved in the shell. // The exact live token is DAW-only — confirm via // reaper.GetThingFromPoint(reaper.GetMousePosition()) in ReaScript if unsure. -ui::ReaperSurface classifyReaperSurface(const std::string& info, bool haveTrack); +ui::ReaperSurface classifyReaperSurface(const std::string& info); // The raw component-state bytes the preset carries, exposed so the round-trip // test can decode them back through sample_map::deserializeComponentState and diff --git a/src/shell/actions/CLAUDE.md b/src/shell/actions/CLAUDE.md index cd53922..65ea724 100644 --- a/src/shell/actions/CLAUDE.md +++ b/src/shell/actions/CLAUDE.md @@ -41,8 +41,8 @@ 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. - `package_export_action` — the "export bank as package" skin: survey and report first, confirm what is absent (and, separately, a destination being replaced), pick a destination, write. Every prompt in the flow lives here so `shell/package/export_bank` stays promptless. Read-only against the project — it holds the session by `const&`, so no ext-state write, generation bump or undo point is reachable. Registration rides `main.cpp`'s action table (`EXPORT_BANK_PACKAGE`); the panel's tab menu is the second skin over the same body. -- `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` — instrument-drop shell: `probeDropTarget` resolves a screen point to a track + a `ReaperSurface` (via the pure `wire::classifyReaperSurface`, whose token rules `core/wire/CLAUDE.md` owns), and the drop half 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.** +- `drag_out_win` — the host/OS boundary of drag-out: Windows OLE `DoDragDrop`/`CF_HDROP`, copy-only (`DROPEFFECT_MOVE` not offered); macOS/Linux via `SWELL_InitiateDragDropOfFileList`. Also owns `pointerOverHostWindow`, the window-ownership test behind `ui::DropContext::pointerOffHost` — process identity on Windows (`WindowFromPoint` + `GetWindowThreadProcessId`), non-null `WindowFromPoint` under SWELL, whose window list is already this-process-only. +- `instrument_drop_win` — instrument-drop shell: `probeDropTarget` resolves a screen point to a track + a `ReaperSurface` (via the pure `wire::classifyReaperSurface`, whose token rules `core/wire/CLAUDE.md` owns) — it answers only what REAPER named, never whether the pointer is still in REAPER, which is `drag_out_win`'s — and the drop half 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.** - `arrange_drop_win` — the drag-out gesture's arrange outcome: `arrangeTimeAtScreenX` (pointer column → time via `GetSet_ArrangeView2`'s one-pixel-span reading — inferred, not SDK-documented) and `performArrangeDrop` (snap the drop time, then one `InsertMedia` per capture on the pointer's track — assumed, not confirmed, to land end-to-end via REAPER's own cursor advance — in ONE undo block, counting only InsertMedia's reported successes, with the caller's track selection and edit cursor restored). The one timeline-placing shell here, per the invariant above; it never captures and never writes the bank. - `package_import_action` — the bindable / bank-menu / file-drop skin over `shell/package`'s `importBankPackage`. Owns the **ledger gate**, which runs BEFORE the file picker (a refusal must not cost the user a file choice) and is keyed on the session's `LedgerStatus` alone — never on `PruneReport::blockedByTracking`, whose undecodable-`rsusage_*` arm governs deletion-time protection and would refuse an import that only writes birth records. Builds and shows every message the import produces, but the ledger-refusal body itself is `core/package::ledgerRefusalMessage` — a pure fold this TU only supplies the channel-correct namespace to — so the wording is assertable without a DAW. `doImportBankPackage`/`doImportBankPackageFile` return the minted bank id on a landed import (empty otherwise) so a caller can focus it; the verb itself is promptless. - `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. diff --git a/src/shell/actions/drag_out_win.cpp b/src/shell/actions/drag_out_win.cpp index a420c28..2b738fb 100644 --- a/src/shell/actions/drag_out_win.cpp +++ b/src/shell/actions/drag_out_win.cpp @@ -244,6 +244,23 @@ bool canInitiateDragOut(const std::vector& absolutePaths) { return true; } +bool pointerOverHostWindow(int screenX, int screenY) { + // Process identity, not window identity: REAPER's arrange, its floating FX and VST editor + // windows, its menus and its docker chrome are separate top-level HWNDs with no common + // ancestor, so no GetAncestor/GetParent walk reaches them all — but every one of them is + // created by the process this extension is loaded into. + // Mouse capture does not affect WindowFromPoint: it hit-tests the desktop window tree, not + // input routing. It does skip hidden and DISABLED windows, so a REAPER window disabled + // behind a modal dialog could fall through to whatever sits beneath — no such dialog can be + // up while our panel holds capture for a drag. + const POINT pt{screenX, screenY}; + const HWND hwnd = WindowFromPoint(pt); + if (!hwnd) return false; + DWORD pid = 0; + GetWindowThreadProcessId(hwnd, &pid); + return pid == GetCurrentProcessId(); +} + } // namespace reasampler #else // ---- macOS / Linux (SWELL) ----------------------------------------------------- @@ -277,6 +294,15 @@ bool canInitiateDragOut(const std::vector& absolutePaths) { return !absolutePaths.empty(); } +// SWELL's WindowFromPoint walks only THIS process's window list (SWELL_topwindows on +// generic/GDK, [NSApp orderedWindows] on macOS — swell-wnd-generic.cpp / swell-wnd.mm), so a +// non-null return already carries the process-ownership meaning Win32 needs GetWindowThread- +// ProcessId for, and a point over any other application returns null. +bool pointerOverHostWindow(int screenX, int screenY) { + POINT pt{screenX, screenY}; + return WindowFromPoint(pt) != nullptr; +} + } // namespace reasampler #endif diff --git a/src/shell/actions/drag_out_win.h b/src/shell/actions/drag_out_win.h index 0631d3d..e179edf 100644 --- a/src/shell/actions/drag_out_win.h +++ b/src/shell/actions/drag_out_win.h @@ -1,7 +1,8 @@ #pragma once -// drag_out_win — the OS/COM initiation half of native OS drag-out: hands a resolved -// existing-file path list to the OS's drag-drop machinery so the user can drop bank -// samples into Explorer / another app / another DAW. The pure gesture-boundary +// drag_out_win — the host/OS boundary of native OS drag-out: whether the pointer has +// crossed it, whether the OS is ready to take the drag, and the hand-off itself, which +// gives a resolved existing-file path list to the OS's drag-drop machinery so the user can +// drop bank samples into Explorer / another app / another DAW. The pure gesture-boundary // decision + path-list assembly live in drag_out.*. // // COPY-ONLY is STRUCTURAL on Windows: DoDragDrop's effect mask is DROPEFFECT_COPY @@ -32,4 +33,12 @@ bool initiateDragOut(HWND__* panelHwnd, const std::vector& absolute // probe HGLOBAL immediately; SWELL exposes no probe, so macOS/Linux reduces to the non-empty check. bool canInitiateDragOut(const std::vector& absolutePaths); +// True iff a window belonging to the host (REAPER) process sits under the screen point — +// the POSITIVE proof ui::DropContext::pointerOffHost is the negation of. Every REAPER +// surface qualifies whether or not GetThingFromPoint can name it: the arrange, a TCP, a +// floating FX or VST editor window, the transport, the toolbar, a menu, the docker chrome +// and our own panel are all windows of this process, because the extension runs inside it. +// Side-effect-free; safe on every mouse-move. +bool pointerOverHostWindow(int screenX, int screenY); + } // namespace reasampler diff --git a/src/shell/actions/instrument_drop_win.cpp b/src/shell/actions/instrument_drop_win.cpp index 44e3649..d385c49 100644 --- a/src/shell/actions/instrument_drop_win.cpp +++ b/src/shell/actions/instrument_drop_win.cpp @@ -83,7 +83,7 @@ DropProbe probeDropTarget(int screenX, int screenY) { // doc-comment says so), so the track and the surface are two independent facts and both // are reported. The pure classifier owns every token rule. out.track = GetThingFromPoint(screenX, screenY, info, sizeof(info)); - out.surface = classifyReaperSurface(info, out.track != nullptr); + out.surface = classifyReaperSurface(info); return out; } diff --git a/src/shell/actions/instrument_drop_win.h b/src/shell/actions/instrument_drop_win.h index 8fc762d..df9ca43 100644 --- a/src/shell/actions/instrument_drop_win.h +++ b/src/shell/actions/instrument_drop_win.h @@ -29,12 +29,14 @@ namespace reasampler { // (ui::decideDropClass) turns this plus the payload size into an outcome. struct DropProbe { MediaTrack* track = nullptr; // the track under the pointer (null if none / not a track) - ui::ReaperSurface surface = ui::ReaperSurface::OffReaper; + ui::ReaperSurface surface = ui::ReaperSurface::Other; }; -// Wraps GetThingFromPoint and hands its (info string, track) pair to the pure +// Wraps GetThingFromPoint: its track comes back as-is, its info string goes to the pure // wire::classifyReaperSurface. Cheap enough to run on every mouse-move, but the panel // evaluates it only OUTSIDE its own client rect — the internal drag never pays for it. +// It reports only what REAPER named; whether the pointer is still inside REAPER at all is +// drag_out_win::pointerOverHostWindow's, and no surface reported here implies an exit. DropProbe probeDropTarget(int screenX, int screenY); // Adds a fresh ReaSampler 9000 instance to `track` and applies `presetBytes` as its diff --git a/src/shell/panel/panel_drag.cpp b/src/shell/panel/panel_drag.cpp index 6d62efd..5e20a37 100644 --- a/src/shell/panel/panel_drag.cpp +++ b/src/shell/panel/panel_drag.cpp @@ -199,6 +199,12 @@ LiveDrop resolveLiveDrop(int x, int y) { const DropProbe probe = probeDropTarget(sp.x, sp.y); ctx.surface = probe.surface; ctx.haveTrack = probe.track != nullptr; + // Two independent probes because they answer two independent questions: REAPER's + // hit-test names the surface, window ownership decides whether we are still in REAPER + // at all. The SDK hit-test cannot answer the second — GetThingFromPoint documents no + // off-REAPER return, so its silence over the transport or the toolbar used to read as + // "the user left" and gave the live drag away to OLE. + ctx.pointerOffHost = !pointerOverHostWindow(sp.x, sp.y); out.track = probe.track; out.screenX = sp.x; } @@ -219,11 +225,12 @@ LiveDrop resolveLiveDrop(int x, int y) { // loop, so the internal drag must be fully wound down first, and only once the payload is known // to be hand-off-able. This runs on every qualifying move — do not memoize a failed attempt. // -// Residual (accepted): once the pointer has left REAPER, dragging back INTO a REAPER window +// Residual (accepted): once the pointer has genuinely left every REAPER window, dragging back IN // mid-modal-loop delivers a CF_HDROP to REAPER's own file-import drop target rather than to our // gesture law. NOT confirmed by experiment — inferred from REAPER's handling of external file // drops, and the inferred outcome (an item at the drop point) coincides with what our own -// arrange path would have done. +// arrange path would have done. This is now the ONLY way to see that import ghost mid-drag; +// reaching it from a surface still inside REAPER was the defect pointerOffHost closed. void handOffToOs() { // Resolve BEFORE tearing anything down (the resolver reads the live drag payload), then let // the pure rule couple the two side effects: an unresolvable payload leaves the internal diff --git a/tests/test_drag_out.cpp b/tests/test_drag_out.cpp index 5cac9fb..fd5e597 100644 --- a/tests/test_drag_out.cpp +++ b/tests/test_drag_out.cpp @@ -2,8 +2,9 @@ // as the sibling pure tests: assert the gesture law and the path-list assembly directly. // // Covers: -// * The full class matrix: every ReaperSurface x {single, multi} x {track, no track}, inside -// and outside the client rect, plus the half-open edge and a non-zero panel origin. +// * The full class matrix: every ReaperSurface x {single, multi} x {track, no track} x +// {over a host window, off it}, inside and outside the client rect, plus the half-open edge +// and a non-zero panel origin. // * Reversibility and speed-independence, stated as properties: a class transition sequence // resolves the same forwards and backwards, and one unresolvable evaluation cannot change // any later evaluation's outcome. @@ -28,7 +29,8 @@ static int g_fail = 0; static const PanelClientRect kPanel{0, 0, 400, 300}; -// A live single-card drag over `surface`, with a track resolved unless stated otherwise. +// A live single-card drag over `surface`, still inside REAPER, with a track resolved unless +// stated otherwise. static DropContext ctx(ReaperSurface surface, bool single = true, bool haveTrack = true) { DropContext c; c.drag = DragState{/*dragging=*/true, /*hasArmedSamples=*/true}; @@ -38,13 +40,22 @@ static DropContext ctx(ReaperSurface surface, bool single = true, bool haveTrack return c; } +// The same drag with the shell's positive off-REAPER proof set. `surface` is whatever the SDK +// hit-test last reported; the point of the gate is that it no longer matters. +static DropContext offHost(ReaperSurface surface = ReaperSurface::Other, bool single = true, + bool haveTrack = false) { + DropContext c = ctx(surface, single, haveTrack); + c.pointerOffHost = true; + return c; +} + // Every surface the law enumerates, so the matrix tests iterate rather than list. static const ReaperSurface kAllSurfaces[] = { - ReaperSurface::OffReaper, ReaperSurface::TrackPanel, ReaperSurface::FxSurface, - ReaperSurface::FxEmbed, ReaperSurface::Arrange, ReaperSurface::Other, + ReaperSurface::TrackPanel, ReaperSurface::FxSurface, ReaperSurface::FxEmbed, + ReaperSurface::Arrange, ReaperSurface::Other, }; -// Pins kAllSurfaces against ReaperSurface::Count so a 7th surface added to the enum without a +// Pins kAllSurfaces against ReaperSurface::Count so a 6th surface added to the enum without a // matching entry here fails the BUILD, not just a silently-incomplete matrix — the compiler // alone does not enforce this (no -Wswitch/-Wall or /W4 anywhere in the build; see // panel_drag.cpp's onLBtnUp for the same caveat on DropClass). @@ -66,6 +77,10 @@ static void testInsideClientIsAlwaysInternal() { CHECK(decideDropClass(200, 150, kPanel, ctx(s, single)) == DropClass::Internal); CHECK(decideDropClass(0, 0, kPanel, ctx(s, single)) == DropClass::Internal); CHECK(decideDropClass(399, 299, kPanel, ctx(s, single)) == DropClass::Internal); + // Even a stale off-host proof cannot reinterpret a point inside our own client rect: + // the inside-client answer is resolved ahead of the hand-off gate, so re-entering the + // panel always resumes the bank-to-bank gesture. + CHECK(decideDropClass(200, 150, kPanel, offHost(s, single)) == DropClass::Internal); } } } @@ -73,7 +88,7 @@ static void testInsideClientIsAlwaysInternal() { // The half-open boundary: x+width and y+height are OUTSIDE, the pixel just inside is Internal — // matches the panel's other hit-tests so the edge is claimed consistently. static void testBoundaryHalfOpen() { - const DropContext off = ctx(ReaperSurface::OffReaper); + const DropContext off = offHost(); CHECK(decideDropClass(399, 150, kPanel, off) == DropClass::Internal); CHECK(decideDropClass(400, 150, kPanel, off) == DropClass::OsHandoff); CHECK(decideDropClass(200, 299, kPanel, off) == DropClass::Internal); @@ -83,7 +98,7 @@ static void testBoundaryHalfOpen() { // A non-zero panel origin — the boundary tracks the rect, not the absolute axes. static void testOffsetPanelRect() { const PanelClientRect p{50, 20, 100, 80}; // spans x[50,150) y[20,100) - const DropContext off = ctx(ReaperSurface::OffReaper); + const DropContext off = offHost(); CHECK(decideDropClass(100, 60, p, off) == DropClass::Internal); CHECK(decideDropClass(49, 60, p, off) == DropClass::OsHandoff); CHECK(decideDropClass(150, 60, p, off) == DropClass::OsHandoff); @@ -129,9 +144,8 @@ static void testSingleOverOtherReaperUiRefuses() { } // Off REAPER entirely -> the OS drag-out, the one irreversible transition. -static void testSingleOffReaperIsOsHandoff() { - CHECK(decideDropClass(kOutX, kOutY, kPanel, ctx(ReaperSurface::OffReaper)) == - DropClass::OsHandoff); +static void testSingleOffHostIsOsHandoff() { + CHECK(decideDropClass(kOutX, kOutY, kPanel, offHost()) == DropClass::OsHandoff); } // --- Matrix: outside the client, multi card ------------------------------------ @@ -154,8 +168,8 @@ static void testMultiOverArrangeIsArrangeInsert() { } // Multi off REAPER is the classic multi-file drag-out, unchanged. -static void testMultiOffReaperIsOsHandoff() { - CHECK(decideDropClass(kOutX, kOutY, kPanel, ctx(ReaperSurface::OffReaper, false)) == +static void testMultiOffHostIsOsHandoff() { + CHECK(decideDropClass(kOutX, kOutY, kPanel, offHost(ReaperSurface::Other, false)) == DropClass::OsHandoff); } @@ -180,11 +194,36 @@ static void testNullTrackWithSurfaceRefuses() { } } -// A track under the pointer never turns OffReaper into a REAPER-internal outcome: OffReaper is -// the shell's "the info string was empty and there was no track" verdict, and the law trusts it. -static void testOffReaperIgnoresTrackFlag() { - CHECK(decideDropClass(kOutX, kOutY, kPanel, - ctx(ReaperSurface::OffReaper, true, false)) == DropClass::OsHandoff); +// --- The hand-off gate --------------------------------------------------------- + +// THE REGRESSION FLOOR. No reading of REAPER's hit-test can reach the modal OLE loop while the +// pointer is over a host window — asserted over EVERY surface the probe can report, both payload +// sizes and both track verdicts, not just the toolbar/transport cell that was reported. The +// defect was structural (an unnamed token read as "the user left REAPER"), so the guarantee has +// to be structural too. +static void testNoInReaperCombinationCanHandOff() { + for (ReaperSurface s : kAllSurfaces) { + for (bool single : {true, false}) { + for (bool haveTrack : {true, false}) { + CHECK(decideDropClass(kOutX, kOutY, kPanel, ctx(s, single, haveTrack)) != + DropClass::OsHandoff); + } + } + } +} + +// The converse: once the shell has PROVEN the pointer left, the hand-off does not depend on what +// the last hit-test happened to say or on whether a track was resolved — the gate sits ahead of +// the surface switch, so a stale surface reading cannot suppress a genuine exit. +static void testOffHostHandsOffWhateverTheSurfaceSaid() { + for (ReaperSurface s : kAllSurfaces) { + for (bool single : {true, false}) { + for (bool haveTrack : {true, false}) { + CHECK(decideDropClass(kOutX, kOutY, kPanel, offHost(s, single, haveTrack)) == + DropClass::OsHandoff); + } + } + } } // --- Not-a-drag ---------------------------------------------------------------- @@ -192,14 +231,16 @@ static void testOffReaperIgnoresTrackFlag() { // No armed samples, or not dragging -> None regardless of position or surface. static void testNoDragOrNoSamplesIsNone() { for (ReaperSurface s : kAllSurfaces) { - DropContext c = ctx(s); - c.drag = DragState{/*dragging=*/true, /*hasArmedSamples=*/false}; - CHECK(decideDropClass(kOutX, kOutY, kPanel, c) == DropClass::None); - CHECK(decideDropClass(200, 150, kPanel, c) == DropClass::None); + for (bool off : {false, true}) { + DropContext c = off ? offHost(s) : ctx(s); + c.drag = DragState{/*dragging=*/true, /*hasArmedSamples=*/false}; + CHECK(decideDropClass(kOutX, kOutY, kPanel, c) == DropClass::None); + CHECK(decideDropClass(200, 150, kPanel, c) == DropClass::None); - c.drag = DragState{/*dragging=*/false, /*hasArmedSamples=*/true}; - CHECK(decideDropClass(kOutX, kOutY, kPanel, c) == DropClass::None); - CHECK(decideDropClass(200, 150, kPanel, c) == DropClass::None); + c.drag = DragState{/*dragging=*/false, /*hasArmedSamples=*/true}; + CHECK(decideDropClass(kOutX, kOutY, kPanel, c) == DropClass::None); + CHECK(decideDropClass(200, 150, kPanel, c) == DropClass::None); + } } } @@ -219,6 +260,13 @@ static void testClassTransitionsAreReversible() { CHECK(decideDropClass(kOutX, kOutY, kPanel, fx) == DropClass::InstrumentDrop); CHECK(decideDropClass(kOutX, kOutY, kPanel, arrange) == DropClass::ArrangeInsert); CHECK(decideDropClass(200, 150, kPanel, inside) == DropClass::Internal); + + // The hand-off leg is irreversible only because the SHELL goes modal on it. The law itself + // stays stateless across it: evaluating an off-host context leaves the next in-REAPER + // evaluation exactly where it was. + CHECK(decideDropClass(kOutX, kOutY, kPanel, offHost()) == DropClass::OsHandoff); + CHECK(decideDropClass(kOutX, kOutY, kPanel, arrange) == DropClass::ArrangeInsert); + CHECK(decideDropClass(200, 150, kPanel, inside) == DropClass::Internal); } // One unresolvable evaluation (a surface with no track, which refuses) followed by a resolvable @@ -245,9 +293,10 @@ static void testDragSpeedCannotChangeTheOutcome() { const DropContext destination = ctx(ReaperSurface::TrackPanel); const DropClass flick = decideDropClass(kOutX, kOutY, kPanel, destination); - // The slow path crosses everything else first. + // The slow path crosses everything else first, off-host legs included. for (ReaperSurface s : kAllSurfaces) { (void)decideDropClass(kOutX - 10, kOutY, kPanel, ctx(s)); + (void)decideDropClass(kOutX - 10, kOutY, kPanel, offHost(s)); (void)decideDropClass(200, 150, kPanel, ctx(s)); // and back through the client } CHECK(decideDropClass(kOutX, kOutY, kPanel, destination) == flick); @@ -262,9 +311,13 @@ static void testNoLiveOutsideCombinationResolvesToNone() { for (ReaperSurface s : kAllSurfaces) { for (bool single : {true, false}) { for (bool haveTrack : {true, false}) { - const DropClass c = decideDropClass(kOutX, kOutY, kPanel, ctx(s, single, haveTrack)); - CHECK(c != DropClass::None); - CHECK(c != DropClass::Internal); + for (bool off : {false, true}) { + const DropClass c = decideDropClass( + kOutX, kOutY, kPanel, + off ? offHost(s, single, haveTrack) : ctx(s, single, haveTrack)); + CHECK(c != DropClass::None); + CHECK(c != DropClass::Internal); + } } } } @@ -409,15 +462,16 @@ int main() { testSingleOverFxEmbedRefuses(); testSingleOverArrangeIsArrangeInsert(); testSingleOverOtherReaperUiRefuses(); - testSingleOffReaperIsOsHandoff(); + testSingleOffHostIsOsHandoff(); testMultiOverInstrumentSurfacesRefuses(); testMultiOverArrangeIsArrangeInsert(); - testMultiOffReaperIsOsHandoff(); + testMultiOffHostIsOsHandoff(); testMultiOverOtherReaperUiRefuses(); testNullTrackWithSurfaceRefuses(); - testOffReaperIgnoresTrackFlag(); + testNoInReaperCombinationCanHandOff(); + testOffHostHandsOffWhateverTheSurfaceSaid(); testNoDragOrNoSamplesIsNone(); testClassTransitionsAreReversible(); diff --git a/tests/test_instrument_drop.cpp b/tests/test_instrument_drop.cpp index 157327d..7971e07 100644 --- a/tests/test_instrument_drop.cpp +++ b/tests/test_instrument_drop.cpp @@ -186,39 +186,39 @@ static void testBadClassIdRejected() { using ui::ReaperSurface; -static ReaperSurface onTrack(const std::string& info) { - return classifyReaperSurface(info, /*haveTrack=*/true); +static ReaperSurface surfaceFor(const std::string& info) { + return classifyReaperSurface(info); } // The FX chain / floating-FX windows. static void testFxWindowIsFxSurface() { - CHECK(onTrack("fx_chain") == ReaperSurface::FxSurface); - CHECK(onTrack("fx_0") == ReaperSurface::FxSurface); - CHECK(onTrack("fx_12") == ReaperSurface::FxSurface); + CHECK(surfaceFor("fx_chain") == ReaperSurface::FxSurface); + CHECK(surfaceFor("fx_0") == ReaperSurface::FxSurface); + CHECK(surfaceFor("fx_12") == ReaperSurface::FxSurface); } // The FX-button family within the TCP/MCP — the surface the glyph-only rule used to be limited // to, still an instrument surface (as TrackPanel, which resolves identically). static void testTcpMcpFxFamilyIsTrackPanel() { - CHECK(onTrack("tcp.fx") == ReaperSurface::TrackPanel); - CHECK(onTrack("mcp.fx") == ReaperSurface::TrackPanel); - CHECK(onTrack("tcp.fxbyp") == ReaperSurface::TrackPanel); - CHECK(onTrack("tcp.fxparm") == ReaperSurface::TrackPanel); - CHECK(onTrack("mcp.fxlist") == ReaperSurface::TrackPanel); - CHECK(onTrack("tcp.fx.1") == ReaperSurface::TrackPanel); // appended info - CHECK(onTrack("tcp.fx extra") == ReaperSurface::TrackPanel); // appended info, arbitrary form + CHECK(surfaceFor("tcp.fx") == ReaperSurface::TrackPanel); + CHECK(surfaceFor("mcp.fx") == ReaperSurface::TrackPanel); + CHECK(surfaceFor("tcp.fxbyp") == ReaperSurface::TrackPanel); + CHECK(surfaceFor("tcp.fxparm") == ReaperSurface::TrackPanel); + CHECK(surfaceFor("mcp.fxlist") == ReaperSurface::TrackPanel); + CHECK(surfaceFor("tcp.fx.1") == ReaperSurface::TrackPanel); // appended info + CHECK(surfaceFor("tcp.fx extra") == ReaperSurface::TrackPanel); // appended info, arbitrary form } // THE ROOT-CAUSE FIX: the bare panel token and every non-FX sub-element are now the instrument // hotspot too. A TCP too narrow to draw the FX button reports "tcp", which under the old // glyph-only rule produced a cue-less no-op. static void testWholeTrackPanelIsTheHotspot() { - CHECK(onTrack("tcp") == ReaperSurface::TrackPanel); // bare track control panel - CHECK(onTrack("mcp") == ReaperSurface::TrackPanel); // bare mixer control panel - CHECK(onTrack("tcp.mute") == ReaperSurface::TrackPanel); // mute button - CHECK(onTrack("tcp.vol") == ReaperSurface::TrackPanel); // volume fader - CHECK(onTrack("tcp.meter") == ReaperSurface::TrackPanel); // meter - CHECK(onTrack("tcp.f") == ReaperSurface::TrackPanel); // truncated token — still the panel + CHECK(surfaceFor("tcp") == ReaperSurface::TrackPanel); // bare track control panel + CHECK(surfaceFor("mcp") == ReaperSurface::TrackPanel); // bare mixer control panel + CHECK(surfaceFor("tcp.mute") == ReaperSurface::TrackPanel); // mute button + CHECK(surfaceFor("tcp.vol") == ReaperSurface::TrackPanel); // volume fader + CHECK(surfaceFor("tcp.meter") == ReaperSurface::TrackPanel); // meter + CHECK(surfaceFor("tcp.f") == ReaperSurface::TrackPanel); // truncated token — still the panel } // The embed strip is where a ReaSampler 9000 instance already draws inline via @@ -226,44 +226,42 @@ static void testWholeTrackPanelIsTheHotspot() { // instance on the first. Matched before the "tcp"/"mcp" rule, so widening the panel hotspot // cannot swallow it; do not reorder these two checks in the classifier. static void testEmbedStripIsItsOwnSurface() { - CHECK(onTrack("tcp.fxembed") == ReaperSurface::FxEmbed); - CHECK(onTrack("mcp.fxembed") == ReaperSurface::FxEmbed); - CHECK(onTrack("tcp.fxembed.1") == ReaperSurface::FxEmbed); - CHECK(onTrack("mcp.fxembed extra") == ReaperSurface::FxEmbed); + CHECK(surfaceFor("tcp.fxembed") == ReaperSurface::FxEmbed); + CHECK(surfaceFor("mcp.fxembed") == ReaperSurface::FxEmbed); + CHECK(surfaceFor("tcp.fxembed.1") == ReaperSurface::FxEmbed); + CHECK(surfaceFor("mcp.fxembed extra") == ReaperSurface::FxEmbed); } // The arrange, including a token with appended information. static void testArrangeIsArrange() { - CHECK(onTrack("arrange") == ReaperSurface::Arrange); - CHECK(onTrack("arrange extra") == ReaperSurface::Arrange); + CHECK(surfaceFor("arrange") == ReaperSurface::Arrange); + CHECK(surfaceFor("arrange extra") == ReaperSurface::Arrange); } // Anything else REAPER names is Other — a defined refusal, never a guessed outcome. Includes // tokens REAPER may add in future versions. static void testUnnamedReaperSurfacesAreOther() { - CHECK(onTrack("spacer_0") == ReaperSurface::Other); - CHECK(onTrack("trans") == ReaperSurface::Other); - CHECK(onTrack("envcp") == ReaperSurface::Other); - CHECK(onTrack("ruler") == ReaperSurface::Other); - CHECK(onTrack("something_reaper_adds_in_2030") == ReaperSurface::Other); + CHECK(surfaceFor("spacer_0") == ReaperSurface::Other); + CHECK(surfaceFor("trans") == ReaperSurface::Other); + CHECK(surfaceFor("envcp") == ReaperSurface::Other); + CHECK(surfaceFor("ruler") == ReaperSurface::Other); + CHECK(surfaceFor("something_reaper_adds_in_2030") == ReaperSurface::Other); } -// The empty info string splits on whether a track came back with it. No track means the pointer -// has left REAPER (the OS hand-off's trigger); a track with no info means we are over REAPER on -// a surface we cannot name, which must refuse rather than be treated as off-REAPER. -static void testEmptyInfoSplitsOnTrackPresence() { - CHECK(classifyReaperSurface("", /*haveTrack=*/false) == ReaperSurface::OffReaper); - CHECK(classifyReaperSurface("", /*haveTrack=*/true) == ReaperSurface::Other); +// An empty info string is Other, NOT an off-REAPER verdict. GetThingFromPoint documents no +// off-REAPER return at all, so its silence over the transport, the toolbar or the docker chrome +// says only "nothing I name" — reading it as "the user left REAPER" is what handed live drags to +// OLE mid-gesture. Whether the pointer left REAPER is a window-ownership fact the shell proves +// separately (drag_out_win::pointerOverHostWindow) and feeds to the law as +// DropContext::pointerOffHost; no classifier output can produce a hand-off on its own. +static void testEmptyInfoIsOtherNotOffReaper() { + CHECK(surfaceFor("") == ReaperSurface::Other); } -// The SDK's documented null-track-with-valid-info case: the surface is read from the string -// alone, so the classifier reports it faithfully and the gesture law decides what a missing -// track means for that surface. -static void testNullTrackStillClassifiesTheSurface() { - CHECK(classifyReaperSurface("arrange", false) == ReaperSurface::Arrange); - CHECK(classifyReaperSurface("tcp", false) == ReaperSurface::TrackPanel); - CHECK(classifyReaperSurface("fx_chain", false) == ReaperSurface::FxSurface); -} +// There is deliberately no second empty-info case: track presence stopped being an input to the +// classifier when the off-REAPER verdict left the vocabulary. "No classifier output can trigger a +// hand-off" is now structural (no such member exists; test_drag_out's static_assert pins the +// member count) rather than something a per-token loop could falsify. // Do not reintroduce a per-surface "capture carries" loop test: buildInstrumentDropPreset takes // only sampleId (proven by testPresetRoundTripsThroughInstrumentReader), and per-surface @@ -324,8 +322,7 @@ int main() { testEmbedStripIsItsOwnSurface(); testArrangeIsArrange(); testUnnamedReaperSurfacesAreOther(); - testEmptyInfoSplitsOnTrackPresence(); - testNullTrackStillClassifiesTheSurface(); + testEmptyInfoIsOtherNotOffReaper(); testAddFailureLeavesNothingToRollBack(); testPresetFailureRollsBackTheCreatedIndex(); From b71a05fbefad749687aab2af6c34bf6b500208d8 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 16:42:59 -0400 Subject: [PATCH 2/2] Close the bridged-process hole in the drag-out hand-off gate OsHandoff now needs the window-ownership proof AND a hit-test that named nothing, so a bridged plugin's UI can't read as off-REAPER. --- src/core/ui/CLAUDE.md | 10 +++-- src/core/ui/drag_out.cpp | 14 +++++-- src/core/ui/drag_out.h | 27 ++++++------ src/core/wire/CLAUDE.md | 2 +- src/core/wire/instrument_drop.h | 7 +--- src/shell/actions/CLAUDE.md | 4 +- src/shell/actions/drag_out_win.cpp | 14 ++++--- src/shell/actions/drag_out_win.h | 13 +++--- src/shell/actions/instrument_drop_win.h | 4 +- src/shell/panel/panel_drag.cpp | 12 +++--- tests/test_drag_out.cpp | 55 +++++++++++++++++++++---- tests/test_instrument_drop.cpp | 15 +++---- 12 files changed, 113 insertions(+), 64 deletions(-) diff --git a/src/core/ui/CLAUDE.md b/src/core/ui/CLAUDE.md index 422db3c..b323abd 100644 --- a/src/core/ui/CLAUDE.md +++ b/src/core/ui/CLAUDE.md @@ -90,8 +90,12 @@ L7 sub-pass, 2026-07-27): REAPER entirely is the one irreversible transition, because the OS hand-off goes modal — so it must be PROVEN (a window-ownership test in the shell), never inferred from a hit-test token REAPER could not name; an unrecognised surface *inside* REAPER - refuses. A first-move class lock and a drag-lifetime "cannot hand off" latch both - existed here and were removed — do not reintroduce either. + refuses. That proof is necessary but NOT sufficient: a plugin bridged into + `reaper_host*.exe` fails the process test while still being REAPER's own UI, so the + hand-off additionally requires REAPER's hit-test to have named nothing — a recognised + token can only ever veto the hand-off, never produce one. A first-move class lock and + a drag-lifetime "cannot hand off" latch both existed here and were removed — do not + reintroduce either. ## Modules @@ -99,7 +103,7 @@ L7 sub-pass, 2026-07-27): - `bank_grid` — REAPER-free grid layout, selection, keyboard-nav, and thumbnail-cache-key logic for the docked bank panel. - `tab_strip` — REAPER-free scrollable tab-strip layout + hit-test for the named-banks strip. - `prune_button` — pure layout/hit-test for the `bank_panel` footer Prune button. -- `drag_out` — the pure drag-out gesture law plus path-list assembly. Owns the `ReaperSurface` vocabulary (TrackPanel / FxSurface / FxEmbed / Arrange / Other — `core/wire/instrument_drop` classifies REAPER's info token INTO it; there is deliberately no off-REAPER member, since no token can carry that fact), `decideDropClass` (surface × single-vs-multi payload → Internal / InstrumentDrop / ArrangeInsert / Refuse / OsHandoff / None), and `cueForDropClass`. `OsHandoff` is reachable ONLY through `DropContext::pointerOffHost`, a positive window-ownership proof the shell owes the law (`drag_out_win::pointerOverHostWindow`), gated ahead of the surface switch — so no reading of REAPER's hit-test can give a live drag away to OLE. **No `DropClass` means "nothing happens"**: a surface with no defined outcome for the payload resolves to `Refuse`, which the shell shows as a cursor, so "no silent no-op release" is a property of the enumeration rather than of any call site. +- `drag_out` — the pure drag-out gesture law plus path-list assembly. Owns the `ReaperSurface` vocabulary (TrackPanel / FxSurface / FxEmbed / Arrange / Other — `core/wire/instrument_drop` classifies REAPER's info token INTO it; there is deliberately no off-REAPER member, since no token can carry that fact), `decideDropClass` (surface × single-vs-multi payload → Internal / InstrumentDrop / ArrangeInsert / Refuse / OsHandoff / None), and `cueForDropClass`. `OsHandoff` is reachable ONLY through `DropContext::pointerOffHost` — a positive window-ownership proof the shell owes the law (`drag_out_win::pointerOverHostWindow`) — AND only where REAPER's own hit-test named nothing (`Other`, no track), gated ahead of the surface switch, so neither evidence source can give a live drag away to OLE on its own. **No `DropClass` means "nothing happens"**: a surface with no defined outcome for the payload resolves to `Refuse`, which the shell shows as a cursor, so "no silent no-op release" is a property of the enumeration rather than of any call site. - `theme` — pure palette module: role→color mapping, REAPER-grey neutral ladder + the pastel accent system, the keyboard strip's spectral ramp, WCAG contrast-floor helpers + `compositeOver` (the effective color of a translucent fill, so alpha overlays are testable). Only the ramp's MID stop is its own constant; lo/hi are still aliases of `accent/primary`/`accent/tertiary`, so a categorical accent move CAN still reorder the ramp — `testSpectralRampLuminanceIsMonotonic` is the build-time catch, not the structure. - `component_geometry` — pure button/slider/list-row geometry + hover hit-test helpers. - `action_bar` — pure task-grouped action-bar layout/hit-test: clusters (Capture / Placement / Maintenance / Tagging / Switching). diff --git a/src/core/ui/drag_out.cpp b/src/core/ui/drag_out.cpp index 39d7184..9917eeb 100644 --- a/src/core/ui/drag_out.cpp +++ b/src/core/ui/drag_out.cpp @@ -24,10 +24,16 @@ DropClass decideDropClass(int px, int py, const PanelClientRect& client, if (!ctx.drag.dragging || !ctx.drag.hasArmedSamples) return DropClass::None; if (insideClient(px, py, client)) return DropClass::Internal; - // The hand-off gate sits AHEAD of the surface switch and is the only route to OsHandoff, so - // no reading of REAPER's hit-test — recognised, unrecognised, or empty — can reach the modal - // OLE loop while the pointer is still over a host window. - if (ctx.pointerOffHost) return DropClass::OsHandoff; + // The ONE route to OsHandoff, ahead of the surface switch, and it needs BOTH halves. The + // shell's ownership proof alone is not sufficient: REAPER runs a bridged plugin's UI in a + // separate process, so a window failing that test can still be REAPER's own UI. A recognised + // token therefore VETOES the hand-off instead of producing one — a veto can only move the + // answer toward staying inside REAPER, so pointerOffHost remains the only member whose true + // value can reach OLE. An off-host point REAPER DID name falls through to that surface's own + // outcome below: over a bridged FX window that is the instrument drop, as it is for a native + // one. + if (ctx.pointerOffHost && ctx.surface == ReaperSurface::Other && !ctx.haveTrack) + return DropClass::OsHandoff; switch (ctx.surface) { case ReaperSurface::TrackPanel: diff --git a/src/core/ui/drag_out.h b/src/core/ui/drag_out.h index 3c30c6a..96d8b9e 100644 --- a/src/core/ui/drag_out.h +++ b/src/core/ui/drag_out.h @@ -6,10 +6,9 @@ // // THE LAW: the class is resolved from what is under the cursor on EVERY move; every transition // is reversible until release or until the pointer leaves REAPER entirely; the OS hand-off is -// reserved for leaving REAPER — which the shell must PROVE (DropContext::pointerOffHost), never -// infer from a hit-test token it could not name — and every REAPER-internal target executes -// natively on release. Do not reintroduce a first-move class lock or a drag-lifetime "blocked" -// latch. +// reserved for leaving REAPER (DropContext::pointerOffHost owns what proves that) and every +// REAPER-internal target executes natively on release. Do not reintroduce a first-move class +// lock or a drag-lifetime "blocked" latch. // // Path-list assembly: turns armed sample ids into the absolute path list an OS drop carries // (Windows CF_HDROP / macOS file-list pasteboard) — set algebra only; the shell resolves each id @@ -34,10 +33,8 @@ struct DragState { }; // What REAPER reports under the pointer, reduced to the surfaces the law distinguishes. -// Produced from GetThingFromPoint's info token by wire::classifyReaperSurface. There is -// deliberately NO off-REAPER member: this vocabulary describes what REAPER's own hit-test -// named, and "the pointer left REAPER" is a window-ownership fact no token can carry — -// it enters through DropContext::pointerOffHost instead. +// Produced from GetThingFromPoint's info token by wire::classifyReaperSurface. No off-REAPER +// member: that fact enters through DropContext::pointerOffHost, which owns the reasoning. enum class ReaperSurface { TrackPanel, // TCP/MCP, ANY sub-element: the WHOLE panel is the instrument hotspot FxSurface, // fx_* — the FX chain and floating-FX windows @@ -68,12 +65,14 @@ struct DropContext { bool singlePayload = false; // payload holds EXACTLY one capture (arms InstrumentDrop) ReaperSurface surface = ReaperSurface::Other; // only read outside the client rect bool haveTrack = false; // GetThingFromPoint returned a non-null MediaTrack* - // The ONE gate on the irreversible OS hand-off, and the only member whose true value can - // produce it. It is a POSITIVE proof the shell owes the law — a window-ownership test - // (drag_out_win::pointerOverHostWindow), not "the hit-test named nothing I recognise", - // which is equally true of the transport, the toolbar and the docker chrome. Defaults - // false so an unset context stays inside REAPER: the failure mode of forgetting to - // resolve it must be a refusal, never a drag given away to OLE. + // THE ONE HOME for the hand-off's evidence rule. This is the only member whose true value + // can reach the irreversible OS hand-off, and it is a POSITIVE proof the shell owes the law + // — a window-ownership test (drag_out_win::pointerOverHostWindow), never "the hit-test named + // nothing I recognise", which is equally true of the transport, the toolbar and the docker + // chrome; reading an unnamed token as an exit is what handed live drags to OLE mid-gesture. + // Defaults false so an unset context stays inside REAPER: the failure mode of forgetting to + // resolve it must be a refusal, never a drag given away to OLE. NECESSARY but not sufficient + // — decideDropClass's gate carries the second half and why it needs one. bool pointerOffHost = false; }; diff --git a/src/core/wire/CLAUDE.md b/src/core/wire/CLAUDE.md index a6ef1da..aa23b19 100644 --- a/src/core/wire/CLAUDE.md +++ b/src/core/wire/CLAUDE.md @@ -81,7 +81,7 @@ This directory owns two cross-artifact contracts specifically: - `reasampler_uid.h` — SDK-free header owning the FOREVER-FROZEN VST3 class-UID integer macros (stable + beta pairs, `REASAMPLER_PROC_UID_*` / `REASAMPLER_PROC_UID_BETA_*`) and the `REASAMPLER_ACTIVE_UID_*` channel-selector macros. Split out of `reasampler_vst.h` so the pure extension side (`instrument_drop`) can derive the `.vstpreset` class-ID hex string without pulling in the VST3 SDK. Both `reasampler_vst.h` (runtime `FUID`) and `instrument_drop` (preset hex string) source from this single header — the binary identity and the preset-file identity cannot diverge. - `assignment_request` — pure ingest-assign wire: typed request record carrying the drop payload from the `ingest` shell through to the VST3 bridge. - `bake_wire` — the resample bake's request/outcome pair on ONE per-instance key (`rsbake_`): the instrument writes a `BakeRequest`, invokes the extension's action synchronously, and reads the extension's `BakeOutcome` back over the same key inside that one call. Not a handshake — a call and a return, and it must not grow a claim protocol. Also the ONE home of the bake action's command-id suffix and of the leading underscore `NamedCommandLookup` needs but `rec->Register("command_id", …)` does not, so both artifacts name one action. `BakeStatus` values are WIRE INTEGERS: never renumber, only append, and an unrecognized value decodes as `Failed` rather than as the numeric default `Ok`. It owns BOTH ends' reading of that key, since the key's contents are the only evidence either side gets: `classifyBakeAnswer` (instrument side — six kinds, of which `Unanswered`, the request still sitting there untouched, separates "nothing wrote an outcome over our key" from a refusal — it does NOT identify a landing that never ran, since a skipped key and a rejected answer-write look identical from here) and `classifyBakeScan` + `kMaxRequestAgeSeconds` (extension side — the per-key Land / RefuseWrongProject / ClearStale / IgnoreUnreadable / IgnoreNotARequest verdict over every open tab, stated without a REAPER type so the multi-tab matrix is unit-provable). `BakeScanTally` + `describeBakeScan` + `describeBakeKey` are that same reading counted and spoken — the rationale lives at the types. **The report's absence is NOT evidence the landing never ran**, and no sentence either artifact prints may say it is: `answered` is pass-wide and counts a QUEUED write, so a pass can answer some other key while skipping ours, or have our own answer's `SetProjExtState` rejected. The summary is therefore silent only when the pass answered somebody, left no key unanswered, and every answer was READ BACK from its own key; `describeBakeKey` prints one line per enumerated key regardless, which is the only thing that names WHICH key — the counts cannot. The read-back verdict behind `BakeWriteProof` is `ext_state_read.h`'s `extStateWriteLanded`, above. `bakeLandingAfterPersist` is the ONE route to a `Banked` landing: every `Land` verdict is assigned `Unpersisted` and passed through it, so no shell path — a dedup hit least of all, since its target may be an entry the same pass just added — can claim the word without the pass's persist having reported success. What `Banked` claims is exactly that `persisted` input — a saved project was active and the bank write was issued — and the enum comment is that claim's one home. -- `instrument_drop` — pure FX-drop payload builder: constructs a Steinberg-format `.vstpreset` image (channel-active class ID + the instrument's own component state, capture pre-selected) the shell applies via `TrackFX_SetPreset`; owns `classifyReaperSurface`, the prefix classifier mapping a `GetThingFromPoint` info token onto `core/ui/drag_out`'s `ReaperSurface`. Classifier ordering is load-bearing: the embed strip is matched before the `tcp`/`mcp` panel family, which claims the WHOLE track panel rather than just its FX sub-elements. An empty token is `Other`, never an off-REAPER verdict — the SDK documents no off-REAPER return, so the toolbar/transport/docker chrome all report nothing REAPER names. All-or-nothing contract — caller rolls back via `TrackFX_Delete` on any failure. +- `instrument_drop` — pure FX-drop payload builder: constructs a Steinberg-format `.vstpreset` image (channel-active class ID + the instrument's own component state, capture pre-selected) the shell applies via `TrackFX_SetPreset`; owns `classifyReaperSurface`, the prefix classifier mapping a `GetThingFromPoint` info token onto `core/ui/drag_out`'s `ReaperSurface`. Classifier ordering is load-bearing: the embed strip is matched before the `tcp`/`mcp` panel family, which claims the WHOLE track panel rather than just its FX sub-elements. An empty token is `Other`, never an off-REAPER verdict (`core/ui/CLAUDE.md` owns that rule). All-or-nothing contract — caller rolls back via `TrackFX_Delete` on any failure. - `sample_usage` — instance-usage wire: `UsageRecord`, `planUsagePublish` (fresh/heal/clean-replace/union/remint publish plan), `foldUsageRecords`/`usageHeldPaths` (liveness fold — protect-all when records exist but no instance is live; abort→protect-all on unreadable record; `counted` carries key-attributed live records), `identityMatches` (ReaSampler 9000 FX identity). REAPER-free, unit-tested. The mirror of `assignment_request` on the instrument→extension direction: the wire format and the two safety-critical decisions (what to write on publish, which records count at prune time) are pure so they are provable without a DAW. It lives here because it is a *wire format* with an instrument-side writer; the fold's output is consumed by `core/tracking`'s authority, which owns every consumer-facing decision built on it. ## Gotchas diff --git a/src/core/wire/instrument_drop.h b/src/core/wire/instrument_drop.h index 371033b..720e779 100644 --- a/src/core/wire/instrument_drop.h +++ b/src/core/wire/instrument_drop.h @@ -67,11 +67,8 @@ std::vector buildInstrumentDropPreset(const std::string& sampleId) // why such a drop landed nowhere. // "arrange*" -> Arrange. // anything else, "" INCLUDED -> Other. A surface we cannot name: refuse -// visibly, never guess an outcome. An empty token is NOT evidence the pointer -// left REAPER — the SDK documents no off-REAPER return at all, and the transport, -// toolbar, menu bar and docker chrome all report nothing REAPER names. Reading it -// as "off REAPER" is what handed live drags to OLE mid-gesture; whether the -// pointer left is a window-ownership fact, resolved in the shell. +// visibly, never guess an outcome. An empty token is NOT evidence the pointer left +// REAPER — see ui::DropContext::pointerOffHost for why that rule exists. // The exact live token is DAW-only — confirm via // reaper.GetThingFromPoint(reaper.GetMousePosition()) in ReaScript if unsure. ui::ReaperSurface classifyReaperSurface(const std::string& info); diff --git a/src/shell/actions/CLAUDE.md b/src/shell/actions/CLAUDE.md index 65ea724..35221ee 100644 --- a/src/shell/actions/CLAUDE.md +++ b/src/shell/actions/CLAUDE.md @@ -41,8 +41,8 @@ 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. - `package_export_action` — the "export bank as package" skin: survey and report first, confirm what is absent (and, separately, a destination being replaced), pick a destination, write. Every prompt in the flow lives here so `shell/package/export_bank` stays promptless. Read-only against the project — it holds the session by `const&`, so no ext-state write, generation bump or undo point is reachable. Registration rides `main.cpp`'s action table (`EXPORT_BANK_PACKAGE`); the panel's tab menu is the second skin over the same body. -- `drag_out_win` — the host/OS boundary of drag-out: Windows OLE `DoDragDrop`/`CF_HDROP`, copy-only (`DROPEFFECT_MOVE` not offered); macOS/Linux via `SWELL_InitiateDragDropOfFileList`. Also owns `pointerOverHostWindow`, the window-ownership test behind `ui::DropContext::pointerOffHost` — process identity on Windows (`WindowFromPoint` + `GetWindowThreadProcessId`), non-null `WindowFromPoint` under SWELL, whose window list is already this-process-only. -- `instrument_drop_win` — instrument-drop shell: `probeDropTarget` resolves a screen point to a track + a `ReaperSurface` (via the pure `wire::classifyReaperSurface`, whose token rules `core/wire/CLAUDE.md` owns) — it answers only what REAPER named, never whether the pointer is still in REAPER, which is `drag_out_win`'s — and the drop half 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.** +- `drag_out_win` — the host/OS boundary of drag-out: Windows OLE `DoDragDrop`/`CF_HDROP`, copy-only (`DROPEFFECT_MOVE` not offered); macOS/Linux via `SWELL_InitiateDragDropOfFileList`. Also owns `pointerOverHostWindow`, the window-ownership test behind `ui::DropContext::pointerOffHost` (`core/ui/CLAUDE.md` owns the hand-off rule it feeds) — process identity on Windows (`WindowFromPoint` + `GetWindowThreadProcessId`), non-null `WindowFromPoint` under SWELL, whose window list is already this-process-only. Its shipped exception: a plugin bridged into `reaper_host*.exe` reports off-host though the pointer never left REAPER. +- `instrument_drop_win` — instrument-drop shell: `probeDropTarget` resolves a screen point to a track + a `ReaperSurface` (via the pure `wire::classifyReaperSurface`, whose token rules `core/wire/CLAUDE.md` owns) — it answers only what REAPER named, never whether the pointer is still in REAPER — and the drop half 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.** - `arrange_drop_win` — the drag-out gesture's arrange outcome: `arrangeTimeAtScreenX` (pointer column → time via `GetSet_ArrangeView2`'s one-pixel-span reading — inferred, not SDK-documented) and `performArrangeDrop` (snap the drop time, then one `InsertMedia` per capture on the pointer's track — assumed, not confirmed, to land end-to-end via REAPER's own cursor advance — in ONE undo block, counting only InsertMedia's reported successes, with the caller's track selection and edit cursor restored). The one timeline-placing shell here, per the invariant above; it never captures and never writes the bank. - `package_import_action` — the bindable / bank-menu / file-drop skin over `shell/package`'s `importBankPackage`. Owns the **ledger gate**, which runs BEFORE the file picker (a refusal must not cost the user a file choice) and is keyed on the session's `LedgerStatus` alone — never on `PruneReport::blockedByTracking`, whose undecodable-`rsusage_*` arm governs deletion-time protection and would refuse an import that only writes birth records. Builds and shows every message the import produces, but the ledger-refusal body itself is `core/package::ledgerRefusalMessage` — a pure fold this TU only supplies the channel-correct namespace to — so the wording is assertable without a DAW. `doImportBankPackage`/`doImportBankPackageFile` return the minted bank id on a landed import (empty otherwise) so a caller can focus it; the verb itself is promptless. - `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. diff --git a/src/shell/actions/drag_out_win.cpp b/src/shell/actions/drag_out_win.cpp index 2b738fb..cb2d171 100644 --- a/src/shell/actions/drag_out_win.cpp +++ b/src/shell/actions/drag_out_win.cpp @@ -247,14 +247,18 @@ bool canInitiateDragOut(const std::vector& absolutePaths) { bool pointerOverHostWindow(int screenX, int screenY) { // Process identity, not window identity: REAPER's arrange, its floating FX and VST editor // windows, its menus and its docker chrome are separate top-level HWNDs with no common - // ancestor, so no GetAncestor/GetParent walk reaches them all — but every one of them is - // created by the process this extension is loaded into. + // ancestor, so no GetAncestor/GetParent walk reaches them all — but they are windows of the + // process this extension is loaded into, bar the bridged-plugin case the header names. // Mouse capture does not affect WindowFromPoint: it hit-tests the desktop window tree, not - // input routing. It does skip hidden and DISABLED windows, so a REAPER window disabled - // behind a modal dialog could fall through to whatever sits beneath — no such dialog can be - // up while our panel holds capture for a drag. + // input routing. It does skip hidden and DISABLED windows, so a REAPER window disabled behind + // a modal dialog raised mid-drag (a REAPER timer or another extension can raise one — capture + // is not a modality lock) would fall through to whatever sits beneath and read as off-host. + // Accepted, unverified: not reproduced, and nothing here was checked against REAPER's own + // dialog behavior. const POINT pt{screenX, screenY}; const HWND hwnd = WindowFromPoint(pt); + // Not an unknown resolved toward hand-off: a null return is the desktop tree stating there is + // no window at that point at all, which is a determinate "no host window here". if (!hwnd) return false; DWORD pid = 0; GetWindowThreadProcessId(hwnd, &pid); diff --git a/src/shell/actions/drag_out_win.h b/src/shell/actions/drag_out_win.h index e179edf..f13a9b9 100644 --- a/src/shell/actions/drag_out_win.h +++ b/src/shell/actions/drag_out_win.h @@ -33,11 +33,14 @@ bool initiateDragOut(HWND__* panelHwnd, const std::vector& absolute // probe HGLOBAL immediately; SWELL exposes no probe, so macOS/Linux reduces to the non-empty check. bool canInitiateDragOut(const std::vector& absolutePaths); -// True iff a window belonging to the host (REAPER) process sits under the screen point — -// the POSITIVE proof ui::DropContext::pointerOffHost is the negation of. Every REAPER -// surface qualifies whether or not GetThingFromPoint can name it: the arrange, a TCP, a -// floating FX or VST editor window, the transport, the toolbar, a menu, the docker chrome -// and our own panel are all windows of this process, because the extension runs inside it. +// True iff a window belonging to the host (REAPER) process sits under the screen point — the +// POSITIVE proof ui::DropContext::pointerOffHost is the negation of (that member owns the +// evidence rule). Nearly every REAPER surface qualifies whether or not GetThingFromPoint can +// name it, because the extension runs inside that process. +// The exception, and it ships: a plugin set to "Run as separate/dedicated process" draws its +// UI out of reaper_host32/64.exe, and WindowFromPoint descends to the deepest child — so this +// reports false over a bridged plugin's editor although the pointer never left REAPER. The +// law's hand-off gate absorbs that by also requiring REAPER's own hit-test to name nothing. // Side-effect-free; safe on every mouse-move. bool pointerOverHostWindow(int screenX, int screenY); diff --git a/src/shell/actions/instrument_drop_win.h b/src/shell/actions/instrument_drop_win.h index df9ca43..70125eb 100644 --- a/src/shell/actions/instrument_drop_win.h +++ b/src/shell/actions/instrument_drop_win.h @@ -35,8 +35,8 @@ struct DropProbe { // Wraps GetThingFromPoint: its track comes back as-is, its info string goes to the pure // wire::classifyReaperSurface. Cheap enough to run on every mouse-move, but the panel // evaluates it only OUTSIDE its own client rect — the internal drag never pays for it. -// It reports only what REAPER named; whether the pointer is still inside REAPER at all is -// drag_out_win::pointerOverHostWindow's, and no surface reported here implies an exit. +// Reports only what REAPER named; no output here implies an exit (ui::DropContext:: +// pointerOffHost owns that rule). DropProbe probeDropTarget(int screenX, int screenY); // Adds a fresh ReaSampler 9000 instance to `track` and applies `presetBytes` as its diff --git a/src/shell/panel/panel_drag.cpp b/src/shell/panel/panel_drag.cpp index 5e20a37..e7fc3d4 100644 --- a/src/shell/panel/panel_drag.cpp +++ b/src/shell/panel/panel_drag.cpp @@ -199,11 +199,9 @@ LiveDrop resolveLiveDrop(int x, int y) { const DropProbe probe = probeDropTarget(sp.x, sp.y); ctx.surface = probe.surface; ctx.haveTrack = probe.track != nullptr; - // Two independent probes because they answer two independent questions: REAPER's - // hit-test names the surface, window ownership decides whether we are still in REAPER - // at all. The SDK hit-test cannot answer the second — GetThingFromPoint documents no - // off-REAPER return, so its silence over the transport or the toolbar used to read as - // "the user left" and gave the live drag away to OLE. + // Two probes because they answer two independent questions: what REAPER named, and + // whether we are still inside REAPER at all. The law's hand-off gate reads both — see + // ui::DropContext::pointerOffHost. ctx.pointerOffHost = !pointerOverHostWindow(sp.x, sp.y); out.track = probe.track; out.screenX = sp.x; @@ -249,6 +247,10 @@ void handOffToOs() { invalidatePanel(); initiateDragOut(g_panel.hwnd, paths); // COPY-ONLY; blocking on Windows + + // The drag state was reset above, so the WM_LBUTTONUP that ends the modal loop takes + // onLBtnUp's not-dragging branch and never restores the arrow — do it here instead. + SetCursor(LoadCursor(nullptr, IDC_ARROW)); } // Resolves the topmost INTERACTIVE element under client (x, y) for hover feedback, diff --git a/tests/test_drag_out.cpp b/tests/test_drag_out.cpp index fd5e597..6ebb259 100644 --- a/tests/test_drag_out.cpp +++ b/tests/test_drag_out.cpp @@ -41,7 +41,8 @@ static DropContext ctx(ReaperSurface surface, bool single = true, bool haveTrack } // The same drag with the shell's positive off-REAPER proof set. `surface` is whatever the SDK -// hit-test last reported; the point of the gate is that it no longer matters. +// hit-test reported at that point — it still matters: the hand-off needs REAPER to have named +// nothing too, so the defaults here (Other, no track) are the only combination that hands off. static DropContext offHost(ReaperSurface surface = ReaperSurface::Other, bool single = true, bool haveTrack = false) { DropContext c = ctx(surface, single, haveTrack); @@ -212,20 +213,57 @@ static void testNoInReaperCombinationCanHandOff() { } } -// The converse: once the shell has PROVEN the pointer left, the hand-off does not depend on what -// the last hit-test happened to say or on whether a track was resolved — the gate sits ahead of -// the surface switch, so a stale surface reading cannot suppress a genuine exit. -static void testOffHostHandsOffWhateverTheSurfaceSaid() { +// The converse, stated as the gate's exact shape: with the ownership proof set, the hand-off +// happens EXACTLY on the cells where REAPER's hit-test also named nothing — no surface, no track. +// Both directions in one loop, so neither half can be weakened without a failure: a gate that +// dropped the token condition fails on the named cells, one that over-tightened fails on Other. +static void testOffHostHandsOffOnlyWhereReaperNamedNothing() { for (ReaperSurface s : kAllSurfaces) { for (bool single : {true, false}) { for (bool haveTrack : {true, false}) { - CHECK(decideDropClass(kOutX, kOutY, kPanel, offHost(s, single, haveTrack)) == - DropClass::OsHandoff); + const bool namedNothing = (s == ReaperSurface::Other) && !haveTrack; + const DropClass c = + decideDropClass(kOutX, kOutY, kPanel, offHost(s, single, haveTrack)); + CHECK((c == DropClass::OsHandoff) == namedNothing); } } } } +// The shipped exception the token condition exists for: REAPER runs a bridged plugin's UI in +// reaper_host*.exe, so the window-ownership test reports off-host over a floating bridged FX +// editor even though the pointer never left REAPER. There the recognised token vetoes the +// hand-off and the surface's own REAPER-internal outcome stands — identical to the native case. +static void testOffHostOverANamedSurfaceKeepsTheReaperOutcome() { + CHECK(decideDropClass(kOutX, kOutY, kPanel, offHost(ReaperSurface::FxSurface, true, true)) == + DropClass::InstrumentDrop); + CHECK(decideDropClass(kOutX, kOutY, kPanel, offHost(ReaperSurface::TrackPanel, true, true)) == + DropClass::InstrumentDrop); + CHECK(decideDropClass(kOutX, kOutY, kPanel, offHost(ReaperSurface::Arrange, true, true)) == + DropClass::ArrangeInsert); + CHECK(decideDropClass(kOutX, kOutY, kPanel, offHost(ReaperSurface::FxEmbed, true, true)) == + DropClass::Refuse); + + // And not just for those four spot values: over every named cell the ownership proof changes + // nothing at all, which is what "a veto can only move the answer toward staying in REAPER" + // means operationally. + for (ReaperSurface s : kAllSurfaces) { + if (s == ReaperSurface::Other) continue; // the one surface the gate can hand off from + for (bool single : {true, false}) { + for (bool haveTrack : {true, false}) { + CHECK(decideDropClass(kOutX, kOutY, kPanel, offHost(s, single, haveTrack)) == + decideDropClass(kOutX, kOutY, kPanel, ctx(s, single, haveTrack))); + } + } + } + // Same equality on the remaining named cell: Other WITH a track is a surface REAPER did + // attribute, so it refuses off-host exactly as it does on-host. + for (bool single : {true, false}) { + CHECK(decideDropClass(kOutX, kOutY, kPanel, + offHost(ReaperSurface::Other, single, true)) == DropClass::Refuse); + } +} + // --- Not-a-drag ---------------------------------------------------------------- // No armed samples, or not dragging -> None regardless of position or surface. @@ -471,7 +509,8 @@ int main() { testNullTrackWithSurfaceRefuses(); testNoInReaperCombinationCanHandOff(); - testOffHostHandsOffWhateverTheSurfaceSaid(); + testOffHostHandsOffOnlyWhereReaperNamedNothing(); + testOffHostOverANamedSurfaceKeepsTheReaperOutcome(); testNoDragOrNoSamplesIsNone(); testClassTransitionsAreReversible(); diff --git a/tests/test_instrument_drop.cpp b/tests/test_instrument_drop.cpp index 7971e07..b2a090e 100644 --- a/tests/test_instrument_drop.cpp +++ b/tests/test_instrument_drop.cpp @@ -248,20 +248,15 @@ static void testUnnamedReaperSurfacesAreOther() { CHECK(surfaceFor("something_reaper_adds_in_2030") == ReaperSurface::Other); } -// An empty info string is Other, NOT an off-REAPER verdict. GetThingFromPoint documents no -// off-REAPER return at all, so its silence over the transport, the toolbar or the docker chrome -// says only "nothing I name" — reading it as "the user left REAPER" is what handed live drags to -// OLE mid-gesture. Whether the pointer left REAPER is a window-ownership fact the shell proves -// separately (drag_out_win::pointerOverHostWindow) and feeds to the law as -// DropContext::pointerOffHost; no classifier output can produce a hand-off on its own. +// An empty info string is Other, NOT an off-REAPER verdict — ui::DropContext::pointerOffHost +// owns why. static void testEmptyInfoIsOtherNotOffReaper() { CHECK(surfaceFor("") == ReaperSurface::Other); } -// There is deliberately no second empty-info case: track presence stopped being an input to the -// classifier when the off-REAPER verdict left the vocabulary. "No classifier output can trigger a -// hand-off" is now structural (no such member exists; test_drag_out's static_assert pins the -// member count) rather than something a per-token loop could falsify. +// There is deliberately no second empty-info case: track presence is not an input to the +// classifier, and "no classifier output alone can trigger a hand-off" is structural (no such +// member exists) rather than something a per-token loop could falsify. // Do not reintroduce a per-surface "capture carries" loop test: buildInstrumentDropPreset takes // only sampleId (proven by testPresetRoundTripsThroughInstrumentReader), and per-surface