diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index f5941f9..1e62aca 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -6,52 +6,47 @@ in `docs/LAYOUT_LOG.md`. ## The Iris layout repair is submitted -**Iris PR #19** (`layout/one-ask` at `cadfba0`, 123 commits over upstream -`main` at `ca2b4b2`) is open and waiting on Bryan. It **replaces #18**, which -is closed and whose branch `split/18-position-chain` stays as the reviewed -baseline the newer work was measured against. Bryan decided on 2026-09-19 -that the experiment replaces it, because it is more performant where it -matters. +**Iris PR #19** (`layout/one-ask`) replaces closed #18. The current tip is +`84dad21`: `add6774` fixes collapsed-share placement, retained mask ownership, +and a redraw-on-reparent defect; `84dad21` removes repeated work in the test +harness. `docs/LAYOUT_LOG.md` records the review and its regression coverage. +The core design remains sound; naming and round-to-nearest are unchanged. -The checkout is `/home/bob/repos/iris-layout-experiment`, on branch -`wip/hint-first`, which is the same commit as `layout/one-ask`. Both are -pushed to `origin` (`iris-ai/iris`). What the branch does and what it leaves -undone is in the PR description; the settled protocol is in `docs/LAYOUT.md` -under "Frames, decided boxes and padding". +There is one Iris Git repository, stored with this checkout's submodule: -Verification at `cadfba0`, all run in this checkout: +- `/home/bob/repos/iris` is the active `layout/one-ask` worktree. +- `/home/bob/repos/ai-app-2/iris` stays on `main` at the app's `32f6ad8` pin. + It shares Git storage with the active worktree, rather than being a second + development repository. Keep it until the app's integration is ready. +- The standalone `iris-layout-experiment` clone is retired. Superseded fork + branches are removed; unique experiment tips and the #18 measurement + baseline are preserved under `archive/2026-09-19/` tags. A complete bundle + is also under `~/.local/state/iris/archive/`. -| check | result | -| --- | --- | -| `cargo fmt --all --check`, clippy `-D warnings`, with and without `layout-diagnostics` | clean | -| `cargo test --workspace` | 123 suite, 20 core, 11 generated, green | -| `cargo test --release --test generated` | 11/11 | -| shrinker, 400 seeds, depth 5, sixteen cases | agree, 60 s | -| 1000 seeds at depth 6 | agree, 177 s | -| 2000-seed depth-4 scan, sixteen cases | agree, 295 s (82,203 widgets) | -| cold dump of 400 trees at depth 5, diffed against `f6242aa` | identical, 34,488 widget boxes | -| counters, seeds 1 and 13 at depth 8 | unchanged from `f6242aa` (the table is in `docs/LAYOUT.md`) | -| five renders, the `tabs` replay, a live resize against a cold render | all byte-identical to `a888717` | - -The renderer was Vulkan on the host's RX 7900 XT: `src/default/render.rs` -asks for `Backends::PRIMARY` and `.expect()`s the adapter, so a PNG at all -means it was not the software fallback. +Verification of the fixes: format and workspace clippy with `-D warnings`, +workspace tests with and without `layout-diagnostics` (128 suite tests, +20 core tests, 21 with diagnostics, and 11 generated tests), and the release fast oracle pass. +The 400-seed depth-5 shrinker, 1,000-seed depth-6 run, and 2,000-seed +breadth scan all pass. The cold dump covers 34,488 boxes; 650 change because +of the collapsed-share correction. With only that correction withheld, +the remaining fixes reproduce the baseline dump exactly. +The headless `tabs` example and the exact collapsed-share repro were rendered +and inspected; the latter places the blue tail at x=220..400. The machine's +Vulkan device is the RX 7900 XT through RADV native context. ## What is next, in order -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`. -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. -4. **Round-to-nearest**, CPU and shader together as one verified change. +1. **Bryan's review of #19.** Fixes are themselves unreviewed code: repeat + `pre-submit-review` over each round's changes, and re-run the three + fuzzers and dump diff for edits to `Span`, `Painter`, or `render_state`. + The ordinary oracle does not replace absolute geometry and + retained-primitive expectations. +2. **Integrate the app's Iris capabilities before changing its pin.** + `32f6ad8` has 45 commits not reachable from the review branch; shared UI + ownership, richer masks, Android support, and app-side performance work + must survive the integration. Then migrate `SizeRule` beside widgets, + alignment properties, `rest` to `leftover`, and removal of `OrthoSize`. +3. **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)". diff --git a/docs/LAYOUT.md b/docs/LAYOUT.md index 56def26..d79fa22 100644 --- a/docs/LAYOUT.md +++ b/docs/LAYOUT.md @@ -302,6 +302,11 @@ physical pixels, unchanged. ## Masks +This section and "UI ownership and frame access" describe the app's pinned +Iris implementation. The upstream #19 review branch still has single +rectangular masks and separate `UiData`/`UiRenderState`; adopting it requires +integrating the app-side capabilities as well as migrating layout calls. + A `Mask` references a rectangle primitive and its parent mask. Nested masks multiply coverage. Plain `.masked()` creates an undrawn rectangle at the widget's region; `.masked_by(shape)` draws the shape behind the content and @@ -489,8 +494,8 @@ of its parent's frame and a local redraw walked back up the parent chain in defensively on every use. - A retained drawing is reusable only when its `Holds` contains the new box on both axes, its parent and region-node choice match, it is on the layer it - was drawn on, and the widget is clean. A valid ordinary subtree moves by - recursive remap; a region node moves by one entry. A container that draws a + was drawn on, its inherited mask matches, and the widget is clean. A valid + ordinary subtree moves by recursive remap; a region node moves by one entry. A container that draws a child to learn its size uses `Painter::child_layer_at`, the layer the child will actually occupy. - An answer's validity and its final drawing's validity are independent. A @@ -506,7 +511,9 @@ of its parent's frame and a local redraw walked back up the parent chain in draw it. Dirty size dependencies settle deepest-first. - A widget that creates a mask clips to and reports its box. Its own mask and its inherited mask are distinct retained state: the former says which mask - a move rewrites, while a local redraw receives the latter. + a move rewrites, while a local redraw receives the latter. The owner retains + one mask reference and reuses its slot across redraws, independently of + primitive references; dropping the mask or undrawing the owner releases it. - A span's leftover/no-leftover boundary is a strict structural decision, not a tolerance. Derive the boundary through the inverse of the expression that places children. A cap may not contain `leftover`, because feeding the diff --git a/docs/LAYOUT_LOG.md b/docs/LAYOUT_LOG.md index 3544f64..0ae69cd 100644 --- a/docs/LAYOUT_LOG.md +++ b/docs/LAYOUT_LOG.md @@ -6,12 +6,60 @@ nothing here is rediscovered. Each entry says who found it and when. 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) +## Follow-up implementation review (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. +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 @@ -80,11 +128,11 @@ 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 +### Regression coverage -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. +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