Skip to content

lib/ui: labels that measure themselves, grids that don't own your model - #617

Merged
InauguralPhysicist merged 1 commit into
mainfrom
ui-sprint-widget-fixes
Jul 16, 2026
Merged

lib/ui: labels that measure themselves, grids that don't own your model#617
InauguralPhysicist merged 1 commit into
mainfrom
ui-sprint-widget-fixes

Conversation

@InauguralPhysicist

Copy link
Copy Markdown
Collaborator

Second half of the lib/ui gap series from DeslanStudio's desktop-shell port — the widget-level fixes. Stacked on #615 (merged).

What changed

label carries w/h (#561). It was the only text widget without them, and _layout_toolbar/_layout_box read child.w/child.h unconditionally — so the most natural toolbar child crashed the layout engine outright. Measured from the text at construction and refreshed on every _layout pass: a label's text changes at runtime (a BPM or time display restamps every frame) and a size stamped once goes stale the first time the clock ticks.

A label draws no chrome, so unlike badge there's nothing to pad — its box is exactly its text, and spacing comes from the container's gap. (The issue suggested + pad; this seemed the more truthful bounding box. Easy to change if you'd rather.)

grid.owns_cells (#572). The widget flipped cells[r][c] itself and only then reported, so an app whose model is the source of truth had to overwrite grid.cells after every click and every model-side change (undo, pattern switch, randomize) or watch the two drift. owns_cells = 0 makes input report (row, col) and touch nothing. The keyboard path in ui_focus.eigs flips cells too and honors the same rule — otherwise mouse and keyboard would disagree about who owns the pattern.

Grid row-label gutter (#572). ax - 60 at scale 1 was hardcoded; DeslanStudio positions its grid at x=76 purely to make that constant land inside its panel. Now row_label_w/row_label_scale with the historical values as defaults, and vertical centering uses the real text_height of the scale rather than an assumed 7px.

Piano key release (#570). The note-off was a piano_kb special case in dispatch's mouseup that only ran when the mouseup hit-tested back to the same widget. Press a key and drag off it — or release over a button, whose branch returned early — and the note sounded forever. piano_kb now registers a clear_pressed entry and dispatch runs the registry release walk on every mouseup, retiring the special case.

Hover feedback (#594). checkbox and toggle received .hover from dispatch and their renderers ignored it. Both now draw the translucent shade button already used, factored into a shared _draw_hover_shade — an overlay rather than a color swap, so it composes with a per-widget bg (#566) instead of erasing it.

One scope judgment to flag

#570 asked for "either a vertical: 1 mode or a documented note that the widget is a horizontal trigger strip only." I took the second option. DeslanStudio paints its own key sidebar on a canvas and the upstream Qt port does the same, so a vertical mode would ship with zero consumers exercising it — against the forcing-function rule. The widget's scope is now documented in its header and STDLIB instead. Say the word and I'll build the mode.

The other two #570 claims (pressed_note never clears; on_note never called with 0) no longer reproduced — see the evidence comment on the issue. The real bug was the residual above, which the original report didn't have.

Verification

Red-then-green, each fix stashed independently:

  • label w measured from its text — expected 36, got null, plus the issue's exact crash: cannot apply '-' to num and null at _layout_toolbar
  • non-owning grid leaves the cell untouched — expected 0, got 1
  • key released even though mouseup missed it — expected -1, got 0
  • hovered checkbox draws a shade — expected 1, got 0

test_ui 143 → 176 assertions. Render behavior is asserted by capturing what the renderer emits (the shade is the only gfx_rrect carrying an alpha).

  • Release suite: 2890/2890
  • ASan+UBSan, detect_leaks=1: clean, leak tally still 0

The lone ASan invariant_weak.eigs timeout is #616 — unrelated, and it reproduced identically on #615's branch.

Closes #561
Closes #572
Closes #594

Second half of the lib/ui gap series from DeslanStudio's desktop-shell
port — the widget-level fixes.

label w/h (#561): label was the only text widget without w/h, and
_layout_toolbar/_layout_box read child.w/child.h unconditionally, so the
most natural toolbar child — a caption, a BPM or time display — crashed
the layout engine outright. The box is measured from the text at
construction and refreshed on every _layout pass, because a label's text
changes at runtime and a size stamped once would go stale the first time
a clock ticked. A label draws no chrome, so unlike badge there is nothing
to pad: its box is exactly its text, and spacing comes from the
container's gap.

grid.owns_cells (#572): the widget flipped cells[r][c] itself and only
then reported, so an app whose model is the source of truth had to
overwrite grid.cells after every click AND after every model-side change
(undo, pattern switch, randomize) or watch the two drift. owns_cells = 0
makes input report (row, col) and touch nothing. The keyboard path in
ui_focus.eigs flips cells too and honors the same rule — otherwise mouse
and keyboard would disagree about who owns the pattern.

grid row-label gutter (#572): ax - 60 at scale 1 was hardcoded, so long
labels overflowed left and a grid at x < 60 clipped them off the window;
DeslanStudio positions its grid at x=76 purely to make that constant land
inside its panel. Now row_label_w/row_label_scale, with the historical
values as defaults, and vertical centering uses the real text_height of
the scale instead of an assumed 7px.

Piano key release (#570): the note-off was a piano_kb special case in
dispatch's mouseup that only ran when the mouseup hit-tested back to the
same widget. Press a key and drag off it — or release over a button,
whose branch returned early — and the note sounded forever. piano_kb now
registers a clear_pressed entry and dispatch runs the registry release
walk on every mouseup, which retires the special case. Found while
verifying #570, whose other two claims no longer reproduced; the widget's
scope (a horizontal trigger strip, not a piano-roll sidebar) is now
documented rather than given a vertical mode no consumer would exercise —
DeslanStudio and the Qt original both paint that sidebar on a canvas.

Hover feedback (#594): checkbox and toggle received .hover from dispatch
and their renderers ignored it. Both now draw the translucent shade
button already used, factored into a shared _draw_hover_shade — an
overlay rather than a color swap, so it composes with a per-widget bg
(#566) instead of erasing it.

Verified red-then-green: every assertion below fails with its fix removed
(label w/h null and the issue's exact _layout_toolbar crash; cells
pre-flipped; the key stuck at pressed_note 0 after releasing off-widget;
no shade emitted on a hovered checkbox). test_ui 143 -> 176 assertions,
suite 2890/2890, ASan+UBSan with detect_leaks=1 clean (leak tally still
0; the lone invariant_weak timeout is #616, unrelated and reproducing on
main).

Closes #561
Closes #572
Closes #594
@InauguralPhysicist
InauguralPhysicist merged commit 68e9db0 into main Jul 16, 2026
17 checks passed
@InauguralPhysicist
InauguralPhysicist deleted the ui-sprint-widget-fixes branch July 16, 2026 15:15
InauguralPhysicist added a commit that referenced this pull request Jul 16, 2026
Fixes a regression I shipped in #617. That PR made _layout remeasure a
label from its text on every pass, so a caption whose text changes at
runtime keeps a box matching what is drawn. It did that
unconditionally — which silently overwrites a size the app set itself.

Caught by running DeslanStudio's suite against main before the next pin
bump. Its modal file dialog backs a full-window overlay with an EMPTY
label stamped to the window size, because containers are hit-transparent
where no child sits and something has to catch the clicks. Remeasuring an
empty label's text collapsed it to w=0, the catcher stopped catching, and
the dialog silently stopped being modal:

    FAIL: open dialog blocks the transport bar (modal catcher)
    FAIL: closed dialog lets clicks through again

auto_size (default 1) keeps the #617 behavior: the box tracks the text,
which is what a label should do and what #561 needed. Setting it to 0
says the box is the app's — a fixed-width slot for changing text, a
padded caption, or a plain sized rect like that catcher. The constructor
measures once either way, so #561's original crash (a null size reaching
_layout_toolbar) stays impossible even with auto_size off.

Chose an explicit opt-out over inferring one from "did the text change".
The inference would have fixed the catcher by accident — its text never
changes — while still stripping the deliberate +8 padding off every
consumer label that DOES restamp, and it would have done it silently.
That is the failure class this repo hunts, not one to ship.

Verified red-then-green: the new assertions collapse to w=0 / h=14 and
the catcher stops hit-testing without the fix. DeslanStudio's suite goes
24/24 against this branch with its catcher opting out, and stays 24/24
against the v0.31.0 tag it currently pins (the field is inert there), so
the consumer change lands independently of the release.

test_ui 196 -> 201 assertions, suite 2890/2890.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant