diff --git a/CMakeLists.txt b/CMakeLists.txt index d37baef..91d5899 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1149,7 +1149,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..d4d79df 100644 --- a/src/core/ui/drag_out.cpp +++ b/src/core/ui/drag_out.cpp @@ -48,4 +48,10 @@ PathList assemblePathList(const std::vector& resolved) { return out; } +OsHandoff decideOsHandoff(const std::vector& paths) { + OsHandoff out; + out.handOffToOs = !paths.empty(); + return out; +} + } // namespace reasampler::ui diff --git a/src/core/ui/drag_out.h b/src/core/ui/drag_out.h index b859945..02b0e24 100644 --- a/src/core/ui/drag_out.h +++ b/src/core/ui/drag_out.h @@ -69,4 +69,21 @@ 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 ----------------------------------------------------- + +// 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 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) +// 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..a420c28 100644 --- a/src/shell/actions/drag_out_win.cpp +++ b/src/shell/actions/drag_out_win.cpp @@ -62,6 +62,30 @@ 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 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)); +} + +// 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 +100,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 +116,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 +136,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 +208,7 @@ private: (fe.tymed & TYMED_HGLOBAL) && fe.dwAspect == DVASPECT_CONTENT; } - ULONG refs_ = 1; + volatile LONG refs_ = 1; HGLOBAL hdrop_ = nullptr; }; @@ -192,10 +216,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; @@ -213,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) ----------------------------------------------------- @@ -239,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..0631d3d 100644 --- a/src/shell/actions/drag_out_win.h +++ b/src/shell/actions/drag_out_win.h @@ -26,4 +26,10 @@ namespace reasampler { // (DROPEFFECT_COPY); the return is advisory — a failed drag surfaces no error. bool initiateDragOut(HWND__* panelHwnd, const std::vector& absolutePaths); +// 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/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..7b3887f 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,39 @@ 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 — 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); + // 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 // 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..aa5ffb5 100644 --- a/src/shell/panel/panel_drag.cpp +++ b/src/shell/panel/panel_drag.cpp @@ -297,25 +297,40 @@ 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). + // 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.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. 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(); - // Empty path list -> nothing draggable (all stale/missing); do not start a drag. - if (!paths.empty()) - 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 @@ -368,6 +383,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: @@ -377,14 +393,34 @@ 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. 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; + 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); + if (fx.valid()) dropTrack = fx.track; + } + if (!inside && 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 { @@ -398,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 f169dbf..d37b10b 100644 --- a/src/shell/panel/panel_state.h +++ b/src/shell/panel/panel_state.h @@ -320,6 +320,10 @@ struct PanelState { // when the pointer is not over an FX button. MediaTrack* instrumentDropTrack = nullptr; + // 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 // drawing and mutates via footer click / scroll-wheel. bankPanelTailSetting is the // capture actions' read seam. 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..1a15a7b 100644 --- a/tests/test_drag_out.cpp +++ b/tests/test_drag_out.cpp @@ -221,6 +221,38 @@ static void testMixedTallies() { CHECK(l.skippedDuplicate == 1); } +// --- OS hand-off ordering ----------------------------------------------------- +// +// 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. 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.handOffToOs); +} + +// 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.handOffToOs); + const OsHandoff many = decideOsHandoff({"a.wav", "b.wav", "c.wav"}); + CHECK(many.handOffToOs); +} + +// 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.handOffToOs); +} + int main() { testInsidePanelStaysInternal(); testLeavingClientAreaIsOsDrag(); @@ -244,6 +276,10 @@ int main() { testAllSkippedYieldsEmpty(); testMixedTallies(); + testEmptyPathsHandsOffNothingAndKeepsInternalDrag(); + testResolvablePayloadHandsOff(); + 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 b6357c3..0b0ec1a 100644 --- a/tests/test_instrument_drop.cpp +++ b/tests/test_instrument_drop.cpp @@ -235,6 +235,50 @@ static void testEmbedStripIsNotHotspot() { CHECK(!infoNamesFxHotspot("mcp.fxembed extra")); } +// 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 -------------------------------------------------- + +// 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(); @@ -249,6 +293,11 @@ int main() { testNonFxSurfacesAreNotHotspot(); testEmbedStripIsNotHotspot(); + testAddFailureLeavesNothingToRollBack(); + testPresetFailureRollsBackTheCreatedIndex(); + testSuccessKeepsTheInstance(); + testNoInstanceIsNeverLoaded(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; }