From 1b4d0e67b735f55bd9171783f78dd2776b77e984 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 05:50:38 -0400 Subject: [PATCH] Loop-crossfade-ux review fixes: parked-drag no longer fakes LOOP OFF, waveform label contrast fixed, hover memoizes its bank read Also corrects the cap-area, em-dash, glyph-overhang and heuristic-comment findings noted in review. --- docs/TODO.md | 2 +- src/core/instrument/ui/loop_marks.cpp | 12 +++++- src/core/ui/theme.h | 8 ++++ .../instrument/editor_input_waveform.cpp | 11 +++--- .../instrument/editor_paint_waveform.cpp | 39 +++++++++++++++---- src/shell/instrument/editor_session.cpp | 24 +++++++++--- src/shell/instrument/reasampler_editor.h | 15 +++++-- tests/test_loop_marks.cpp | 16 ++++++++ tests/test_sample_chrome.cpp | 5 ++- tests/test_theme.cpp | 20 ++++++++++ 10 files changed, 125 insertions(+), 27 deletions(-) diff --git a/docs/TODO.md b/docs/TODO.md index a36407c..acc9f6f 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -172,7 +172,7 @@ Forward-looking follow-ups. Deferred by decision, not oversight — each entry r **The wart.** A zero-attack `AttackEnd` vertex is drawn at the same pixel as `Origin` (the envelope's non-draggable start anchor), which for an AHD envelope sits at the start marker's frame. Because a node's nominal pick-box area is smaller than the marker's full-height grab-column area, and `resolveWaveformClaim`'s rule is "smallest area among hit candidates wins," the draggable `AttackEnd` node still claims the click over the start marker when the two coincide — and, at a loop starting there, over the crossfade tab. Folding the staged pass into the shared arbitration slot did not change this specific outcome, since the rule that decides node-vs-marker priority is unchanged from what the contour-node fix established. `Origin` itself is excluded from `nodeAtPoint`'s candidate set entirely (never draggable, never a hit), so the common case — attack > 0, no coincidence — is unaffected. -**RESOLVED — Γ-W2-T2 (`loop-crossfade-ux`), incidentally.** Giving every mark the cap-grip the crossfade already had is what closed it: the start marker now carries an 11x10 cap in the overlay's top strip, whose nominal area (110) is smaller than the node's fixed pick box (169), so the cap wins the coincident pixel and the marker is reachable again. No priority rule was added and `resolveWaveformClaim` is byte-for-byte unchanged — every cap is one `markerHandleRect`, so the cap slot's nominal area did not move and the `cap < node < column` ordering still holds. Below the cap strip the node keeps the click, which is correct: that is where the node is actually drawn for any non-degenerate envelope. Pinned by `testAMarkCapOutranksACoincidentEnvelopeNodeInTheTopStrip` (`tests/test_spline_edit.cpp`). `Origin` was not touched and `isDraggable`'s shape rules are unchanged. +**RESOLVED — Γ-W2-T2 (`loop-crossfade-ux`), incidentally.** Giving every mark the cap-grip the crossfade already had is what closed it: the start marker now carries an 11x10 cap in the overlay's top strip, whose nominal area (110) is smaller than the node's fixed pick box (169), so the cap wins the coincident pixel and the marker is reachable again. No priority rule was added and `resolveWaveformClaim` is byte-for-byte unchanged — but the cap slot's own nominal area DID move, from the old clipped-actual measure (60 at frame 0) to the new nominal 110 every cap now feeds it (`markerHandleRect`'s own unclipped area). That move leaves the `cap < node < column` ordering unchanged only because 110 is still under the node's fixed 169 — the outcome held, not the area. Below the cap strip the node keeps the click, which is correct: that is where the node is actually drawn for any non-degenerate envelope. Pinned by `testAMarkCapOutranksACoincidentEnvelopeNodeInTheTopStrip` (`tests/test_spline_edit.cpp`). `Origin` was not touched and `isDraggable`'s shape rules are unchanged. ## Active-bank indicator placement (B4 polish) diff --git a/src/core/instrument/ui/loop_marks.cpp b/src/core/instrument/ui/loop_marks.cpp index 82cc51a..5c4f6af 100644 --- a/src/core/instrument/ui/loop_marks.cpp +++ b/src/core/instrument/ui/loop_marks.cpp @@ -45,8 +45,16 @@ LoopWrite applyLoopMarks(const LoopMarks& m) { LoopWrite w; const bool spanAlive = m.loopEnd > m.loopStart; w.loop.hasLoop = m.hasLoop && spanAlive; - w.loop.start = m.loopStart; - w.loop.end = m.loopEnd; + if (m.parked && !m.hasLoop) { + // A parked pair (never set) must round-trip back to parked, not to a real "LOOP OFF" + // span — resolveLoopMarks only re-parks a span spanUsable would refuse, so collapse it + // deliberately rather than writing back the default bounds' own alive span. + w.loop.start = m.loopStart; + w.loop.end = m.loopStart; + } else { + w.loop.start = m.loopStart; + w.loop.end = m.loopEnd; + } w.crossfade = (spanAlive && m.crossfade > 0) ? m.crossfade : 0; w.start = m.start; return w; diff --git a/src/core/ui/theme.h b/src/core/ui/theme.h index ab47446..ff53310 100644 --- a/src/core/ui/theme.h +++ b/src/core/ui/theme.h @@ -104,6 +104,14 @@ inline constexpr double kLoopSpanFillAlpha = 0.20; // body floor Font::Micro answers to. inline constexpr double kCardNameScrimAlpha = 0.75; +// The waveform mark caption/label scrim: bg/base composited at this alpha UNDER the loop-state +// caption and the four mark labels, so text/dim (Font::Micro, body class) stays readable when +// the envelope's accent/primary peak reaches into the label's rect. Higher than +// kCardNameScrimAlpha because text/dim needs more cover than text/primary to clear the same +// 4.5:1 body floor against the same lime worst case — test_theme.cpp composes this exact value +// against accent/primary to pin the floor both roles answer to. +inline constexpr double kWaveformLabelScrimAlpha = 0.90; + // Relative luminance per WCAG 2.1 (sRGB linearization + 0.2126/0.7152/0.0722 weighting). Alpha // is ignored — a translucent overlay's effective color is the caller's to compose first // (compositeOver). diff --git a/src/shell/instrument/editor_input_waveform.cpp b/src/shell/instrument/editor_input_waveform.cpp index c3a4c3a..2cb5130 100644 --- a/src/shell/instrument/editor_input_waveform.cpp +++ b/src/shell/instrument/editor_input_waveform.cpp @@ -64,11 +64,12 @@ bool ReaSamplerEditor::mouseDownWaveform(const FaceLayout& fl, int x, int y) { // keeps a cap apart from ITS OWN column; this is the cross-affordance case on top of that). // resolveWaveformClaim (spline_edit.h) is the ONE arbitration: it measures each claimant's // own NOMINAL target area and lets the smallest hit win, since a fixed check order shadows - // whichever one loses the tie — this seam regressed twice from exactly that fix. Giving - // every mark a cap changed WHICH mark the cap slot resolves to, not the slot's nominal area - // (every cap is one markerHandleRect) and not the ordering cap < node < column, so the - // arbitration itself is unchanged. Never add here (kAdd is only tried once nothing else has - // claimed the click, below). + // whichever one loses the tie — this seam regressed twice from exactly that fix. Giving every + // mark a cap changed WHICH mark the cap slot resolves to AND the slot's own nominal area + // (every cap is one markerHandleRect, so it moved from the old clipped-actual measure — 60 at + // frame 0 — to the nominal 110); the `cap < node < column` ordering held anyway, because 110 + // is still under the node's fixed pick-box area. Never add here (kAdd is only tried once + // nothing else has claimed the click, below). WaveformClaim node; if (envNodeHit.hit) { constexpr std::int64_t side = 2 * kNodeGrabRadius + 1; diff --git a/src/shell/instrument/editor_paint_waveform.cpp b/src/shell/instrument/editor_paint_waveform.cpp index c398fb7..0038702 100644 --- a/src/shell/instrument/editor_paint_waveform.cpp +++ b/src/shell/instrument/editor_paint_waveform.cpp @@ -84,10 +84,11 @@ const char* markLabel(WaveMark m) { return ""; } -// Font::Micro is proportional, so this is a generous per-character estimate: the label box may -// end up wider than the glyphs, never narrower — an under-estimate would let the suppression -// rule place two boxes that visibly collide. -constexpr int kMicroCharPx = 6; +// Font::Micro is proportional, so this is a per-character estimate, not a measured font metric — +// an under-estimate doesn't produce a visible collision (the kit's own DT_END_ELLIPSIS clips the +// box first), it produces a silently TRUNCATED label ("LOO…"). Matches kTooltipCharPx +// (panel_state.h), the codebase's other unmeasured Segoe UI estimate. +constexpr int kMicroCharPx = 7; int markLabelWidth(WaveMark m) { int n = 0; for (const char* s = markLabel(m); *s; ++s) ++n; @@ -101,7 +102,11 @@ void drawMarkCap(LICE_IBitmap* bmp, WaveMark which, const Rect& cap, int mx, LIC if (cap.empty()) return; const int top = cap.y; const int bot = cap.bottom(); - const int arm = kMarkerHandleHalfWidth; // the cap's own half-width, so glyph == grip + // The cap's own half-width, so glyph == grip — true for LOOP/END/XFADE. START is the + // exception: its triangle tip (mx - 1 + arm + 2, below) reaches past markerHandleRect's own + // right edge rather than sitting inside it; not yet corrected, since narrowing the tip is a + // visual change past this fix's scope. + const int arm = kMarkerHandleHalfWidth; switch (which) { case WaveMark::kStart: // A play flag: it points into the material that will play. @@ -153,6 +158,16 @@ void drawCrossfadeWedge(LICE_IBitmap* bmp, const OverlayArea& overlay, std::int6 } } } + +// Backing for the state caption and the four mark labels: text/dim and text/primary both read +// under floor against a full-scale envelope peak (accent/primary) with no backing at all — see +// kWaveformLabelScrimAlpha's own note. Sized to the box the caller already resolved, never the +// whole band, so this only darkens the text's own row. +void scrimLabelBox(LICE_IBitmap* bmp, const Rect& box) { + if (box.empty()) return; + LICE_FillRect(bmp, box.x, box.y, box.width, box.height, toLice(roleColor(Role::BgBase)), + static_cast(kWaveformLabelScrimAlpha), 0); +} } // namespace void ReaSamplerEditor::paintWaveform(LICE_IBitmap* bmp, const Rect& band, @@ -236,11 +251,17 @@ void ReaSamplerEditor::paintWaveform(LICE_IBitmap* bmp, const Rect& band, // The state caption, centred in the span: the two OFF states say different things because // they mean different things, and Trigger's refusal names its own reason. const char* caption = nullptr; - if (!loopLive) caption = "LOOP - GATE ONLY"; + if (!loopLive) caption = "LOOP \xe2\x80\x94 GATE ONLY"; // "LOOP — GATE ONLY" (em dash, UTF-8) else if (!m.hasLoop) caption = m.parked ? "DRAG TO SET LOOP" : "LOOP OFF"; if (caption != nullptr && rx > lx) { - kitTextCentered(bmp, Rect::ltrb(lx, overlayRect.y, rx, overlayRect.bottom()), caption, - Font::Micro, Role::TextDim); + // Tight box (kMarkLabelHeight, not the whole overlay) centered on the same midline the + // full-height rect already centered DT_VCENTER text on, so the scrim darkens only the + // caption's own row. + const int capH = kMarkLabelHeight; + const int capY = overlayRect.y + (overlayRect.height - capH) / 2; + const Rect capBox = Rect::ltrb(lx, capY, rx, capY + capH); + scrimLabelBox(bmp, capBox); + kitTextCentered(bmp, capBox, caption, Font::Micro, Role::TextDim); } // Labels beneath the trace and the handles in z-order; the promoted one is re-drawn ON TOP @@ -254,6 +275,7 @@ void ReaSamplerEditor::paintWaveform(LICE_IBitmap* bmp, const Rect& band, const WaveMarkLabels labels = layoutMarkLabels(overlay, frames, marks, labelW, promoted); for (int i = 0; i < kWaveMarkCount; ++i) { if (i == promoted || labels.box[i].empty()) continue; + scrimLabelBox(bmp, labels.box[i]); kitTextCentered(bmp, labels.box[i], markLabel(static_cast(i)), Font::Micro, Role::TextDim); } @@ -285,6 +307,7 @@ void ReaSamplerEditor::paintWaveform(LICE_IBitmap* bmp, const Rect& band, paintEnvelopeOverlay(bmp, overlay, frames); if (promoted >= 0 && promoted < kWaveMarkCount && !labels.box[promoted].empty()) { + scrimLabelBox(bmp, labels.box[promoted]); kitTextCentered(bmp, labels.box[promoted], markLabel(static_cast(promoted)), Font::Micro, Role::TextPrimary); } diff --git a/src/shell/instrument/editor_session.cpp b/src/shell/instrument/editor_session.cpp index 29179b2..3a40299 100644 --- a/src/shell/instrument/editor_session.cpp +++ b/src/shell/instrument/editor_session.cpp @@ -55,6 +55,7 @@ void ReaSamplerEditor::refreshFromBank() { thumbCache_.clear(); // a bank edit may have re-captured/removed a sample; drop stale peaks pcmCache_.clear(); // and its decoded PCM (the waveform + snap source) holdNeedValid_ = false; // …and the bake-Hold answer derived from the bank's loop intrinsic + marksValid_ = false; // …and pickedMarkers' own memo of the same intrinsic channelPcmId_.clear(); channelPcm_ = ChannelPcm{}; if (!processor_) { @@ -215,6 +216,16 @@ void ReaSamplerEditor::loadSelection(const std::string& id) { } ReaSamplerEditor::SetupMarkers ReaSamplerEditor::pickedMarkers(std::int64_t frames) const { + // Answer once per distinct input; refreshFromBank drops the memo along with holdNeed's, + // which answers the same "is there a bank read to pay" question for the bake-Hold predicate + // — hoverWaveform calls this on every WM_MOUSEMOVE inside the band, not just marker grabs, + // so the bridge read + JSON parse below is not a cost to pay per pixel of mouse travel. Frames + // is not part of the key: it is a function of selectedId_ alone (monoPcmFor's own cache), + // busted by the same refreshFromBank that busts this memo. + const HoldNeedKey key{selectedId_, params_.loopOverride, params_.loopCrossfadeFrames, + params_.startPoint}; + if (marksValid_ && key == marksKey_) return marksCache_; + instrument::ui::StoredLoop stored; stored.override_ = params_.loopOverride; stored.crossfade = params_.loopCrossfadeFrames; @@ -222,9 +233,7 @@ ReaSamplerEditor::SetupMarkers ReaSamplerEditor::pickedMarkers(std::int64_t fram // Read the loop intrinsic from the live bank blob (the same path selectSample uses); when // that is not readable (extension absent / not yet parsed) the instance-owned ref carries // the same intrinsics. Skipped entirely once an override is already set — the resolve would - // discard it, and the bridge read + JSON parse it costs is real (mouseDownWaveform's - // arbitration calls this on every waveform click, not just marker grabs, to know whether a - // cap or column candidate hits at all). + // discard it, and the bridge read + JSON parse it costs is real. if (processor_ && !params_.loopOverride) { std::optional sel; auto banksJson = @@ -236,11 +245,16 @@ ReaSamplerEditor::SetupMarkers ReaSamplerEditor::pickedMarkers(std::int64_t fram } if (sel) stored.intrinsic = sel->loop; } - return instrument::ui::resolveLoopMarks(stored, frames); + const SetupMarkers m = instrument::ui::resolveLoopMarks(stored, frames); + marksKey_ = key; + marksCache_ = m; + marksValid_ = true; + return m; } bool ReaSamplerEditor::HoldNeedKey::operator==(const HoldNeedKey& o) const { - if (sampleId != o.sampleId || crossfade != o.crossfade) return false; + if (sampleId != o.sampleId || crossfade != o.crossfade || startPoint != o.startPoint) + return false; if (loopOverride.has_value() != o.loopOverride.has_value()) return false; if (!loopOverride) return true; return loopOverride->hasLoop == o.loopOverride->hasLoop && diff --git a/src/shell/instrument/reasampler_editor.h b/src/shell/instrument/reasampler_editor.h index a9cee85..08c883a 100644 --- a/src/shell/instrument/reasampler_editor.h +++ b/src/shell/instrument/reasampler_editor.h @@ -337,9 +337,10 @@ private: // instance's own SampleRefs as the self-contained fallback. "" when unresolvable. std::string samplePathFor(const std::string& sampleId) const; - // The effective loop + start markers for the loaded capture. The state machine behind them - // — which stored source wins, when the pair re-parks, what the two OFF states mean — is the - // pure loop_marks module's; this only reads the bank intrinsic it cannot see. + // The effective loop + start markers for the loaded capture; the state machine behind them + // is the pure loop_marks module's. With no override set this costs a bridge read plus a bank + // parse — hoverWaveform calls it on every WM_MOUSEMOVE, so the answer is memoized (see + // HoldNeedKey below, which this shares) rather than paid per move. SetupMarkers pickedMarkers(std::int64_t frames) const; // Whether the loop controls answer at all: the sustain loop is Gate-only, so in Trigger the @@ -365,16 +366,22 @@ private: bool resolveBakeHoldNeeded(); // What that answer was last computed against. Invalidated wholesale by refreshFromBank, - // which is where the bank half of the input changes. + // which is where the bank half of the input changes. pickedMarkers' own memo (below) shares + // this key shape rather than declaring a second one; resolveBakeHoldNeeded leaves startPoint + // at its default (nullopt) since its own answer doesn't depend on it. struct HoldNeedKey { std::string sampleId; std::optional loopOverride; std::int64_t crossfade = 0; + std::optional startPoint; bool operator==(const HoldNeedKey& other) const; }; HoldNeedKey holdNeedKey_; bool holdNeedValid_ = false; bool holdNeedAnswer_ = false; + mutable HoldNeedKey marksKey_; + mutable SetupMarkers marksCache_; + mutable bool marksValid_ = false; // Writes `m` into params_ as the loop/start override. Does NOT call commitAndReload — // callers decide live-drag vs final commit. diff --git a/tests/test_loop_marks.cpp b/tests/test_loop_marks.cpp index 9d37b20..ae63db9 100644 --- a/tests/test_loop_marks.cpp +++ b/tests/test_loop_marks.cpp @@ -176,6 +176,21 @@ static void testEditingTheStartMarkerWhileOffLeavesTheEnableAndCrossfadeAlone() CHECK(w.start == 512); } +// Dragging the START marker while parked (never set) must not turn "DRAG TO SET LOOP" into +// "LOOP OFF" — the ordinary gesture that exposed the bug, distinct from the retained-span case +// above. +static void testEditingTheStartMarkerWhileParkedStaysParked() { + LoopMarks m = resolveLoopMarks(StoredLoop{}, kFrames); + CHECK(m.parked && !m.hasLoop); + m.start = 512; + const LoopMarks after = writeThenRead(m); + CHECK(!after.hasLoop); + CHECK(after.parked); + const LoopBounds d = defaultLoopBounds(kFrames); + CHECK(after.loopStart == d.start && after.loopEnd == d.end); + CHECK(after.start == 512); +} + static void testANegativeCrossfadeNeverReachesTheStore() { LoopMarks m = resolveLoopMarks(storedSpan(true, 4000, 6000, 0), kFrames); m.crossfade = -1; @@ -197,6 +212,7 @@ int main() { testDraggingALoopMarkWhileOffTurnsItOn(); testEditingTheStartMarkerWhileOffLeavesTheEnableAndCrossfadeAlone(); + testEditingTheStartMarkerWhileParkedStaysParked(); testANegativeCrossfadeNeverReachesTheStore(); if (g_fail == 0) { diff --git a/tests/test_sample_chrome.cpp b/tests/test_sample_chrome.cpp index 331c347..050195c 100644 --- a/tests/test_sample_chrome.cpp +++ b/tests/test_sample_chrome.cpp @@ -157,8 +157,9 @@ static void testHoldCellIsReservedAndFollowsTheVelocityCellGrammar() { // the agreed remedy if it cannot is to narrow the enable's segments, never to move the floor. static void testTheControlRunLeavesTheTitleReadableAtTheEditorFloor() { const ChromeRects r = chromeRects(chromeBand(), kKnob); - // "ReaSampler 9000" (15 chars) plus a bracketed 20-char capture name, at the toolbar font's - // generous ~7 px/char estimate: 38 * 7. + // "ReaSampler 9000" (15 chars) plus a bracketed 20-char capture name, at an assumed 7 px/char + // for the toolbar font: 38 * 7. A HEURISTIC, not a measured Segoe UI metric — no font-metric + // measurement backs this number; it gates a phase-wide acceptance criterion regardless. constexpr int kTitleTextFloorPx = 266; CHECK(r.title.width >= kTitleTextFloorPx); // Nothing in the run reaches into the title's slot. diff --git a/tests/test_theme.cpp b/tests/test_theme.cpp index 799f570..1de6897 100644 --- a/tests/test_theme.cpp +++ b/tests/test_theme.cpp @@ -269,6 +269,25 @@ static void testCardNameScrimClearsBodyFloorOnItsWorstBackground() { < textFloor(TextClass::Body)); } +// The waveform band's state caption and its four mark labels (editor_paint_waveform.cpp) draw +// text/dim (the caption, the non-promoted labels) and text/primary (the promoted label) — both +// Font::Micro, both body class — over a bg/base scrim at kWaveformLabelScrimAlpha, itself drawn +// over whatever drawEnvelope left in that rect. Unlike the card name strip, the worst case here +// is text/DIM, not text/primary, so the alpha is higher than kCardNameScrimAlpha's — pin both +// roles against the worst background (a full-scale envelope peak, accent/primary) and pin the +// defect the scrim exists to close. +static void testWaveformLabelScrimClearsBodyFloorOnItsWorstBackground() { + const KitColor scrim = roleColor(Role::BgBase); + const KitColor onFill = compositeOver(scrim, roleColor(Role::AccentPrimary), + kWaveformLabelScrimAlpha); + CHECK(contrastRatio(roleColor(Role::TextDim), onFill) >= textFloor(TextClass::Body)); + CHECK(contrastRatio(roleColor(Role::TextPrimary), onFill) >= textFloor(TextClass::Body)); + // Without the scrim, text/dim on the bare accent-lime fill is UNDER floor (~1.58:1) — pins + // the defect the scrim exists to close, so a future removal of the scrim fails this first. + CHECK(contrastRatio(roleColor(Role::TextDim), roleColor(Role::AccentPrimary)) + < textFloor(TextClass::Body)); +} + // --- Single point of change (structural guarantee) ---------------------------- // // roleColor is the ONLY color source; there is no other public accessor that yields a @@ -366,6 +385,7 @@ int main() { testTextOnPastelFillClearsBodyFloor(); testTextOnHoverSurfaceClearsFloor(); testCardNameScrimClearsBodyFloorOnItsWorstBackground(); + testWaveformLabelScrimClearsBodyFloorOnItsWorstBackground(); testCurveTraceOnHoverSurfaceClearsFloor(); testSecondaryTertiaryAreDistinguishable(); testOverlayTraceClearsIndicatorFloorOnTheWaveform();