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();