Two rounds past the ninth sweep: `b295c8b` reads a share under a parent that divides nothing as a minimum, which is what Bryan said it means and what a span already did, sharing one comparison with it; `2dba90b` grows images in the generated trees, which is the only widget here whose hint is a length in pixels and so the only one that reaches the case. The dump baseline is 34,571 boxes now that the trees carry images, and a seed no longer names the tree it used to -- the log says what that costs and where those defects live instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
909 lines
52 KiB
Markdown
909 lines
52 KiB
Markdown
# Layout findings log
|
|
|
|
What the sessions reviewing Iris's retained layout found, kept so that
|
|
nothing here is rediscovered. Each entry says who found it and when.
|
|
**Delete this file when #19 lands and its fixes are in**; what must outlive
|
|
it (settled design, the measurement method) belongs in `docs/LAYOUT.md`, and
|
|
the current plan is in `docs/HANDOFF.md`.
|
|
|
|
## Images in the generated trees (2026-09-20)
|
|
|
|
`Image` is the only widget here whose size hint is a length in pixels, so it is
|
|
the only one that exercises a rule beside a hint -- which is what the two
|
|
findings above are about, and why neither was reachable from a generated tree.
|
|
Bryan asked for one in the trees, as a committed 64x64 checkerboard reusing one
|
|
texture handle. `2dba90b`: `Kind::Image` is a fifth leaf drawn one time in five,
|
|
stepping to a plain rect under the shrinker; the picture is
|
|
`src/assets/checkerboard.png`, purple and black in 8 px cells, included rather
|
|
than opened; and a `TextureHandle` is a counted reference, so the first image in
|
|
a tree uploads and the rest clone it. Measured: seed 1 at depth 4 grows 13
|
|
images and holds 1 texture, and `a_tree_of_images_uploads_one_texture` asserts
|
|
it.
|
|
|
|
The cost is that a seed no longer names the tree it used to, so the corpus in
|
|
`generated.rs` is a corpus and not a set of regression cases -- 20 and 86 no
|
|
longer grow the trees whose defects they caught. Both live on as shrunk fixtures
|
|
in `unsettled.rs`, which are trees rather than numbers, and both files now say
|
|
so. New dump baseline: **34,571** boxes over the 400 depth-5 trees. All three
|
|
scans pass over the new trees (400 at depth 5 in 62.79s, 1,000 at depth 6 in
|
|
160.20s, 2,000 at depth 4 in 299.58s).
|
|
|
|
## Ninth sweep: the widget vocabulary, and the eighth sweep's fix (2026-09-20)
|
|
|
|
Over the part no earlier round named -- `src/widget/trait_fns.rs` and
|
|
`wrapper.rs`, `core/src/widget/widgets.rs`, the `util` additions, `examples/`
|
|
and the two manifests -- and once more over `77ed7a2`, which was the eighth
|
|
sweep's own commit. Eight findings, all in `c2b8bf8`. The cold dump over 400
|
|
depth-5 trees is byte-identical to `77ed7a2` across all **34,488** boxes, and
|
|
all three seed scans pass (400 at depth 5 in 63.27s, 1,000 at depth 6 in
|
|
160.45s, 2,000 at depth 4 in 302.52s).
|
|
|
|
**A hint overrode a rule.** `Widgets::declared_lens` asked
|
|
`rules[axis].declared()` and fell through to the widget's own `size_hint`
|
|
whenever that answered `None` -- which it does for a share, because a share is
|
|
not a declaration. So a widget carrying `width(leftover(1))` *and* hinting a
|
|
pixel length of its own was given a box of the hint, against the rule and
|
|
against the comment inside the function ("a hint still narrows the box where no
|
|
rule does"). `Painter::size_hint` spells the same rule-else-hint step three
|
|
hundred lines up and gets it right, with the reason written on it; both read
|
|
`Widgets::exact_len` now, and `declared_lens` is the part of its answer that
|
|
needs nobody to divide it.
|
|
|
|
`Image` is the only widget in the repository whose hint is a declared length
|
|
(every other hints `LEFTOVER`, whose `declared()` is `None` either way), and
|
|
neither the tests nor the generator builds one, so nothing here could reach the
|
|
difference -- which is why the dump is unchanged, and why
|
|
`a_share_rule_beats_the_widgets_own_pixel_size` builds a widget of its own. It
|
|
records the box it was asked in: 400 with the rule and 50 without, and 50
|
|
either way at `77ed7a2`. The alternative reading -- a hint narrows even under a
|
|
share rule -- would mean changing that comment instead, and is Bryan's to
|
|
prefer if he does.
|
|
|
|
**What a leftover means where nothing divides it** (Bryan, 2026-09-20, on the
|
|
finding above). A leftover under a parent that does not divide is still a
|
|
leftover, acting as a minimum: where the `px` and `rel` parts come to less than
|
|
the box it fills the rest, and where they come to more they overflow as normal.
|
|
The length is `max(box, px + rel*box)` -- which is the same `max` he gave for
|
|
`Scroll`'s content length on 2026-09-18, generalised to every non-dividing
|
|
parent.
|
|
|
|
Measured at `c2b8bf8`, a probe recording the box it is asked in, in a 400 px
|
|
window, under `.wrapper()` against a one-child span:
|
|
|
|
rule nothing divides a span divides
|
|
leftover(1) 400 400
|
|
px(50) + leftover(1) 400 400
|
|
px(500) + leftover(1) 400 500
|
|
rel(0.5) + leftover(1) 400 400
|
|
px(500), no leftover 500 500
|
|
|
|
One row disagrees. A leftover whose fixed part is longer than the box loses the
|
|
overflow where nothing divides it. The span is right and says so in place --
|
|
"One that also asked for pixels or a fraction keeps those and overflows" -- and
|
|
`only_a_pure_leftover_child_disappears_when_nothing_is_left` asserts it at
|
|
100..120 of a 100 px row. The cause is that `LayoutLen::declared` refuses to
|
|
answer for anything carrying leftover weight, so the non-dividing path never
|
|
learns the fixed part and falls back to the offer. The same length without the
|
|
share does overflow (drawn -50..450, its alignment centring it), so what
|
|
swallows it is the share and not the overflow.
|
|
|
|
**Fixed in `b295c8b`**, after Bryan asked for it if it was cheap and pointed out
|
|
that the machinery is a minimum size of `rel(1.0)` and should share a path with
|
|
one. What made it cheap was saying it as the place the parent gives rather than
|
|
as a declaration: where the fixed part is the longer, `widget_at` hands the
|
|
child `fixed.as_desc().fills()`, which is what a declared length already comes
|
|
to, and `active.placed` keeps it -- so nothing about `Declared`, `ActiveData` or
|
|
the resize path had to change. A place that is already the child's placement is
|
|
skipped, because a parent that divides has given the share whatever it was owed;
|
|
without that guard a span's slot was re-placed and its child moved from 0..500
|
|
to -50..450.
|
|
|
|
The comparison is one operation now, `Painter::longer_than`, and both callers
|
|
share it: the span's room for the shares it divides, and a share past the box it
|
|
was given. It narrows the widget's window range where the span replaced it,
|
|
since a comparison the framework makes on an arbitrary parent's behalf is one
|
|
more reason its drawing holds rather than the only one. `SizeRule::Min` of
|
|
`rel(1.0)` is the same operation and belongs on the same path when it lands.
|
|
|
|
Two things were tried and dropped. An escalation for a changed rule
|
|
(`shares_past_a_length`) turned out to buy nothing: the reported size is the
|
|
rule resolved, so a changed rule changes the answer and the parent refuses its
|
|
own drawing -- the tests pass without it. And `Holds::crossing`, a named
|
|
constructor for the range, had one caller and read better spelled there.
|
|
|
|
**The root still reads the old way.** `root_layout` asks
|
|
`Widgets::declared_lens` with no painter, so a mixed share on the root is the
|
|
window rather than the longer of the two. It would mean the comparison in a
|
|
second place, and the root's box is the window and is relaid out on every
|
|
resize; nothing here puts a share on a root.
|
|
|
|
**Marking a widget for redraw had no name.** Twenty-one sites under `tests/`
|
|
said it as `widgets_mut().get_dyn_mut(id);` with the widget thrown away: five
|
|
with a `let _ =` in front, one with a comment explaining what the line was for
|
|
("taking mutable access is the ordinary content-change signal"), and one inside
|
|
a local function already called `mark`. The framework's own word for it is in
|
|
its comments -- "marked for redraw" -- and `Widgets::mark_for_redraw` is now
|
|
the method. `revision_cost.rs` keeps the long spelling and says why in place,
|
|
since it is deliberately in the API subset an old worktree also has.
|
|
|
|
**`assert_same_regions` could not see the defect the eighth sweep had just
|
|
fixed.** It zips the warm and cold id lists, so a list naming one widget twice
|
|
compares fewer boxes than it lists and reports nothing. It now rejects a
|
|
repeated id and two lists of different lengths, which is the same check applied
|
|
to the whole class rather than to the four fixtures that had it wrong -- and it
|
|
verifies that round's claim about the other nine: all eighteen cases pass.
|
|
|
|
**Bare pairs where the framework has named ones.** `random.rs`'s `Lens` and
|
|
`Aligns` were `[Option<LayoutLen>; 2]` and `[Option<AxisAlign>; 2]`, read as
|
|
`[0]`/`[1]` and zipped against a hand-written `[Axis::X, Axis::Y]` in two rigs.
|
|
They are `SizeRules` and `Align`, which is what the framework calls those
|
|
pairs; `Align` took the `Index<Axis>` every other per-axis pair on this branch
|
|
has, and `RegionAlign::from` does the "an axis left out is centred" step both
|
|
rigs were spelling per axis. The three sites that wrote the axis pair out say
|
|
`Axis::BOTH`, which the layout core already says ten times. The plan's
|
|
`Debug` is why `Align` gained one.
|
|
|
|
**Forty-five lines nothing references.** `BothAxis<T>`, `AxisT`, `XAxis` and
|
|
`YAxis` -- a const trait, two marker types and three accessors -- have no user
|
|
anywhere in the workspace, and are the mechanism `impl_axis_index!` replaced,
|
|
in the very file this branch took `Vec2::axis`/`axis_mut` out of. Deleted as a
|
|
drive-by in a block the branch was already rewriting, the way `MASK_NONE` was;
|
|
drop it if the scope matters more. They pre-date the PR, so a plain "is this
|
|
name used" scan over the diff does not surface them.
|
|
|
|
**One word for two things.** `Wrapper` (the widget) arrived on this branch
|
|
beside core's `WidgetWrapper` (a dynamic borrow guard), both in the prelude.
|
|
The alias had two uses in one file and `DynBorrower<dyn Widget>` is what they
|
|
are, so it is gone rather than renamed. `Wrapper::new`, `Wrapper::empty` and
|
|
its hand-written `Default` were three names for one value, two unused.
|
|
|
|
**Smaller.** `Arena::get_mut` was the only `pub(crate)` among `pub` siblings on
|
|
a public type. `Selector` rounded the pointer onto the pixel grid in order to
|
|
add two values already on it, losing the precision the platform gave it for
|
|
nothing -- the step between the two regions is taken on the grid instead, which
|
|
also makes it agree with `Selectable`, the other caller of `select`. And the
|
|
two `debug` profile settings now carry their reason where the next reader
|
|
looks: `profile.dev`'s was added in a commit about renaming `rest` and
|
|
explained nowhere, and `profile.test`'s only in the message of the commit that
|
|
made the tests one target.
|
|
|
|
### Tripped a rule and left as it stands
|
|
|
|
- `examples/random.rs` is a viewer for the fuzz generator rather than a
|
|
demonstration of a feature, which is what an example is for here. Left:
|
|
`scripts/run-headless.sh` opens an example by name, so this is how a
|
|
generated tree is put on a screen at all.
|
|
- It also carries a fifth copy of the six-line `env` helper, on top of the four
|
|
under `tests/`. Same answer as the eighth sweep gave: sharing it means a new
|
|
file for six lines of `std`.
|
|
- `IRIS_SEED`/`IRIS_DEPTH`, `IRIS_GENERATED_SEED(S)`/`IRIS_GENERATED_DEPTH` and
|
|
`IRIS_DUMP_SEEDS`/`IRIS_DUMP_DEPTH` are three names for two knobs. Left: the
|
|
prefixes are what lets one shell set a rig's seed without changing another's,
|
|
and the two rigs that grow the same tree from the same number do share the
|
|
unprefixed pair.
|
|
- `set_size_rule` takes a `SizeRule` while `set_size_rules` takes
|
|
`impl Into<SizeRule>` per axis, so seven callers write `SizeRule::Exact(len)`
|
|
where two write `Some(len)`. Left: widening the single-axis one makes
|
|
`harness.rs`'s `len.into()` ambiguous between two conversions. The doc that
|
|
called it "for a caller holding a pair" -- which it is not, since it takes
|
|
two values -- now just says both axes at once.
|
|
- `Wrapper::set` is `replace` with the result dropped, and both have a caller
|
|
in `examples/tabs`. Left: it is `Option::insert` beside `Option::replace`,
|
|
and the tab bar wants each.
|
|
- `Masked`'s size comment credits `Scroll` with the same reasoning rather than
|
|
restating it. Left: the reason it gives is its own ("it clips what is inside
|
|
to that box"), and the cross-reference is to a design parallel, not an API.
|
|
- The nine `RegionAlign`/`CardinalAlign` constants include four nothing names
|
|
(`TOP_CENTER`, `CENTER_RIGHT`, `BOT_CENTER`, `V_CENTER`). Left: they are one
|
|
vocabulary of nine positions and six cardinals, and deleting the members
|
|
nobody has needed yet is the rule written on one member of a set.
|
|
|
|
## Eighth sweep: the tests, and the seventh sweep's fix (2026-09-20)
|
|
|
|
Over the part no earlier round named -- the 6,300 lines under `tests/`, which
|
|
is more than half of what #19 adds -- and once more over `f8aa0c5`, which was
|
|
the seventh sweep's own fix and so unreviewed. Seven findings, all in
|
|
`77ed7a2`. No library code changed: the cold dump over 400 depth-5 trees is
|
|
byte-identical to `f8aa0c5` across all **34,488** boxes, and the seed scans
|
|
have nothing to find, since nothing that decides a layout moved.
|
|
|
|
That count is not the 34,492 the four rounds before this one recorded, nor
|
|
the 34,490 `77ed7a2`'s own message gives. The dump prints eight lines that
|
|
are not a box -- cargo's two, libtest's four, and two blanks -- so a `wc -l`
|
|
of the run comes to 34,496 and any partial filter lands somewhere between.
|
|
The boxes are the lines matching `^[0-9]+ [0-9]+ `, and there are 34,488 of
|
|
them at both `f8aa0c5` and `77ed7a2`. What every round actually established
|
|
is still true, since each diffed two dumps rather than trusting a count.
|
|
|
|
**A shrunk fixture that names one widget three times.** `width`, `sized` and
|
|
`align` set a rule on the widget they are handed and give its own id back;
|
|
only `pad` and `wrapper` make a new widget. So in
|
|
|
|
let wrapped = wtext("Wrapping shapes").size(16).wrap(true).add(&mut h.rsc);
|
|
let sized = wrapped.width(76).add(&mut h.rsc);
|
|
let aligned = sized;
|
|
|
|
all three names are one text, and all three went into the vector of ids the
|
|
case compares warm against cold. Measured: `plant` and `plant_fixed` list six
|
|
ids and hold four widgets, `plant_pair` lists four and holds three,
|
|
`plant_scrolled` lists eight and holds seven; the other nine fixtures are
|
|
honest. So four cases check fewer boxes than they say, and the doc comment of
|
|
each quotes the inflated count as the size of the tree the shrinker reduced
|
|
to -- which is the number a reader uses to judge whether a case is still the
|
|
minimal one. The names are gone and the counts are what the fixtures build.
|
|
`trace_unsettled.rs` carries a copy of two of these fixtures and had the same
|
|
aliases.
|
|
|
|
The check that mattered here is that a rebuilt fixture is the same tree: a
|
|
regression test whose fixture quietly changed still passes and no longer
|
|
covers its defect. Each was diffed against its old self -- same widget slots,
|
|
same regions, for both settings of `swapped`.
|
|
|
|
**A helper at the top of the file, and seven copies of its body below it.**
|
|
`assert_same_regions` collects every widget whose box moved and reports them
|
|
together. Six tests call it; seven more spell the eight lines out instead,
|
|
byte for byte. They call it now, and it took `#[track_caller]` so the panic
|
|
names the case rather than the helper.
|
|
|
|
**The GPU rigs' adapter probe, written twice.** `draw_cost` and `chain_cost`
|
|
each held a `config` (identical) and an adapter probe (identical but for the
|
|
feature it asks for and what it returns). The probe leaks its `Instance` on
|
|
purpose -- a Vulkan loader may unload the driver as a test thread exits --
|
|
and only `draw_cost` said so, with `chain_cost` referring the reader to it.
|
|
Both come from `tests/gpu/mod.rs` now, shared through `#[path]` the way
|
|
`scenario/mod.rs` already is, with the justification on the thing it is
|
|
about.
|
|
|
|
**The mask a widget is clipped by, resolved three times**, two of them a
|
|
byte-identical closure defined inside a loop. `mask_bounds` takes the
|
|
`MaskIdx` rather than the widget, because the third site reads the slot it
|
|
saved before the frame: that a redraw keeps the same slot is exactly what it
|
|
is checking, and a helper that looked the slot up again would have made that
|
|
assertion pass for the wrong reason.
|
|
|
|
**A field nothing reads.** `Layered::_revision` existed to be incremented, to
|
|
mark its widget dirty. Two tests in the same file already do that with
|
|
`widgets_mut().get_dyn_mut(id)`, which is the idiom `tests/scenario/mod.rs`
|
|
uses as well. The underscore was hiding the dead-code warning that would have
|
|
said so.
|
|
|
|
**A claim the test below it does not make.** `plan.rs` said "Every
|
|
simplification is strictly smaller, so taking them in turn reaches a fixed
|
|
point instead of circling", and then asserted `small.size() <= node.size()`.
|
|
Measured: 53 of one tree's 101 simplifications keep the widget count, because
|
|
`Plan::size` counts widgets and dropping an alignment or stepping `Wrapped ->
|
|
OneLine` does not change it. The assertion is the right one and the claim was
|
|
not. What actually rules out circling is that those steps are one-way too --
|
|
a `Some` becomes a `None`, a kind steps down a ladder with no way back up --
|
|
and the comment now says that. The test is `no_simplification_of_a_plan_is_larger_than_it`.
|
|
|
|
**Numbers and lines that had gone stale.** `generated.rs` says "Eight that
|
|
have never failed" and "the ten the others check"; both said one fewer,
|
|
having been written when `SEEDS` had nine entries and not updated when
|
|
`d8ae9c3` added seed 20. The `should_panic` scroll test ended in an
|
|
`h.frame()` that cannot run, because `Harness::set_root` lays the tree out
|
|
and is where the panic comes from -- verified by deleting it. Two
|
|
`drop(tree)` calls sat at the end of their own scope.
|
|
|
|
### Tripped a rule and left as it stands
|
|
|
|
- `env` is written four times under `tests/` (`layout_dump`,
|
|
`layout_diagnostics`, `revision_cost`, `scenario`). `revision_cost`'s is
|
|
deliberate and documented: that file is kept in the API subset an old
|
|
worktree also has, so it can be dropped in and measured there, and a
|
|
`#[path]` module would break that. Sharing the other three means either a
|
|
new file for six lines of `std` or pulling `scenario`'s 494 lines into two
|
|
more binaries.
|
|
- `determinism.rs`'s `BranchesOnMeasurement` is `iris::random::Branch` with
|
|
the same four fields and nearly the same body, and `unsettled.rs` beside it
|
|
uses the real `Branch`. The difference is load-bearing: the copy states no
|
|
contract, so the framework must re-ask it at every width, which is the
|
|
whole point of a test about whether a re-measure branches the same way.
|
|
`Branch` also has a `size_hint` that changes how a span treats it.
|
|
- Three tests call `h.frame()` immediately after `h.set_root`, which already
|
|
frames. Unlike the `should_panic` one, these are reachable and assert that
|
|
a settled second frame adds no draws. Left; the redundancy reads as noise
|
|
but removing it removes a check.
|
|
- `primitive_bounds` and `primitive_masks` share the walk from a widget's
|
|
primitives to their instances and differ only in the field they take.
|
|
Left: naming the intermediate means exporting the instance type into the
|
|
test, which is more coupling than the four shared lines are worth.
|
|
- `Harness::replay` ignores each sample's `t_ms`, and the parser rejects
|
|
time running backwards with a comment about the wait between samples --
|
|
which is `replay-touch`'s behaviour, not the harness's. Left: the parser
|
|
is shared by both replays and the rule is the real one, but a harness
|
|
gesture has no timing, so nothing in-process can measure a fling velocity.
|
|
- `layout_diagnostics.rs`'s `report` takes `_harness` and reads it, because
|
|
it is only used under `layout-diagnostics`. An underscore on a parameter
|
|
the body uses is backwards, but the alternative is a `cfg_attr`ed allow.
|
|
|
|
## Seventh sweep: the rigs, `Fixed`, and the sixth sweep's fix (2026-09-20)
|
|
|
|
Over what no earlier round named -- `src/random.rs` and `tests/scenario/`,
|
|
`core/src/fixed.rs`, `scripts/run-headless.sh` -- and once more over
|
|
`b7b8d09`, which was itself unreviewed because it was the sixth sweep's own
|
|
fix. Six findings, all in `f8aa0c5`. The cold dump over 400 depth-5 trees is
|
|
byte-identical to `b7b8d09` across all 34,492 boxes, and all three seed scans
|
|
pass (400 at depth 5 in 62.75s, 1,000 at depth 6 in 162.37s, 2,000 at depth 4
|
|
in 305.25s).
|
|
|
|
**A scroll still asking a question it has already answered.** The sixth
|
|
sweep found `anchor != Px::ZERO` in `Scroll`'s content test and removed it;
|
|
the operand beside it is the same defect and survived. `self.content_len` is
|
|
`answer_px.max(container_len)`, so content that fits has nothing to scroll
|
|
through, and `update_amt` on the line above clamps `amt` to
|
|
`content_len - container_len`, which is then zero. So in
|
|
|
|
let content = match self.amt != Px::ZERO || self.content_len != self.container_len {
|
|
|
|
the first disjunct can never decide the match: `amt != ZERO` implies
|
|
`content_len != container_len`, which the second already tests. It is now
|
|
`match self.content_len > self.container_len`, the exact complement of the
|
|
`content_len <= container_len` that guards the contract fifteen lines above.
|
|
Verified by asserting the implication in place and running the whole suite,
|
|
the scrolling tests included; it held. This is the lesson of "the fixes a
|
|
review produces are themselves unreviewed code" arriving on schedule -- one
|
|
round's fix left the same mistake in the expression it was editing.
|
|
|
|
**Things nothing reads, in the new arithmetic.** `Fixed::to_scale` converts a
|
|
value between two fixed-point grids, and `shift_round` exists only to serve
|
|
it. Both arrived on this branch; the only caller either has ever had is
|
|
`a_coarser_grid_rounds_and_a_finer_one_does_not`, the test written for them.
|
|
Every other `Fixed` method has real callers (checked one by one), and nothing
|
|
needs a grid conversion: `ratio`, `mul` and `div` already cross grids where
|
|
layout has to. All three deleted. `to_scale` was also the one operation in
|
|
the file that could overflow silently without documenting it -- going to a
|
|
finer grid is `self.0 << (TO - SHIFT)` and the test only ever went coarse,
|
|
fine, coarse with a value small enough to survive the trip.
|
|
|
|
**`Len` arithmetic written a component at a time.** `Len::align` built a
|
|
`Len` whose `px` is always `Px::ZERO`, then added to and subtracted from both
|
|
of its components by hand:
|
|
|
|
start: Len::from_parts(at.rel.sub(self.rel.mul(rel)), at.px.sub(self.px.mul(rel))),
|
|
end: Len::from_parts(at.rel.add(self.rel.mul(rest)), at.px.add(self.px.mul(rest))),
|
|
|
|
`Len::scale` is "both parts by the same fraction" and `Len` has `Add` and
|
|
`Sub`, so the whole rule is `at - self.scale(rel)` and
|
|
`at + self.scale(Rel::ONE.sub(rel))` -- the point the alignment names, less
|
|
the part of the length before it. Identical arithmetic, which the dump
|
|
confirms. It is the only place in the codebase that expanded a `Len`
|
|
operation like this; `LayoutLen::apply_leftover` touches `rel` alone, which
|
|
is genuinely one component.
|
|
|
|
**A question asked through a value, one line from its sibling.** `445287c`
|
|
moved every method that answers a question about a value to `&self`.
|
|
`LayoutLen::without_leftover` was missed, and its own doc comment calls
|
|
`apply_leftover` -- which takes `&self` -- "the opposite reading of the same
|
|
value". Now `&self` too. Every other by-value method in the workspace that
|
|
does not return `Self` was checked: the rest are conversions on numbers
|
|
(`Fixed::raw`, `to_f32`) or builders.
|
|
|
|
**A rig that scales a gesture against a mode the output no longer has.**
|
|
`run-headless.sh --mode` sets `out_w`/`out_h` beside the `swaymsg output ...
|
|
mode`, because those two numbers are the extent `replay-touch` passes to
|
|
`zwlr_virtual_pointer_v1::motion_absolute` -- the recording's coordinates are
|
|
a fraction of them. `--resize`, added in `b7b8d09`'s round, changed the mode
|
|
and left the extent alone, so `--resize 800x600@60Hz --replay flick.touch`
|
|
replayed every sample at `x * 800 / 1920` and finished looking like a run
|
|
that worked. Both are one `set_mode` function now, so a third mode change
|
|
cannot get it wrong. Found by reading rather than by running: this VM has no
|
|
recorded gesture that also resizes, which is exactly why it went unnoticed.
|
|
|
|
**A comment the plan/build split stranded.** `src/random.rs`'s "a row takes
|
|
the height it is given rather than its tallest child" sat above
|
|
`let gap = self.rng.below(3) as i32 * 4`, telling a reader that the line
|
|
consumes no randomness. It describes the `set_size_rules` that `98d4e98`
|
|
moved into `Build::kind`, and the line it was left above is one of the two
|
|
draws in the function. Moved to the rule it is about, and the
|
|
seed-stability half dropped: building a plan consumes no randomness at all
|
|
now, so there is nothing left for that clause to say.
|
|
|
|
### Tripped a rule and left as it stands
|
|
|
|
- `Align::tuple`, `UiVec2::partial_align` and `Vec2::partial_align` have no
|
|
callers anywhere. All three pre-date this PR and are outside its diff, so
|
|
they belong to the pre-review-gate sweep rather than to this branch.
|
|
- `Vec2::align`/`partial_align` are `UiVec2`'s two functions again with
|
|
`UiVec2::from(*self)` in front. Also pre-existing, and merging them means
|
|
deciding whether `Vec2` should have them at all.
|
|
- `impl_op!` has four grammars for one macro, and `core/src/util/vec2.rs`
|
|
spells two of them one line apart -- `impl_op!(impl Add for Vec2: add x y)`
|
|
beside `impl_op!(Vec2 Sub sub; x y)`. The `impl ... for ...:` arm has that
|
|
one caller. Pre-dates this PR; the two arms this PR added (`same ...`) are
|
|
a real distinction, since a type mixing a fraction and an offset has no
|
|
meaning for a bare `f32`.
|
|
- `AxisAlign`, `RegionAlign` and `Align` lost `Eq` when `AxisAlign` became a
|
|
`Rel` wrapper, which `Rel` derives. Nothing needs it, so it was left rather
|
|
than adding a derive with no reader.
|
|
- `Holds::through` special-cases a fully unbounded range before inverting a
|
|
fraction, which the general path would also answer correctly through
|
|
`narrow`. Left: it is the one place a `Px::MIN`-to-`Px::MAX` interval is
|
|
shifted and divided, and the early return says that no fraction can narrow
|
|
"every length".
|
|
- `Scroll`'s `content_len <= container_len` can only be equality, given the
|
|
`max` that built it. Left: `<=` reads as "the content fits", which is what
|
|
the branch means, and the mirror `>` now reads as "it overflows".
|
|
- `scenario::Case::name`, `window` and `Shuffle::of` take `self` on `Copy`
|
|
enums. They are test-rig code the `&self` pass did not cover, and an enum
|
|
with no fields is the one place taking a value costs a caller nothing.
|
|
|
|
## Sixth sweep: the shader boundary and the position widgets (2026-09-20)
|
|
|
|
Over what the five earlier rounds did not name -- the WGSL prelude and how
|
|
it is assembled, the position widgets, `orientation/`, and the sensor walk.
|
|
**Scoped against `upstream/main` at `ca2b4b2`, which is PR #19's real base**;
|
|
the first half of this sweep used the local `main` and had to be redone, for
|
|
which see "The branch layout" in `docs/HANDOFF.md`. Two findings.
|
|
The cold dump is byte-identical to `1096c31` and all three seed scans pass
|
|
(400 at depth 5 in 69.07s, 1,000 at depth 6 in 169.29s, 2,000 at depth 4 in
|
|
300.75s).
|
|
|
|
**A number both sides count in, written twice.** `module_source` already
|
|
builds each shader's preamble from `iris_core`'s own constants, with a
|
|
comment saying why: "a grid the two disagree about puts every coordinate
|
|
somewhere else". The move-chain work then added, to the very file it
|
|
prepends, a second copy of two of its own numbers -- `const MOVE_NONE` and
|
|
`const CHAIN_LIMIT`, under "Keep in step with `iris_core::CHAIN_LIMIT`" --
|
|
asking a reader by hand for what the mechanism beside it exists to do, and
|
|
duplicating the reasoning `CHAIN_LIMIT`'s Rust declaration already carries.
|
|
Both are injected now and the shader declares neither.
|
|
`every_shader_validates` composes the real preamble so it covers the change;
|
|
what it could never have caught is the two numbers drifting apart, which is
|
|
now unrepresentable.
|
|
|
|
`MASK_NONE` went in beside them, replacing a bare `4294967295u` in `masked`
|
|
-- where the CPU deliberately keeps `MaskIdx` and `MoveIdx` as separate
|
|
types so the two cannot be swapped. **That literal pre-dates this PR**; it
|
|
is a one-line drive-by in a block the PR was already rewriting, taken
|
|
because leaving it means a named sentinel for one index and a magic number
|
|
for its sibling four lines apart. Drop it if the scope matters more.
|
|
|
|
**A scroll positioning content it does not position.** Two halves, one
|
|
mistake, both new in this PR -- the base's `Scroll` has none of this
|
|
machinery. The author believed `Scroll` places its own content.
|
|
|
|
`content_len` is `answer_px.max(container_len)`, so it is never less than
|
|
the box. Two lines on, `slack` was `(container_len - content_len).max(ZERO)`
|
|
and `anchor` was `slack * align.rel()` -- provably always zero, whatever the
|
|
alignment, with `moved` then testing `anchor != ZERO` for nothing.
|
|
Instrumented at `1096c31`, a centred scroll over 50 px of content in a
|
|
200 px box prints `slack=0 align=AxisAlign(0.5) anchor=0` and centres the
|
|
content anyway: the framework does it, by placing the inner's answer in the
|
|
whole box, which comes out as `rel 0.5 - px 25` and resolves at any length.
|
|
The comment credited the arithmetic for behaviour it could not produce.
|
|
|
|
The same belief cost a redraw. The contract for content that fits was
|
|
guarded by `align == AxisAlign::NEG`, on the reasoning that at any other
|
|
alignment "it moves with every length the box takes and the drawing holds
|
|
for that length alone". It does not move -- the framework's placement is a
|
|
fraction of the box -- and the default alignment is the middle, so the
|
|
common case was the guarded one. `Painter::px_len` holds a drawing to the
|
|
length it read unless the widget says otherwise, so with no `holds` stated
|
|
the scroll redrew on every box change. Measured with `distinct_widgets`: 1
|
|
at `CENTER`, 0 at `TOP_LEFT`. Dropping the alignment test gives 0 at all
|
|
three, which is what `a_fitting_scroll_holds_for_every_box_its_content_fits_in`
|
|
asserts; it fails at `1096c31` on the `CENTER` case. `align` had no other
|
|
reader, so the widget no longer asks its own alignment at all -- which is
|
|
the tell that both halves were one mistake.
|
|
|
|
**`UiSpan::translated` and `UiRegion::translated`** arrived on this branch
|
|
and are reachable only from each other, which is why a plain "is this name
|
|
used anywhere else" scan does not see them: neither looks unused on its own.
|
|
The region one carried a performance argument for a function nobody calls.
|
|
Both deleted.
|
|
|
|
### Kept although they pre-date this PR
|
|
|
|
`GlyphAtlas::glyph_count`, `TextBuffer::new_empty` and the let-chain in
|
|
`TextEdit::apply_event` that #10 had expanded into a nested `if`. All three
|
|
were found under the wrong base and are outside #19's diff; Bryan said to
|
|
keep them anyway (2026-09-20), since the two are dead either way and the
|
|
third is a straight restoration.
|
|
|
|
### Tripped a rule and left as it stands
|
|
|
|
- `diag::untrace_widget` is new here with no caller, which is the test the
|
|
`translated` pair was deleted by. Left: it is the "removed" half of what
|
|
`trace_widget`'s own doc promises ("until explicitly removed or cleared"),
|
|
and `clear_traced_widgets` is the "cleared" half and does have one. A rig
|
|
drives this API from outside.
|
|
- `UiSpan::flip` swaps `start.rel` with `end.rel` and `start.px` with
|
|
`end.px`, where `Len` has exactly those two fields and one swap of the
|
|
structs would do. Pre-dates this PR.
|
|
- `UiVec2` translates under three names -- `shift`, `offset`, and
|
|
`UiRegion::offset` -- beside `Len::offset`, which adds pixels to one end
|
|
and is a different operation. Pre-existing, and one vocabulary is Bryan's
|
|
call rather than a sweep's.
|
|
- The nine `#[allow(unused_variables)]` are all on trait methods with empty
|
|
default bodies, where the parameter names are the signature's
|
|
documentation and underscoring them would hide it from implementors.
|
|
- `GpuPages::update` grows before it drains and both go through the one
|
|
queue, so the copy is ordered before the writes. Correct as it stands.
|
|
|
|
## Fifth sweep: the retained path, the renderer and the diagnostics (2026-09-20)
|
|
|
|
Over the parts the four earlier rounds did not read -- the renderer, the
|
|
text store, the input default, the harness -- and once more over `redraw`.
|
|
Six findings, `d8d5122` through `1096c31`, the last two from Bryan's reading
|
|
of the first four. The cold dump is byte-identical to `781199a` and all three
|
|
seed scans pass (400 at depth 5 in 69.45s, 1,000 at depth 6 in 214.69s, 2,000
|
|
at depth 4 in 419.50s).
|
|
|
|
**A contract was kept where it no longer held** (`d8d5122`). The sibling of
|
|
`713e3e7`, in the same function. `redraw` keeps the narrower of the old and
|
|
the fresh contract so widening and narrowing back do not churn the parent.
|
|
The drawing's half asks `was_holds.contains(window, rel_base, region)` first;
|
|
the answer's half did not. A widget whose answer contract widened in a frame
|
|
that also resized the window therefore kept a range the new window is
|
|
outside, and the parent's next ask refused it and redrew the whole subtree --
|
|
throwing away the drawing that widget had just made. Cost, not geometry: the
|
|
size kept is the size just reported. It needs both a resize the root does not
|
|
absorb and a mark on a deeper widget in the same frame, which is what
|
|
`a_contract_this_window_is_outside_is_not_kept` builds; it draws the leaf
|
|
twice at `781199a` and once with the guard in.
|
|
|
|
**Configuring a surface under its own texture** (`02048ea`). wgpu 30 says at
|
|
both `Surface::configure` and `Surface::get_current_texture` that configuring
|
|
while a texture the surface handed out is still alive panics. The
|
|
`Suboptimal` arm of `UiRenderer::draw` configured with the texture it was
|
|
about to draw with in hand, so the first suboptimal frame -- a resize or a
|
|
display change on some drivers -- takes the app down rather than rebuilding
|
|
the swapchain. The texture is good for that frame, so it is drawn with and
|
|
presented and the rebuild happens after `present` consumes it. Not
|
|
reproducible on demand here; the claim rests on wgpu's own documented panic.
|
|
|
|
This one is the reason for the sweep recorded under "The code written before
|
|
the review gate" below: nothing about the arm was hard, and it was written
|
|
that way anyway.
|
|
|
|
**A counter named the wrong contract** (`9b4cc32`). `AxisHolds` is four
|
|
contracts and `diag::outside` counted three: a refusal because this window
|
|
is outside the range the drawing was made for bumped "reuse outside: a rel
|
|
base". A window range is pixels and a rel base pin is a window-unit length an
|
|
unchanged window can still change, so the rig answered "why did that
|
|
redraw?" with the wrong one for every resize. Same class as `8088a1f`.
|
|
|
|
**Things nothing reads** (`7502176`, `1096c31`). `Axis::pair`,
|
|
`RegionAlign::NEAR` and `Painter::text_data` arrived on this branch with no
|
|
caller and never got one. The comment beside a span's cross-axis accumulator
|
|
said a scalable child "makes Children scalable too" -- `Children` names
|
|
nothing in this repository, and what it makes scalable is the span.
|
|
|
|
`text_data` was held back a round on the grounds that it is the only way a
|
|
widget inside `draw` can reach `TextData`, and the app's integration might
|
|
want it. That reasoning is wrong: nothing in iris is kept for the app's sake,
|
|
because the app is to be largely rewritten against this API rather than
|
|
ported call by call (Bryan, 2026-09-20).
|
|
|
|
**A question asked through a value** (`445287c`). `7502176` moved
|
|
`Holds::contains` from `&self` to `self` to match its five siblings, which
|
|
was the wrong way to reconcile them. A method taking `self` can only be
|
|
called on a value, so a caller holding a reference has to dereference to ask
|
|
-- `Copy` or not (Bryan, 2026-09-20). Every method that answers a question
|
|
about a value now takes `&self`: `Holds`, `AxisHolds` and `LayoutHolds`
|
|
throughout, `LayoutLen::{is_px, is_only_leftover, declared, fills}` and
|
|
`Size::within_box`. Builders that return a changed copy still take `self`.
|
|
|
|
### Tripped a rule and left as it stands
|
|
|
|
- `TextData`'s spare store clones the whole string into `Placed` on every
|
|
re-break. Bounded at 128 entries, but the clone is per re-break and
|
|
proportional to the text; a transcript-sized text would pay it on every
|
|
width change. Left because a cheaper key changes what "two texts of the
|
|
same words share an answer" means, which is a design question.
|
|
- `TextEdit`'s undo history pushes a whole copy of the text per changed
|
|
keystroke and is never bounded, and `apply_event` clones the text on every
|
|
event including the arrow keys. **`apply_event` is not in this PR's diff at
|
|
all**: it was last touched by #10 and #16, both already on `upstream/main`.
|
|
It keeps resurfacing in sweeps because they diffed against the local `main`
|
|
-- see "The branch layout" in `docs/HANDOFF.md`. Bryan wants the
|
|
unbounded push and the clone dealt with as a change of their own
|
|
(2026-09-20).
|
|
- `ActivationState::update` writes four arms where the `Start`/`On` and
|
|
`End`/`Off` pairs are identical, and `is_off` is `!is_on`. Also verbatim
|
|
from `main`.
|
|
- `TextView::draw`'s empty-with-hint branch matches on `self.hint` again
|
|
after `is_some()` guarded it, so its `None` arm is unreachable. The guard
|
|
cannot become an `if let` because `self.render(painter)` needs `&mut self`
|
|
between the two. Left rather than cloning the handle to satisfy the shape.
|
|
- `CurrentSurfaceTexture::Lost` is answered by reconfiguring, where wgpu says
|
|
to recreate the surface. It will not panic, and recreating needs the
|
|
window; worth doing with the next renderer change rather than this one.
|
|
|
|
## Quality sweep of the whole branch (2026-09-20)
|
|
|
|
A fourth sweep, over the layout core, the arithmetic, the atlas, the sensor
|
|
walk and the fuzz rig rather than over naming. Four findings, all on
|
|
`layout/one-ask` past `1ebd4d3`; the cold dump is byte-identical to it and
|
|
all three seed scans pass (400 at depth 5 in 65.09s, 1,000 at depth 6 in
|
|
161.27s, 2,000 at depth 4 in 301.90s).
|
|
|
|
**A kept contract was judged against the wrong box** (`713e3e7`). `redraw`
|
|
keeps the narrower guarantee a parent holds when the fresh drawing covers
|
|
it, so widening and narrowing back do not churn the parent. It asked
|
|
`was_holds.contains(.., active.placement)` -- where the answer put the
|
|
drawing -- when `holds` is about `active.region`, the box the drawing was
|
|
made in. The two differ on every axis a widget reported less than it was
|
|
offered, so such a widget marked its parent every time its contract
|
|
widened. Cost, not geometry: accepting is always safe, since `region` is
|
|
always inside the old range, so refusing only escalates. `resize` and
|
|
`try_reuse` both already ask about `region`.
|
|
`widening_what_a_drawing_holds_for_does_not_relay_out_the_parent` fails at
|
|
`1ebd4d3` and passes with the line changed; the existing
|
|
`widening_and_restoring_a_contract_does_not_invalidate_its_reader` cannot
|
|
see it, because its leaf reports `LEFTOVER`, which fills its box.
|
|
|
|
**Two things nothing read** (`aea0387`). `ActiveData::size_deps` was written
|
|
on every draw and cleared on every undraw, and read nowhere -- a `Vec` per
|
|
active widget. The `Painter`'s own copy is the live one, used in `draw_at`
|
|
to record whoever asked about a child it did not draw. `SizeRule::apply`
|
|
had no caller and would have been wrong with one: it answers the rule's own
|
|
length where `draw_at` resolves a fraction against the rel base first.
|
|
|
|
**Three reuse rejections said nothing** (`8088a1f`). Of the eight
|
|
rejections in `try_reuse`, a changed inherited mask counted and traced
|
|
nothing, an undrawn record traced without counting, and a changed
|
|
region-node choice counted without tracing. The mask one is what this
|
|
branch's repair was about, so the rig could not answer "why did that
|
|
redraw?" for it. Adding a counter meant editing a variant list and a name
|
|
list at the same index; they are one declaration now.
|
|
|
|
**A fuzz case ran only in the long scan** (`69ba915`). `Case::SizeResize`
|
|
was in `ALL` and in none of `generated.rs`'s `case!` invocations, so the
|
|
size-then-resize order -- which the enum's own comment argues is not the
|
|
same test as the other order -- was never checked by `cargo test`. The
|
|
tests and the list of which cases have one come from one macro invocation,
|
|
and a case missing from it now fails a test.
|
|
|
|
### Tripped a rule and left as it stands
|
|
|
|
- `Span` reads every child's cross length through `place_at(..).len(!axis)`
|
|
even where `has_exact_size(!axis)` makes it moot. The read looks like an
|
|
unwanted dependency, but `depend_on` only matters for a child that is not
|
|
in `children`, which is how `undraw` keeps a measured-then-dropped child
|
|
reachable. For a placed child it does nothing.
|
|
- `PixelRegion::contains` is inclusive at both ends, so two adjacent
|
|
widgets both claim the boundary step. Senses on one layer never block
|
|
each other, so both receiving it is what the design says.
|
|
- `CursorData::sense` is meaningless until `should_run` fills it, which the
|
|
code says in place and proposes a prepare stage for. A real
|
|
unrepresentable-state finding, but it is the event API's shape rather
|
|
than this branch's.
|
|
- `Wrapper` with no child answers `Size::default()`, which is `LEFTOVER`.
|
|
It reads as "nothing" but matches `impl Widget for ()`, whose comment
|
|
says a gap takes the default length so a span gives it a share.
|
|
- `ALL` in `tests/scenario/mod.rs` is still a hand-kept list of every
|
|
`Case`; `Case::name`'s match is the compiler-checked one. A variant left
|
|
out of `ALL` is invisible to the shrinker's `--case` selection too.
|
|
|
|
## Naming and logic sweep (2026-09-19)
|
|
|
|
Settled with Bryan across one session, on the branch past `58ce74d`. Nothing
|
|
here changed what layout computes: the cold dump is byte-identical to
|
|
`58ce74d` at every commit.
|
|
|
|
**How a description is said.** A `PlaceDescAxis` is built by chaining off the
|
|
value that says it -- `UiSpan::within_desc`/`shifted_desc`, `Len::as_desc` --
|
|
never by a constructor naming the type, because a constructor sends the
|
|
reader back to the start of the line. The `_desc` suffix is what says which
|
|
type comes out. `PlaceDescAxis::on_axis(axis)` lifts one axis into a pair
|
|
with the whole box across it; `on` alone was rejected as contentless and
|
|
reserved for events. `from_axes` is the constructor taking a function, beside
|
|
the `from_axis` taking one axis and two values.
|
|
|
|
**Arithmetic that needed a comment became a name.** `UiSpan::place` was the
|
|
aligned-placement rule written out three times; `LayoutLen::without_leftover`
|
|
was the sibling `apply_leftover` never had, at six sites; `is_px`,
|
|
`is_only_leftover` and `declared` name field comparisons the surrounding
|
|
comments had to translate; `Holds::covers` was interval containment by hand.
|
|
Seven module-level functions became methods on the value each took first.
|
|
|
|
**Every pair is a struct of two per-axis values, read with `[axis]`.**
|
|
`LayoutHolds` was four two-element arrays, so none of its own operations
|
|
could be written once; it is `AxisHolds` on `x` and `y`, and `and`, `covers`
|
|
and `contains` lost their loops. `impl_axis_index!` gives every pair
|
|
`Index<Axis>`/`IndexMut<Axis>`, replacing eighteen `axis`/`axis_mut`
|
|
methods -- `const_index` keeps them usable in const context. The bare
|
|
`[Option<LayoutLen>; 2]` became `Declared` of `Option<Len>`, which makes
|
|
"a share is never a declaration" structural rather than two filters and a
|
|
comment.
|
|
|
|
**Two findings in the logic, both one mistake.** A value computed from other
|
|
state was being stored as if it were state, and in both cases the visible
|
|
symptom was something that looked like an off-by-one:
|
|
|
|
- A `Span` carried `start` as a third accumulator beside `fixed` and `taken`,
|
|
assigned at three points, when every assignment was `reached(fixed,
|
|
taken)`. Both ends of a slot are now read where they are used; the variable
|
|
and two of the three calls per child go, and the gap added after the last
|
|
child derives nothing rather than needing to be subtracted.
|
|
- The measuring loop's `cursor` added `px` and `rel` by hand where the
|
|
placing loop below said `fixed += len.without_leftover()` -- the same sum,
|
|
one of them named.
|
|
|
|
**One property that held but nothing guarded.** A `Scroll`'s draw writes
|
|
`amt` and `snap_end`, so a second draw at another viewport reads what the
|
|
first wrote. Warm matches cold only because re-clamping is idempotent and
|
|
monotone. The seed scans build `Scroll`s and never scroll one, so this was
|
|
untested; `a_scrolled_view_resized_lands_where_a_cold_layout_puts_it` scrolls
|
|
four distances, one past the end, then widens. It passes.
|
|
|
|
## Follow-up implementation review (2026-09-19)
|
|
|
|
The ask/place split, window-unit frames, exact validity preimages, and
|
|
bottom-up dirty settling implement the settled design. Keep this approach.
|
|
It does not guarantee one body call per widget: an unhinted descendant that
|
|
reports leftover weight still needs a room ask and a slot ask. The explicit
|
|
measurement redesign remains deferred until an app screen justifies it.
|
|
|
|
The fixes below are on `layout/one-ask` in `/home/bob/repos/iris`:
|
|
|
|
- A collapsed share advances both span cursors. The regression covers one
|
|
and two collapsed children in all four directions.
|
|
- A masking widget owns a mask reference and reclaims its existing slot on
|
|
redraw. Primitives retain their own references. Removing the mask,
|
|
undrawing its owner, freeing the widget, and replacing the root release
|
|
ownership; a changed inherited mask rejects drawing reuse. Tests check
|
|
actual primitive mask indices, movement with and without a region node,
|
|
child draw counts, clip removal/addition, and empty-mask slot reuse.
|
|
- **A further handover defect:** `draw_inner` saved the old parent only
|
|
after a redraw replaced `ActiveData`. The old parent therefore kept the
|
|
child in its list and could undraw the subtree after its new parent drew
|
|
it. Capture the old parent before replacing the record. A branch-switch
|
|
regression reproduces disappearing content when its new parent owns a
|
|
region node, and also checks the ordinary reuse path.
|
|
|
|
The mask and handover tests fail on the reviewed code and pass with the
|
|
fixes. The mask fix preserves child reuse rather than redrawing descendants
|
|
on every mask repaint. No naming sweep or rounding-policy change is included.
|
|
|
|
Validation: workspace tests with and without diagnostics, the release fast
|
|
oracle, and all three prescribed seed scans pass. Comparing 34,488 cold
|
|
boxes against `cadfba0` finds 650 changes; withholding just the collapsed-slot
|
|
fix reproduces the baseline exactly. This is an expected geometry correction,
|
|
not a cost-only change whose dump should remain identical. The tabs example
|
|
and an exact 400 px collapsed-share fixture were rendered and inspected.
|
|
|
|
Two test-harness savings leave the random stream and coverage unchanged:
|
|
`generated` constructs one plan per seed for its sixteen scenarios, and the
|
|
warm/cold comparison constructs its diagnostic ancestry lookup only after
|
|
finding a mismatch. No overall speedup is claimed; no deep profile was run.
|
|
|
|
**Integration is larger than an API rename.** The app's pinned `32f6ad8`
|
|
has 45 commits not reachable from this review branch. In particular, the
|
|
app's nested/shape masks and shared `Ui` ownership are absent here: #19's
|
|
mask is still a single rectangle and `set_mask` rejects nested masks. Keep
|
|
the app pin until those existing capabilities have been integrated. The
|
|
"Masks" and "UI ownership" sections of `LAYOUT.md` describe the app-side
|
|
implementation, not everything already present on the upstream review branch.
|
|
|
|
## Original review of #19 at `cadfba0` (2026-09-19)
|
|
|
|
The original review read `cadfba0`, then the tip of `layout/one-ask`.
|
|
`cargo fmt --all --check`, clippy `-D warnings` and the 123-test suite were
|
|
clean there. These are the original failures, fixed by the follow-up above.
|
|
|
|
### A span misplaces the slot after a collapsed `leftover` child
|
|
|
|
`src/widget/position/span.rs:101` keeps two cursors while it places: `fixed`,
|
|
everything taken so far, and `start`, where the next slot begins. The branch
|
|
that drops a share child with no room to divide advances `fixed` by the gap
|
|
and not `start`:
|
|
|
|
```rust
|
|
if len.leftover > Weight::ZERO && len.px == Px::ZERO && len.rel == Rel::ZERO && !shares {
|
|
painter.undraw(child);
|
|
fixed.px += self.gap;
|
|
continue; // `start` still excludes this gap
|
|
}
|
|
let from = start;
|
|
```
|
|
|
|
In a 400 px row with `gap(10)` over children of 200 px, `leftover(1)` and
|
|
180 px -- exactly full, so nothing is left over and the share collapses --
|
|
the tail is placed at `(215, 0)..(395, 100)`. Its slot was 210..400, a gap
|
|
too long and a gap too early, and the declared 180 was then centred in it.
|
|
The row reports a total that ends at 400. A child drawn with `rel` lengths
|
|
is stretched into the extra gap instead of being centred in it, because the
|
|
slot fills.
|
|
|
|
Recomputing the cursor in that branch fixes it, and the tail lands at
|
|
`(220, 0)..(400, 100)`:
|
|
|
|
```rust
|
|
start = shared(fixed, taken, total.leftover, room);
|
|
```
|
|
|
|
The 123-test suite passes with the line in. Nothing in it or in the
|
|
generated oracle catches the defect: warm and cold layouts are wrong
|
|
identically, so an oracle comparing the two cannot see it. This is the
|
|
sharp form of the handoff's older "a vanished child leaves a double gap"
|
|
item, which described the accounting and not the misplacement.
|
|
|
|
### A reused child keeps the mask its parent replaced
|
|
|
|
`ActiveData::parent_mask` is recorded and documented as the inherited mask
|
|
"the one a redraw of it must not be handed back", but `try_reuse`
|
|
(`core/src/ui/render_state.rs:565`) never compares it with `info.mask`: it
|
|
gates on dirtiness, layer, move parent and region-node status only.
|
|
`Painter::set_mask` pushes a fresh `MaskIdx` on every draw, so a masking
|
|
widget that redraws while its child is reused leaves that child's
|
|
primitives naming a mask nobody updates again:
|
|
|
|
```
|
|
frame 1: masked mask=Id(0) inner mask=Id(0)
|
|
frame 2 (the masked widget alone marked): masked mask=Id(1) inner mask=Id(0)
|
|
frame 3 (the subtree moves up to y=10):
|
|
mask 0: y starts at 50 <- what the inner's primitives are clipped by
|
|
mask 1: y starts at 10 <- the live one, clipping nothing
|
|
```
|
|
|
|
The child's drawing is then clipped 40 px too high and its top is cut off.
|
|
`main` guarded this with `active.mask == mask` in its reuse gate and
|
|
re-marked the owners of a rebuilt mask in `remask_shape_users`; the rewrite
|
|
dropped both.
|
|
|
|
Returning `None` from `try_reuse` where `active.parent_mask != info.mask`
|
|
fixes the repro and keeps the suite green, but it redraws the whole subtree
|
|
whenever a masking ancestor redraws. The better fix is to give `set_mask` a
|
|
per-widget mask slot kept across redraws, the way `UiRenderState::move_slot`
|
|
already keeps a move entry, so the index is stable and `reposition` goes on
|
|
updating the one the descendants name.
|
|
|
|
### Regression coverage
|
|
|
|
Both paths now have regressions in `tests/cases/layout.rs` and
|
|
`tests/cases/retained.rs`; the follow-up above records the additional cases.
|
|
The descriptions above preserve the original failure at `cadfba0`.
|
|
|
|
### Clarity, in the order worth doing
|
|
|
|
Everything about naming is done: the unswept `extent`, the two same-typed
|
|
boxes on `ActiveData` and `Painter`'s four holds accumulators in `5642f20`,
|
|
`Part::All` in `aeb60e5`, and `Place`/`Part` themselves in `58ce74d`. The
|
|
settled vocabulary and the ask API are in `docs/LAYOUT.md`.
|
|
|
|
The clarity sweep of `3da1c71` and `7e2b4cd` closed the first three items
|
|
that stood here. Both are cold-dump identical to `6c84b6f`, so none of it
|
|
moved a box, and all three seed scans passed on `7e2b4cd` (400 at depth 5 in
|
|
66.25s, 1,000 at depth 6 in 188.34s, 2,000 at depth 4 in 300.80s) because
|
|
`reposition` and `redepth` were restructured on the retained path:
|
|
|
|
- `Answer` {size, holds} and `Drawn` {answer, drawing_holds} replace
|
|
`(Size, LayoutHolds)` and the three-tuple with two `LayoutHolds` in it.
|
|
`try_reuse` answers `bool` rather than `Option<()>`.
|
|
- `Span::along` is `Span::slot`; `far` is `row`, `shares` is `has_room`
|
|
beside a named `any_leftover`, and `reached` guards on the weight it
|
|
divides by rather than on the numerator.
|
|
- `DrawInfo::px` was the rel base in pixels, and all three readers printed
|
|
it as the box the widget drew in. Removed; each reads
|
|
`region.to_px(window)`. `Placing::window` existed only to feed it.
|
|
- `diag::outside` holds the 23 counter lines that were inside `try_reuse`.
|
|
- `ActiveData::is_region_node` replaces four copies of
|
|
`move_idx != parent_move`; `Axis::BOTH` replaces `AXES` in three modules;
|
|
`Len::rel_min`, `rel_max` and the unused `select_len` are gone.
|
|
|
|
`1ebd4d3` then closed the `PlaceSpan` item. `PlaceSpan` and `RelBase` are
|
|
`pub` and `ui/mod.rs` re-exports `place` by name rather than by glob, the way
|
|
it already did for `painter`, so the six `pub(crate)` accessors
|
|
(`stated_rel_base`, `narrows_rel_base`, `within_span`, `is_sized`,
|
|
`does_fill`, `with_rel_base`) are gone and `in_parent` matches
|
|
`(at.span, declared)`. `!at.is_sized()` was dead: `PlaceSpan::Sized` is built
|
|
only by `Len::as_desc`, which sets `RelBase::Len(self)` in the same literal,
|
|
and deleting `with_rel_base` removes the only writer that could have
|
|
separated them. Visibility here is plain `pub` plus a named re-export
|
|
wherever the path can be hidden (Bryan, 2026-09-19); `pub(super)` is for
|
|
inherent methods on types the crate exports, where it cannot.
|
|
|
|
What is left, none of it urgent:
|
|
|
|
- `widget_at` does three linear scans per child (`children.contains`,
|
|
`under.iter_mut().find`, `depend_on`), so a span of *n* children is
|
|
O(n^2) per draw. Not a problem at today's sizes; it is worth knowing
|
|
before a long transcript list lands on it.
|
|
- `DrawInfo` and `ActiveData` both carry `placed` and `asked`, two
|
|
`PlaceDesc` fields distinguished only by position in every literal.
|
|
They are genuinely different and documented, but the names are past
|
|
participles with no operand; a rename is Bryan's vocabulary call.
|