The `iris` submodule's `main` was the fork's own line -- the app's 45 commits -- and not an ancestor of upstream's main, so it sat four merged pull requests behind PR #19's actual base. Every obvious command was therefore wrong in a way that looked plausible: `git diff main...` showed #10, #12, #16 and #17 as the review branch's work, which is how the parley text migration's undo path kept being reported as #19's. The app's line is now `app-pin`, pushed to origin, and `main` tracks `upstream/main`. `.gitmodules` follows `app-pin`, so `submodule update --remote` cannot drag the pin onto upstream. The recorded commit is unchanged at 32f6ad8; only the name it is reachable by has changed. `origin/main` on the fork is left alone: moving it is a non-fast-forward on a shared branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
487 lines
27 KiB
Markdown
487 lines
27 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`.
|
|
|
|
## 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.
|