From 66f2e402b0c35542b0faaf988ffc55665933e73f Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 28 Jul 2026 06:37:42 -0400 Subject: [PATCH] fix(instrument_drop_win): UTF-8 temp path, PID collision guard, embed-strip excluded from drop hotspot --- src/instrument_drop.cpp | 12 ++++-- src/instrument_drop.h | 9 ++++- src/instrument_drop_win.cpp | 67 ++++++++++++++++++++++++---------- tests/test_instrument_drop.cpp | 15 ++++++++ 4 files changed, 78 insertions(+), 25 deletions(-) diff --git a/src/instrument_drop.cpp b/src/instrument_drop.cpp index 01790cb..c7e698e 100644 --- a/src/instrument_drop.cpp +++ b/src/instrument_drop.cpp @@ -92,10 +92,16 @@ std::vector buildInstrumentDropPreset(const std::string& sampleId) bool infoNamesFxHotspot(const std::string& info) { // See the header contract. Prefix rule (S-GA-DropFX): "fx_" names the FX-chain / // floating-FX windows; "tcp.fx" / "mcp.fx" prefixes name the TCP/MCP FX button and its - // sibling FX sub-elements (fxbyp/fxparm/fxembed/fxlist...), tolerant of the - // SDK-documented "may append additional information". Bare "tcp"/"mcp" and non-FX - // sub-elements ("tcp.mute", "tcp.vol") must NOT trigger an instrument drop. + // sibling FX sub-elements (fxbyp/fxparm/fxlist...), tolerant of the SDK-documented "may + // append additional information". Bare "tcp"/"mcp" and non-FX sub-elements ("tcp.mute", + // "tcp.vol") must NOT trigger an instrument drop. + // + // EXCLUDE the embed-strip sub-element ("tcp.fxembed" / "mcp.fxembed"): that is the + // surface where a ReaSampler 9000 embed strip draws inside the TCP/MCP. Dropping a card + // there must NOT add a SECOND instance — the surface is the existing instance's own UI, + // not an FX-chain drop target. It starts with "tcp.fx" so it must be explicitly excluded. auto startsWith = [&info](const char* p) { return info.rfind(p, 0) == 0; }; + if (startsWith("tcp.fxembed") || startsWith("mcp.fxembed")) return false; return startsWith("fx_") || startsWith("tcp.fx") || startsWith("mcp.fx"); } diff --git a/src/instrument_drop.h b/src/instrument_drop.h index 46d321a..7576114 100644 --- a/src/instrument_drop.h +++ b/src/instrument_drop.h @@ -93,8 +93,13 @@ std::vector buildInstrumentDropPreset(const std::string& sampleId) // "tcp.fxbyp", "tcp.fxparm", "tcp.fxembed", "mcp.fxlist", ... — all beginning "tcp.fx" / // "mcp.fx". So the hotspot rule is PREFIX-based (S-GA-DropFX: the earlier exact-token match // on "tcp.fx"/"mcp.fx" was too strict for appended info and sibling FX elements): -// * "fx_" prefix — the FX-chain and floating-FX windows -// * "tcp.fx" prefix / "mcp.fx" prefix — the TCP/MCP FX button + sibling FX sub-elements +// * "fx_" prefix — the FX-chain and floating-FX windows +// * "tcp.fx" / "mcp.fx" prefix — the TCP/MCP FX button + sibling FX sub-elements +// EXCEPT "tcp.fxembed" / "mcp.fxembed" — the embed-strip surface where a ReaSampler 9000 +// instance draws inside the TCP/MCP. Dropping onto the existing instance's own UI must NOT +// add a second instance; the embed surface is explicitly excluded even though it starts +// with "tcp.fx". All other "tcp.fx*" / "mcp.fx*" tokens (fxbyp, fxparm, fxlist, ...) are +// hotspots — they are FX-chain controls, not a running instance's own surface. // Bare "tcp"/"mcp" and non-FX sub-elements (e.g. "tcp.mute", "tcp.vol") are NOT hotspots. // The exact live token over the FX button remains a DAW-only fact — confirm in REAPER (a // deferred ReaScript around reaper.GetThingFromPoint(reaper.GetMousePosition()) prints it). diff --git a/src/instrument_drop_win.cpp b/src/instrument_drop_win.cpp index 4be2de3..363c9c8 100644 --- a/src/instrument_drop_win.cpp +++ b/src/instrument_drop_win.cpp @@ -31,27 +31,49 @@ namespace reasampler { namespace { -// Write `bytes` to a fresh uniquely-named .vstpreset in the OS temp dir and return its -// absolute path; empty string on any failure. The .vstpreset extension is load-bearing — +// Write `bytes` to a fresh uniquely-named .vstpreset in the OS temp dir and return its path; +// returns an empty path on any failure. The .vstpreset extension is load-bearing — // TrackFX_SetPreset's full-path form is documented for .vstpreset files (VST3). The file is // transient: the caller deletes it right after the SetPreset call. -std::string writeTempPreset(const std::vector& bytes) { - static std::atomic counter{0}; - std::error_code ec; - const std::filesystem::path dir = std::filesystem::temp_directory_path(ec); - if (ec) return {}; - const std::filesystem::path path = - dir / ("reasampler_drop_" + std::to_string(counter.fetch_add(1)) + ".vstpreset"); - std::ofstream out(path, std::ios::binary | std::ios::trunc); - if (!out) return {}; - out.write(reinterpret_cast(bytes.data()), - static_cast(bytes.size())); - out.close(); - if (!out) { // short write / flush failure -> don't hand REAPER a truncated preset - std::filesystem::remove(path, ec); +// +// The temp filename embeds the process ID so two concurrent REAPER instances (e.g. stable + +// beta) cannot collide in the shared OS temp dir, and one instance's cleanup cannot +// accidentally delete another's in-flight file. +// +// Non-throwing: every std::filesystem call uses the error_code overload. The whole body is +// wrapped in try/catch to guarantee no exception crosses the REAPER C callback boundary +// (the same discipline persist.cpp uses — see its non-throwing scanPruneOrphans comment). +// +// Returns the path object (not a narrow string) so the caller can: +// (a) pass path.u8string() to TrackFX_SetPreset — UTF-8 on MSVC, not ACP-converted, +// so a temp dir with accented or CJK user-name bytes is handled correctly; +// (b) delete via the retained path object — not via re-parsing the narrow string — +// so the cleanup cannot leak if the conversion above were to round-trip incorrectly. +std::filesystem::path writeTempPreset(const std::vector& bytes) { + try { + static std::atomic counter{0}; + std::error_code ec; + const std::filesystem::path dir = std::filesystem::temp_directory_path(ec); + if (ec) return {}; + // PID in the name keeps files from distinct REAPER instances distinct in the shared + // temp dir — prevents cross-instance collisions and spurious post-apply deletions. + const std::string name = + "reasampler_drop_" + std::to_string(GetCurrentProcessId()) + + "_" + std::to_string(counter.fetch_add(1)) + ".vstpreset"; + const std::filesystem::path path = dir / name; + std::ofstream out(path, std::ios::binary | std::ios::trunc); + if (!out) return {}; + out.write(reinterpret_cast(bytes.data()), + static_cast(bytes.size())); + out.close(); + if (!out) { // short write / flush failure -> don't hand REAPER a truncated preset + std::filesystem::remove(path, ec); + return {}; + } + return path; + } catch (...) { return {}; } - return path.string(); } } // namespace @@ -77,7 +99,7 @@ bool loadInstrumentOntoTrack(MediaTrack* track, const std::vector& // Materialize the .vstpreset FIRST so an I/O failure leaves the track untouched (no FX // added yet — nothing to roll back). - const std::string presetPath = writeTempPreset(presetBytes); + const std::filesystem::path presetPath = writeTempPreset(presetBytes); if (presetPath.empty()) return false; // The CHANNEL-correct FX name: "VST3:ReaSampler 9000" on stable, "VST3:ReaSampler 9000 @@ -100,9 +122,14 @@ bool loadInstrumentOntoTrack(MediaTrack* track, const std::vector& // serializer produced (instrument_drop::buildInstrumentDropPreset -> // sample_map::serializeComponentState). Unlike the former "vst_chunk" named-config-parm // write, a failure here is REPORTED (false), not silently ignored. - if (ok) ok = TrackFX_SetPreset(track, fxIndex, presetPath.c_str()); + // + // 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()); - // The preset file is transient regardless of outcome; best-effort cleanup (temp dir). + // The preset file is transient regardless of outcome; delete via the retained path object + // (not a re-parsed narrow string) so cleanup cannot leak even if the UTF-8 conversion + // round-trip were incorrect. std::error_code ec; std::filesystem::remove(presetPath, ec); diff --git a/tests/test_instrument_drop.cpp b/tests/test_instrument_drop.cpp index 69151f5..08ffaa6 100644 --- a/tests/test_instrument_drop.cpp +++ b/tests/test_instrument_drop.cpp @@ -218,6 +218,20 @@ static void testNonFxSurfacesAreNotHotspot() { CHECK(!infoNamesFxHotspot("envcp")); // envelope control panel — a track thing, not FX } +// The embed-strip sub-element is NOT a hotspot. "tcp.fxembed" / "mcp.fxembed" is the surface +// where a ReaSampler 9000 instance draws inline in the TCP/MCP via IReaperUIEmbedInterface. +// Dropping a card there must NOT add a SECOND instance on top of the existing embed — the +// drop should be ignored (no instrument drop), even though the token starts with "tcp.fx". +// This documents the explicit exclusion in infoNamesFxHotspot and would catch a regression if +// the exclude guard were accidentally removed. +static void testEmbedStripIsNotHotspot() { + CHECK(!infoNamesFxHotspot("tcp.fxembed")); // TCP embed strip — existing instance's surface + CHECK(!infoNamesFxHotspot("mcp.fxembed")); // MCP embed strip — existing instance's surface + // With hypothetically appended info (SDK "may append") — still excluded. + CHECK(!infoNamesFxHotspot("tcp.fxembed.1")); + CHECK(!infoNamesFxHotspot("mcp.fxembed extra")); +} + int main() { testClassIdHexPinnedPerChannel(); testPresetRoundTripsThroughInstrumentReader(); @@ -230,6 +244,7 @@ int main() { testTcpMcpFxFamilyIsHotspot(); testFxWindowStillHotspot(); testNonFxSurfacesAreNotHotspot(); + testEmbedStripIsNotHotspot(); if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0;