iris-ai
61a2f601bb
The handoff had grown a chronicle of what each commit renamed. What is durable about it -- the three names, and the one argument a container uses to say them -- is design, so it goes to `docs/LAYOUT.md` as its own section and the handoff keeps a pointer plus where the branch stands. `docs/PLAN.md`'s "frame/extent experiment" entry described a prototype in retired words; it becomes a pointer, and the one finding it held that was not recorded elsewhere -- why the extent-relative child-frame trial was removed -- joins the failed hypotheses in `docs/LAYOUT.md`.
156 lines
7.7 KiB
Markdown
156 lines
7.7 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`.
|
|
|
|
## 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`. What is left:
|
|
|
|
- `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.
|