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.

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;

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).

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;

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<float>(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<WaveMark>(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<WaveMark>(promoted)),
                         Font::Micro, Role::TextPrimary);
     }

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<SelectedSample> 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 &&

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<SampleLoop> loopOverride;
         std::int64_t crossfade = 0;
+        std::optional<std::int64_t> 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.