ai-app/docs/LAYOUT_LOG.md

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.

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 Scrolls 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:

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):

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.