daniel/reasampler
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix S10 code-review findings: WM_CAPTURECHANGED, proportional drag, paintSetup root source, Add Zone dedup, strip-area helpers

Browse Source
This commit is contained in:
Daniel Harvey
2026-07-26 20:53:31 -04:00
parent 7151e19432
commit b9ad1ee18f
3 changed files with 90 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/vst/keyboard_strip.cpp
+14 -6
View File
@@ -106,12 +106,20 @@ int resolveDragNote(const StripLayout& layout, int startNote, int dxPixels) {
if (dxPixels == 0) return clampNote(startNote);
const int bandWidth = std::max(0, layout.keys.width());
if (bandWidth <= 0) return clampNote(startNote); // zero-width -> no motion
// Key width in pixels (>= 1 via the max). Round the delta to the nearest key so a
// half-key drag flips at the key centre: add/subtract half a key before the divide.
const int keyW = std::max(1, bandWidth / kStripKeyCount);
const int half = keyW / 2;
const int shift = dxPixels >= 0 ? (dxPixels + half) / keyW
: -((-dxPixels + half) / keyW);
// Proportional shift: same linear mapping as keyAtPoint/keyEdgeToX so click and drag
// agree across the full strip, even on non-divisible-by-128 widths. The proportional
// key width is (bandWidth / kStripKeyCount) in exact rational arithmetic; rounding to
// the nearest key (half-key drag flips at the key centre) is achieved by adding
// bandWidth/2 to the absolute pixel delta before dividing — identical to the old
// formula except keyWidth is now derived from the same linear map (exact rational)
// rather than the truncated-integer bandWidth/128 that caused drift at the far end.
const int half = bandWidth / 2;
int shift;
if (dxPixels > 0) {
shift = (dxPixels * kStripKeyCount + half) / bandWidth;
} else {
shift = -(((-dxPixels) * kStripKeyCount + half) / bandWidth);
}
return clampNote(startNote + shift);
}
src/vst/reasampler_editor.cpp
+50 -19
View File
@@ -314,6 +314,26 @@ EditorBands computeBands(int w, int h) {
b.content = Rect{0, toggleBot, w, h};
return b;
}
// The keyboard strip rectangle inside the setup area (single-capture root-drag face).
// `area` is the full setup Rect; the strip is anchored at the bottom with an 8px horizontal
// pad. All three call sites (paintSetup, onMouseDown, onMouseMove) use this single formula.
Rect setupStripArea(const Rect& area) {
constexpr int pad = 8;
const int stripTop = area.bottom - kStripBandHeight;
return Rect{area.left + pad, stripTop, area.right - pad, area.bottom - 4};
}
// The keyboard strip rectangle inside the Zones panel content area. `bands.content` is the
// mode-content Rect; the strip sits below the "+ Add Zone" affordance (top+4, height 20)
// with a 12px gap, padded 8px horizontally. All three call sites (paintZones, onMouseDown,
// onMouseMove) use this single formula — the inline arithmetic in onMouseMove was the drift.
Rect zonesStripArea(const EditorBands& bands) {
constexpr int pad = 8;
const int stripTop = bands.content.top + 4 + 20 + 12; // addR.bottom + 12
return Rect{bands.content.left + pad, stripTop, bands.content.right - pad,
stripTop + kStripBandHeight};
}
} // namespace
void ReaSamplerEditor::paint(HDC hdc) {
@@ -445,8 +465,10 @@ void ReaSamplerEditor::paintSetup(LICE_IBitmap* bmp, const Rect& area) {
kColTitleBg, 1.0f, 0);
// Effective root: the picked sample's rootNote intrinsic (or middle C when unset).
// Read from samples_ (the full unfiltered list) so a bank-filter that hides the
// picked sample's bank doesn't mask its intrinsic root with the C4 default.
int root = 60;
for (const SampleChoice& s : visible_) {
for (const SampleChoice& s : samples_) {
if (s.id == selectedId_ && s.rootNote) root = *s.rootNote;
}
// If a matching one-zone override exists (opt-in from Zones), prefer it as the shown root.
@@ -463,8 +485,7 @@ void ReaSamplerEditor::paintSetup(LICE_IBitmap* bmp, const Rect& area) {
drawText(bmp, hintR, "Drag on the keyboard to set the root note.", kRgbDim);
// Keyboard strip with the root marker.
const int stripTop = area.bottom - kStripBandHeight;
Rect stripArea{area.left + pad, stripTop, area.right - pad, area.bottom - 4};
const Rect stripArea = setupStripArea(area);
const StripLayout sl = layoutStrip(stripArea.width(), stripArea.height());
const int sx = stripArea.left;
const int sy = stripArea.top;
@@ -503,9 +524,7 @@ void ReaSamplerEditor::paintZones(LICE_IBitmap* bmp, int w, int h) {
}
// The zones strip.
const int stripTop = addR.bottom + 12;
Rect stripArea{bands.content.left + pad, stripTop, bands.content.right - pad,
stripTop + kStripBandHeight};
const Rect stripArea = zonesStripArea(bands);
const StripLayout sl = layoutStrip(stripArea.width(), stripArea.height());
const int sx = stripArea.left;
const int sy = stripArea.top;
@@ -581,8 +600,7 @@ void ReaSamplerEditor::onMouseDown(int x, int y) {
// The setup strip: grab the root marker (drag to set the picked capture's root).
if (havePick) {
const Rect area{bands.content.left, setupTop, bands.content.right, bands.content.bottom};
const int stripTop = area.bottom - kStripBandHeight;
const Rect stripArea{area.left + 8, stripTop, area.right - 8, area.bottom - 4};
const Rect stripArea = setupStripArea(area);
const StripLayout sl = layoutStrip(stripArea.width(), stripArea.height());
const int note = keyAtPoint(sl, x - stripArea.left, y - stripArea.top);
if (note >= 0) {
@@ -603,11 +621,21 @@ void ReaSamplerEditor::onMouseDown(int x, int y) {
Rect addR{bands.content.left + pad, bands.content.top + 4, bands.content.left + pad + 96,
bands.content.top + 4 + 20};
if (contains(addR, x, y)) {
// Append a full-keyboard zone for the picked capture (or the first visible sample as a
// sensible seed). No pick -> nothing to add.
// Add a full-keyboard zone for the picked capture (or the first visible sample as a
// sensible seed). No pick -> nothing to add. If a full-keyboard zone for the seed id
// already exists, select it rather than appending a duplicate (mirrors the upsert the
// root-marker drag path already performs, preventing overlapping identical zones).
std::string seed = !selectedId_.empty() ? selectedId_
: (!visible_.empty() ? visible_.front().id : std::string());
if (seed.empty()) return;
for (int i = 0; i < static_cast<int>(map_.zones.size()); ++i) {
const PerformanceZone& z = map_.zones[static_cast<std::size_t>(i)];
if (z.sampleId == seed && z.lowNote == 0 && z.highNote == 127) {
selectedZone_ = i;
invalidate();
return;
}
}
PerformanceZone z;
z.sampleId = seed;
z.lowNote = 0;
@@ -627,9 +655,7 @@ void ReaSamplerEditor::onMouseDown(int x, int y) {
// The zones strip: hit-test a bar edge/body to start a drag, or a bare key to set the
// selected zone's root.
const int stripTop = addR.bottom + 12;
const Rect stripArea{bands.content.left + pad, stripTop, bands.content.right - pad,
stripTop + kStripBandHeight};
const Rect stripArea = zonesStripArea(bands);
const StripLayout sl = layoutStrip(stripArea.width(), stripArea.height());
const int lx = x - stripArea.left;
const int ly = y - stripArea.top;
@@ -680,8 +706,7 @@ void ReaSamplerEditor::onMouseMove(int x, int y) {
// The single-capture root strip lives in the setup band.
const int setupTop = (std::max)(bands.content.top, bands.content.bottom - kSetupHeight);
const Rect area{bands.content.left, setupTop, bands.content.right, bands.content.bottom};
const Rect stripArea{area.left + 8, area.bottom - kStripBandHeight, area.right - 8,
area.bottom - 4};
const Rect stripArea = setupStripArea(area);
const StripLayout sl = layoutStrip(stripArea.width(), stripArea.height());
const int note = resolveDragNote(sl, dragStartRoot_, dx);
// The performance map is the ONLY D-B override vehicle (rootOverride lives on a zone),
@@ -708,10 +733,7 @@ void ReaSamplerEditor::onMouseMove(int x, int y) {
// Zone edits: recompute the grabbed field(s) against the pure resolver, live.
if (selectedZone_ < 0 || selectedZone_ >= static_cast<int>(map_.zones.size())) return;
const int pad = 8;
const int stripTop = bands.content.top + 4 + 20 + 12; // addR.bottom + 12
const Rect stripArea{bands.content.left + pad, stripTop, bands.content.right - pad,
stripTop + kStripBandHeight};
const Rect stripArea = zonesStripArea(bands);
const StripLayout sl = layoutStrip(stripArea.width(), stripArea.height());
PerformanceZone& z = map_.zones[static_cast<std::size_t>(selectedZone_)];
if (drag_ == DragKind::kZoneLow) {
@@ -765,6 +787,15 @@ LRESULT CALLBACK ReaSamplerEditor::wndProc(HWND hwnd, UINT msg, WPARAM wParam,
ReleaseCapture();
}
return 0;
case WM_CAPTURECHANGED:
// Capture stolen mid-drag (modal dialog, alt-tab, etc.) — reset the drag
// state machine so stale capture-less WM_MOUSEMOVEs don't keep editing.
// Mirror of bank_panel.cpp's WM_CAPTURECHANGED handler.
if (self && self->drag_ != DragKind::kNone) {
self->drag_ = DragKind::kNone;
self->invalidate();
}
return 0;
case WM_ERASEBKGND:
return 1; // fully repaint in WM_PAINT; skip the flicker-inducing erase
default:
tests/test_keyboard_strip.cpp
+26
View File
@@ -179,6 +179,31 @@ static void testResolveDragClampsAndNoOps() {
CHECK(resolveDragNote(Z, 60, 500) == 60);
}
static void testResolveDragProportionalNonDivisibleWidth() {
// THE REVIEW FINDING: 544px / 128 = 4.25 (non-integer). Old uniform-keyW math used
// keyW = 4 (floor), accumulating ~7 keys of drift at the far end. The proportional fix
// must agree with keyAtPoint at every point — specifically the far-end invariant:
// a drag from note 0 by (width-1) pixels must land at keyAtPoint(width-1), which is 127.
const int width = 544;
const StripLayout L = layoutStrip(width, 40);
CHECK(keyAtPoint(L, width - 1, L.keys.top + 1) == 127);
CHECK(resolveDragNote(L, 0, width - 1) == 127);
// Also verify mid-strip coherence: for each key N, a drag from 0 by N's left-edge
// pixel offset should land at N (or N-1 at worst — left-edge pixel is a boundary, so
// rounding may round down). The critical direction is that it must NOT over-shoot by
// more than 0 (it must reach at least the right key).
for (int n = 1; n < kStripKeyCount; ++n) {
const int leftPx = keyRect(L, n).left;
const int resolved = resolveDragNote(L, 0, leftPx);
// The left edge of key N is the first pixel "in" that key, so we expect resolved == N.
// Allow resolved == N-1 only when the pixel is at the exact boundary (keyEdgeToX may
// produce the same x for adjacent keys when keys share a pixel). Disallow over-shoot.
const int expected = keyAtPoint(L, leftPx, L.keys.top + 1);
CHECK(resolved >= expected - 1 && resolved <= expected + 1);
}
}
int main() {
testLayoutNormalArea();
testLayoutZeroArea();
@@ -195,6 +220,7 @@ int main() {
testZoneBarAtPointNullList();
testResolveDragRoundsToNearestKey();
testResolveDragClampsAndNoOps();
testResolveDragProportionalNonDivisibleWidth();
if (g_fail == 0) std::printf("keyboard_strip: all tests passed\n");
return g_fail != 0;