From 9812690b9690d09ddc842d743d894f91eefbaf79 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 30 Jul 2026 09:35:18 -0400 Subject: [PATCH] Fix stale hover latch on drag release, tooltip anchor, border off-by-one; pin gutter golden test, drop DPI framing --- CMakeLists.txt | 2 +- src/core/instrument/CLAUDE.md | 5 ++ src/core/instrument/ui/keyboard_strip.h | 7 ++- src/core/instrument/ui/sample_chrome.cpp | 2 +- src/shell/instrument/editor_input.cpp | 5 ++ src/shell/instrument/editor_input_chrome.cpp | 1 - src/shell/instrument/editor_paint_chrome.cpp | 7 ++- tests/test_keyboard_strip.cpp | 55 ++++++++++++++++---- 8 files changed, 64 insertions(+), 20 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d3086b9..e0adb22 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1034,7 +1034,7 @@ target_link_libraries(capture_browser_tests PRIVATE capture_browser) add_test(NAME capture_browser_tests COMMAND capture_browser_tests) add_executable(keyboard_strip_tests tests/test_keyboard_strip.cpp) -target_link_libraries(keyboard_strip_tests PRIVATE keyboard_strip) +target_link_libraries(keyboard_strip_tests PRIVATE keyboard_strip sample_bands) add_test(NAME keyboard_strip_tests COMMAND keyboard_strip_tests) # waveform_view (S11): the pure marker geometry + zero-crossing snap. Links waveform_view (+ diff --git a/src/core/instrument/CLAUDE.md b/src/core/instrument/CLAUDE.md index 33f45de..09a390c 100644 --- a/src/core/instrument/CLAUDE.md +++ b/src/core/instrument/CLAUDE.md @@ -242,3 +242,8 @@ slider couldn't. Two pure modules split the forward (draw) and inverse (edit) ma Channel-mode (D-E) bus-renegotiation design and the earlier Preserve-onset-latency framing in the S16 guardrails. Root `CLAUDE.md` is the current source of truth for both — do not reintroduce either superseded design. +- **`keyboard_strip`'s width-uniformity guarantee is client-pixel only.** Its test sweep + covers client-pixel widths (including multiples standing in for larger client areas); + nothing in the instrument implements `IPlugViewContentScaleSupport`, so host-side DPI + scaling of the plugin window — which would resample the uniform integer key widths at the + physical-pixel level — is unverified. diff --git a/src/core/instrument/ui/keyboard_strip.h b/src/core/instrument/ui/keyboard_strip.h index 2c9fd2b..4576431 100644 --- a/src/core/instrument/ui/keyboard_strip.h +++ b/src/core/instrument/ui/keyboard_strip.h @@ -1,9 +1,7 @@ // keyboard_strip.h — piano-keyboard geometry for the editor's root strip: per-class key // rects, hit-test, the root marker, and the note name a hovered key reports. // -// Same-class keys are one integer width by construction. An arbitrary band width is not -// divisible by the 75 white keys, so the residue lands in symmetric end margins — uniform -// key widths and gap-free edge-to-edge tiling cannot both hold, and uniformity wins. +// See src/core/instrument/CLAUDE.md for the uniform-width-vs-edge-to-edge-tiling tradeoff. #pragma once @@ -21,7 +19,8 @@ inline constexpr int kStripKeyCount = 128; inline constexpr int kStripWhiteKeyCount = 75; struct StripLayout { - Rect band; // the surface handed in, edge to edge + Rect band; // the surface handed in, edge to edge (no src/ consumer; kept as the tests' + // reference bound instead of recomputing Rect::ltrb(0, 0, w, h) at each call site) Rect keys; // the tiled key area, kStripWhiteKeyCount * whiteWidth, centred in band int whiteWidth = 0; int blackWidth = 0; diff --git a/src/core/instrument/ui/sample_chrome.cpp b/src/core/instrument/ui/sample_chrome.cpp index ca010a8..52debb0 100644 --- a/src/core/instrument/ui/sample_chrome.cpp +++ b/src/core/instrument/ui/sample_chrome.cpp @@ -4,7 +4,7 @@ #include -#include "core/instrument/ui/sample_bands.h" // kPad / kTitleHeight / kChromeRowHeight +#include "core/instrument/ui/sample_bands.h" // kPad namespace reasampler::instrument::ui { diff --git a/src/shell/instrument/editor_input.cpp b/src/shell/instrument/editor_input.cpp index b439f93..2bb0bc1 100644 --- a/src/shell/instrument/editor_input.cpp +++ b/src/shell/instrument/editor_input.cpp @@ -79,6 +79,11 @@ void ReaSamplerEditor::onMouseUp(int x, int y) { drag_ = DragKind::kNone; dragParamId_ = -1; curvePointIndex_ = -1; + // hover_ is deliberately not re-resolved during a drag (see resolveHover's caller), so it + // still names wherever the drag started. Re-resolve now against the release position, for + // every drag kind — otherwise the next paint latches a stale hover (wrong note name/tooltip, + // wrong control outline) until the next WM_MOUSEMOVE. + resolveHover(x, y); // A scrollbar drag is transient UI (no parameter change), and the processor-side knobs // (the preview-velocity -2 sentinel, voice count, master gain) are per-instance settings // that don't reload the instrument. Master gain is an atomic the audio thread reads diff --git a/src/shell/instrument/editor_input_chrome.cpp b/src/shell/instrument/editor_input_chrome.cpp index 5886f83..bb7cd36 100644 --- a/src/shell/instrument/editor_input_chrome.cpp +++ b/src/shell/instrument/editor_input_chrome.cpp @@ -75,7 +75,6 @@ bool ReaSamplerEditor::mouseDownChrome(const FaceLayout& fl, int x, int y) { const StripLayout sl = layoutStrip(cr.rootStrip.width, cr.rootStrip.height); if (keyAtPoint(sl, x - cr.rootStrip.x, y - cr.rootStrip.y) >= 0) { drag_ = DragKind::kRootMarker; - dragStartX_ = x; dragStartParams_ = params_; onMouseMove(x, y); return true; diff --git a/src/shell/instrument/editor_paint_chrome.cpp b/src/shell/instrument/editor_paint_chrome.cpp index 0603ae3..e4528c6 100644 --- a/src/shell/instrument/editor_paint_chrome.cpp +++ b/src/shell/instrument/editor_paint_chrome.cpp @@ -205,13 +205,16 @@ void ReaSamplerEditor::paintChromeTooltip(LICE_IBitmap* bmp, const FaceLayout& f const std::string label = noteName(hover_.index); const int textW = static_cast(label.size()) * kTooltipCharPx; - const TooltipBox tb = computeTooltip(area.x + key.x, area.y + key.y, key.width, key.height, + // Anchor y/h to the whole strip row, not the hovered key: a black key (18px) is shorter + // than a white key (30px), and anchoring to the key rect placed the chip 12px higher for + // black keys — landing on the keyboard's bottom edge and, near the root, on the badge. + const TooltipBox tb = computeTooltip(area.x + key.x, area.y, key.width, area.height, textW, kTooltipTextH, w, h, TooltipSpec{}); if (tb.empty()) return; const Rect box = Rect::ltrb(tb.x, tb.y, tb.x + tb.width, tb.y + tb.height); fillSurface(bmp, toKitBox(box), Role::BgCell, InteractionState::Hover); - LICE_DrawRect(bmp, box.x, box.y, box.width, box.height, + LICE_DrawRect(bmp, box.x, box.y, box.width - 1, box.height - 1, toLice(roleColor(Role::LineHairline)), 1.0f, 0); kitTextCentered(bmp, box, label.c_str(), Font::Label, Role::TextPrimary); } diff --git a/tests/test_keyboard_strip.cpp b/tests/test_keyboard_strip.cpp index fabaed9..a9aec23 100644 --- a/tests/test_keyboard_strip.cpp +++ b/tests/test_keyboard_strip.cpp @@ -2,14 +2,23 @@ // framework. Same fast assert loop as the sibling pure tests. // // Covers: layoutStrip (normal, degenerate, sub-key-width); same-class key-width uniformity -// swept across editor widths AND DPI scale factors; the tiled key area staying centred +// swept across editor client widths (including multiples of them, standing in for larger +// client sizes — see the client-pixel-only note below); the tiled key area staying centred // inside a band that spans the full width it was handed; whiteIndexOf / isNaturalKey across // octave boundaries and the 0..127 extremes; keyRect tiling and black-over-white overlap; // keyAtPoint resolving black-over-white by zone and missing off-band; the root affordance's -// hit-to-marker round trip; resolveDragNote clamping a wandering pointer; and noteName under -// the C4 (MIDI 60) DAW convention. +// hit-to-marker round trip; resolveDragNote clamping a wandering pointer; noteName under the +// C4 (MIDI 60) DAW convention; and the gutter pinned at the shipped default window size. +// +// Client-pixel-only guarantee: every width swept below is a CLIENT-pixel width. Nothing in +// the instrument implements IPlugViewContentScaleSupport, so if a host scales the plugin +// window itself, uniform integer key widths get resampled at the physical-pixel level — +// unverified by this suite (see src/core/instrument/CLAUDE.md). #include "../src/core/instrument/ui/keyboard_strip.h" +#include "../src/core/instrument/ui/sample_bands.h" // kPad, to derive the strip width the + // shipped default window (840x620, + // editor_session.cpp) hands the strip #include #include @@ -24,11 +33,12 @@ static int g_fail = 0; // A comfortable strip: wide enough that every class is several pixels across. static StripLayout wideStrip() { return layoutStrip(1280, 30); } -// The widths a real editor hands the strip — the 560px minimum client up to a wide -// window — crossed with the DPI scale factors Windows actually reports. +// The widths a real editor hands the strip — the 560px minimum client up to a wide window. static const int kBaseWidths[] = {544, 600, 640, 700, 749, 750, 751, 824, 900, 1000, 1024, 1103, 1264, 1600, 1920, 2400}; -static const double kDpiScales[] = {1.0, 1.25, 1.5, 1.75, 2.0}; +// Multiplies each base width to widen client-pixel coverage (e.g. a maximized/larger client +// area) — NOT a host DPI/content-scale factor; see the client-pixel-only note above. +static const double kWidthMultipliers[] = {1.0, 1.25, 1.5, 1.75, 2.0}; // --- layoutStrip -------------------------------------------------------------- @@ -61,11 +71,11 @@ static void testDegenerateSizesYieldNoKeys() { CHECK(keyAtPoint(narrow, 10, 10) == -1); } -// --- the sharp one: same-class widths are uniform at every width and DPI scale --- +// --- the sharp one: same-class widths are uniform at every client width tested --- -static void testSameClassKeysAreEqualWidthAcrossWidthsAndDpiScales() { +static void testSameClassKeysAreEqualWidthAcrossClientWidths() { for (const int base : kBaseWidths) { - for (const double scale : kDpiScales) { + for (const double scale : kWidthMultipliers) { const int w = static_cast(base * scale); const int h = static_cast(30 * scale); const StripLayout L = layoutStrip(w, h); @@ -98,7 +108,7 @@ static void testSameClassKeysAreEqualWidthAcrossWidthsAndDpiScales() { static void testKeyAreaSpansTheBandWithinOneKeyAtEveryTestedWidth() { for (const int base : kBaseWidths) { - for (const double scale : kDpiScales) { + for (const double scale : kWidthMultipliers) { const int w = static_cast(base * scale); const StripLayout L = layoutStrip(w, 30); CHECK(L.band.width == w); // the strip always spans the width it was handed @@ -268,10 +278,32 @@ static void testNoteNamesFollowTheC4Convention() { CHECK(noteName(500) == "G9"); } +// --- the gutter at the shipped default window size ----------------------------- + +// The rule-based sweep above pins `margins == w % 75` and `leftMargin == margins/2` at every +// width, but pins no concrete number — Daniel is making a visual call on the specific gutter +// at the shipped default, and neither kPad nor the 840 default is covered by another test +// firing if either ever changes. The strip is a sawtooth with period kStripWhiteKeyCount (75) +// px of window width, and the shipped 840 default lands on residue 74 — the cycle's maximum: +// one pixel of resize (840->841) collapses both gutters to zero and grows every white key +// from 10px to 11px. +static void testGutterAtTheShippedDefaultWindowSize() { + constexpr int kShippedDefaultWindowW = 840; // editor_session.cpp's ViewRect default + const int stripW = kShippedDefaultWindowW - 2 * kPad; // chromeRects insets rootStrip by kPad + CHECK(stripW == 824); + + const StripLayout L = layoutStrip(stripW, 30); + CHECK(L.whiteWidth == 10); + const int margins = L.band.width - L.keys.width; + CHECK(margins == 74); + const int leftMargin = L.keys.x - L.band.x; + CHECK(leftMargin == 37); +} + int main() { testLayoutFillsTheBandAndCentresTheKeys(); testDegenerateSizesYieldNoKeys(); - testSameClassKeysAreEqualWidthAcrossWidthsAndDpiScales(); + testSameClassKeysAreEqualWidthAcrossClientWidths(); testKeyAreaSpansTheBandWithinOneKeyAtEveryTestedWidth(); testIsNaturalKeyAcrossAnOctaveAndTheExtremes(); testWhiteIndexCountsNaturalsBelowTheNote(); @@ -284,6 +316,7 @@ int main() { testHitTestingAKeyMarksThatSameKey(); testDragTracksThePointerAndClampsWhenItWanders(); testNoteNamesFollowTheC4Convention(); + testGutterAtTheShippedDefaultWindowSize(); if (g_fail == 0) { std::printf("keyboard_strip: all tests passed\n");