Ω-W1-T5 review fixes: unify the drag frame resolve onto frameToX/xToFrame; honest comments on grabbableMarks and the grey-loop-mark contrast trade

src/core/instrument/ui/waveform_view.cpp

@@ -4,7 +4,6 @@
 
 #include <algorithm>
 #include <cstddef>
-#include <cstdlib>  // std::abs (int overload)
 
 #include "core/instrument/ui/sample_bands.h"  // waveformLanes (the band's lane inventory)
 #include "core/ui/component_geometry.h"       // waveformColumnCount (THE draw chain's columns)
@@ -170,13 +169,13 @@ std::int64_t resolveDragFrame(const OverlayArea& area, std::int64_t frameCount,
     if (dxPixels == 0) return start;
     const int w = std::max(0, area.rect.width);
     if (frameCount <= 0 || w <= 0) return start;  // no room to move
-    // Proportional shift, rounded to the nearest frame (same linear map as frameToX/xToFrame).
-    const std::int64_t magnitude =
-        (static_cast<std::int64_t>(std::abs(dxPixels)) * frameCount +
-         static_cast<std::int64_t>(w) / 2) /
-        static_cast<std::int64_t>(w);
-    const std::int64_t shift = dxPixels > 0 ? magnitude : -magnitude;
-    return clampFrame(start + shift, frameCount);
+    // THE unified mapping, not a second one: find the start frame's own column (frameToX),
+    // walk it by dxPixels, and read the frame back off the resulting pixel (xToFrame) — so a
+    // drag always resolves to the column under the cursor, never a proportional approximation
+    // of it. A prior independent linear map here could disagree with frameToX/xToFrame's
+    // truncating column partition, most visibly when frames < columns.
+    const int grabX = frameToX(area, frameCount, start);
+    return xToFrame(area, frameCount, grabX + dxPixels);
 }
 
 std::int64_t nearestZeroCrossing(const AudioSample* pcm, std::int64_t frames,

src/core/instrument/ui/waveform_view.h

@@ -83,6 +83,14 @@ inline constexpr int kMarkerGrabWidth = 5;
 // for every f whenever frames <= columns, i.e. exactly where the choice of end is observable).
 // Above that, several frames share a column and the round trip snaps to the column's first
 // frame, which is the quantization the shared column already is.
+//
+// KNOWN CONVENTION MISMATCH, not a bug: in the frames < columns regime a mark lands on the
+// LAST column of its frame's span (above), while the spline contour and the staged AHD
+// polyline map normalized t in [0,1] straight across the same box and so land on a span's
+// FIRST column instead. Both land inside the frame's own span, so the mapping contract holds,
+// but the two conventions can disagree by up to a span's width for captures under ~1000
+// frames. Unifying them is a real option, left alone here since it touches the spline/AHD draw
+// path rather than this pair.
 
 // x pixel of `frame`: the column that draws it. `frameCount` itself is a span's EXCLUSIVE end,
 // not a frame, and maps to area.right(). Frame is clamped to [0, frameCount] first;
@@ -169,9 +177,11 @@ int crossfadeWedgeHeight(int x0, int x1, int x);
 int markerAtPoint(const OverlayArea& area, std::int64_t frameCount, const std::int64_t* frames,
                   int count, int x, int y);
 
-// Resolves a drag to a new frame: `startFrame` shifted by round(dxPixels * frameCount /
-// areaWidth), clamped to [0, frameCount]. The shell applies between-marker clamps (e.g.
-// start <= loopEnd) after this per-marker resolve.
+// Resolves a drag to a new frame: locates `startFrame`'s own column via frameToX, walks it by
+// dxPixels, and reads the frame back via xToFrame — the SAME partition, so the result is always
+// the frame the cursor's column actually draws, never a proportional approximation of it.
+// Clamped to [0, frameCount]. The shell applies between-marker clamps (e.g. start <= loopEnd)
+// after this per-marker resolve.
 std::int64_t resolveDragFrame(const OverlayArea& area, std::int64_t frameCount,
                               std::int64_t startFrame, int dxPixels);
 

src/shell/instrument/editor_controls.cpp

@@ -164,9 +164,11 @@ instrument::ui::WaveMarks ReaSamplerEditor::waveMarksFor(const SetupMarkers& m)
 }
 
 instrument::ui::WaveMarks ReaSamplerEditor::grabbableMarks(const SetupMarkers& m) const {
-    // Drawn IFF grabbable. Kept as its own fold because paint and hit-test stay separate
-    // questions, but do NOT re-add a suppression here: the Gate-with-loop-off marks are drawn
-    // grey precisely so they can still be dragged, and dragging one is what turns the enable on.
+    // Drawn IFF grabbable is the product rule, so this is an exact alias of waveMarksFor and
+    // cannot currently diverge from it. Kept as its own seam anyway because paint and hit-test
+    // are separate questions in principle — but do NOT re-add a suppression here: the
+    // Gate-with-loop-off marks are drawn grey precisely so they can still be dragged, and
+    // dragging one is what turns the enable on.
     return waveMarksFor(m);
 }
 

src/shell/instrument/editor_paint_waveform.cpp

@@ -46,9 +46,11 @@ constexpr Role kRoleStartMarker = Role::OverlayTrace;
 constexpr Role kRoleLoopMarker  = Role::AccentSecondary;
 // A loop mark whose enable is off keeps its position and its cap — and its full weight, since
 // it is still draggable. It changes HUE, not opacity: the dim teal it replaces read as broken
-// rather than as off. Grey against the lime is deliberately under the 3:1 state-indicator floor
-// the two-neighbour rule (core/ui/CLAUDE.md) sets for a LIVE mark — an inactive control is
-// exempt, and that lower contrast is the off cue.
+// rather than as off. Grey against the lime deliberately sits under the 3:1 state-indicator
+// floor the two-neighbour rule (core/ui/CLAUDE.md) sets — on a mark that stays LIVE and still
+// drives the loop enable, so this is NOT the WCAG 1.4.11 inactive-component carve-out. It is a
+// deliberate trade (the off cue reads as lower contrast on a control that can still be
+// grabbed), pending Daniel's eye in the DAW. Do not change the colour to chase it.
 constexpr Role kRoleLoopMarkerOff = Role::TextDim;
 
 // Mark weights. The crossfade is a SOFT boundary and rides below the loop pair's weight at rest.

tests/test_waveform_view.cpp

@@ -10,9 +10,10 @@
 // waveformOverlayArea (the overlay IS the drawn column band, inset symmetrically);
 // markerAtPoint (grab band, first-match on overlap, off-area + null-array rejection);
 // markerHandleRect (the top-strip tab that keeps coincident markers independently grabbable);
-// resolveDragFrame (round-to-nearest-frame, clamp to [0,frameCount], zero-delta/zero-width
-// no-ops); nearestZeroCrossing (nearest sign-change, sample-on-zero, equidistant-tie-to-lower,
-// no-crossing keeps target, target clamp, degenerate buffers); the four marks (per-mark cap
+// resolveDragFrame (drag lands on the frameToX/xToFrame column under the cursor, clamp to
+// [0,frameCount], zero-delta/zero-width no-ops); nearestZeroCrossing (nearest sign-change,
+// sample-on-zero, equidistant-tie-to-lower, no-crossing keeps target, target clamp, degenerate
+// buffers); the four marks (per-mark cap
 // resolve, the reverse cap order that keeps a coincident pair separable, label sides/nudging,
 // the suppression rule and its promoted-first placement, the crossfade wedge ramp);
 // waveformSurface (two stacked
@@ -263,6 +264,36 @@ static void testResolveDragFrameDegenerate() {
     CHECK(resolveDragFrame(ov, 1000, 5000, 0) == 1000);
 }
 
+// A drag must land on the column under the cursor — the SAME frameToX/xToFrame partition, never
+// a proportional approximation of it. Pins the contract itself (grabX = frameToX(startFrame),
+// result = xToFrame(grabX + dxPixels)) rather than a captured number, in the frames < columns
+// regime where the two disagree: a prior independent linear map here left a marker at frame 10
+// (1000px/37 frames, start=10, +3px) when a fresh xToFrame(x) at the same cursor column
+// resolves to frame 11 — exactly the class of drift a second frame<->pixel map produces.
+static void testResolveDragFrameLandsOnCursorColumn() {
+    const OverlayArea ov = overlayOf(Rect::ltrb(0, 0, 1000, 60));
+    const std::int64_t frameCount = 37;
+    const std::int64_t startFrame = 10;
+    const int dx = 3;
+    const int grabX = frameToX(ov, frameCount, startFrame);
+    const std::int64_t cursorFrame = xToFrame(ov, frameCount, grabX + dx);
+    CHECK(cursorFrame == 11);                              // the contract's own derivation
+    CHECK(resolveDragFrame(ov, frameCount, startFrame, dx) == cursorFrame);
+    // General form, swept across both frames<columns and frames>columns: the resolved frame's
+    // OWN column (frameToX) must contain the cursor pixel the drag actually landed on.
+    const std::int64_t counts[] = {5, 37, 251, 9973};
+    const int deltas[] = {-97, -3, -1, 1, 3, 97};
+    for (std::int64_t n : counts) {
+        for (std::int64_t start = 0; start < n; start += (std::max<std::int64_t>)(1, n / 11)) {
+            for (int d : deltas) {
+                const std::int64_t got = resolveDragFrame(ov, n, start, d);
+                const int wantGrabX = frameToX(ov, n, start);
+                CHECK(got == xToFrame(ov, n, wantGrabX + d));
+            }
+        }
+    }
+}
+
 // --- nearestZeroCrossing ------------------------------------------------------
 
 static void testZeroCrossingNearest() {
@@ -786,6 +817,7 @@ int main() {
     testResolveDragFrameClamps();
     testResolveDragFrameRounds();
     testResolveDragFrameDegenerate();
+    testResolveDragFrameLandsOnCursorColumn();
 
     testZeroCrossingNearest();
     testZeroCrossingSampleOnZero();