Record what the step 1 review found

The pre-submit review of the six one-ask commits found a scroll placing
content that fits into a window-length box rather than its viewport, and
three comments still calling window lengths frame lengths.
This commit is contained in:
iris-ai committed 2026-09-19 01:24:22 -04:00
1 parent 2a3072b90a
commit 2fa734cd63
2 files changed
+66 -18

No files matched your search

+21 -18
View File
@@ -14,20 +14,21 @@ detached comparison checkout is `/home/bob/repos/iris-layout-baseline`. It is
the reviewed baseline this work must preserve or improve. the reviewed baseline this work must preserve or improve.
The continuation is `/home/bob/repos/iris-layout-experiment`, now on branch The continuation is `/home/bob/repos/iris-layout-experiment`, now on branch
**`wip/one-ask`** at **`a30971e`**, six commits over `4328eac` (the head of **`wip/one-ask`** at **`a888717`**, eight commits over `4328eac` (the head of
`wip/transparent-frames`, which is unchanged). It replaces the old step 3 `wip/transparent-frames`, which is unchanged). It replaces the old step 3
plan with the one-ask protocol below, and `1512d84` and `23523ee` make the plan with the one-ask protocol below, `1512d84` and `23523ee` make the frame
frame a length of the window. It passes every check: a length of the window, and `e8a5792` is what the step 1 review found. It
passes every check:
| check at `a30971e` | result | | check at `a888717` | result |
| --- | --- | | --- | --- |
| `cargo fmt --all --check`, clippy `-D warnings`, with and without `layout-diagnostics` | clean | | `cargo fmt --all --check`, clippy `-D warnings`, with and without `layout-diagnostics` | clean |
| `cargo test --workspace` (debug) | 122 suite, 20 core, 11 generated, all green | | `cargo test --workspace` (debug) | 123 suite, 20 core, 11 generated, all green |
| `cargo test --release --test generated` | 11/11 | | `cargo test --release --test generated` | 11/11 |
| shrinker, 400 seeds, depth 5, all sixteen cases | agree, 66 s | | shrinker, 400 seeds, depth 5, all sixteen cases | agree, 73 s (34,488 widgets) |
| 1000 seeds at depth 6 | agree, 170 s | | 1000 seeds at depth 6 | agree, 187 s |
| 2000-seed depth-4 scan, all sixteen cases | agree, 310 s (82,203 widgets) | | 2000-seed depth-4 scan, all sixteen cases | agree, 347 s (82,203 widgets) |
| renders and the `tabs` replay against #18 | inspected, see below | | renders, the `tabs` replay and the `random` resize against #18 | inspected, see below |
What implementing it corrected in the plan is in `docs/LAYOUT_LOG.md`; the What implementing it corrected in the plan is in `docs/LAYOUT_LOG.md`; the
four that would have shipped as wrong layout are a share inside padding four that would have shipped as wrong layout are a share inside padding
@@ -40,10 +41,12 @@ The renders and the replay are done and recorded in `docs/LAYOUT_LOG.md`:
against #18, `minimal` and `tabs` are byte-identical (before and after the against #18, `minimal` and `tabs` are byte-identical (before and after the
reference gesture), `random` differs in two pixels of glyph antialiasing, reference gesture), `random` differs in two pixels of glyph antialiasing,
`text` moves one padded block one pixel, and a live resize of `random` `text` moves one padded block one pixel, and a live resize of `random`
matches a cold render at that size byte for byte. matches a cold render at that size byte for byte. Re-run at `a888717`, all
five are byte-identical to the same renders at `a30971e`, so the scroll fix
below changed none of them.
Not done: the retained-cost work (step 5) and a pre-submit review of the six Not done: the retained-cost work (step 5). Step 1's review is done and what
commits as one diff (step 1). it found is in `docs/LAYOUT_LOG.md`.
The worker's older step 3/4 experiment is preserved as branch The worker's older step 3/4 experiment is preserved as branch
`wip/step3-experiment` (one commit over `4328eac`) and as `wip/step3-experiment` (one commit over `4328eac`) and as
@@ -263,12 +266,12 @@ a second draw back.
### 1. Review the six commits ### 1. Review the six commits
Run the pre-submit review over the six commits as one diff against **Done at `a888717`.** The pre-submit review over the six commits as one
`4328eac`. `1512d84` rewrote much of what the first two did to `painter.rs` diff against `4328eac` found one wrong layout -- a scroll placing content
and `render_state.rs` and was reviewed as it was written, but the six have that fits into a window-length box rather than the viewport -- and three
never been read as one change, and the parts of the one-ask protocol it did comments left describing lengths as fractions of the frame. Both are in
not touch -- `place_at`, the twice-asked deferral, `Span`'s two passes -- `docs/LAYOUT_LOG.md`; the fix is `e8a5792`, pinned by
were written as a probe and reviewed only by their tests. `scroll::content_that_fits_is_placed_in_the_viewport_and_not_in_the_window`.
### 2. Make the frame a length and the box a region ### 2. Make the frame a length and the box a region
+45
View File
@@ -130,6 +130,51 @@ did not bring it back. Either an edit fixed it that I could not identify, or
the rig is not as deterministic as it looks. If a seed ever fails once and the rig is not as deterministic as it looks. If a seed ever fails once and
not again, suspect the second. not again, suspect the second.
## What the step 1 review found (worker, 2026-09-19)
The pre-submit review of the six commits `4328eac..a30971e` as one diff.
Build, lint and every fuzzer were already clean, and the review still found
one wrong layout.
- **A scroll placed content that fits into a box the length of the
window.** `Scroll` asked for its content box as
`Place::Fill(Part::From(content))` with `content` defaulting to
`UiSpan::FULL`, and only replaced it when the content is scrolled or
longer than the viewport. A `Part::From` span is in window lengths, so
`UiSpan::FULL` -- `rel 0` to `rel 1` -- is the whole window rather than the
whole box. The 300 px viewport of a 400 px window put its content in a
400 px box anchored at the viewport's start: a 50 px child centred at
275..325 where #18 has it at 225..275. It is the common case, not a corner
one -- any content shorter than its viewport takes it.
Nothing in the repository caught it. The warm/cold oracle cannot: both
sides are wrong the same way, and it compares warm with cold rather than
with what is right. The render set cannot either: every scroll in
`minimal`, `tabs`, `random`, `text` and `view` is either window-length on
its axis or has content longer than its viewport, so all five are
byte-identical before and after the fix. Only a hand-written expectation
found it, which is the argument for keeping some.
Fixed at `e8a5792` by saying the whole of a box as `Part::All`, the one
expression that cannot mean anything else -- and, being the place the
child was already asked in, one that makes the placement a no-op. Pinned
by `scroll::content_that_fits_is_placed_in_the_viewport_and_not_in_the_window`.
- **`Part::From`'s own documentation still called its spans frame lengths**,
which is what the scroll above read them as, and two comments in
`in_parent` still described the window ranges as frame ranges converted
through a frame. Corrected at `a888717`. When a unit changes, the comments
naming the old one are not decoration: this one cost a layout defect.
Checks re-run at `a888717`: `cargo fmt --all --check`, clippy with and
without `layout-diagnostics`, 123 suite tests, 20 core, 11 generated, the
400-seed depth-5 shrinker over all sixteen cases (73 s, 34,488 widgets), the
1000-seed depth-6 oracle (187 s), the 2000-seed depth-4 scan (347 s, 82,203
widgets), all five renders at 1920x1200, the `tabs` replay, and a live
resize of `random` to 900x1200 against a cold render there. The renderer was
confirmed as Venus on the host's RX 7900 XT rather than llvmpipe. Every
render is byte-identical to the same render at `a30971e`.
## What the one-ask protocol found (planner, 2026-09-18, third session) ## What the one-ask protocol found (planner, 2026-09-18, third session)
Branch `wip/one-ask` at `3091fb8` over `4328eac`. The change is described in Branch `wip/one-ask` at `3091fb8` over `4328eac`. The change is described in