diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index 81be509..f5941f9 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -1,8 +1,8 @@ # Handoff -Where the work in flight stands. The durable layout design, the measurement -method and the findings that outlived the working log are in -`docs/LAYOUT.md`. +Where the work in flight stands. The durable layout design and the +measurement method are in `docs/LAYOUT.md`; what the review of #19 found is +in `docs/LAYOUT_LOG.md`. ## The Iris layout repair is submitted @@ -39,23 +39,25 @@ means it was not the software fallback. ## 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 the three fuzzers and the dump diff for anything that touches `Span`, `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 widget rather than a wrapper, alignment as a widget property, `rest` 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 derivation, the form to use and what to re-check are in `docs/LAYOUT.md` under "Rendering the grid (pending)". ## 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 inner span beside one in another get three quarters to one quarter. No other layout system does that; confirm it is wanted. diff --git a/docs/LAYOUT_LOG.md b/docs/LAYOUT_LOG.md new file mode 100644 index 0000000..3544f64 --- /dev/null +++ b/docs/LAYOUT_LOG.md @@ -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.