From 08007608336b73684634517e831420c1a1dc01cd Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Wed, 29 Jul 2026 23:49:53 -0400 Subject: [PATCH 1/3] Fix drag-out losing audio and FX-container drops losing the capture; re-home ingest under shell/actions --- CMakeLists.txt | 2 +- src/app/main.cpp | 2 +- src/core/ui/drag_out.cpp | 7 +++ src/core/ui/drag_out.h | 15 +++++ src/core/wire/instrument_drop.cpp | 11 ++++ src/core/wire/instrument_drop.h | 21 +++++++ src/shell/actions/CLAUDE.md | 12 ++-- src/shell/actions/drag_out_win.cpp | 43 +++++++++---- src/{ => shell/actions}/ingest.cpp | 2 +- src/{ => shell/actions}/ingest.h | 0 src/shell/actions/instrument_drop_win.cpp | 35 +++++++---- src/shell/capture/capture_orchestrator.cpp | 2 +- src/shell/panel/panel_drag.cpp | 37 ++++++++--- src/shell/panel/panel_window.cpp | 2 +- tests/test_drag_out.cpp | 51 +++++++++++++++ tests/test_instrument_drop.cpp | 72 ++++++++++++++++++++++ 16 files changed, 269 insertions(+), 45 deletions(-) rename src/{ => shell/actions}/ingest.cpp (99%) rename src/{ => shell/actions}/ingest.h (100%) diff --git a/CMakeLists.txt b/CMakeLists.txt index c057d46..3b55909 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1129,7 +1129,7 @@ add_library(reaper_reasampler MODULE src/shell/actions/design_view_actions.cpp src/shell/actions/bank_actions.cpp src/shell/actions/prune_action.cpp - src/ingest.cpp + src/shell/actions/ingest.cpp src/core/model/bank_book.cpp src/core/model/bank_book_json.cpp src/core/model/owned_manifest.cpp diff --git a/src/app/main.cpp b/src/app/main.cpp index 65469e9..2361822 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -22,7 +22,7 @@ #include "core/capture/render_settings.h" // captureActionTable #include "core/version/app_version.h" // appVersion -#include "ingest.h" +#include "shell/actions/ingest.h" #include "shell/actions/action_registry.h" // the registration table #include "shell/actions/bank_actions.h" // multi-bank action family #include "shell/actions/design_view_actions.h" // Design View action family diff --git a/src/core/ui/drag_out.cpp b/src/core/ui/drag_out.cpp index 1a84c94..c817a31 100644 --- a/src/core/ui/drag_out.cpp +++ b/src/core/ui/drag_out.cpp @@ -48,4 +48,11 @@ PathList assemblePathList(const std::vector& resolved) { return out; } +OsHandoff decideOsHandoff(const std::vector& paths) { + OsHandoff out; + out.startOsDrag = !paths.empty(); + out.releaseInternalDrag = out.startOsDrag; + return out; +} + } // namespace reasampler::ui diff --git a/src/core/ui/drag_out.h b/src/core/ui/drag_out.h index b859945..5c6ccf0 100644 --- a/src/core/ui/drag_out.h +++ b/src/core/ui/drag_out.h @@ -69,4 +69,19 @@ struct PathList { // exact-string — the shell normalizes case/slashes upstream if it wants Windows-style dedup. PathList assemblePathList(const std::vector& resolved); +// --- OS hand-off ordering ----------------------------------------------------- + +// The two side effects the shell performs when a drag crosses out of REAPER. They are ONE +// decision, not two: winding the internal drag down (release capture, clear drag state) for a +// hand-off that then cannot happen consumes the gesture — the user sees a drag that silently +// did nothing and drags again. Never release without starting. +struct OsHandoff { + bool startOsDrag = false; // hand `paths` to the OS drag initiator + bool releaseInternalDrag = false; // first wind down mouse capture + panel drag state +}; + +// Decides the hand-off from the assembled path list. Empty (everything stale/unresolvable) +// means the internal drag stays live rather than dying half-torn-down. +OsHandoff decideOsHandoff(const std::vector& paths); + } // namespace reasampler::ui diff --git a/src/core/wire/instrument_drop.cpp b/src/core/wire/instrument_drop.cpp index 3732b32..9c3cfda 100644 --- a/src/core/wire/instrument_drop.cpp +++ b/src/core/wire/instrument_drop.cpp @@ -76,6 +76,17 @@ std::vector buildInstrumentDropPreset(const std::string& sampleId) return buildVstPresetBytes(vstClassIdHex(), instrumentDropStateBytes(sampleId)); } +DropOutcome decideDropOutcome(const DropAttempt& attempt) { + DropOutcome out; + if (attempt.addedFxIndex < 0) return out; // add failed — nothing exists to roll back + if (!attempt.presetApplied) { + out.rollbackFxIndex = attempt.addedFxIndex; + return out; + } + out.loaded = true; + return out; +} + bool infoNamesFxHotspot(const std::string& info) { // See the header contract for the prefix rule and the embed-strip exclusion. auto startsWith = [&info](const char* p) { return info.rfind(p, 0) == 0; }; diff --git a/src/core/wire/instrument_drop.h b/src/core/wire/instrument_drop.h index 7363964..27b2296 100644 --- a/src/core/wire/instrument_drop.h +++ b/src/core/wire/instrument_drop.h @@ -68,4 +68,25 @@ bool infoNamesFxHotspot(const std::string& info); // assert the capture is selected. Not called by the shell. std::vector instrumentDropStateBytes(const std::string& sampleId); +// --- All-or-nothing rollback -------------------------------------------------- + +// What the shell observed while executing one drop, reduced to the two REAPER +// results the contract turns on. +struct DropAttempt { + int addedFxIndex = -1; // TrackFX_AddByName's return; < 0 = nothing was created + bool presetApplied = false; // TrackFX_SetPreset's return +}; + +// The verdict. `rollbackFxIndex >= 0` obliges the caller to TrackFX_Delete it before +// returning — an instance whose capture never landed must not survive the drop. +struct DropOutcome { + bool loaded = false; + int rollbackFxIndex = -1; +}; + +// Pure so the contract is provable without a DAW: the rollback obligation is decided +// here, not inline in the shell, and holds identically for every drop surface (FX +// button, FX chain/container, Media-Explorer import). +DropOutcome decideDropOutcome(const DropAttempt& attempt); + } // namespace reasampler::wire diff --git a/src/shell/actions/CLAUDE.md b/src/shell/actions/CLAUDE.md index 338801c..f28e6ce 100644 --- a/src/shell/actions/CLAUDE.md +++ b/src/shell/actions/CLAUDE.md @@ -37,13 +37,11 @@ is owned by other directories and only skinned here. ## Gotchas -- **Structural wart, not yet fixed:** `ingest.cpp` / `ingest.h`, plus `ext_keys.h` - and `resource.h`, physically live at `src/` root rather than under - `shell/actions/` — Phase Q's reorg did not re-home these files into - `core/`/`shell/`/`app/`. `ingest` is documented here as its nearest sibling by - role, but the files themselves are not in this directory. This is a code - organization issue, not a documentation one — see Open questions in the - originating dispatch report. +- **Structural wart, partly closed:** `ingest.cpp` / `ingest.h` now live in this + directory. `ext_keys.h` and `resource.h` still sit at `src/` root: `ext_keys.h` + is consumed mostly from `shell/instrument/`, so it is not this directory's to + claim, and `resource.h` is a build input paired with `src/resource.rc` (the SWELL + resgen step) rather than a shell module. - Media-Explorer import is single-file, pull-on-action (`OpenMediaExplorer` + `MediaExplorerGetLastPlayedFileInfo`) — there is no enumerate-selected-files or register-a-drop-handler API on the Media Explorer surface. diff --git a/src/shell/actions/drag_out_win.cpp b/src/shell/actions/drag_out_win.cpp index 71b92f1..6abe66a 100644 --- a/src/shell/actions/drag_out_win.cpp +++ b/src/shell/actions/drag_out_win.cpp @@ -62,6 +62,29 @@ HGLOBAL buildHDrop(const std::vector& paths) { return h; } +// COM reference counts MUST be interlocked. A CF_HDROP target is free to marshal the data +// object into another apartment and finish the copy on a background thread AFTER DoDragDrop +// has returned (Explorer's async file copy does exactly this). A plain ++/-- there races the +// source thread's post-DoDragDrop Release: one lost increment destroys the object — and with +// it the source HGLOBAL — before the target reads it, and the drop lands with no file. That +// race is intermittent and a retry usually wins it; do not "simplify" these back. +inline ULONG comAddRef(volatile LONG& refs) { + return static_cast(InterlockedIncrement(&refs)); +} + +// DoDragDrop requires the calling thread to be OLE-initialized — CoInitialize alone is not +// enough, and an uninitialized thread fails the call outright, so the drag never starts. +// Relying on REAPER having done it is a first-use hazard: whether it has depends on what else +// ran first in the session. OleInitialize is per-thread refcounted, so this is additive to +// whatever the host did; we deliberately never OleUninitialize — the extension lives for the +// process, and unbalancing a REAPER-owned apartment is the hazard worth avoiding, not this. +// RPC_E_CHANGED_MODE means the thread joined an MTA, where OLE drag-drop is unavailable. +bool ensureOleForThisThread() { + static thread_local int state = 0; // 0 untried, 1 ready, -1 unavailable + if (state == 0) state = SUCCEEDED(OleInitialize(nullptr)) ? 1 : -1; + return state > 0; +} + // Minimal IDropSource: continue until the (left) button releases or Escape cancels; always // request the copy cursor. This is the standard textbook drop source — no custom feedback. class DropSource final : public IDropSource { @@ -76,11 +99,11 @@ public: *ppv = nullptr; return E_NOINTERFACE; } - ULONG STDMETHODCALLTYPE AddRef() override { return ++refs_; } + ULONG STDMETHODCALLTYPE AddRef() override { return comAddRef(refs_); } ULONG STDMETHODCALLTYPE Release() override { - const ULONG r = --refs_; + const LONG r = InterlockedDecrement(&refs_); if (r == 0) delete this; - return r; + return static_cast(r); } // IDropSource HRESULT STDMETHODCALLTYPE QueryContinueDrag(BOOL escapePressed, DWORD keyState) override { @@ -92,7 +115,7 @@ public: return DRAGDROP_S_USEDEFAULTCURSORS; // let OLE draw the standard copy cursor } private: - ULONG refs_ = 1; + volatile LONG refs_ = 1; }; // Minimal IDataObject exposing exactly one format (CF_HDROP / TYMED_HGLOBAL). The HDROP is @@ -112,11 +135,11 @@ public: *ppv = nullptr; return E_NOINTERFACE; } - ULONG STDMETHODCALLTYPE AddRef() override { return ++refs_; } + ULONG STDMETHODCALLTYPE AddRef() override { return comAddRef(refs_); } ULONG STDMETHODCALLTYPE Release() override { - const ULONG r = --refs_; + const LONG r = InterlockedDecrement(&refs_); if (r == 0) delete this; - return r; + return static_cast(r); } // IDataObject — the two that matter for a drag source. @@ -184,7 +207,7 @@ private: (fe.tymed & TYMED_HGLOBAL) && fe.dwAspect == DVASPECT_CONTENT; } - ULONG refs_ = 1; + volatile LONG refs_ = 1; HGLOBAL hdrop_ = nullptr; }; @@ -192,10 +215,8 @@ private: bool initiateDragOut(HWND__* /*panelHwnd*/, const std::vector& absolutePaths) { if (absolutePaths.empty()) return false; + if (!ensureOleForThisThread()) return false; - // REAPER's main thread is already OLE-initialized (it hosts OLE drag targets); we - // deliberately do NOT call OleInitialize — pairing OleUninitialize across a - // REAPER-owned apartment is the kind of thing that bites. HGLOBAL hdrop = buildHDrop(absolutePaths); if (!hdrop) return false; diff --git a/src/ingest.cpp b/src/shell/actions/ingest.cpp similarity index 99% rename from src/ingest.cpp rename to src/shell/actions/ingest.cpp index 6837700..b173fdf 100644 --- a/src/ingest.cpp +++ b/src/shell/actions/ingest.cpp @@ -2,7 +2,7 @@ // REAPER-facing, DAW-verified; the pure serialization it drives (assignment_request) // is CTest-tested. -#include "ingest.h" +#include "shell/actions/ingest.h" #include #include diff --git a/src/ingest.h b/src/shell/actions/ingest.h similarity index 100% rename from src/ingest.h rename to src/shell/actions/ingest.h diff --git a/src/shell/actions/instrument_drop_win.cpp b/src/shell/actions/instrument_drop_win.cpp index b6b0213..8bdf650 100644 --- a/src/shell/actions/instrument_drop_win.cpp +++ b/src/shell/actions/instrument_drop_win.cpp @@ -20,6 +20,7 @@ #define REAPERAPI_WANT_GetThingFromPoint #define REAPERAPI_WANT_TrackFX_AddByName #define REAPERAPI_WANT_TrackFX_Delete +#define REAPERAPI_WANT_TrackFX_GetCount #define REAPERAPI_WANT_TrackFX_SetPreset #define REAPERAPI_WANT_Undo_BeginBlock2 #define REAPERAPI_WANT_Undo_EndBlock2 @@ -29,6 +30,9 @@ namespace reasampler { // Real-namespace-home using-declarations (Q-W6: the namespaces.h shim is retired). using version::vstPluginName; +using wire::decideDropOutcome; +using wire::DropAttempt; +using wire::DropOutcome; using wire::infoNamesFxHotspot; namespace { @@ -99,25 +103,32 @@ bool loadInstrumentOntoTrack(MediaTrack* track, const std::vector& // (beta extension <-> beta VST) has no literal to drift. const std::string fxName = "VST3:" + vstPluginName(); - // Negative `instantiate` => always create a NEW instance. recFX = false: a - // normal track FX chain instance, not a record/monitoring FX. - const int fxIndex = TrackFX_AddByName(track, fxName.c_str(), /*recFX=*/false, - /*instantiate=*/-1); - bool ok = fxIndex >= 0; + // An EXPLICIT top-level insertion position (instantiate <= -1000 IS the position, -1000 + // = first in chain), not the bare -1. Both always create a new instance; the bare form + // additionally leaves placement to REAPER's ambient FX-chain insert point, which a drop + // onto an FX container/chain-window moves — so the index handed to TrackFX_SetPreset and + // the instance just created stop denoting the same FX and the capture never lands. The + // bare-form retry keeps the reference path alive if the positional form is ever refused. + // recFX = false: a normal track FX chain instance, not a record/monitoring FX. + const int insertPos = TrackFX_GetCount(track); + int fxIndex = TrackFX_AddByName(track, fxName.c_str(), /*recFX=*/false, + /*instantiate=*/-1000 - insertPos); + if (fxIndex < 0) + fxIndex = TrackFX_AddByName(track, fxName.c_str(), /*recFX=*/false, /*instantiate=*/-1); // u8string() gives UTF-8 bytes on MSVC (not ACP-converted), so a temp dir under // an accented or CJK user-name is handled correctly by REAPER's path APIs. - if (ok) ok = TrackFX_SetPreset(track, fxIndex, presetPath.u8string().c_str()); + DropAttempt attempt; + attempt.addedFxIndex = fxIndex; + if (fxIndex >= 0) + attempt.presetApplied = TrackFX_SetPreset(track, fxIndex, presetPath.u8string().c_str()); std::error_code ec; std::filesystem::remove(presetPath, ec); // transient regardless of outcome - // All-or-nothing: if the preset apply fails, remove the FX instance we just - // added so the track is left exactly as it was. - if (!ok && fxIndex >= 0) { - TrackFX_Delete(track, fxIndex); - } - return ok; + const DropOutcome outcome = decideDropOutcome(attempt); + if (outcome.rollbackFxIndex >= 0) TrackFX_Delete(track, outcome.rollbackFxIndex); + return outcome.loaded; } bool performInstrumentDrop(MediaTrack* track, const std::vector& presetBytes) { diff --git a/src/shell/capture/capture_orchestrator.cpp b/src/shell/capture/capture_orchestrator.cpp index 524e34e..f545702 100644 --- a/src/shell/capture/capture_orchestrator.cpp +++ b/src/shell/capture/capture_orchestrator.cpp @@ -12,7 +12,7 @@ #include "shell/panel/panel_input.h" // bankPanelTailSetting / bankPanelRefresh #include "core/capture/tail_control.h" // TailSetting #include "core/model/provenance.h" // model::Provenance -#include "ingest.h" // ingestAssignActiveInstance +#include "shell/actions/ingest.h" // ingestAssignActiveInstance #include "shell/persist/session.h" // ReaSamplerSession #include "shell/capture/insert.h" // runInsert / InsertRequest #include "shell/capture/realtime_lifecycle.h" // the in-flight realtime state diff --git a/src/shell/panel/panel_drag.cpp b/src/shell/panel/panel_drag.cpp index 9f0d5db..17735d9 100644 --- a/src/shell/panel/panel_drag.cpp +++ b/src/shell/panel/panel_drag.cpp @@ -298,8 +298,13 @@ void onMouseMove(int x, int y) { if (gesture == DragGesture::OsDrag) { // Resolve the payload to existing on-disk paths BEFORE tearing down internal - // drag state (the resolver reads dragSourceBankId / dragSampleIds). + // drag state (the resolver reads dragSourceBankId / dragSampleIds), then let the + // pure rule couple the two side effects: an unresolvable payload must leave the + // internal drag intact rather than wind it down for a hand-off that never runs — + // a half-torn-down drag reads to the user as "the drag did nothing, try again". const std::vector paths = resolveDragPathsForOs(); + const ui::OsHandoff handoff = ui::decideOsHandoff(paths); + if (!handoff.releaseInternalDrag) return; // DoDragDrop runs its own modal loop and takes over mouse capture, so the internal // drag must be fully wound down first. @@ -313,8 +318,7 @@ void onMouseMove(int x, int y) { g_panel.dragPrimaryId.clear(); invalidatePanel(); - // Empty path list -> nothing draggable (all stale/missing); do not start a drag. - if (!paths.empty()) + if (handoff.startOsDrag) initiateDragOut(g_panel.hwnd, paths); // COPY-ONLY; blocking on Windows return; } @@ -377,14 +381,27 @@ void resetDragState() { // OsDragOut is never seen here: the pointer-left-client handoff happens live in onMouseMove. void onLBtnUp(int x, int y) { if (g_panel.dragging) { - // Drop-and-load: a release while hover-tracking a valid FX hotspot instantiates a - // ReaSampler 9000 on that track preloaded with the dragged capture — NOT a bank move, - // NOT an OS drag, NEVER a timeline insert. Takes priority over the in-grid / cross-bank - // drop. Single-capture only, so dragSampleIds.front() is the capture. - if (g_panel.instrumentDropTrack && g_panel.dragSampleIds.size() == 1) { + // Drop-and-load: a release over a valid FX hotspot instantiates a ReaSampler 9000 on + // that track preloaded with the dragged capture — NOT a bank move, NOT an OS drag, + // NEVER a timeline insert. Takes priority over the in-grid / cross-bank drop. + // Single-capture only, so dragSampleIds.front() is the capture. + // + // Re-resolve at the RELEASE point rather than trusting only the hover-tracked target: + // WM_MOUSEMOVE is coalesced, so a fast drag onto a dense surface (an FX chain row, a + // container) can release over a hotspot no processed move ever reported. Strictly + // additive — a release-point miss falls back to the tracked target, so the + // hover-then-release-on-the-FX-button path is untouched. + const bool singleCapture = g_panel.dragSampleIds.size() == 1; + MediaTrack* dropTrack = g_panel.instrumentDropTrack; + if (singleCapture) { + POINT sp{x, y}; + ClientToScreen(g_panel.hwnd, &sp); + const FxDropTarget fx = resolveFxDropTarget(sp.x, sp.y); + if (fx.valid()) dropTrack = fx.track; + } + if (dropTrack && singleCapture) { const std::string sampleId = g_panel.dragSampleIds.front(); - performInstrumentDrop(g_panel.instrumentDropTrack, - buildInstrumentDropPreset(sampleId)); + performInstrumentDrop(dropTrack, buildInstrumentDropPreset(sampleId)); // Read-only over the bank + arrange: the only mutations are the new FX instance + // its state (both undoable in performInstrumentDrop). } else { diff --git a/src/shell/panel/panel_window.cpp b/src/shell/panel/panel_window.cpp index 00b4ce7..9cc90f8 100644 --- a/src/shell/panel/panel_window.cpp +++ b/src/shell/panel/panel_window.cpp @@ -14,7 +14,7 @@ #include "shell/panel/panel_window.h" #include "shell/panel/draw_kit.h" -#include "ingest.h" +#include "shell/actions/ingest.h" #ifdef _WIN32 #include // GET_X_LPARAM / GET_Y_LPARAM (SWELL supplies them on mac/linux) diff --git a/tests/test_drag_out.cpp b/tests/test_drag_out.cpp index 3511c44..ed1cfae 100644 --- a/tests/test_drag_out.cpp +++ b/tests/test_drag_out.cpp @@ -221,6 +221,52 @@ static void testMixedTallies() { CHECK(l.skippedDuplicate == 1); } +// --- OS hand-off ordering ----------------------------------------------------- +// +// The drag-out-lands-without-audio regression: the shell used to wind its internal drag down +// (release capture, clear drag state) BEFORE checking whether the payload had resolved to any +// on-disk file. A payload that resolved to nothing therefore consumed the gesture — the drag +// died half-torn-down and the user saw a drop with no audio and dragged again. The two side +// effects are one decision; these pin that. + +// Nothing draggable -> hand off nothing AND keep the internal drag alive. +static void testEmptyPathsHandsOffNothingAndKeepsInternalDrag() { + const OsHandoff h = decideOsHandoff({}); + CHECK(!h.startOsDrag); + CHECK(!h.releaseInternalDrag); +} + +// A resolvable payload -> release the internal drag, then start the OS drag. +static void testResolvablePayloadHandsOff() { + const OsHandoff one = decideOsHandoff({"C:/proj/bank/a.wav"}); + CHECK(one.startOsDrag); + CHECK(one.releaseInternalDrag); + const OsHandoff many = decideOsHandoff({"a.wav", "b.wav", "c.wav"}); + CHECK(many.startOsDrag); + CHECK(many.releaseInternalDrag); +} + +// The coupling itself: releasing without starting is the defect, so it must be unreachable +// for every input the assembler can produce. +static void testNeverReleasesWithoutStarting() { + const std::vector> inputs = { + {}, {"a.wav"}, {"a.wav", "b.wav"}, {""}, + }; + for (const std::vector& in : inputs) { + const OsHandoff h = decideOsHandoff(in); + CHECK(!(h.releaseInternalDrag && !h.startOsDrag)); + } +} + +// End to end through the assembler: a selection whose every entry is stale/unresolvable +// yields the keep-the-drag verdict, which is exactly the case the shell used to mishandle. +static void testAllSkippedSelectionKeepsInternalDrag() { + const PathList l = assemblePathList({missing("g1.wav"), unresolved()}); + const OsHandoff h = decideOsHandoff(l.paths); + CHECK(!h.startOsDrag); + CHECK(!h.releaseInternalDrag); +} + int main() { testInsidePanelStaysInternal(); testLeavingClientAreaIsOsDrag(); @@ -244,6 +290,11 @@ int main() { testAllSkippedYieldsEmpty(); testMixedTallies(); + testEmptyPathsHandsOffNothingAndKeepsInternalDrag(); + testResolvablePayloadHandsOff(); + testNeverReleasesWithoutStarting(); + testAllSkippedSelectionKeepsInternalDrag(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; } diff --git a/tests/test_instrument_drop.cpp b/tests/test_instrument_drop.cpp index 2a6a934..ff4cda5 100644 --- a/tests/test_instrument_drop.cpp +++ b/tests/test_instrument_drop.cpp @@ -234,6 +234,72 @@ static void testEmbedStripIsNotHotspot() { CHECK(!infoNamesFxHotspot("mcp.fxembed extra")); } +// --- Drop surface parity: container vs FX button ------------------------------ +// +// The container-drop regression (instance loads, capture does not): the two surfaces must be +// one code path carrying one payload. The payload is a function of the capture alone, so the +// surface cannot influence it — and both surfaces must classify as hotspots so both reach it. + +// Every surface a drop can land on admits the SAME capture and yields a payload that decodes +// back to it. The payload takes the capture and nothing else, so the surface cannot influence +// it — walking the surfaces here is what pins that the container family is not second-class. +static void testEveryDropSurfaceCarriesTheCapture() { + const char* surfaces[] = { + "fx_chain", // FX chain window / container list — the reported failing surface + "fx_0", // floating FX window + "tcp.fx", // TCP FX button — the reference path + "mcp.fx", // MCP FX button + }; + const std::string sampleId = "cap-7f3a"; + for (const char* info : surfaces) { + CHECK(infoNamesFxHotspot(info)); + const ParsedPreset p = parsePreset(buildInstrumentDropPreset(sampleId)); + CHECK(p.ok); + CHECK(p.classId == vstClassIdHex()); + const ComponentState cs = deserializeComponentState(p.compChunk, kRate); + CHECK(cs.selectionId == sampleId); + } +} + +// --- All-or-nothing rollback -------------------------------------------------- + +// The add failed: nothing was created, so there is nothing to delete and nothing loaded. +static void testAddFailureLeavesNothingToRollBack() { + const DropOutcome out = decideDropOutcome(DropAttempt{-1, false}); + CHECK(!out.loaded); + CHECK(out.rollbackFxIndex < 0); +} + +// The instance was created but the preset did not apply: the caller MUST delete that exact +// index — an instance without its capture is the orphan the contract forbids. +static void testPresetFailureRollsBackTheCreatedIndex() { + const DropOutcome zero = decideDropOutcome(DropAttempt{0, false}); + CHECK(!zero.loaded); + CHECK(zero.rollbackFxIndex == 0); + const DropOutcome later = decideDropOutcome(DropAttempt{4, false}); + CHECK(!later.loaded); + CHECK(later.rollbackFxIndex == 4); + // Container-addressed indices (0x2000000-flagged) roll back by the same rule — the + // obligation follows the index REAPER handed back, whatever space it names. + const DropOutcome inContainer = decideDropOutcome(DropAttempt{0x2000000 + 5, false}); + CHECK(!inContainer.loaded); + CHECK(inContainer.rollbackFxIndex == 0x2000000 + 5); +} + +// Both halves succeeded: loaded, and nothing to undo. +static void testSuccessKeepsTheInstance() { + const DropOutcome out = decideDropOutcome(DropAttempt{2, true}); + CHECK(out.loaded); + CHECK(out.rollbackFxIndex < 0); +} + +// A "preset applied" report with no instance behind it can never read as loaded. +static void testNoInstanceIsNeverLoaded() { + const DropOutcome out = decideDropOutcome(DropAttempt{-1, true}); + CHECK(!out.loaded); + CHECK(out.rollbackFxIndex < 0); +} + int main() { testClassIdHexPinnedPerChannel(); testPresetRoundTripsThroughInstrumentReader(); @@ -248,6 +314,12 @@ int main() { testNonFxSurfacesAreNotHotspot(); testEmbedStripIsNotHotspot(); + testEveryDropSurfaceCarriesTheCapture(); + testAddFailureLeavesNothingToRollBack(); + testPresetFailureRollsBackTheCreatedIndex(); + testSuccessKeepsTheInstance(); + testNoInstanceIsNeverLoaded(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; } From 875d5b4632b9d67279ffd6cbe616a0355be8ef6e Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 30 Jul 2026 00:09:00 -0400 Subject: [PATCH 2/3] Fix drag-handoff bugs: gate FX re-resolve on outside-panel, cache unresolvable OS-drag verdict, block double FX-add retry --- src/core/ui/drag_out.cpp | 3 +- src/core/ui/drag_out.h | 14 ++++---- src/shell/actions/drag_out_win.cpp | 25 ++++++++++++--- src/shell/actions/drag_out_win.h | 9 ++++++ src/shell/actions/instrument_drop_win.cpp | 19 +++++++---- src/shell/panel/panel_drag.cpp | 39 +++++++++++++++++++---- src/shell/panel/panel_state.h | 6 ++++ tests/test_drag_out.cpp | 37 +++++++-------------- tests/test_instrument_drop.cpp | 33 ++++++------------- 9 files changed, 110 insertions(+), 75 deletions(-) diff --git a/src/core/ui/drag_out.cpp b/src/core/ui/drag_out.cpp index c817a31..d4d79df 100644 --- a/src/core/ui/drag_out.cpp +++ b/src/core/ui/drag_out.cpp @@ -50,8 +50,7 @@ PathList assemblePathList(const std::vector& resolved) { OsHandoff decideOsHandoff(const std::vector& paths) { OsHandoff out; - out.startOsDrag = !paths.empty(); - out.releaseInternalDrag = out.startOsDrag; + out.handOffToOs = !paths.empty(); return out; } diff --git a/src/core/ui/drag_out.h b/src/core/ui/drag_out.h index 5c6ccf0..02b0e24 100644 --- a/src/core/ui/drag_out.h +++ b/src/core/ui/drag_out.h @@ -71,13 +71,15 @@ PathList assemblePathList(const std::vector& resolved); // --- OS hand-off ordering ----------------------------------------------------- -// The two side effects the shell performs when a drag crosses out of REAPER. They are ONE -// decision, not two: winding the internal drag down (release capture, clear drag state) for a -// hand-off that then cannot happen consumes the gesture — the user sees a drag that silently -// did nothing and drags again. Never release without starting. +// Whether an empty/unresolvable payload should hand off to the OS at all. This decides ONLY +// the empty-payload third of the failure space — an unresolvable payload must leave the +// internal drag live rather than winding it down (release capture, clear drag state) for a +// hand-off that then never happens, which reads to the user as "the drag did nothing, try +// again". A resolved-but-OS-not-ready hand-off (OLE unavailable, HDROP build failure) is a +// separate, shell-side readiness gate (drag_out_win::canInitiateDragOut) checked BEFORE the +// shell tears down internal drag state — this struct does not model that path. struct OsHandoff { - bool startOsDrag = false; // hand `paths` to the OS drag initiator - bool releaseInternalDrag = false; // first wind down mouse capture + panel drag state + bool handOffToOs = false; // true: wind down internal drag state, then start the OS drag }; // Decides the hand-off from the assembled path list. Empty (everything stale/unresolvable) diff --git a/src/shell/actions/drag_out_win.cpp b/src/shell/actions/drag_out_win.cpp index 6abe66a..a420c28 100644 --- a/src/shell/actions/drag_out_win.cpp +++ b/src/shell/actions/drag_out_win.cpp @@ -64,10 +64,11 @@ HGLOBAL buildHDrop(const std::vector& paths) { // COM reference counts MUST be interlocked. A CF_HDROP target is free to marshal the data // object into another apartment and finish the copy on a background thread AFTER DoDragDrop -// has returned (Explorer's async file copy does exactly this). A plain ++/-- there races the -// source thread's post-DoDragDrop Release: one lost increment destroys the object — and with -// it the source HGLOBAL — before the target reads it, and the drop lands with no file. That -// race is intermittent and a retry usually wins it; do not "simplify" these back. +// has returned; Explorer's async file copy is suspected to do exactly this, though that +// specific behavior is not confirmed by experiment. A plain ++/-- there races the source +// thread's post-DoDragDrop Release: one lost increment destroys the object — and with it the +// source HGLOBAL — before the target reads it, and the drop lands with no file. That race is +// intermittent and a retry usually wins it; do not "simplify" these back. inline ULONG comAddRef(volatile LONG& refs) { return static_cast(InterlockedIncrement(&refs)); } @@ -234,6 +235,15 @@ bool initiateDragOut(HWND__* /*panelHwnd*/, const std::vector& abso return hr == DRAGDROP_S_DROP && effect == DROPEFFECT_COPY; } +bool canInitiateDragOut(const std::vector& absolutePaths) { + if (absolutePaths.empty()) return false; + if (!ensureOleForThisThread()) return false; + HGLOBAL hdrop = buildHDrop(absolutePaths); + if (!hdrop) return false; + GlobalFree(hdrop); // probe only — initiateDragOut builds its own on the real attempt + return true; +} + } // namespace reasampler #else // ---- macOS / Linux (SWELL) ----------------------------------------------------- @@ -260,6 +270,13 @@ bool initiateDragOut(HWND__* panelHwnd, const std::vector& absolute return true; // fire-and-forget; SWELL owns the drag from here (no accept/cancel return) } +// SWELL exposes no readiness probe ahead of SWELL_InitiateDragDropOfFileList (which itself +// reports no accept/cancel outcome) — the non-empty check is the only thing knowable in +// advance on this platform. +bool canInitiateDragOut(const std::vector& absolutePaths) { + return !absolutePaths.empty(); +} + } // namespace reasampler #endif diff --git a/src/shell/actions/drag_out_win.h b/src/shell/actions/drag_out_win.h index 1179a7c..21c0e3b 100644 --- a/src/shell/actions/drag_out_win.h +++ b/src/shell/actions/drag_out_win.h @@ -26,4 +26,13 @@ namespace reasampler { // (DROPEFFECT_COPY); the return is advisory — a failed drag surfaces no error. bool initiateDragOut(HWND__* panelHwnd, const std::vector& absolutePaths); +// Cheap, side-effect-free readiness probe: true iff initiateDragOut would actually be able to +// START a drag for `absolutePaths` right now. Call this BEFORE tearing down internal drag +// state (release capture, clear drag fields) — an OS that isn't ready (OLE unavailable, or the +// path list can't build a CF_HDROP) must not consume the gesture the same way an empty payload +// would. On Windows this re-runs the same OLE-init + HDROP-build checks initiateDragOut does, +// freeing the probe HGLOBAL immediately; SWELL exposes no such probe, so macOS/Linux reduces to +// the non-empty check alone. +bool canInitiateDragOut(const std::vector& absolutePaths); + } // namespace reasampler diff --git a/src/shell/actions/instrument_drop_win.cpp b/src/shell/actions/instrument_drop_win.cpp index 8bdf650..7b3887f 100644 --- a/src/shell/actions/instrument_drop_win.cpp +++ b/src/shell/actions/instrument_drop_win.cpp @@ -104,16 +104,23 @@ bool loadInstrumentOntoTrack(MediaTrack* track, const std::vector& const std::string fxName = "VST3:" + vstPluginName(); // An EXPLICIT top-level insertion position (instantiate <= -1000 IS the position, -1000 - // = first in chain), not the bare -1. Both always create a new instance; the bare form - // additionally leaves placement to REAPER's ambient FX-chain insert point, which a drop - // onto an FX container/chain-window moves — so the index handed to TrackFX_SetPreset and - // the instance just created stop denoting the same FX and the capture never lands. The - // bare-form retry keeps the reference path alive if the positional form is ever refused. + // = first in chain), not the bare -1 — this form is documented in the SDK header. Both + // always create a new instance; the bare form additionally leaves placement to REAPER's + // ambient FX-chain insert point, which a drop onto an FX container/chain-window is + // suspected (unconfirmed by experiment) to move — if so, the index handed to + // TrackFX_SetPreset and the instance just created would stop denoting the same FX and the + // capture would never land. The bare-form retry keeps the reference path alive if the + // positional form is ever refused. // recFX = false: a normal track FX chain instance, not a record/monitoring FX. const int insertPos = TrackFX_GetCount(track); int fxIndex = TrackFX_AddByName(track, fxName.c_str(), /*recFX=*/false, /*instantiate=*/-1000 - insertPos); - if (fxIndex < 0) + // Only retry with the bare form when the chain is PROVABLY unchanged (count still + // insertPos): a negative return with the count grown means the positional add DID create + // an instance and just reported -1 — retrying then would add a SECOND instance, leaving + // the first orphaned (no preset applied, unreachable for rollback), which is exactly the + // all-or-nothing violation this contract forbids. + if (fxIndex < 0 && TrackFX_GetCount(track) == insertPos) fxIndex = TrackFX_AddByName(track, fxName.c_str(), /*recFX=*/false, /*instantiate=*/-1); // u8string() gives UTF-8 bytes on MSVC (not ACP-converted), so a temp dir under diff --git a/src/shell/panel/panel_drag.cpp b/src/shell/panel/panel_drag.cpp index 17735d9..6ef18d4 100644 --- a/src/shell/panel/panel_drag.cpp +++ b/src/shell/panel/panel_drag.cpp @@ -297,14 +297,32 @@ void onMouseMove(int x, int y) { g_panel.instrumentDropTrack = nullptr; if (gesture == DragGesture::OsDrag) { + if (g_panel.dragOsHandoffBlocked) { + // Already known un-hand-off-able for this gesture (empty/unresolvable payload, + // or the OS wasn't ready) — skip the fs::exists work and the readiness probe on + // every move; keep the drag alive with no drop-target highlight. + g_panel.dropKind = DropKind::None; + g_panel.dropBankId.clear(); + invalidatePanel(); + return; + } + // Resolve the payload to existing on-disk paths BEFORE tearing down internal // drag state (the resolver reads dragSourceBankId / dragSampleIds), then let the // pure rule couple the two side effects: an unresolvable payload must leave the // internal drag intact rather than wind it down for a hand-off that never runs — // a half-torn-down drag reads to the user as "the drag did nothing, try again". + // canInitiateDragOut extends the same coupling to the OS-readiness failure modes + // (OLE unavailable, HDROP build failure) that the pure decision cannot see. const std::vector paths = resolveDragPathsForOs(); const ui::OsHandoff handoff = ui::decideOsHandoff(paths); - if (!handoff.releaseInternalDrag) return; + if (!handoff.handOffToOs || !canInitiateDragOut(paths)) { + g_panel.dragOsHandoffBlocked = true; + g_panel.dropKind = DropKind::None; + g_panel.dropBankId.clear(); + invalidatePanel(); + return; + } // DoDragDrop runs its own modal loop and takes over mouse capture, so the internal // drag must be fully wound down first. @@ -318,8 +336,7 @@ void onMouseMove(int x, int y) { g_panel.dragPrimaryId.clear(); invalidatePanel(); - if (handoff.startOsDrag) - initiateDragOut(g_panel.hwnd, paths); // COPY-ONLY; blocking on Windows + initiateDragOut(g_panel.hwnd, paths); // COPY-ONLY; blocking on Windows return; } // Inside the client: classify the in-grid gesture (reorder/replace vs move/copy) and @@ -372,6 +389,7 @@ void resetDragState() { g_panel.dragTargetSlot = -1; g_panel.dragPrimaryId.clear(); g_panel.instrumentDropTrack = nullptr; + g_panel.dragOsHandoffBlocked = false; } // Commits (or abandons) a drag on button-up. The resolved pure CardGesture decides: @@ -388,12 +406,19 @@ void onLBtnUp(int x, int y) { // // Re-resolve at the RELEASE point rather than trusting only the hover-tracked target: // WM_MOUSEMOVE is coalesced, so a fast drag onto a dense surface (an FX chain row, a - // container) can release over a hotspot no processed move ever reported. Strictly - // additive — a release-point miss falls back to the tracked target, so the - // hover-then-release-on-the-FX-button path is untouched. + // container) can release over a hotspot no processed move ever reported. Gated on + // !inside exactly like onMouseMove's live resolve — GetThingFromPoint can return a + // track+FX hit at a release point that is still inside the panel's own client rect, and + // an in-grid release must always go through the reorder/replace/move/copy path below, + // never be reinterpreted as an FX add. Strictly additive otherwise — a release-point + // miss falls back to the tracked target, so the hover-then-release-on-the-FX-button + // path is untouched. const bool singleCapture = g_panel.dragSampleIds.size() == 1; MediaTrack* dropTrack = g_panel.instrumentDropTrack; - if (singleCapture) { + RECT cr{}; + GetClientRect(g_panel.hwnd, &cr); + const bool inside = (x >= cr.left && x < cr.right && y >= cr.top && y < cr.bottom); + if (!inside && singleCapture) { POINT sp{x, y}; ClientToScreen(g_panel.hwnd, &sp); const FxDropTarget fx = resolveFxDropTarget(sp.x, sp.y); diff --git a/src/shell/panel/panel_state.h b/src/shell/panel/panel_state.h index f169dbf..9b6f981 100644 --- a/src/shell/panel/panel_state.h +++ b/src/shell/panel/panel_state.h @@ -320,6 +320,12 @@ struct PanelState { // when the pointer is not over an FX button. MediaTrack* instrumentDropTrack = nullptr; + // Latched once an OsDrag hand-off attempt for THIS gesture resolves to "cannot hand off" + // (empty/unresolvable payload, or the OS isn't ready) — skips re-running + // resolveDragPathsForOs (an fs::exists per sample) and the readiness probe on every + // subsequent move while the drag stays alive. Cleared by resetDragState. + bool dragOsHandoffBlocked = false; + // Authoritative tail setting lives in ReaSamplerSession, not here; panel reads it for // drawing and mutates via footer click / scroll-wheel. bankPanelTailSetting is the // capture actions' read seam. diff --git a/tests/test_drag_out.cpp b/tests/test_drag_out.cpp index ed1cfae..1a15a7b 100644 --- a/tests/test_drag_out.cpp +++ b/tests/test_drag_out.cpp @@ -223,39 +223,26 @@ static void testMixedTallies() { // --- OS hand-off ordering ----------------------------------------------------- // -// The drag-out-lands-without-audio regression: the shell used to wind its internal drag down +// The empty-payload teardown regression: the shell used to wind its internal drag down // (release capture, clear drag state) BEFORE checking whether the payload had resolved to any -// on-disk file. A payload that resolved to nothing therefore consumed the gesture — the drag -// died half-torn-down and the user saw a drop with no audio and dragged again. The two side -// effects are one decision; these pin that. +// on-disk file. For an unresolvable payload, initiateDragOut is never reached — no OS drop +// happens at all — but the teardown ran anyway, so the drag just silently stopped mid-gesture +// with no highlight and no drop. The user has to press and start an entirely new drag; retrying +// the SAME stale selection resolves to the same empty payload and fails identically. These pin +// the fix: the two side effects (teardown, hand-off) are one decision. // Nothing draggable -> hand off nothing AND keep the internal drag alive. static void testEmptyPathsHandsOffNothingAndKeepsInternalDrag() { const OsHandoff h = decideOsHandoff({}); - CHECK(!h.startOsDrag); - CHECK(!h.releaseInternalDrag); + CHECK(!h.handOffToOs); } -// A resolvable payload -> release the internal drag, then start the OS drag. +// A resolvable payload -> hand off (release the internal drag, then start the OS drag). static void testResolvablePayloadHandsOff() { const OsHandoff one = decideOsHandoff({"C:/proj/bank/a.wav"}); - CHECK(one.startOsDrag); - CHECK(one.releaseInternalDrag); + CHECK(one.handOffToOs); const OsHandoff many = decideOsHandoff({"a.wav", "b.wav", "c.wav"}); - CHECK(many.startOsDrag); - CHECK(many.releaseInternalDrag); -} - -// The coupling itself: releasing without starting is the defect, so it must be unreachable -// for every input the assembler can produce. -static void testNeverReleasesWithoutStarting() { - const std::vector> inputs = { - {}, {"a.wav"}, {"a.wav", "b.wav"}, {""}, - }; - for (const std::vector& in : inputs) { - const OsHandoff h = decideOsHandoff(in); - CHECK(!(h.releaseInternalDrag && !h.startOsDrag)); - } + CHECK(many.handOffToOs); } // End to end through the assembler: a selection whose every entry is stale/unresolvable @@ -263,8 +250,7 @@ static void testNeverReleasesWithoutStarting() { static void testAllSkippedSelectionKeepsInternalDrag() { const PathList l = assemblePathList({missing("g1.wav"), unresolved()}); const OsHandoff h = decideOsHandoff(l.paths); - CHECK(!h.startOsDrag); - CHECK(!h.releaseInternalDrag); + CHECK(!h.handOffToOs); } int main() { @@ -292,7 +278,6 @@ int main() { testEmptyPathsHandsOffNothingAndKeepsInternalDrag(); testResolvablePayloadHandsOff(); - testNeverReleasesWithoutStarting(); testAllSkippedSelectionKeepsInternalDrag(); if (g_fail == 0) std::printf("All tests passed.\n"); diff --git a/tests/test_instrument_drop.cpp b/tests/test_instrument_drop.cpp index ff4cda5..a944b40 100644 --- a/tests/test_instrument_drop.cpp +++ b/tests/test_instrument_drop.cpp @@ -237,29 +237,15 @@ static void testEmbedStripIsNotHotspot() { // --- Drop surface parity: container vs FX button ------------------------------ // // The container-drop regression (instance loads, capture does not): the two surfaces must be -// one code path carrying one payload. The payload is a function of the capture alone, so the -// surface cannot influence it — and both surfaces must classify as hotspots so both reach it. - -// Every surface a drop can land on admits the SAME capture and yields a payload that decodes -// back to it. The payload takes the capture and nothing else, so the surface cannot influence -// it — walking the surfaces here is what pins that the container family is not second-class. -static void testEveryDropSurfaceCarriesTheCapture() { - const char* surfaces[] = { - "fx_chain", // FX chain window / container list — the reported failing surface - "fx_0", // floating FX window - "tcp.fx", // TCP FX button — the reference path - "mcp.fx", // MCP FX button - }; - const std::string sampleId = "cap-7f3a"; - for (const char* info : surfaces) { - CHECK(infoNamesFxHotspot(info)); - const ParsedPreset p = parsePreset(buildInstrumentDropPreset(sampleId)); - CHECK(p.ok); - CHECK(p.classId == vstClassIdHex()); - const ComponentState cs = deserializeComponentState(p.compChunk, kRate); - CHECK(cs.selectionId == sampleId); - } -} +// one code path carrying one payload. buildInstrumentDropPreset takes only sampleId, so the +// payload side of that claim is already proven once by testPresetRoundTripsThroughInstrumentReader +// and every "fx_"/"tcp.fx"/"mcp.fx" surface classifying as a hotspot is proven by +// testTcpMcpFxFamilyIsHotspot / testFxWindowStillHotspot. A loop that reruns both against a +// fixed sampleId per surface string can't distinguish the surfaces (the loop body is identical +// every iteration) — it isn't a stronger test than those two, so there is no separate test here. +// The one thing that DOES vary by surface — the shell's TrackFX_AddByName `instantiate` value +// picked for a container drop vs. a bare FX-button drop — lives in instrument_drop_win.cpp and +// is untestable without a live DAW (GetThingFromPoint/TrackFX_AddByName have no pure model). // --- All-or-nothing rollback -------------------------------------------------- @@ -314,7 +300,6 @@ int main() { testNonFxSurfacesAreNotHotspot(); testEmbedStripIsNotHotspot(); - testEveryDropSurfaceCarriesTheCapture(); testAddFailureLeavesNothingToRollBack(); testPresetFailureRollsBackTheCreatedIndex(); testSuccessKeepsTheInstance(); From c91bf03ef4763c684e5b5039c7e3395d9cfb0e41 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 30 Jul 2026 06:56:20 -0400 Subject: [PATCH 3/3] Fix drag-drop review findings: unify teardown, gate FX-add on outside-panel, trim comments --- src/shell/actions/drag_out_win.h | 11 ++++------- src/shell/panel/panel_drag.cpp | 12 ++---------- src/shell/panel/panel_state.h | 6 ++---- tests/test_instrument_drop.cpp | 16 ++++------------ 4 files changed, 12 insertions(+), 33 deletions(-) diff --git a/src/shell/actions/drag_out_win.h b/src/shell/actions/drag_out_win.h index 21c0e3b..0631d3d 100644 --- a/src/shell/actions/drag_out_win.h +++ b/src/shell/actions/drag_out_win.h @@ -26,13 +26,10 @@ namespace reasampler { // (DROPEFFECT_COPY); the return is advisory — a failed drag surfaces no error. bool initiateDragOut(HWND__* panelHwnd, const std::vector& absolutePaths); -// Cheap, side-effect-free readiness probe: true iff initiateDragOut would actually be able to -// START a drag for `absolutePaths` right now. Call this BEFORE tearing down internal drag -// state (release capture, clear drag fields) — an OS that isn't ready (OLE unavailable, or the -// path list can't build a CF_HDROP) must not consume the gesture the same way an empty payload -// would. On Windows this re-runs the same OLE-init + HDROP-build checks initiateDragOut does, -// freeing the probe HGLOBAL immediately; SWELL exposes no such probe, so macOS/Linux reduces to -// the non-empty check alone. +// Side-effect-free readiness probe for initiateDragOut. Call BEFORE tearing down internal drag +// state — an OS that isn't ready (OLE unavailable, HDROP build failure) must not consume the +// gesture like an empty payload would. Windows re-runs the OLE-init + HDROP checks and frees the +// probe HGLOBAL immediately; SWELL exposes no probe, so macOS/Linux reduces to the non-empty check. bool canInitiateDragOut(const std::vector& absolutePaths); } // namespace reasampler diff --git a/src/shell/panel/panel_drag.cpp b/src/shell/panel/panel_drag.cpp index 6ef18d4..aa5ffb5 100644 --- a/src/shell/panel/panel_drag.cpp +++ b/src/shell/panel/panel_drag.cpp @@ -327,13 +327,7 @@ void onMouseMove(int x, int y) { // DoDragDrop runs its own modal loop and takes over mouse capture, so the internal // drag must be fully wound down first. if (GetCapture() == g_panel.hwnd) ReleaseCapture(); - g_panel.dragArmed = false; - g_panel.dragging = false; - g_panel.dropKind = DropKind::None; - g_panel.dropBankId.clear(); - g_panel.cardGesture = CardGesture::None; - g_panel.dragTargetSlot = -1; - g_panel.dragPrimaryId.clear(); + resetDragState(); invalidatePanel(); initiateDragOut(g_panel.hwnd, paths); // COPY-ONLY; blocking on Windows @@ -424,7 +418,7 @@ void onLBtnUp(int x, int y) { const FxDropTarget fx = resolveFxDropTarget(sp.x, sp.y); if (fx.valid()) dropTrack = fx.track; } - if (dropTrack && singleCapture) { + if (!inside && dropTrack && singleCapture) { const std::string sampleId = g_panel.dragSampleIds.front(); performInstrumentDrop(dropTrack, buildInstrumentDropPreset(sampleId)); // Read-only over the bank + arrange: the only mutations are the new FX instance + @@ -440,8 +434,6 @@ void onLBtnUp(int x, int y) { } else if (g == CardGesture::Replace) { // Replace targets the OCCUPANT of the target slot with the single grabbed card. const bool isBanks = g_panel.dragSourceRegion == Region::Banks; - RECT cr{}; - GetClientRect(g_panel.hwnd, &cr); const int w = cr.right - cr.left, h = cr.bottom - cr.top; const RECT region = isBanks ? banksRegionRect(w, h) : poolRegionRect(w, h); const RegionDisplay disp = regionDisplay(region, isBanks, g_panel.dragSourceRegion); diff --git a/src/shell/panel/panel_state.h b/src/shell/panel/panel_state.h index 9b6f981..d37b10b 100644 --- a/src/shell/panel/panel_state.h +++ b/src/shell/panel/panel_state.h @@ -320,10 +320,8 @@ struct PanelState { // when the pointer is not over an FX button. MediaTrack* instrumentDropTrack = nullptr; - // Latched once an OsDrag hand-off attempt for THIS gesture resolves to "cannot hand off" - // (empty/unresolvable payload, or the OS isn't ready) — skips re-running - // resolveDragPathsForOs (an fs::exists per sample) and the readiness probe on every - // subsequent move while the drag stays alive. Cleared by resetDragState. + // Latched once an OsDrag hand-off resolves "cannot hand off" for this gesture — skips + // re-running resolveDragPathsForOs (fs::exists per sample) each move. Cleared by resetDragState. bool dragOsHandoffBlocked = false; // Authoritative tail setting lives in ReaSamplerSession, not here; panel reads it for diff --git a/tests/test_instrument_drop.cpp b/tests/test_instrument_drop.cpp index a944b40..3572391 100644 --- a/tests/test_instrument_drop.cpp +++ b/tests/test_instrument_drop.cpp @@ -234,18 +234,10 @@ static void testEmbedStripIsNotHotspot() { CHECK(!infoNamesFxHotspot("mcp.fxembed extra")); } -// --- Drop surface parity: container vs FX button ------------------------------ -// -// The container-drop regression (instance loads, capture does not): the two surfaces must be -// one code path carrying one payload. buildInstrumentDropPreset takes only sampleId, so the -// payload side of that claim is already proven once by testPresetRoundTripsThroughInstrumentReader -// and every "fx_"/"tcp.fx"/"mcp.fx" surface classifying as a hotspot is proven by -// testTcpMcpFxFamilyIsHotspot / testFxWindowStillHotspot. A loop that reruns both against a -// fixed sampleId per surface string can't distinguish the surfaces (the loop body is identical -// every iteration) — it isn't a stronger test than those two, so there is no separate test here. -// The one thing that DOES vary by surface — the shell's TrackFX_AddByName `instantiate` value -// picked for a container drop vs. a bare FX-button drop — lives in instrument_drop_win.cpp and -// is untestable without a live DAW (GetThingFromPoint/TrackFX_AddByName have no pure model). +// Do not reintroduce a per-surface "capture carries" loop test: buildInstrumentDropPreset takes +// only sampleId (proven by testPresetRoundTripsThroughInstrumentReader), and per-surface hotspot +// coverage already exists (testTcpMcpFxFamilyIsHotspot / testFxWindowStillHotspot) — a loop with +// an identical body per surface string can't distinguish them. // --- All-or-nothing rollback --------------------------------------------------