Record what the review of Iris #19 found
Two live defects at cadfba0, each with the repro that showed it and the fix that answered it: a span places the slot after a collapsed leftover child a gap early and a gap too long, and a child reused under a parent that redrew keeps the mask index that parent replaced, so its drawing is clipped where the subtree used to be. Neither is covered by a test, which is why the suite is green over both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
3ecd676bcf
commit
67584cedda
2 files changed
+130
-8
No files matched your search
+10
-8
@@ -1,8 +1,8 @@
|
|||||||
# Handoff
|
# Handoff
|
||||||
|
|
||||||
Where the work in flight stands. The durable layout design, the measurement
|
Where the work in flight stands. The durable layout design and the
|
||||||
method and the findings that outlived the working log are in
|
measurement method are in `docs/LAYOUT.md`; what the review of #19 found is
|
||||||
`docs/LAYOUT.md`.
|
in `docs/LAYOUT_LOG.md`.
|
||||||
|
|
||||||
## The Iris layout repair is submitted
|
## The Iris layout repair is submitted
|
||||||
|
|
||||||
@@ -39,23 +39,25 @@ means it was not the software fallback.
|
|||||||
|
|
||||||
## What is next, in order
|
## What is next, in order
|
||||||
|
|
||||||
1. **Bryan's review of #19.** Fixes to it are unreviewed code: repeat the
|
1. **The two defects the review of #19 found**, written up with their
|
||||||
|
repros and fixes in `docs/LAYOUT_LOG.md`: a span misplaces the slot after
|
||||||
|
a collapsed `leftover` child, and a reused child keeps the mask its
|
||||||
|
parent replaced. Both are live at `cadfba0` and neither has a test.
|
||||||
|
2. **Bryan's review of #19.** Fixes to it are unreviewed code: repeat the
|
||||||
`pre-submit-review` passes over whatever each round changes, and re-run
|
`pre-submit-review` passes over whatever each round changes, and re-run
|
||||||
the three fuzzers and the dump diff for anything that touches `Span`,
|
the three fuzzers and the dump diff for anything that touches `Span`,
|
||||||
`Painter` or `render_state`.
|
`Painter` or `render_state`.
|
||||||
2. **The app's Iris pin**, once #19 lands. It is still on the old submodule
|
3. **The app's Iris pin**, once #19 lands. It is still on the old submodule
|
||||||
commit, and the branch changes API the app uses: `SizeRule` beside a
|
commit, and the branch changes API the app uses: `SizeRule` beside a
|
||||||
widget rather than a wrapper, alignment as a widget property, `rest`
|
widget rather than a wrapper, alignment as a widget property, `rest`
|
||||||
renamed to `leftover`, `OrthoSize` gone.
|
renamed to `leftover`, `OrthoSize` gone.
|
||||||
3. **Round-to-nearest**, CPU and shader together as one verified change.
|
4. **Round-to-nearest**, CPU and shader together as one verified change.
|
||||||
Bryan approved it on 2026-09-17 and neither half has landed; the
|
Bryan approved it on 2026-09-17 and neither half has landed; the
|
||||||
derivation, the form to use and what to re-check are in `docs/LAYOUT.md`
|
derivation, the form to use and what to re-check are in `docs/LAYOUT.md`
|
||||||
under "Rendering the grid (pending)".
|
under "Rendering the grid (pending)".
|
||||||
|
|
||||||
## Smaller layout items, none urgent
|
## Smaller layout items, none urgent
|
||||||
|
|
||||||
- An undrawn `leftover` child still contributes its gap, so a vanished child
|
|
||||||
leaves a double gap.
|
|
||||||
- Nested spans pass `leftover` weight up, so three leftover children in one
|
- Nested spans pass `leftover` weight up, so three leftover children in one
|
||||||
inner span beside one in another get three quarters to one quarter. No
|
inner span beside one in another get three quarters to one quarter. No
|
||||||
other layout system does that; confirm it is wanted.
|
other layout system does that; confirm it is wanted.
|
||||||
|
|||||||
@@ -0,0 +1,120 @@
|
|||||||
|
# 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`.
|
||||||
|
|
||||||
|
## What the review of #19 found (2026-09-19)
|
||||||
|
|
||||||
|
Read at `cadfba0` in the `iris/` submodule of this checkout, which is the
|
||||||
|
same commit as `layout/one-ask`. `cargo fmt --all --check`, clippy
|
||||||
|
`-D warnings` and the 123-test suite are clean there, so both defects below
|
||||||
|
are live behaviour rather than anything the tests say.
|
||||||
|
|
||||||
|
### 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 is a `Place::Fill`.
|
||||||
|
|
||||||
|
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.
|
||||||
|
|
||||||
|
### Neither path has a test
|
||||||
|
|
||||||
|
The suite passes with and without both fixes. A span case belongs in
|
||||||
|
`tests/cases/layout.rs` and a mask-reuse case in `tests/cases/retained.rs`
|
||||||
|
when they land.
|
||||||
|
|
||||||
|
### Clarity, in the order worth doing
|
||||||
|
|
||||||
|
- **`extent` is back.** `core/src/ui` says `extent` 138 times, its comments
|
||||||
|
say "box" 153 times, and the types are all `UiRegion`. Bryan renamed this
|
||||||
|
to `placement` on 2026-09-17; `frame` survived with its new meaning as a
|
||||||
|
length, `extent` did not. One sweep over `Painter::extent`,
|
||||||
|
`ActiveData::{extent, part}`, `placed_extent`, `frame_and_extent` and
|
||||||
|
`LayoutHolds::{extent, extent_len}`.
|
||||||
|
- `ActiveData::part` and `::extent` are both boxes told apart only by
|
||||||
|
prose, and `draw_at` writes `part: extent` from one value. `asked_in` and
|
||||||
|
`drawn_in` would say it in the names.
|
||||||
|
- `Painter` accumulates a `LayoutHolds` under four names that do not match
|
||||||
|
the four fields they become (`window_own`, `extent_own`, `frame_own_len`,
|
||||||
|
`extent_len`). One `own: LayoutHolds` field deletes the mapping.
|
||||||
|
- `draw_inner` returns `(Size, LayoutHolds, LayoutHolds)`, two same-typed
|
||||||
|
values ordered by convention. A named pair makes the swap unwriteable.
|
||||||
|
- `DrawInfo::px` is computed at all four construction sites and read only by
|
||||||
|
`diag::draw_request` and two `debug_assert!` messages. Compute it in the
|
||||||
|
assert.
|
||||||
|
- The `layout-diagnostics` block inside `try_reuse` is 23 lines of counters
|
||||||
|
in the middle of the decision; one `diag::outside(...)` call would keep
|
||||||
|
the branch readable.
|
||||||
|
- `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.
|
||||||
|
- `Part::All` is exactly `Part::Of(UiSpan::FULL)`: `Of` composes through
|
||||||
|
`within`, and `through(Len::FULL)` maps a range back to itself, which
|
||||||
|
`the_whole_of_a_box_maps_back_to_itself` already asserts. It buys a
|
||||||
|
separate arm in `in_parent` and in `Part::of`. Keeping it as sugar is
|
||||||
|
defensible; the case analysis being wider than the geometry is the thing
|
||||||
|
to decide about rather than to leave unstated.
|
||||||
Reference in new issue
Block a user