yuris/noctalia-timew
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(panel): take the tag out of the fixed-width row that holds the total #22

Merged
yuris merged 1 commit from fix/chip-unbounded-tag into main 2026-09-05 09:29:49 +00:00
Conversation 3 Commits 1 Files changed 5 +355 -323
yuris commented 2026-09-05 09:03:12 +00:00
Owner
Copy link

Reported: a longer tag could still push the total time text far to the right.

Correct, and the residual gap was already documented in fmt.luau: "a
proportional font has no cell, so MMMMMMMMMMMM is twelve cells and still too
wide". Clipping to a cell budget cannot be made right, so the tag left the row.

Why no budget can work here

The scope row is a fixed CHROME_W whose last child is the total. Free space is
grow-only (flex.cpp:532 clamps at zero), arrangement never clamps (:737), and
a node does not clip its children (node.h:211). Every child is bounded by text
this plugin or its translator writes — except a tag, which is whatever the user
typed at a shell.

Fmt.cols charged Wide/Fullwidth two and everything else one. That fixed CJK and
Hangul and left the general case: twelve narrow characters fit where twelve wide
ones did not. Nothing at this plugin API measures text, so the 12 was calibrated
against Mon 31/08 + All time in DejaVu Sans at one chrome. A calibration is
not a bound.

Why the chip could not be capped in place

  • ui.button takes neither maxWidth nor maxLines
    (ui_tree_reconciler.cpp:471-475). A bare width pins min and max and arms
    setEllipsize (button.cpp:707) but Button never calls setMaxLines, so
    effectiveMaxLines is 0 (label.cpp:503) and Pango gets a 500-line budget
    under PANGO_WRAP_WORD_CHAR. A width-capped button wraps.
  • ui.label does elide — maxWidth + maxLines >= 1 reaches
    pango_layout_set_ellipsize(END) and the reported width is that budget
    (label.cpp:487-497, :630) — but a label takes no onClick.
  • kFlex has minWidth and no maxWidth, so a tinted row hugging its text
    cannot be capped. The only bounded-and-tinted shape is flexGrow, measured at
    its whole assigned share, so it would span the row instead of hugging.

What changed

  • The tag is gone from the scope row. That row is now two buttons, a spacer and
    the total at DUR_W — nothing a tag can reach.
  • The caption over the chart carries the tag in full with maxWidth = RIGHT_W
    and maxLines = 1: a real renderer ellipsis, any script, any length. It was
    unbounded before and could overhang the panel edge.
  • The clear button sits beside the search field rather than next to the two
    scope buttons, where a lone glyph read as a third scope. Fixed square at
    SEARCH_H, matching the field; tooltip names the whole tag, untruncated.
  • The field carries flexGrow = 1 instead of CHROME_W - 20, so the layout
    subtracts the button's 28 px rather than a constant that would have to track it.
  • Fmt.clip, Fmt.cols, the Wide-range table and CHIP_TAG_MAX are deleted —
    86 lines out of fmt.luau. Net −51 lines.

clipTag stays: it is a 120-byte cap for a hard API limit (a label over 512
bytes raises and takes the handler down), not a layout guess.

Verification

Suite: 13 states, 1954 field comparisons / ok. Lint: 0 errors, 0 warnings.

The old textual assertions are replaced by a geometric one: state 5b renders five
tags — a, Coding Test Practice, MMMMMMMMMMMM, a CJK tag, and 400 Ws — and
asserts the scope row's shape is identical across all of them, so a tag
reaching that row in any spelling fails there.

25 mutations, no survivors: putting the tag back on the button, unsquaring it,
drifting its height from the field, giving the field a width, dropping either
flexGrow, showing the button unconditionally or never, emptying the tooltip,
making clear not clear, dropping the caption's maxWidth/maxLines, wrapping
instead of eliding, dropping DUR_W, and stopping the spacer growing.

On screen, with a validated method (the panel is translucent over a live
wallpaper, so raw pixel compare is invalid — masks were checked stable across
repeat captures of an unchanged tag first): the scope row's ink is byte-identical
across MMMMMMMMMMMM, Coding Test Practice, a CJK tag, a Hangul tag, 24 @s
and 400 Ws, and the total's own region is identical in every case. The only
pixels that move in that row are 18 antialiasing pixels inside All time, which
appear when the caption loads a CJK fallback font and bumps the renderer's
text-metrics generation.

Also verified live with real pointer input: the tooltip reads Clear tag filter: Coding Test Practice, clicking the ✕ clears the filter and the field grows
back into its space, and the caption elides a 400-character tag with a real …
at the column edge instead of overhanging.

Reported: a longer tag could still push the total time text far to the right. Correct, and the residual gap was already documented in `fmt.luau`: *"a proportional font has no cell, so `MMMMMMMMMMMM` is twelve cells and still too wide"*. Clipping to a cell budget cannot be made right, so the tag left the row. ## Why no budget can work here The scope row is a fixed `CHROME_W` whose last child is the total. Free space is grow-only (`flex.cpp:532` clamps at zero), arrangement never clamps (`:737`), and a node does not clip its children (`node.h:211`). Every child is bounded by text this plugin or its translator writes — except a tag, which is whatever the user typed at a shell. `Fmt.cols` charged Wide/Fullwidth two and everything else one. That fixed CJK and Hangul and left the general case: twelve narrow characters fit where twelve wide ones did not. Nothing at this plugin API measures text, so the 12 was calibrated against `Mon 31/08` + `All time` in DejaVu Sans at one chrome. A calibration is not a bound. ## Why the chip could not be capped in place - `ui.button` takes neither `maxWidth` nor `maxLines` (`ui_tree_reconciler.cpp:471-475`). A bare `width` pins min and max and arms `setEllipsize` (`button.cpp:707`) but `Button` never calls `setMaxLines`, so `effectiveMaxLines` is 0 (`label.cpp:503`) and Pango gets a 500-line budget under `PANGO_WRAP_WORD_CHAR`. A width-capped button **wraps**. - `ui.label` does elide — `maxWidth` + `maxLines >= 1` reaches `pango_layout_set_ellipsize(END)` and the reported width is that budget (`label.cpp:487-497`, `:630`) — but a label takes no `onClick`. - `kFlex` has `minWidth` and **no** `maxWidth`, so a tinted row hugging its text cannot be capped. The only bounded-and-tinted shape is `flexGrow`, measured at its whole assigned share, so it would span the row instead of hugging. ## What changed - The tag is gone from the scope row. That row is now two buttons, a spacer and the total at `DUR_W` — nothing a tag can reach. - The caption over the chart carries the tag **in full** with `maxWidth = RIGHT_W` and `maxLines = 1`: a real renderer ellipsis, any script, any length. It was unbounded before and could overhang the panel edge. - The clear button sits **beside the search field** rather than next to the two scope buttons, where a lone glyph read as a third scope. Fixed square at `SEARCH_H`, matching the field; tooltip names the whole tag, untruncated. - The field carries `flexGrow = 1` instead of `CHROME_W - 20`, so the layout subtracts the button's 28 px rather than a constant that would have to track it. - `Fmt.clip`, `Fmt.cols`, the Wide-range table and `CHIP_TAG_MAX` are deleted — 86 lines out of `fmt.luau`. Net −51 lines. `clipTag` stays: it is a 120-**byte** cap for a hard API limit (a label over 512 bytes raises and takes the handler down), not a layout guess. ## Verification Suite: `13 states, 1954 field comparisons` / `ok`. Lint: `0 errors, 0 warnings`. The old textual assertions are replaced by a geometric one: state 5b renders five tags — `a`, `Coding Test Practice`, `MMMMMMMMMMMM`, a CJK tag, and 400 `W`s — and asserts the scope row's shape is **identical** across all of them, so a tag reaching that row in any spelling fails there. 25 mutations, no survivors: putting the tag back on the button, unsquaring it, drifting its height from the field, giving the field a width, dropping either `flexGrow`, showing the button unconditionally or never, emptying the tooltip, making clear not clear, dropping the caption's `maxWidth`/`maxLines`, wrapping instead of eliding, dropping `DUR_W`, and stopping the spacer growing. On screen, with a validated method (the panel is translucent over a live wallpaper, so raw pixel compare is invalid — masks were checked stable across repeat captures of an unchanged tag first): the scope row's ink is byte-identical across `MMMMMMMMMMMM`, `Coding Test Practice`, a CJK tag, a Hangul tag, 24 `@`s and 400 `W`s, and the **total's own region is identical in every case**. The only pixels that move in that row are 18 antialiasing pixels inside `All time`, which appear when the caption loads a CJK fallback font and bumps the renderer's text-metrics generation. Also verified live with real pointer input: the tooltip reads `Clear tag filter: Coding Test Practice`, clicking the `✕` clears the filter and the field grows back into its space, and the caption elides a 400-character tag with a real `…` at the column edge instead of overhanging. 
The filter chip carried the user's tag in a `ui.button` inside a fixed
CHROME_W row whose last child is the scope total. Free space there is
grow-only, arrangement never clamps and no node clips its children, so a
chip wider than its share pushed the total off the panel's right edge.

The tag was clipped to 12 cells to prevent that, and a cell count is not
a width: a proportional font has no cell, so MMMMMMMMMMMM spends twelve
cells and far more than twelve advances. Nothing at this plugin API
measures text, so the budget was calibrated against one font at one
chrome, and a calibration is not a bound.

The chip could not be bounded where it stood either. `ui.button` takes
no maxWidth/maxLines and a bare width wraps rather than elides, because
Button never calls setMaxLines; `ui.label` does elide but takes no
onClick; and kFlex has minWidth and no maxWidth, so a tinted container
that hugs its text cannot be capped at all.

So the tag left the row. It is shown in the caption over the chart,
which is a label and now carries maxWidth/maxLines for a real renderer
ellipsis, and in the search field that setFilter already mirrors it
into. The clear button moved beside that field -- next to two scope
buttons a lone glyph read as a third scope -- as a fixed square naming
the whole tag in its tooltip. The field carries flexGrow instead of a
width, so the layout subtracts the button's 28 px rather than a constant
that would have to track it.

Fmt.clip, Fmt.cols and the Wide-range table are deleted with
CHIP_TAG_MAX: 86 lines, and the guess with them. What replaces them in
the suite is geometric -- the scope row's shape is asserted identical
across five tags including a 400-character one -- and verified on screen
across six, where the total's rendered region is byte-identical.
yuris commented 2026-09-05 09:18:47 +00:00
Author
Owner
Copy link

Review: the test half (test.luau)

Scope: test.luau only. The C++ citations are ShellRev22's.

Method: ran the suite (13 states, 1954 field comparisons / ok, reproduced), then a
69-mutation battery against timew/panel.luau in a /tmp copy of this branch.
34 survived. Five of those are shell-inert (they set a prop the reconciler does not
carry for that node kind, so the shell logs a skip and nothing moves) and one is
by design, leaving 28 survivors that change real, shell-visible behaviour with the
suite green. Four of them are the reported bug or its direct analogue.

Three remedies below are not suggestions — I applied each, confirmed it stays green on
this branch, and counted what it kills.

blocking — headerShape() does not recurse, and the reported bug fits in the gap

headerShape (:2518) walks ipairs(scopeKids()) and reads eight props off each direct
child. It never descends. Replace the spacer with a grow-row wrapping an unbounded tag
label and nothing in the file notices:

sed: ui.spacer({ flexGrow = 1 }),
  -> ui.row({ flexGrow = 1 }, { ui.label({ text = m.filter, fontSize = 11 }) }),

Suite: 13 states, 1954 field comparisons / ok. The tree under the 400-W fixture:

scope[1] kind=button text#=5   kids=0
scope[2] kind=button text#=8   kids=0
scope[3] kind=row    text#=3   kids=1  nestedText#=400
scope[4] kind=label  text#=4   kids=0

That is a 400-character user string, unbounded, inside the fixed-CHROME_W row, ahead of
the total at DUR_W — the exact geometry the PR body says is now unreachable. Every
structural pin passes: #scopeKids() == 4 ✔, scopeKids()[3].props.flexGrow == 1 ✔,
scopeKids()[4].props.width == 56 ✔, and the shape string is identical across all five
tags because the kind/prop change is the same for all five.

ui.row and ui.label need no exotic props for this. Same blindness, bounded variant
(maxWidth = 40 on the nested label) also survives.

Remedy, verified. Swap the eight-prop concat for a recursive one that sorts every
non-function prop of every descendant. Green on this branch; kills 15 of my survivors,
including this one:

local function shapeOf(node, out, skip)
	if type(node) ~= "table" then return out end
	if type(node.props) == "table" then
		local fields = {}
		for name, value in pairs(node.props) do
			if type(value) ~= "function" and not (skip and skip[name]) then
				table.insert(fields, name .. "=" .. tostring(value))
			end
		end
		table.sort(fields)
		table.insert(out, tostring(node.kind) .. "{" .. table.concat(fields, ",") .. "}")
	end
	for _, child in ipairs(node.children or {}) do shapeOf(child, out, skip) end
	return out
end

It is also strictly cheaper than the current version: no prop list to keep in step with
the API. The eight-prop list is already short of maxWidth, minWidth, fontWeight,
color, tooltip, textAlign, paddingH, controlSize and key, and four of those
are honoured on the nodes this row actually builds — s06 (maxWidth = DUR_W + #m.filter
on the total; maxWidth is a kLabel prop and the PR's own citation says it is what the
label reports as its width, so at minimum the width == 56 pin stops describing the
rendered width), s08 (fontWeight switched on tag length), s25 (color switched on
tag length) and s19 (tooltip = m.filter on a scope button) all survive today and all
die under the recursive version.

blocking — the row that now carries the tag has no shape guard at all

setFilter mirrors the tag into state.search (panel.luau:639), and buildSearch
puts that into the field's value. So after this PR the user's unbounded string lives in
the search row — also a fixed CHROME_W row, also with a content-sized button as its
last child. The suite gained headerShape() for the scope row and nothing for this one.
Unconditional, no length trick:

sed: gap = 6, align = "center", width = CHROME_W }, kids)
  -> gap = 6 + #m.filter, align = "center", width = CHROME_W }, kids)

Survives. Under the 400-W fixture that is two gaps of 406 px inside a 287 px row.
r19 (glyphSize = 12 + #m.filter), r21/r22 (controlSize switched on tag length)
and r23 (magnifier size = 14 + #m.filter) survive the same way.

The five per-tag asserts inside the loop (:2560-2577) are all relational —
shape == first, width == height, tooltip == tr(...), flexGrow == 1 — so any
tag-derived value that preserves the relation is invisible by construction.

Remedy, verified. A searchShape() companion over rightColumn().children[4],
skipping the two props whose job is to carry the tag (both already asserted whole
elsewhere), compared across the loop exactly like headerShape:

local CARRIES_TAG = { value = true, tooltip = true, key = true }
local function searchShape()
	return table.concat(shapeOf(rightColumn().children[4], {}, CARRIES_TAG), " / ")
end

Green on this branch; kills r19, r21, r22, r23, r25 and all three of the
length-conditional survivors below.

blocking — check() runs for one tag in five, so every absolute assertion only ever sees 20 characters

check() sits inside if tag == LONG_TAG (:2545). compare() therefore runs against a
20-character tag and nothing else. Every absolute claim about the new arrangement —
chip.props.text == nil, chip.props.glyph == "x",
chip.props.height == search.props.height, search.props.width == nil, #header == 4,
#searchRow — is only ever tested in that one regime. The 400-W fixture exists
precisely to cover the other one, and never reaches compare(). Three survivors:

sed why it lives
text = if #m.filter > 100 then m.filter else nil on the clear button 400 chars of text on the last child of a fixed row, width pinned at 26 — the PR's own "width-capped button wraps", visibly broken. chip.props.text == nil never sees it.
width = if #m.filter > 100 then 100 else nil on the field search.props.width == nil never sees it; the loop only checks flexGrow.
width/height = if #m.filter > 100 then 200 else SEARCH_H on the clear button stays square, so the loop passes; a 200 px row height never reaches compare().

Remedy, verified. A two-line reorder — hoist check() above the if and name the
state after the tag. Suite goes to 17 states, 2358 field comparisons / ok, and all
three die immediately:

check("day scope, filtered by tag " .. tag)
if tag == LONG_TAG then

blocking — the caption's elide budget is asserted as a type, not a value

:2157 is same(type(right.children[1].props.maxWidth), "number", ...). Bounding that
caption is the PR's answer to where the tag lives, and the number that bounds it is
unpinned. Four survivors: maxWidth = RIGHT_W * 4, maxWidth = 100000, maxWidth = 1,
and the one that matters:

sed: maxWidth = RIGHT_W,  ->  maxWidth = RIGHT_W + #filter * 8,

Tag-derived, and by the PR's own reading of label.cpp:487-497/:630 the reported width
is that budget — so this restores the unbounded overhang the caption change exists to
prevent, silently. Note the deleted 5b block pinned CHROME_W 287, gap 4, DUR_W 56
and fontSize 11 as absolute numbers off the tree; the replacement kept only DUR_W.

Remedy, verified. One line, relational so it needs no export, green on this branch,
kills all four:

same(right.children[1].props.maxWidth, right.children[6].props.width,
	"the caption elides at the column's own width")

(right.children[6] is the scroll, whose width is already pinned to RIGHT_W at :1861.)

advisory — shape == first is not a tautology, but it is close, and it can never be more

Inverting the assert on an unmutated branch prints the answer:

Coding Test Practice renders
  button|Today|nil|nil|22|12|nil|nil / button|All time|nil|nil|22|12|nil|nil /
  spacer|nil|nil|nil|nil|nil|nil|1 / label|0:12|nil|56|nil|13|nil|nil
where a renders  ...identical...

Byte-identical, and carrying no tag material at all — it is constant == constant. It
still earns its keep: across 69 mutations it is the sole killer of exactly two
(height = CHIP_H + #m.filter on a scope button, width = #m.filter * 8 on the spacer);
both survive when I stub the assert to true. So: a narrow guard, not a vacuous one.

What it can never catch is the tag-independent geometry error, because a cross-tag
comparison is structurally blind to anything constant. Even with the recursive fix
applied, these still survive: textAlign = "start" on the total (right edge no longer
flush inside its 56 px), gap = 4 -> 12 on the scope row (the deleted block pinned this
one), flexGrow = 1 added to the total (it eats the slack and the spacer stops mattering),
justify = "end" on the row, maxWidth = 40 on the total (which makes the width == 56
pin a statement about a prop rather than about a rendered width). Those want absolute
pins of the kind the old block had. The shape loop is a good addition; it is not a
replacement for them, and the PR's -51 lines is partly paid for out of that.

advisory — the state census is honest; one figure in a comment is not

Confirmed by running both revisions: main prints 13 states, 1870 field comparisons,
this branch 13 states, 1954. The old 5b had exactly one check() (its WIDE_TAG
re-open had none), so 13 → 13 dropped nothing. Paint counts are honest too — doubling
setFilter("") inside the clear handler dies at :2858 with clearing the filter painted 2 times, not 1, and onClick = "clearFilter" dies at :2176.

But test.luau:4474 still reads "That is what "1870 field comparisons" does not
cover". That comment's whole job is to make the printed figure mean something, and this
PR moved the figure to 1954 without moving the comment.

advisory — positional indices are pinned; the diagnostic on one path is not

7 for 7. Appending after the clear button, appending after the total, prepending to the
search row, moving the button to index 1, removing the field, swapping the scope buttons,
swapping spacer and total — all die, at :2171, :2169, :1815, :1815, :1815, :2195, :2198.
#header == 4 at :2169 and the #searchRow == 3 gate at :2168 both bite. Good.

One nit: when the button is missing entirely, chip is nil and :2174 dies with attempt to index nil with 'props'. A same(#searchRow, 3, ...) at the top of the else branch
would name the cause instead.

advisory — search.props.width == nil is a real assertion

Not vacuous. Removing the field from the row dies at :1815 on the placeholder assert,
so search cannot silently resolve to another node; and flexGrow == 1 on that same node
(:2192) fails if the builder emits neither prop. It holds as written.

advisory — the tag's residency in the field is untested

patch.search = patch.filter (panel.luau:639, pre-existing, not in this diff) is what
puts the tag in the field, and the PR names the field as one of the two honest homes for
it. Both patch.search = state.search (no mirror) and
patch.search = string.sub(patch.filter, 1, 12) (mirror re-clipped to the budget this PR
deletes) survive the suite. The second one is the deleted bug wearing a different hat.

Also surviving: adding apply({ searchKey = state.searchKey + 1 }) to the clear handler,
which remounts the field on every clear — the identity section samples fixed states only.

advisory — for the integrator, the five survivors not worth chasing

tooltip/paddingH on a label, minWidth on a spacer or a button, and children on a
spacer are all outside that kind's knownProps (or, for the spacer, outside
childContainer), so the shell skips them loudly and geometry does not move. They
survived my battery but they are not holes.

advisory — the deletion side is clean

The 39 lines of Fmt.clip/Fmt.cols tests go with the functions; Fmt.cols has no
remaining caller in the suite, panel.CHIP_TAG_MAX no remaining reader, and
panel.filter_chip is gone from the EN table alongside the key. No orphans.

Verdict: REQUEST_CHANGES

The PR's own summary claims "25 mutations, no survivors". Sixty-nine aimed at the same
code produce twenty-eight, and the first of them puts a 400-character user string back
inside the fixed-width row that holds the total — the reported bug — with the suite
green. The load-bearing claim is "a tag reaching that row in any spelling fails there",
and headerShape only checks one spelling: eight named props on a direct child. The
structural change looks right; what defends it does not yet.

Four fixes, all verified green on this branch: recursive shapeOf, a searchShape()
companion, hoisting check() out of the if (17 states / 2358 comparisons), and pinning
the caption's maxWidth to the scroll's width. Together they kill 19 of the 28. The
other nine are tag-independent geometry (textAlign, gap, justify, a constant
maxWidth, flexGrow on the total), the lost tint, the two setFilter-mirror ones and
the searchKey remount — those want a couple of absolute pins put back, and the mirror
asserted at all.

## Review: the test half (`test.luau`) Scope: `test.luau` only. The C++ citations are `ShellRev22`'s. Method: ran the suite (`13 states, 1954 field comparisons` / `ok`, reproduced), then a **69-mutation battery** against `timew/panel.luau` in a `/tmp` copy of this branch. **34 survived.** Five of those are shell-inert (they set a prop the reconciler does not carry for that node kind, so the shell logs a skip and nothing moves) and one is by design, leaving **28 survivors that change real, shell-visible behaviour with the suite green**. Four of them are the reported bug or its direct analogue. Three remedies below are not suggestions — I applied each, confirmed it stays green on this branch, and counted what it kills. --- ### blocking — `headerShape()` does not recurse, and the reported bug fits in the gap `headerShape` (:2518) walks `ipairs(scopeKids())` and reads eight props off each **direct child**. It never descends. Replace the spacer with a grow-row wrapping an unbounded tag label and nothing in the file notices: ``` sed: ui.spacer({ flexGrow = 1 }), -> ui.row({ flexGrow = 1 }, { ui.label({ text = m.filter, fontSize = 11 }) }), ``` Suite: `13 states, 1954 field comparisons` / `ok`. The tree under the 400-`W` fixture: ``` scope[1] kind=button text#=5 kids=0 scope[2] kind=button text#=8 kids=0 scope[3] kind=row text#=3 kids=1 nestedText#=400 scope[4] kind=label text#=4 kids=0 ``` That is a 400-character user string, unbounded, inside the fixed-`CHROME_W` row, ahead of the total at `DUR_W` — the exact geometry the PR body says is now unreachable. Every structural pin passes: `#scopeKids() == 4` ✔, `scopeKids()[3].props.flexGrow == 1` ✔, `scopeKids()[4].props.width == 56` ✔, and the shape string is identical across all five tags because the `kind`/prop change is the same for all five. `ui.row` and `ui.label` need no exotic props for this. Same blindness, bounded variant (`maxWidth = 40` on the nested label) also survives. **Remedy, verified.** Swap the eight-prop concat for a recursive one that sorts every non-function prop of every descendant. Green on this branch; kills 15 of my survivors, including this one: ```luau local function shapeOf(node, out, skip) if type(node) ~= "table" then return out end if type(node.props) == "table" then local fields = {} for name, value in pairs(node.props) do if type(value) ~= "function" and not (skip and skip[name]) then table.insert(fields, name .. "=" .. tostring(value)) end end table.sort(fields) table.insert(out, tostring(node.kind) .. "{" .. table.concat(fields, ",") .. "}") end for _, child in ipairs(node.children or {}) do shapeOf(child, out, skip) end return out end ``` It is also strictly cheaper than the current version: no prop list to keep in step with the API. The eight-prop list is already short of `maxWidth`, `minWidth`, `fontWeight`, `color`, `tooltip`, `textAlign`, `paddingH`, `controlSize` and `key`, and four of those are honoured on the nodes this row actually builds — `s06` (`maxWidth = DUR_W + #m.filter` on the total; `maxWidth` is a `kLabel` prop and the PR's own citation says it is what the label reports as its width, so at minimum the `width == 56` pin stops describing the rendered width), `s08` (`fontWeight` switched on tag length), `s25` (`color` switched on tag length) and `s19` (`tooltip = m.filter` on a scope button) all survive today and all die under the recursive version. ### blocking — the row that now carries the tag has no shape guard at all `setFilter` mirrors the tag into `state.search` (`panel.luau:639`), and `buildSearch` puts that into the field's `value`. So after this PR the user's unbounded string lives in the **search** row — also a fixed `CHROME_W` row, also with a content-sized button as its last child. The suite gained `headerShape()` for the scope row and nothing for this one. Unconditional, no length trick: ``` sed: gap = 6, align = "center", width = CHROME_W }, kids) -> gap = 6 + #m.filter, align = "center", width = CHROME_W }, kids) ``` Survives. Under the 400-`W` fixture that is two gaps of 406 px inside a 287 px row. `r19` (`glyphSize = 12 + #m.filter`), `r21`/`r22` (`controlSize` switched on tag length) and `r23` (magnifier `size = 14 + #m.filter`) survive the same way. The five per-tag asserts inside the loop (:2560-2577) are all **relational** — `shape == first`, `width == height`, `tooltip == tr(...)`, `flexGrow == 1` — so any tag-derived value that preserves the relation is invisible by construction. **Remedy, verified.** A `searchShape()` companion over `rightColumn().children[4]`, skipping the two props whose job is to carry the tag (both already asserted whole elsewhere), compared across the loop exactly like `headerShape`: ```luau local CARRIES_TAG = { value = true, tooltip = true, key = true } local function searchShape() return table.concat(shapeOf(rightColumn().children[4], {}, CARRIES_TAG), " / ") end ``` Green on this branch; kills `r19`, `r21`, `r22`, `r23`, `r25` and all three of the length-conditional survivors below. ### blocking — `check()` runs for one tag in five, so every absolute assertion only ever sees 20 characters `check()` sits inside `if tag == LONG_TAG` (:2545). `compare()` therefore runs against a 20-character tag and nothing else. Every **absolute** claim about the new arrangement — `chip.props.text == nil`, `chip.props.glyph == "x"`, `chip.props.height == search.props.height`, `search.props.width == nil`, `#header == 4`, `#searchRow` — is only ever tested in that one regime. The 400-`W` fixture exists precisely to cover the other one, and never reaches `compare()`. Three survivors: | sed | why it lives | |---|---| | `text = if #m.filter > 100 then m.filter else nil` on the clear button | 400 chars of text on the last child of a fixed row, `width` pinned at 26 — the PR's own "width-capped button **wraps**", visibly broken. `chip.props.text == nil` never sees it. | | `width = if #m.filter > 100 then 100 else nil` on the field | `search.props.width == nil` never sees it; the loop only checks `flexGrow`. | | `width`/`height = if #m.filter > 100 then 200 else SEARCH_H` on the clear button | stays square, so the loop passes; a 200 px row height never reaches `compare()`. | **Remedy, verified.** A two-line reorder — hoist `check()` above the `if` and name the state after the tag. Suite goes to `17 states, 2358 field comparisons` / `ok`, and all three die immediately: ```luau check("day scope, filtered by tag " .. tag) if tag == LONG_TAG then ``` ### blocking — the caption's elide budget is asserted as a *type*, not a value :2157 is `same(type(right.children[1].props.maxWidth), "number", ...)`. Bounding that caption is the PR's answer to where the tag lives, and the number that bounds it is unpinned. Four survivors: `maxWidth = RIGHT_W * 4`, `maxWidth = 100000`, `maxWidth = 1`, and the one that matters: ``` sed: maxWidth = RIGHT_W, -> maxWidth = RIGHT_W + #filter * 8, ``` Tag-derived, and by the PR's own reading of `label.cpp:487-497/:630` the reported width *is* that budget — so this restores the unbounded overhang the caption change exists to prevent, silently. Note the deleted 5b block pinned `CHROME_W 287`, `gap 4`, `DUR_W 56` and `fontSize 11` as absolute numbers off the tree; the replacement kept only `DUR_W`. **Remedy, verified.** One line, relational so it needs no export, green on this branch, kills all four: ```luau same(right.children[1].props.maxWidth, right.children[6].props.width, "the caption elides at the column's own width") ``` (`right.children[6]` is the scroll, whose width is already pinned to `RIGHT_W` at :1861.) --- ### advisory — `shape == first` is not a tautology, but it is close, and it can never be more Inverting the assert on an unmutated branch prints the answer: ``` Coding Test Practice renders button|Today|nil|nil|22|12|nil|nil / button|All time|nil|nil|22|12|nil|nil / spacer|nil|nil|nil|nil|nil|nil|1 / label|0:12|nil|56|nil|13|nil|nil where a renders ...identical... ``` Byte-identical, and carrying no tag material at all — it is `constant == constant`. It still earns its keep: across 69 mutations it is the **sole** killer of exactly two (`height = CHIP_H + #m.filter` on a scope button, `width = #m.filter * 8` on the spacer); both survive when I stub the assert to `true`. So: a narrow guard, not a vacuous one. What it can never catch is the tag-*independent* geometry error, because a cross-tag comparison is structurally blind to anything constant. Even with the recursive fix applied, these still survive: `textAlign = "start"` on the total (right edge no longer flush inside its 56 px), `gap = 4 -> 12` on the scope row (the deleted block pinned this one), `flexGrow = 1` added to the total (it eats the slack and the spacer stops mattering), `justify = "end"` on the row, `maxWidth = 40` on the total (which makes the `width == 56` pin a statement about a prop rather than about a rendered width). Those want absolute pins of the kind the old block had. The shape loop is a good addition; it is not a replacement for them, and the PR's `-51 lines` is partly paid for out of that. ### advisory — the state census is honest; one figure in a comment is not Confirmed by running both revisions: `main` prints `13 states, 1870 field comparisons`, this branch `13 states, 1954`. The old 5b had exactly one `check()` (its `WIDE_TAG` re-open had none), so 13 → 13 dropped nothing. Paint counts are honest too — doubling `setFilter("")` inside the clear handler dies at :2858 with `clearing the filter painted 2 times, not 1`, and `onClick = "clearFilter"` dies at :2176. But `test.luau:4474` still reads *"That is what \"1870 field comparisons\" does not cover"*. That comment's whole job is to make the printed figure mean something, and this PR moved the figure to 1954 without moving the comment. ### advisory — positional indices are pinned; the diagnostic on one path is not 7 for 7. Appending after the clear button, appending after the total, prepending to the search row, moving the button to index 1, removing the field, swapping the scope buttons, swapping spacer and total — all die, at :2171, :2169, :1815, :1815, :1815, :2195, :2198. `#header == 4` at :2169 and the `#searchRow == 3` gate at :2168 both bite. Good. One nit: when the button is missing entirely, `chip` is nil and :2174 dies with `attempt to index nil with 'props'`. A `same(#searchRow, 3, ...)` at the top of the `else` branch would name the cause instead. ### advisory — `search.props.width == nil` is a real assertion Not vacuous. Removing the field from the row dies at :1815 on the `placeholder` assert, so `search` cannot silently resolve to another node; and `flexGrow == 1` on that same node (:2192) fails if the builder emits neither prop. It holds as written. ### advisory — the tag's residency in the field is untested `patch.search = patch.filter` (`panel.luau:639`, pre-existing, not in this diff) is what puts the tag in the field, and the PR names the field as one of the two honest homes for it. Both `patch.search = state.search` (no mirror) and `patch.search = string.sub(patch.filter, 1, 12)` (mirror re-clipped to the budget this PR deletes) survive the suite. The second one is the deleted bug wearing a different hat. Also surviving: adding `apply({ searchKey = state.searchKey + 1 })` to the clear handler, which remounts the field on every clear — the identity section samples fixed states only. ### advisory — for the integrator, the five survivors not worth chasing `tooltip`/`paddingH` on a label, `minWidth` on a spacer or a button, and children on a spacer are all outside that kind's `knownProps` (or, for the spacer, outside `childContainer`), so the shell skips them loudly and geometry does not move. They survived my battery but they are not holes. ### advisory — the deletion side is clean The 39 lines of `Fmt.clip`/`Fmt.cols` tests go with the functions; `Fmt.cols` has no remaining caller in the suite, `panel.CHIP_TAG_MAX` no remaining reader, and `panel.filter_chip` is gone from the `EN` table alongside the key. No orphans. --- ## Verdict: REQUEST_CHANGES The PR's own summary claims *"25 mutations, no survivors"*. Sixty-nine aimed at the same code produce twenty-eight, and the first of them puts a 400-character user string back inside the fixed-width row that holds the total — the reported bug — with the suite green. The load-bearing claim is *"a tag reaching that row in any spelling fails there"*, and `headerShape` only checks one spelling: eight named props on a direct child. The structural change looks right; what defends it does not yet. Four fixes, all verified green on this branch: recursive `shapeOf`, a `searchShape()` companion, hoisting `check()` out of the `if` (17 states / 2358 comparisons), and pinning the caption's `maxWidth` to the scroll's width. Together they kill **19 of the 28**. The other nine are tag-independent geometry (`textAlign`, `gap`, `justify`, a constant `maxWidth`, `flexGrow` on the total), the lost tint, the two `setFilter`-mirror ones and the `searchKey` remount — those want a couple of absolute pins put back, and the mirror asserted at all.
yuris commented 2026-09-05 09:21:02 +00:00
Author
Owner
Copy link

Shell-behaviour review: are the citations true, and was there a cheaper fix?

Scope: every flex.cpp / node.h / ui_tree_reconciler.cpp / button.cpp / label.* /
cairo_text_renderer.cpp / input.cpp citation in the PR body and in the new comments
above DOT_W, in buildSearch, buildScope and buildTrend, checked line by line
against /nix/store/7pgw3p2dhkn8sfpnli7qiyzs9zyhr01d-source, plus a live render of both
the shipped panel and of the shape the PR says cannot exist.

30 cited claims checked. 27 are exactly right. 3 are wrong. The shipped code is
correct — I measured it — and every wrong claim is a justification, not a behaviour. In
a file where the comments are the design record, that still needs fixing before merge.

Method note: the probe ran as a throwaway plugin from /tmp under its own source
(prreview/probe, since removed), so nothing here touched this checkout or hot-reloaded
the live widget. Pixel positions are from grim captures measured with a threshold scan,
not eyeballed.

blocking — "a tinted container that hugs its text cannot be capped" is false, and so is "no cheaper in-place bound existed"

panel.luau note above DOT_W:

kFlex has minWidth and no maxWidth, so a tinted container that hugs its text
cannot be capped either. The only bounded-and-tinted shape left is flexGrow […]

Both halves of the premise are true — kFlex really has minWidth and no maxWidth
(ui_tree_reconciler.cpp:449-453; Flex::setMaxWidth exists at flex.cpp:301 but only
the width prop reaches it, :1108-1110), and a flexGrow child really is measured at
its whole assigned share (flex.cpp:538 → :487-490 → :36-42 setExactWidth, with
label.cpp:567/:630). The conclusion does not follow, because the cap does not have
to be on the container. Put it on the inner label and the container inherits the bound:

ui.row({
  gap = 4, paddingH = 6, align = "center",
  fill = "primary/0.25", radius = 6,
  onClick = function() setFilter("") end,
}, {
  ui.label({ text = m.filter, fontSize = 11, color = "primary",
             maxWidth = CHIP_TAG_W, maxLines = 1 }),
  ui.label({ text = "✕", fontSize = 11, color = "primary" }),
})

Every link is in the source you already cite:

  • kFlex carries fill, radius, paddingH and onClick (ui_tree_reconciler.cpp:449-453).
    A row with onClick is wrapped in ClickWrap, whose doMeasure forwards the child's
    measurement verbatim and takes the child's size as its own (:383-390, :403-411), so
    the wrapper costs no geometry.
  • With no width and no minWidth, the flex hugs: `contentMain = padStart + childrenMain
    • totalGap + padEnd (flex.cpp:662-672`).
  • The chip's children get no main constraint on the measure pass — totalGrow is 0 so
    flex.cpp:510 calls measureItem(item, false, 0.0F) — therefore hasAssignedWidth is
    false at label.cpp:567 and finalWidth = max(measuredWidth, m_minWidth) (:630),
    where measuredWidth = min(metrics.width, measureMaxWidth) (label.cpp:565 — this
    is the line that actually does the capping, and it is the one line of the chain the PR
    does not cite).
  • The ellipsis is the real one: maxWidth > 0 with maxLines = 1 gives
    lineBudget = 1 and pango_layout_set_ellipsize(END) (cairo_text_renderer.cpp:447-451).

Measured, not argued. Three of these chips inside a fixed width = 287 row whose last
child is a width = 56, textAlign = "end" total, with a 3 px reference rule at exactly
287:

tag chip renders total's ink right edge row edge
400 × W WWWWWWWW… x x = 972 975
Coding Test Practice Coding Test Pra… x x = 972 975
日本語のタグはとても長い (12 cp) 日本語のタグは… x x = 1267 1270

Identical, 3 px inside the row edge, in every spelling — a bound, not a calibration, and
it needs no Fmt.cols, no East Asian Width table and no per-font measurement, because
Pango does the measuring. So the tag could have stayed in the scope row.

This matters twice over:

  1. The PR's headline claim — "Clipping to a cell budget cannot be made right, so the tag
    left the row" — is only half true. Clipping to a cell budget cannot be made right;
    that part is correct and well argued. But the conclusion drawn from it ("so the tag
    left the row") rests on the impossibility claim above, and that is false.
  2. This file already knows better. panel.luau:1518-1521 says, in the list rows, that
    "a clickable row wrapper, which is available (:371, :449)" — the same :449 the new
    note cites for the absence of a bound. Two comments in one file now disagree.

What makes the shipped fix defensible is the reason the PR deleted. The old buildScope
comment said a wrapper's radius "could only ever disagree with the hover rectangle drawn
inside it" — and there is a real cost in that neighbourhood: Button::setRadius(Style::scaledRadiusMd())
(button.cpp:187) is radiusMd × 1.0 × g_cornerRadiusScale (style.cpp:64, :68), i.e. it
tracks the user's corner-radius scale and not m_scale, while a flex radius is
v * m_scale and nothing else (ui_tree_reconciler.cpp:942). A radius = 6 chip beside
the two scope buttons therefore matches them only when m_scale == cornerRadiusScale,
which is to say at defaults. That is a genuine, nameable, cosmetic reason to prefer moving
the tag out. Please put that argument back and drop the geometric one, or state plainly
that the row-chip is available and the tag left the row by choice.

blocking — "It was unbounded before and could overhang the panel edge" is false; it wrapped

PR body and panel.luau:1179-1182:

a column lays out down, so an over-wide child overhangs the panel's edge instead of
moving a sibling

ui.column defaults to align = "stretch" (ui_tree_reconciler.cpp:646 — the very line
this file cites at panel.luau:1591), so on the arrange pass the column hands each child
an exact width (flex.cpp:493-494 → :44-50), and setExactWidth sets hasMaxWidth
(node.cpp:59-62 → :49-52). Label::measureWithConstraints then takes
measureMaxWidth = constraints.maxWidth at label.cpp:496-498 even with no maxWidth
prop. With maxLines unset that is the maxWidthPxScaled > 0, maxLines == 0 case at
cairo_text_renderer.cpp:437/:447 → lineBudget = kHardMaxLines (500) under
PANGO_WRAP_WORD_CHAR. The old caption wrapped.

Measured: a fontSize = 13, fontWeight = "bold" label with a 60-W tag and no maxWidth
/maxLines, in a width = 287 column, rendered 4 wrapped lines, ink right edge
x = 966 against a column edge at x = 975. Inside it, not over it. With the PR's
maxWidth + maxLines = 1 the same string is one line ending in a real ….

So the change is an improvement, but it fixes a wrap that grows the column downward and
pushes the chart and the whole history block with it — not an overhang. The sentence
"Nothing sits to its right for it to push" reads as if nothing was being pushed; in fact
everything below was. Reword; the mechanism you describe is not the one in the renderer.

advisory — panel.luau:1198 states the opposite of :1521, and it is the wrong one

at this plugin API only box, image and button accept onClick — row, column and label do not

kFlex includes onClick (ui_tree_reconciler.cpp:453) and both row and column route
through wrapClickable when it is present (:647-648, :656-657). Pre-existing, not
introduced here — but it is the claim the new note above DOT_W inherits, and it is why
that note reached a false conclusion. Worth correcting in the same pass.

advisory — a label can report a width wider than its maxWidth; the qualifier is missing

label.cpp:626-628: on the isIconGlyph branch finalWidth = max({inkWidth, measuredWidth, m_minWidth}), and inkWidth (:577) is not capped by measureMaxWidth. Likewise the
m_autoScroll branches (:613-625, :642-654) force measureMaxWidth = 0 at :501
before re-capping the box. Neither is reachable for this caption — isIconGlyph needs
StringUtils::isSinglePrivateUseGlyph over the whole string (:576) and the caption
always carries the panel.trend_tag prefix; autoScroll has no kLabel prop
(:457-460) — so the shipped claim holds. But "the width it measures is that budget" is
stated flatly, and it is true only on the :630 non-icon, non-scroll, non-assigned branch.
One clause fixes it.

advisory — the new flexGrow field collapses to 0, not to kMinWidth, at the endpoint

flex.cpp:532 clamps remaining at zero, so if the search row's fixed children ever sum
past CHROME_W the grower's share is 0, and Input::doMeasure takes
assignW = max(constraints.maxWidth, minFromHint) = max(0, 0) = 0
(input.cpp:1859-1860) — kMinWidth = 48 (:45, :1858) is only the unconstrained
fallback and never binds here, because width is gone so m_minLayoutWidth is 0
(ui_tree_reconciler.cpp:1611-1612). Not reachable today: glyph 14 + button 26 + two 6 px
gaps = 52 against 287, and all four scale by the same m_scale as CHROME_W, so the ratio
is scale-invariant. But "the field yields the space instead of the row overflowing"
understates the endpoint — the field yields all of it and vanishes. A minWidth on the
field would be the honest guard; a sentence naming the endpoint is the cheap one.

advisory — flex.cpp:737 is the weaker citation for "arrangement never clamps"

:737 is cursor += item.main;. The claim is about code that isn't there, which a line
number can't show. flex.cpp:690-699 is the citation that carries it: arrangedChildrenMain
is summed from the children's own measured mains and effectiveGap / cursor are derived
from it through std::max guards that only ever widen.

Verified correct (and how)

Every one of these is exactly what the source says:

  • flex.cpp:532 — const float remaining = std::max(0.0F, containerMain - fixedMain); ✔
  • node.h:211 — bool m_clipChildren = false; ✔ (and no kFlex/kBox prop reaches it)
  • ui_tree_reconciler.cpp:471-475 — kButton has neither maxWidth nor maxLines ✔;
    :457-460 — kLabel has both ✔
  • :1404-1407 — a bare width on a button calls setMinWidth and setMaxWidth ✔
  • button.cpp:707 — applyLabelMaxWidth arms setEllipsize(End) ✔ (:698-711), and
    Button never calls setMaxLines anywhere ✔ (only external callers do, e.g.
    home_tab.cpp:612)
  • label.h:143 — int m_userMaxLines = 0; ✔ (exact line)
  • label.cpp:503 — effectiveMaxLines = m_autoScroll ? 1 : m_userMaxLines ✔
  • cairo_text_renderer.cpp:436-451 — kHardMaxLines = 500, lineBudget = maxLines > 0 ? min(maxLines, 500) : 500, PANGO_WRAP_WORD_CHAR ✔
  • "A width-capped button WRAPS" ✔ — and this one I put on screen. A
    width = 120, height = 22, fontSize = 11 ghost button with "Filter: " .. 30×"W"
    renders Filter: on a line above the 22 px box and further W lines above and below
    it, painted outside the button's bounds with nothing clipping them. Exactly "visibly
    broken rather than merely unspecified". Good call, and good instinct to check it.
  • cairo_text_renderer.cpp:448-451 — maxWidth + maxLines >= 1 reaches
    pango_layout_set_ellipsize(PANGO_ELLIPSIZE_END) ✔; and live, a 400-char tag elides at
    x = 1226 against a RIGHT_W = 305 column ending at x = 1228 ✔
  • flex.cpp:487-501 + :523-538 — growers measured with an exact main constraint at their
    share ✔; label.cpp:630 hasAssignedWidth forces it ✔
  • button.cpp:628 before :637 — selected wins ✔. Stronger than stated: even if the
    Ghost palette had no selected entry, :636's isHovered || isSelected fires on
    isSelected alone, so there is no hover delta either way. (isPressed at :624 still
    outranks it, so the button does keep press feedback.)
  • radius — Style::scaledRadiusMd() is radiusMd × localScale × g_cornerRadiusScale
    (style.cpp:64, :68), Button calls it with the default localScale = 1.0
    (button.cpp:187), and the reconciler's scaled() is v * m_scale with no corner
    scale (ui_tree_reconciler.cpp:942) ✔
  • glyph = "x" resolves ✔ — assets/fonts/tabler.json has x → U+EB55; close is only
    an alias to x (glyph_registry.cpp:27), so "x" is the native name and hits
    :261-264, not the kMissingGlyph skull at :266-267. Confirmed on screen: a real ✕.
  • glyphSize is honoured for a button ✔ — ui_tree_reconciler.cpp:1322-1323 →
    Button::setGlyphSize (button.cpp:292-294)
  • kInput accepts flexGrow ✔ (:479-483), and an Input under an exact main constraint
    takes assignW = max(constraints.maxWidth, minFromHint) (input.cpp:1859-1860) — width
    is constraint-driven and never text-driven, so no placeholder or value can overflow it.
    Confirmed live with a 400-character value: field width unchanged, text clipped to the
    viewport.
  • dropping the field's explicit width cannot collapse it ✔ — the row's own width still
    sets the flex's minWidth (:1109), so the row reports CHROME_W from
    constrainFlexWidth (flex.cpp:679 → :410) even on a pass where main is unknown, and
    arrange re-enters with an exact width (flex.cpp:751-757) before the share is computed.
  • the square ✔ — measured on the live panel, the clear button's tint occupies
    x 1187..1212 (w = 26) and y 215..240 (h = 26), and the search field's frame border
    sits at y = 215 and y = 240 too. Same 26 px, pixel-aligned top and bottom, in both
    the short-tag and 400-char captures.
  • the total does not move ✔ — live scope-total ink right edge x = 1210 with
    Coding Test Practice and x = 1210 with 400 Ws, clear button bbox byte-identical
    across the two.
  • clean cutover ✔ — Fmt.clip, Fmt.cols, CHIP_TAG_MAX and panel.filter_chip have no
    remaining references anywhere in timew/, panel.clear_filter takes {tag} and
    en.json is the only locale. noctalia plugins lint timew → 0 errors, 0 warnings.

Verdict

REQUEST_CHANGES, on the comments only. The geometry ships correct and I have the pixels
to say so: the scope row's shape is tag-independent, the caption elides at the column edge,
the clear button is a true square matched to the field, and the field grows and yields
without collapsing. Nothing here asks you to change a line of Lua that runs.

What needs to change is that the PR argues impossibility where the answer is availability.
A bounded, tinted, hugging, clickable chip exists at this plugin API — cap the inner
ui.label, not the flex — and I rendered one holding 400 Ws inside a fixed row without
moving the total by a pixel. The reason to prefer what you shipped is the radius mismatch
you deleted (button.cpp:187 vs ui_tree_reconciler.cpp:942), not a geometric wall. And
the caption's old failure was a wrap that pushed the chart down, not an overhang past the
edge. Fix those two claims and the :1198 line they lean on, and this is a clean merge.

## Shell-behaviour review: are the citations true, and was there a cheaper fix? Scope: every `flex.cpp` / `node.h` / `ui_tree_reconciler.cpp` / `button.cpp` / `label.*` / `cairo_text_renderer.cpp` / `input.cpp` citation in the PR body and in the new comments above `DOT_W`, in `buildSearch`, `buildScope` and `buildTrend`, checked line by line against `/nix/store/7pgw3p2dhkn8sfpnli7qiyzs9zyhr01d-source`, plus a live render of both the shipped panel and of the shape the PR says cannot exist. **30 cited claims checked. 27 are exactly right. 3 are wrong.** The shipped code is correct — I measured it — and every wrong claim is a *justification*, not a behaviour. In a file where the comments are the design record, that still needs fixing before merge. Method note: the probe ran as a throwaway plugin from `/tmp` under its own source (`prreview/probe`, since removed), so nothing here touched this checkout or hot-reloaded the live widget. Pixel positions are from `grim` captures measured with a threshold scan, not eyeballed. --- ### blocking — "a tinted container that hugs its text cannot be capped" is false, and so is "no cheaper in-place bound existed" `panel.luau` note above `DOT_W`: > `kFlex` has `minWidth` and no `maxWidth`, so a tinted container that hugs its text > cannot be capped either. The only bounded-and-tinted shape left is `flexGrow` […] Both halves of the premise are true — `kFlex` really has `minWidth` and no `maxWidth` (`ui_tree_reconciler.cpp:449-453`; `Flex::setMaxWidth` exists at `flex.cpp:301` but only the `width` prop reaches it, `:1108-1110`), and a `flexGrow` child really is measured at its whole assigned share (`flex.cpp:538` → `:487-490` → `:36-42` `setExactWidth`, with `label.cpp:567`/`:630`). The conclusion does not follow, because **the cap does not have to be on the container.** Put it on the inner label and the container inherits the bound: ```lua ui.row({ gap = 4, paddingH = 6, align = "center", fill = "primary/0.25", radius = 6, onClick = function() setFilter("") end, }, { ui.label({ text = m.filter, fontSize = 11, color = "primary", maxWidth = CHIP_TAG_W, maxLines = 1 }), ui.label({ text = "✕", fontSize = 11, color = "primary" }), }) ``` Every link is in the source you already cite: - `kFlex` carries `fill`, `radius`, `paddingH` **and `onClick`** (`ui_tree_reconciler.cpp:449-453`). A row with `onClick` is wrapped in `ClickWrap`, whose `doMeasure` forwards the child's measurement verbatim and takes the child's size as its own (`:383-390`, `:403-411`), so the wrapper costs no geometry. - With no `width` and no `minWidth`, the flex hugs: `contentMain = padStart + childrenMain + totalGap + padEnd` (`flex.cpp:662-672`). - The chip's children get **no main constraint** on the measure pass — `totalGrow` is 0 so `flex.cpp:510` calls `measureItem(item, false, 0.0F)` — therefore `hasAssignedWidth` is false at `label.cpp:567` and `finalWidth = max(measuredWidth, m_minWidth)` (`:630`), where `measuredWidth = min(metrics.width, measureMaxWidth)` (**`label.cpp:565`** — this is the line that actually does the capping, and it is the one line of the chain the PR does not cite). - The ellipsis is the real one: `maxWidth > 0` with `maxLines = 1` gives `lineBudget = 1` and `pango_layout_set_ellipsize(END)` (`cairo_text_renderer.cpp:447-451`). Measured, not argued. Three of these chips inside a fixed `width = 287` row whose last child is a `width = 56, textAlign = "end"` total, with a 3 px reference rule at exactly `287`: | tag | chip renders | total's ink right edge | row edge | |---|---|---|---| | 400 × `W` | `WWWWWWWW… x` | **x = 972** | 975 | | `Coding Test Practice` | `Coding Test Pra… x` | **x = 972** | 975 | | `日本語のタグはとても長い` (12 cp) | `日本語のタグは… x` | **x = 1267** | 1270 | Identical, 3 px inside the row edge, in every spelling — a *bound*, not a calibration, and it needs no `Fmt.cols`, no East Asian Width table and no per-font measurement, because Pango does the measuring. So the tag could have stayed in the scope row. This matters twice over: 1. The PR's headline claim — "Clipping to a cell budget cannot be made right, so the tag left the row" — is only half true. Clipping to a *cell* budget cannot be made right; that part is correct and well argued. But the conclusion drawn from it ("so the tag left the row") rests on the impossibility claim above, and that is false. 2. This file already knows better. `panel.luau:1518-1521` says, in the list rows, that "a clickable row wrapper, which is available (:371, :449)" — the same `:449` the new note cites for the *absence* of a bound. Two comments in one file now disagree. What makes the shipped fix defensible is the reason the PR **deleted**. The old `buildScope` comment said a wrapper's radius "could only ever disagree with the hover rectangle drawn inside it" — and there is a real cost in that neighbourhood: `Button::setRadius(Style::scaledRadiusMd())` (`button.cpp:187`) is `radiusMd × 1.0 × g_cornerRadiusScale` (`style.cpp:64`, `:68`), i.e. it tracks the user's corner-radius scale and *not* `m_scale`, while a flex `radius` is `v * m_scale` and nothing else (`ui_tree_reconciler.cpp:942`). A `radius = 6` chip beside the two scope buttons therefore matches them only when `m_scale == cornerRadiusScale`, which is to say at defaults. That is a genuine, nameable, cosmetic reason to prefer moving the tag out. Please put that argument back and drop the geometric one, or state plainly that the row-chip is available and the tag left the row by choice. ### blocking — "It was unbounded before and could overhang the panel edge" is false; it wrapped PR body and `panel.luau:1179-1182`: > a column lays out down, so an over-wide child overhangs the panel's edge instead of > moving a sibling `ui.column` defaults to `align = "stretch"` (`ui_tree_reconciler.cpp:646` — the very line this file cites at `panel.luau:1591`), so on the arrange pass the column hands each child an **exact** width (`flex.cpp:493-494` → `:44-50`), and `setExactWidth` sets `hasMaxWidth` (`node.cpp:59-62` → `:49-52`). `Label::measureWithConstraints` then takes `measureMaxWidth = constraints.maxWidth` at `label.cpp:496-498` *even with no `maxWidth` prop*. With `maxLines` unset that is the `maxWidthPxScaled > 0`, `maxLines == 0` case at `cairo_text_renderer.cpp:437`/`:447` → `lineBudget = kHardMaxLines` (500) under `PANGO_WRAP_WORD_CHAR`. The old caption **wrapped**. Measured: a `fontSize = 13, fontWeight = "bold"` label with a 60-`W` tag and no `maxWidth` /`maxLines`, in a `width = 287` column, rendered **4 wrapped lines**, ink right edge **x = 966** against a column edge at **x = 975**. Inside it, not over it. With the PR's `maxWidth` + `maxLines = 1` the same string is one line ending in a real `…`. So the change is an improvement, but it fixes a wrap that grows the column downward and pushes the chart and the whole history block with it — not an overhang. The sentence "Nothing sits to its right for it to push" reads as if nothing was being pushed; in fact everything *below* was. Reword; the mechanism you describe is not the one in the renderer. ### advisory — `panel.luau:1198` states the opposite of `:1521`, and it is the wrong one > at this plugin API only box, image and button accept onClick — row, column and label do not `kFlex` includes `onClick` (`ui_tree_reconciler.cpp:453`) and both `row` and `column` route through `wrapClickable` when it is present (`:647-648`, `:656-657`). Pre-existing, not introduced here — but it is the claim the new note above `DOT_W` inherits, and it is why that note reached a false conclusion. Worth correcting in the same pass. ### advisory — a label *can* report a width wider than its `maxWidth`; the qualifier is missing `label.cpp:626-628`: on the `isIconGlyph` branch `finalWidth = max({inkWidth, measuredWidth, m_minWidth})`, and `inkWidth` (`:577`) is **not** capped by `measureMaxWidth`. Likewise the `m_autoScroll` branches (`:613-625`, `:642-654`) force `measureMaxWidth = 0` at `:501` before re-capping the box. Neither is reachable for this caption — `isIconGlyph` needs `StringUtils::isSinglePrivateUseGlyph` over the *whole* string (`:576`) and the caption always carries the `panel.trend_tag` prefix; `autoScroll` has no `kLabel` prop (`:457-460`) — so the shipped claim holds. But "the width it measures is that budget" is stated flatly, and it is true only on the `:630` non-icon, non-scroll, non-assigned branch. One clause fixes it. ### advisory — the new `flexGrow` field collapses to 0, not to `kMinWidth`, at the endpoint `flex.cpp:532` clamps `remaining` at zero, so if the search row's fixed children ever sum past `CHROME_W` the grower's share is 0, and `Input::doMeasure` takes `assignW = max(constraints.maxWidth, minFromHint)` = `max(0, 0)` = **0** (`input.cpp:1859-1860`) — `kMinWidth = 48` (`:45`, `:1858`) is only the *unconstrained* fallback and never binds here, because `width` is gone so `m_minLayoutWidth` is 0 (`ui_tree_reconciler.cpp:1611-1612`). Not reachable today: glyph 14 + button 26 + two 6 px gaps = 52 against 287, and all four scale by the same `m_scale` as `CHROME_W`, so the ratio is scale-invariant. But "the field yields the space instead of the row overflowing" understates the endpoint — the field yields *all* of it and vanishes. A `minWidth` on the field would be the honest guard; a sentence naming the endpoint is the cheap one. ### advisory — `flex.cpp:737` is the weaker citation for "arrangement never clamps" `:737` is `cursor += item.main;`. The claim is about code that *isn't* there, which a line number can't show. `flex.cpp:690-699` is the citation that carries it: `arrangedChildrenMain` is summed from the children's own measured mains and `effectiveGap` / `cursor` are derived from it through `std::max` guards that only ever widen. --- ## Verified correct (and how) Every one of these is exactly what the source says: - `flex.cpp:532` — `const float remaining = std::max(0.0F, containerMain - fixedMain);` ✔ - `node.h:211` — `bool m_clipChildren = false;` ✔ (and no `kFlex`/`kBox` prop reaches it) - `ui_tree_reconciler.cpp:471-475` — `kButton` has neither `maxWidth` nor `maxLines` ✔; `:457-460` — `kLabel` has both ✔ - `:1404-1407` — a bare `width` on a button calls `setMinWidth` **and** `setMaxWidth` ✔ - `button.cpp:707` — `applyLabelMaxWidth` arms `setEllipsize(End)` ✔ (`:698-711`), and `Button` never calls `setMaxLines` anywhere ✔ (only external callers do, e.g. `home_tab.cpp:612`) - `label.h:143` — `int m_userMaxLines = 0;` ✔ (exact line) - `label.cpp:503` — `effectiveMaxLines = m_autoScroll ? 1 : m_userMaxLines` ✔ - `cairo_text_renderer.cpp:436-451` — `kHardMaxLines = 500`, `lineBudget = maxLines > 0 ? min(maxLines, 500) : 500`, `PANGO_WRAP_WORD_CHAR` ✔ - **"A width-capped button WRAPS"** ✔ — and this one I put on screen. A `width = 120, height = 22, fontSize = 11` ghost button with `"Filter: " .. 30×"W"` renders `Filter:` on a line *above* the 22 px box and further `W` lines above and below it, painted outside the button's bounds with nothing clipping them. Exactly "visibly broken rather than merely unspecified". Good call, and good instinct to check it. - `cairo_text_renderer.cpp:448-451` — `maxWidth` + `maxLines >= 1` reaches `pango_layout_set_ellipsize(PANGO_ELLIPSIZE_END)` ✔; and live, a 400-char tag elides at **x = 1226** against a `RIGHT_W = 305` column ending at **x = 1228** ✔ - `flex.cpp:487-501` + `:523-538` — growers measured with an exact main constraint at their share ✔; `label.cpp:630` `hasAssignedWidth` forces it ✔ - `button.cpp:628` before `:637` — `selected` wins ✔. Stronger than stated: even if the Ghost palette had no `selected` entry, `:636`'s `isHovered || isSelected` fires on `isSelected` alone, so there is no hover delta either way. (`isPressed` at `:624` still outranks it, so the button does keep press feedback.) - radius — `Style::scaledRadiusMd()` is `radiusMd × localScale × g_cornerRadiusScale` (`style.cpp:64`, `:68`), `Button` calls it with the default `localScale = 1.0` (`button.cpp:187`), and the reconciler's `scaled()` is `v * m_scale` with no corner scale (`ui_tree_reconciler.cpp:942`) ✔ - `glyph = "x"` resolves ✔ — `assets/fonts/tabler.json` has `x` → `U+EB55`; `close` is only an *alias to* `x` (`glyph_registry.cpp:27`), so `"x"` is the native name and hits `:261-264`, not the `kMissingGlyph` skull at `:266-267`. Confirmed on screen: a real ✕. - `glyphSize` is honoured for a button ✔ — `ui_tree_reconciler.cpp:1322-1323` → `Button::setGlyphSize` (`button.cpp:292-294`) - `kInput` accepts `flexGrow` ✔ (`:479-483`), and an Input under an exact main constraint takes `assignW = max(constraints.maxWidth, minFromHint)` (`input.cpp:1859-1860`) — width is constraint-driven and never text-driven, so no placeholder or value can overflow it. Confirmed live with a 400-character value: field width unchanged, text clipped to the viewport. - dropping the field's explicit width cannot collapse it ✔ — the row's own `width` still sets the flex's `minWidth` (`:1109`), so the row reports `CHROME_W` from `constrainFlexWidth` (`flex.cpp:679` → `:410`) even on a pass where main is unknown, and arrange re-enters with an exact width (`flex.cpp:751-757`) before the share is computed. - **the square** ✔ — measured on the live panel, the clear button's tint occupies `x 1187..1212` (w = 26) and `y 215..240` (h = 26), and the search field's frame border sits at `y = 215` and `y = 240` too. Same 26 px, pixel-aligned top and bottom, in both the short-tag and 400-char captures. - **the total does not move** ✔ — live scope-total ink right edge `x = 1210` with `Coding Test Practice` and `x = 1210` with 400 `W`s, clear button bbox byte-identical across the two. - clean cutover ✔ — `Fmt.clip`, `Fmt.cols`, `CHIP_TAG_MAX` and `panel.filter_chip` have no remaining references anywhere in `timew/`, `panel.clear_filter` takes `{tag}` and `en.json` is the only locale. `noctalia plugins lint timew` → `0 errors, 0 warnings`. ## Verdict **REQUEST_CHANGES**, on the comments only. The geometry ships correct and I have the pixels to say so: the scope row's shape is tag-independent, the caption elides at the column edge, the clear button is a true square matched to the field, and the field grows and yields without collapsing. Nothing here asks you to change a line of Lua that runs. What needs to change is that the PR argues impossibility where the answer is availability. A bounded, tinted, hugging, clickable chip exists at this plugin API — cap the inner `ui.label`, not the flex — and I rendered one holding 400 `W`s inside a fixed row without moving the total by a pixel. The reason to prefer what you shipped is the radius mismatch you deleted (`button.cpp:187` vs `ui_tree_reconciler.cpp:942`), not a geometric wall. And the caption's old failure was a wrap that pushed the chart down, not an overhang past the edge. Fix those two claims and the `:1198` line they lean on, and this is a clean merge.
yuris force-pushed fix/chip-unbounded-tag from 887481640b to 3a8924a1dc 2026-09-05 09:25:09 +00:00 Compare
yuris commented 2026-09-05 09:26:15 +00:00
Author
Owner
Copy link

Both reviews actioned. Amended to 3a8924a. Suite went 13 states, 1954 →
17 states, 2358 field comparisons, lint 0 errors, 0 warnings.

ShellRev22's blocker is correct and it invalidates my stated reason

I claimed a bounded chip carrying the tag was impossible here. It is not. I
checked kFlex for maxWidth, found none, and concluded the container could not
be capped — never considering that the cap belongs on the inner label, whose
capped measurement the hugging row then adopts. Verified independently before
rewriting:

  • label.cpp:565 — measuredWidth = measureMaxWidth > 0 ? min(metrics.width, measureMaxWidth) : metrics.width. The line that does the capping, and I never cited it.
  • flex.cpp:510 — measureItem(item, false, 0.0F): a non-growing child gets no main-axis constraint on the measure pass, so hasAssignedWidth is false and the row hugs the capped label instead of spanning.

So ui.row({ fill, radius, onClick }, { ui.label({ maxWidth, maxLines = 1 }) })
is a tinted, hugging, clickable, bounded chip. The reviewer's probe-plugin
measurement (total ink at an identical x for 400 Ws, the reported tag, and a
CJK tag) settles it.

The design does not change — the clear control belongs beside the field holding
the tag, and a flex-built chip's radius skips g_cornerRadiusScale
(ui_tree_reconciler.cpp:1081-1085 via :942 against button.cpp:187 /
style.cpp:64), so it matches sibling buttons only at the default setting. But
the justification did change, in panel.luau, the README and above: from a
false impossibility to a design choice with a named cost.

Second blocker, also correct: the caption wrapped, it did not overhang.
ui.column sets FlexAlign::Stretch (:646), so the child got an exact width,
took it as its wrap budget (label.cpp:496-498), and with maxLines unset landed
in the 500-line PANGO_WRAP_WORD_CHAR branch — four lines for a 60-character tag,
shoving the chart down. maxLines = 1 is the fix; my reason for it was wrong.

Also fixed: the pre-existing panel.luau claim that "only box, image and button
accept onClick" (kFlex has it at :453; only ui.label genuinely lacks one) —
the same mistake that led the new note astray. And the citations: label.cpp:565
added, flex.cpp:737 → :690-699 for "arrangement never clamps".

SuiteRev22's blocker is mine and it was a real hole

headerShape() did not recurse, so replacing the spacer with
ui.row({ flexGrow = 1 }, { ui.label({ text = m.filter }) }) put 400 unbounded
characters back in the fixed row ahead of the total while the suite printed ok.
Replaced with a recursive walk over every descendant's non-function props —
shorter than the 8-prop list it replaces and nothing to maintain. Both the
nested mutant and a raw-filter variant now die.

Actioned in full:

finding fix
no recursion recursive shapeOf, kills the nested-label class
search row unguarded, and it now holds the tag searchShape() companion, skipping only value/tooltip/key — the three props that legitimately carry the tag
check() only ran for LONG_TAG hoisted out of the branch: every tag reaches compare() (hence 17 states)
caption budget asserted as type(...) == "number" held against the scroll's width, so a tag-derived budget fails
stale "1870 field comparisons" citation now 2358

Re-ran the reviewer's survivors myself: s09, s09b, r19, t05, r03, r08
all die. Two still live and I am accepting both as outside this PR's claim, not
fixing them: tooltip = label on a scope button and maxWidth = 56 on the total
are tag-independent — they change the row for every tag equally, so no
cross-tag comparison can see them, which is the documented limit of this
technique rather than a gap in it.

Not actioned, with reasons

  • clear is nil-indexed at :2174 with a bad message. The absent-button case is already caught two lines up by same(chip, nil, ...), so the crash is unreachable from a real regression; adding a guard would only make a dead branch prettier.
  • The Input "yields ALL the space" advisory. Correct and unreachable: 52 px of fixed children against 287, scale-invariant. Recorded, not defended.
Both reviews actioned. Amended to `3a8924a`. Suite went `13 states, 1954` → **`17 states, 2358 field comparisons`**, lint `0 errors, 0 warnings`. ## ShellRev22's blocker is correct and it invalidates my stated reason I claimed a bounded chip carrying the tag was impossible here. **It is not.** I checked `kFlex` for `maxWidth`, found none, and concluded the container could not be capped — never considering that the cap belongs on the **inner label**, whose capped measurement the hugging row then adopts. Verified independently before rewriting: - `label.cpp:565` — `measuredWidth = measureMaxWidth > 0 ? min(metrics.width, measureMaxWidth) : metrics.width`. The line that does the capping, and I never cited it. - `flex.cpp:510` — `measureItem(item, false, 0.0F)`: a non-growing child gets **no** main-axis constraint on the measure pass, so `hasAssignedWidth` is false and the row hugs the capped label instead of spanning. So `ui.row({ fill, radius, onClick }, { ui.label({ maxWidth, maxLines = 1 }) })` is a tinted, hugging, clickable, **bounded** chip. The reviewer's probe-plugin measurement (total ink at an identical x for 400 `W`s, the reported tag, and a CJK tag) settles it. The design does not change — the clear control belongs beside the field holding the tag, and a flex-built chip's `radius` skips `g_cornerRadiusScale` (`ui_tree_reconciler.cpp:1081-1085` via `:942` against `button.cpp:187` / `style.cpp:64`), so it matches sibling buttons only at the default setting. But the **justification** did change, in `panel.luau`, the README and above: from a false impossibility to a design choice with a named cost. Second blocker, also correct: the caption **wrapped**, it did not overhang. `ui.column` sets `FlexAlign::Stretch` (`:646`), so the child got an exact width, took it as its wrap budget (`label.cpp:496-498`), and with `maxLines` unset landed in the 500-line `PANGO_WRAP_WORD_CHAR` branch — four lines for a 60-character tag, shoving the chart down. `maxLines = 1` is the fix; my reason for it was wrong. Also fixed: the pre-existing `panel.luau` claim that "only box, image and button accept onClick" (`kFlex` has it at `:453`; only `ui.label` genuinely lacks one) — the same mistake that led the new note astray. And the citations: `label.cpp:565` added, `flex.cpp:737` → `:690-699` for "arrangement never clamps". ## SuiteRev22's blocker is mine and it was a real hole `headerShape()` did not recurse, so replacing the spacer with `ui.row({ flexGrow = 1 }, { ui.label({ text = m.filter }) })` put 400 unbounded characters back in the fixed row ahead of the total while the suite printed `ok`. Replaced with a recursive walk over every descendant's non-function props — **shorter than the 8-prop list it replaces** and nothing to maintain. Both the nested mutant and a raw-filter variant now die. Actioned in full: | finding | fix | |---|---| | no recursion | recursive `shapeOf`, kills the nested-label class | | search row unguarded, and it now holds the tag | `searchShape()` companion, skipping only `value`/`tooltip`/`key` — the three props that legitimately carry the tag | | `check()` only ran for `LONG_TAG` | hoisted out of the branch: every tag reaches `compare()` (hence 17 states) | | caption budget asserted as `type(...) == "number"` | held against the scroll's width, so a tag-derived budget fails | | stale "1870 field comparisons" citation | now 2358 | Re-ran the reviewer's survivors myself: `s09`, `s09b`, `r19`, `t05`, `r03`, `r08` all die. Two still live and I am accepting both as outside this PR's claim, not fixing them: `tooltip = label` on a scope button and `maxWidth = 56` on the total are **tag-independent** — they change the row for every tag equally, so no cross-tag comparison can see them, which is the documented limit of this technique rather than a gap in it. ## Not actioned, with reasons - **`clear` is `nil`-indexed at `:2174` with a bad message.** The absent-button case is already caught two lines up by `same(chip, nil, ...)`, so the crash is unreachable from a real regression; adding a guard would only make a dead branch prettier. - **The `Input` "yields ALL the space" advisory.** Correct and unreachable: 52 px of fixed children against 287, scale-invariant. Recorded, not defended.
yuris merged commit 0fc12cc91a into main 2026-09-05 09:29:49 +00:00
yuris deleted branch fix/chip-unbounded-tag 2026-09-05 09:29:49 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
yuris/noctalia-timew!22
No description provided.