diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index b61294a..558f4a9 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -5,6 +5,38 @@ they can be judged and reversed later. Detail lives in RUST.md (and IRIS.md for iris API changes); this file is only the summary. Newest first. Items marked **DEFERRED** are ones the agent chose not to decide alone. +## 2026-09-08 evening (the fling is shared; a cancel is not a release) + +From Iris's four-item phone report; RUST.md's "2026-09-08 (evening)" box +has the reasoning and the tests, IRIS.md the summary. + +- **A `Flinger` that does not know which way the content moves.** Every + scroll area flings now, on either axis, as Iris asked -- and the + physics is one type shared by `List` and `Scroll` rather than a copy + each. The choice worth reviewing is the seam: `Flinger` owns the curve + and the clock, and the *caller* owns the sign convention and where the + content ends. Rejected: teaching `Flinger` a direction, which would + have to be told to it -- and being told is the same thing as not + knowing, with an extra field to get wrong. +- **A cancel is a first-class end to a gesture, not an early release.** + `CursorState::cancelled` is new state on the pointer sample, set by + Android's `ACTION_CANCEL` and the harness's `TouchAction::Cancel`. + Rejected: mapping a cancel to `PressEnd` and having each widget decide + what to suppress, which is what shipped and is why leaving the app + flung the transcript. +- **A `DragGesture` ignores a `Cancel` it caused.** One gesture is + driven by several widgets, so the widget that was pressed can be a + "loser" on the frame its own gesture won. The test is whether the + gesture's own capture id is the holder. This is what makes it safe for + every widget driving a gesture to register the whole `drag_senses()` + set, which is now the rule without exception. +- **`List::place` draws a resized row twice in one frame.** The old + comment accepted a one-frame lag by analogy with `Scroll`'s content + length. That analogy was wrong: a stale *length* only misplaces the + next thing, while a stale *box* is drawn, because a background fills + whatever box it is handed. The extra draw is bounded to frames where a + row's height actually changed. + ## 2026-09-08 (iris ships an icon font, and the drawn mark is deleted) - **Directed by Iris.** Her question on seeing `widget::mark`: "why does diff --git a/docs/IRIS.md b/docs/IRIS.md index 77ab6a7..bc155be 100644 --- a/docs/IRIS.md +++ b/docs/IRIS.md @@ -12,6 +12,69 @@ things still stay out. An entry gives the date, what changed, why, and a short before/after where it helps judge the change without the session that made it. Newest first. +## 2026-09-08: what a cancel means, what a row's box is, and one fling for every scroll area + +Iris's second 2026-09-08 report, from the bench on her phone. Four items, +and each turned out to be a rule stated in one place and missing from its +siblings rather than a special case. + +**A gesture the *platform* takes away is a cancel, not a release** +(`CursorState::cancelled`). Android's `ACTION_CANCEL` used to take the +same arm as `ACTION_UP`, so the system's own swipe up from the bottom +edge to leave the app arrived as a flick released at speed: the +transcript flung while the app was in the background, and came back +somewhere else. A cancelled sample now hands `CursorSense::Cancel` to +the capture holder *and* every widget still tracking the press, clears +both, and derives nothing else from that sample -- no tap, no selection, +no fling. That is the same sense a widget already gets when it loses a +capture race; what is new is that the platform can raise it, and that +the *winner* hears it too when the platform is the one cancelling. + +**A `DragGesture` ignores a cancel when it is the one holding the +capture.** A cancel goes to every pressed widget that did not capture, +and one gesture is routinely driven by several of those: a transcript +row's text block feeds the shared gesture that captures under the +*list's* id, so the block is a "loser" on the very frame its own pan +committed. `Cancel` means "somebody else won", so the question is +whether the holder is us -- and now it is asked. With that, a row's +block registers the whole `drag_senses()` set, which is what the doc on +that set has always said a widget driving a gesture must do; it was the +one place that did not, and it is why panning a code fence sideways and +then tapping made the transcript jump. + +**A row is drawn at the box its own height implies, in the frame that +height changes** (`List::place`). A row is offered its *cached* height +so that an unchanged row takes `draw_inner`'s cheap path; a +`.background(rect(..))` fills whatever box it is handed. So on the frame +a row changed height its text laid out at the new height and its +background painted at the old one -- collapsing or opening a tool card +looked closed while its text was there, then open while it was not. When +the measurement disagrees with the offer, the row is now drawn again at +its true box. The bottom-anchored half had a `reposition` for this, +which writes an offset and never a size, so it could not fix it either: +the same rule, applied to one member of a set of two. + +**Every scroll area flings, on either axis** (`iris::sense::Flinger`). +The fling was `List`'s alone -- the curve, the clock, the incremental +delta, Compose's two release thresholds -- and a `Scroll` dropped its +released velocity on the floor, with a comment explaining that the areas +it wrapped were only a screenful. That stopped being true the moment a +code fence became one. `Flinger` is that machinery as a type both use; +what it deliberately does not know is which way a positive delta moves +the content or where the content ends, because a `List` and a `Scroll` +answer those oppositely. The caller applies `tick`'s delta in its own +convention and calls `stop` at its own wall. `Scroll::drag` now answers +whether it started a fling, which is what `scroll_area` needs to call +`UiData::animate` -- the same split `List::fling` already documented, +for the same reason: only the caller can reach the frame loop. + +**Removed, not worked around**: `tool.rs` no longer flattens its two +`Span`s into one, so a tool group holds its cards 4dp off its own edge +again. The defect that shape was avoiding -- "a `Span` of `Pad`ded +children inside another `Span` places those children a slot out of step" +-- is not reproducible on 2026-09-08, checked both with a headless +render and with a new layer-1 test. + ## 2026-09-08: a gesture can be cancelled, and the pointer belongs to the input handler Two changes to how a drag ends, from defects on Iris's phone (a code diff --git a/docs/IRIS_TODO.md b/docs/IRIS_TODO.md index 709d12f..858d209 100644 --- a/docs/IRIS_TODO.md +++ b/docs/IRIS_TODO.md @@ -785,22 +785,35 @@ Iris's report, verbatim, with a screenshot. Phone: Mali-G715 (Vulkan), ## Found by P1b (2026-09-06), all with a headless repro Each was found by looking at `iris/run-headless.sh transcript -- -p -transcript-ui` rather than at a diff, and each is worked around in -`transcript-ui/src/tool.rs` rather than fixed here. docs/RUST.md's P1b box -has the fuller account. +transcript-ui` rather than at a diff. docs/RUST.md's P1b box has the +fuller account. -- [ ] **A `Span` of `Pad`ded children inside another `Span` places those +**No entry here is worked around any more** (Iris, 2026-09-08: "All of +those should be fixed. There should never be workaround code. Do the +same for those; fix them if they're trivial, diagnose and report if +not."). Two are fixed and ticked; the two that are left are missing +*capabilities* rather than defects being dodged, and each carries its +diagnosis and what building it actually costs. + +- [x] **A `Span` of `Pad`ded children inside another `Span` places those children a slot out of step.** Each child drew its content one sibling's - height below its own box. Repro: `IRIS_TOOLS_EXPANDED=1 + height below its own box. Repro was: `IRIS_TOOLS_EXPANDED=1 iris/run-headless.sh transcript --shot /tmp/x.png -- -p transcript-ui` with `tool.rs`'s group built as `Span(DOWN)[header, Pad(Span(DOWN) - [cards]), bar]` instead of the single `Span` it uses now. Bisected: - removing the inner `Span` fixes it, and so does removing the children's - own `Pad`; the background `Stack`, the `Sized` wrappers and the - `WidgetPtr` per child make no difference. **Not** the `mov`-vs- - `reposition` fault f5b8893 fixed -- it survives that commit. The - workaround costs the group the 4dp inset its Compose counterpart holds - its cards off the edge by, so this is worth fixing. + [cards]), bar]` instead of the single `Span` it used. Bisected at the + time: removing the inner `Span` fixed it, and so did removing the + children's own `Pad`; the background `Stack`, the `Sized` wrappers and + the `WidgetPtr` per child made no difference. **Not** the `mov`-vs- + `reposition` fault f5b8893 fixed -- it survived that commit. + **Not reproducible on 2026-09-08.** Both spans are nested again and the + group has its 4dp inset back; that same headless render puts every + card's content in its own box, and `iris`'s + `a_span_of_padded_children_inside_a_span_draws_each_where_its_box_is` + (`layout_tests.rs`, the same shape inside a `List`, which is the + context the real one is in) pins it at layer 1. Something between + 09-06 and 09-08 fixed it -- most likely the nested-mask pass or the + `mov` work after f5b8893. Left ticked with the original symptom + recorded rather than deleted, in case it comes back. - [x] **`scrollable_on(Axis::X)` on a non-editable `Text` draws nothing.** The panel is drawn and the text inside it is not. A markdown fence does the same to a `TextEdit` and is fine, so it is the widget kind rather @@ -819,6 +832,32 @@ has the fuller account. nothing on screen says it was cut. Whichever end is cut has to be a choice when this lands: a path is identified by its tail, a command by its head. + + **Diagnosed 2026-09-08, and it is not trivial.** parley has no + ellipsis of its own (checked: nothing in the vendored crates), so iris + would build it, and the shape that looks easy is the one that breaks + something. The easy half really is easy: shape at + `max_advance = width - ellipsis_advance` with wrapping on, take line + 0's `text_range()`, and re-shape `text[..end].trim_end() + "…"` with + wrapping off -- parley's own line breaker finds the cut, so nothing + here counts glyph advances by hand. The hard half is that + `TextBuffer` has exactly one string and everything addresses it by + byte offset: the inline spans that carry a fence's colours and a + link's range, `TextEditCtx::byte_at` (which turns a tap into a byte to + match a link against), `Selection`'s `select`/`selected_text`, and + `RowBlocks::apply_delta`. Truncating the buffer moves every one of + those. So the real work is giving `TextBuffer` a **displayed** string + distinct from its source, with one mapping from display byte to source + byte that all of those go through -- worth doing, and not a + by-the-way. Doing it only for text that is neither editable nor + selectable would avoid all of that and is exactly the kind of + exemption that comes back later. + + It also wants an API change while it is open: `TextAttrs::wrap: bool` + cannot say three states. Something like `Overflow::{Wrap, Clip, + Ellipsis(End)}` replaces it, with `End::{Head, Tail}` making + UI_RULES's "choose which end to truncate" a thing a caller must + answer rather than a default nobody reads. - [x] **A chevron the platform cannot fail to have.** **Done 2026-09-08**, twice. First as `iris::widget::mark(dir, dp, colour)`, which rasterised an antialiased triangle into the ordinary texture path @@ -841,9 +880,26 @@ has the fuller account. - [ ] **A tool card's text is not selectable.** `Selection` is keyed `(RowKey, block index)` and a card has no markdown blocks, so nothing in a card registers. Compose's `SelectionContainer` covers tool output, - which is the text people most want to copy. Needs a key for "the nth - text of this row" that a card can mint without colliding with a - message's blocks. + which is the text people most want to copy. + + **Diagnosed 2026-09-08: mechanical, but more than a sitting.** There + is no key collision to design around, which was the open question: + a `TranscriptRow::Tools` has *only* cards and no markdown blocks at + all, so a card is free to number its own texts from 0 in reading + order. What it costs is the registration lifecycle rather than the + key. Each card's `TextEdit`s have to `Selection::register` as they are + built and `unregister` when they are not -- and a card is rebuilt from + several directions (`redraw_card` when a result arrives, + `Shared::set_content` when the group is toggled or a call joins the + run, and the per-card `WidgetPtr` swap), each of which frees widgets + the map would otherwise still point at. That is the exact shape of the + crash `Selection::clear`'s doc records from + docs/REVIEW-2026-09-06.md: a handle in that map outliving the widget + panics on the *next* long press, somewhere else entirely. So the work + is a per-card base index with a stride (and a `debug_assert` that a + card stays inside it), one register/unregister path that every rebuild + route goes through, and a test per route that a rebuilt card leaves no + stale handle behind. ## Build (for the port) diff --git a/docs/LAYOUT.md b/docs/LAYOUT.md index 257f7ec..967e625 100644 --- a/docs/LAYOUT.md +++ b/docs/LAYOUT.md @@ -1074,3 +1074,43 @@ places the code is narrower than the design above, each deliberate: `iris_core::SHAPE_SHADER` *by name* and runs them in a compute pass, so the thing under test is the shader itself rather than a copy of it that would be edited alongside. + +## What a widget's *offered* box may and may not be (2026-09-08) + +Two rules that were each true in one place and missing from a sibling, +found together by Iris's 2026-09-08 phone report. + +**Padding works in whatever container it is placed in, and is an inset or +an outset depending on how tight that container's region is.** Iris's +own words, 2026-09-08: "padding should work no matter what container a +widget is placed in, and acts as both inset and outset depending on how +tight the parent region is." `Pad` offers its child the region it was +handed, inset on each side, and reports `used + padding` — so given a +generous box it insets the child inside it, and given a box already the +size of the content it reports a larger size and the parent grows. What +this rules out is any container that offers a padded child a box and then +ignores what it reported, and any caller that reshapes its tree to avoid +a `Pad` (which `transcript-ui/src/tool.rs` did until 2026-09-08, at the +cost of a tool group's 4dp inset). + +**A widget offered a box it does not fit is drawn again at the box its +own reported size implies, in the same frame.** Not next frame. The +temptation to defer is real — `List::place` offers a row its *cached* +height precisely so that an unchanged row hits `draw_inner`'s cheap +skip-or-move path, and `Scroll` sizes its child region from last frame's +content length for the same reason. But a `Rect` fills whatever region it +is given (`Size::REST`, and `rect.rs`'s `is_size_independent` doc says +why it must), and `.background(rect(..))` is the ordinary way to style +anything — so a one-frame-stale box is a background drawn at the wrong +size while the text inside it is already right. On screen that is a tool +card that looks closed while its text is there and open while it is not. +A `reposition` is not the fix and cannot be: it writes an offset, never a +size. + +The cost is bounded and worth stating, because it is what makes the rule +safe to apply everywhere: the second draw happens only on the frame a +widget's own size actually changes, which is a frame that was already +redrawing it. A widget whose reported size is a function of the box it +was *offered* would disagree every frame and redraw every frame — which +is why `List` requires content-sized rows, and has since long before +this. diff --git a/docs/RUST.md b/docs/RUST.md index 119cbdc..701df73 100644 --- a/docs/RUST.md +++ b/docs/RUST.md @@ -34,7 +34,7 @@ the emulator, not by Mesa" and "the present mode was not the cause" are worth as much as the successes, because they are what stops the next session spending an afternoon on them again. -## Where things stand (2026-09-06) +## Where things stand (2026-09-06; see the 2026-09-08 evening box at the end) Written on picking the branch up after a `/clear`, so the next session can resume from here. P0 is delivered and Iris's phone report v2 is in @@ -8188,3 +8188,149 @@ marks in `run-headless.sh phone --phone` (before and after a tap that opens the card), and the collapse bar's up mark in `IRIS_TOOLS_EXPANDED=1 run-headless.sh transcript`. All three draw, at the size and alignment the drawn triangle had. + +## Iris's phone report, 2026-09-08 (evening: four defects, all fixed) + +Her words, verbatim, from the iris bench on her phone: + +> - if I scroll in a horizontal area and then tap in a vertical area, it +> seems to snap. Leaving and reopening the app also randomly moved the +> vertical scroll. This is ridiculous and it sounds like the code is +> pretty bad / connected. +> - Flinging doesn't work in horizontal scroll areas. Flinging should be +> enabled by default in all scroll areas on android to match composes +> behavior. +> - Collapsing and opening an edit card draws the card background a frame +> late, so it looks closed even when there's text, and then looks open +> even when the text is collapsed. This is also ridiculous and suggests +> the framework is being used in a bad way. + +Plus, on reading the workaround list in `IRIS_TODO.md`: "All of those +should be fixed. There should never be workaround code. Do the same for +those; fix them if they're trivial, diagnose and report if not." And two +notes on the *shape* of a fix: "make sure all fixes you do work under all +circumstances and just make the code more correct rather than tape on +edge cases", with padding as the example -- "padding should work no +matter what container a widget is placed in, and acts as both inset and +outset depending on how tight the parent region is" (that one is written +up in docs/LAYOUT.md, since it is a rule about the layout rather than +about this pass). + +Commits `fc82d9d` and `02b277e`. Every fix below has a layer-1 repro that +was **confirmed to fail with the change backed out**, which is what says +the test is about the defect and not about the code. + +### The two "snaps" were two different faults with one shape + +Both are a press that ended without the thing tracking it being told, so +the *next* press was measured from an origin belonging to a finger long +gone. `DragArbiter::update`'s `Undecided` crosses its slop instantly at +that distance and pans in one step. + +**Leaving and reopening the app** was `MotionAction::Cancel` sharing an +arm with `MotionAction::Up` in `android/view.rs`. The system's own swipe +up from the bottom edge to go home is delivered to the app as moves and +then `ACTION_CANCEL`, so iris read it as a flick released at speed and +flung the transcript while the app was in the background. `CursorState:: +cancelled` is the fix: a cancelled sample delivers `CursorSense::Cancel` +to the capture holder **and** every widget still tracking the press, +clears both, and derives nothing else from that sample. `TouchAction:: +Cancel` in the harness sets the same flag, which is what makes it +testable from a `.touch` file (`touch/flick-cancelled.touch`). +Tests: `gesture_cancel.rs`'s `a_cancelled_flick_does_not_fling` (fails +with `Some(-15249.9)` when a cancel is a release) and +`a_press_ended_by_a_cancel_leaves_no_origin_for_the_next_one`. + +**Panning a code fence and then tapping** was the `Cancel` this file's +earlier entry added, arriving nowhere. A cancel is delivered to the +widget that was **pressed**, not to whoever holds the capture -- and the +widget the press landed on is the fence's own text block, which drives +`Selection`'s shared `DragGesture` under the *list's* id. `row.rs` +registered `click_or_drag() | unclick()`, so it never heard it. It +registers `drag_senses()` now, which is what that set's own doc has +always said a widget driving a gesture must do. + +That change alone would have broken panning, and this is the part worth +keeping: when the shared gesture *wins* a capture, the block is still a +"loser" by `run_sensors`' accounting, so it would be handed a `Cancel` +on the very frame its own pan committed and would release it. +`DragGesture::handle` now ignores a `Cancel` when `pointer.holder()` is +its own `id` -- "somebody else won" is what the sense means, so whether +the winner is us is the question to ask. Confirmed load-bearing: +`catch_a_fling.rs`'s `a_press_on_a_flinging_list_pins_the_content_to_the_ +finger` fails without it. +Test: `gesture_cancel.rs`'s +`panning_a_code_fence_then_tapping_elsewhere_moves_nothing`, which +pushes a real fence into the real screen so the pan has something to +capture it. **A trap worth not re-finding**: the first version of that +test tapped at a fixed y and "failed" by 544px with every fix in place -- +it had landed on a tool group's header and toggled it. The tap has to be +on ordinary text, so the test pushes a paragraph of its own to aim at. + +### The card background was a frame late because a row is offered its cached height + +`List::place` offers an already-measured row a box sized to the height it +cached, so an unchanged row hits `draw_inner`'s cheap skip-or-move path +(the alternative, `draw_twice` every frame, forces a real redraw every +frame -- `place`'s own doc has that history). A `Rect` fills whatever +region it is given, and `.background(rect(..))` is how a tool card is +styled, so on the frame a card changed height its text laid out at the +new height and its background painted at the old one. A row whose +measurement disagrees with its offer is now drawn again at its true box, +in that frame, on both placements -- the bottom-anchored half used a +`reposition`, which writes an offset and never a size, so it could not +fix it either. Test: +`a_row_that_changes_height_draws_its_background_at_the_new_height_immediately`, +which changes each of five rows in both directions so both placements are +covered without the test knowing which is which. + +### The fling was `List`'s alone + +`Scroll` dropped its released velocity on the floor, with a comment +explaining that the areas it wrapped were at most a screenful. That +stopped being true when a code fence became one. `iris::sense::Flinger` +is `List`'s fling as a type both use: the curve, the clock (started at +the first tick, not the release, so a caller on an explicit clock is not +handed a fling that already expired), the incremental delta, Compose's +two release thresholds and the trace line. It deliberately does not know +which way a positive delta moves the content or where the content ends, +because a `List` and a `Scroll` answer those oppositely -- the caller +applies `tick`'s delta in its own convention and calls `stop` at its own +wall. `Scroll` also gains the two things a coasting widget needs and had +no reason to have: the real display density (a hardcoded 1.0 is what made +a one-second coast run for 45 on a list) and `PressState::scrolling`, so +a finger on a coasting fence stops it from the first sample rather than +after `DRAG_SLOP`. +Tests: three in `scroll.rs`, plus `fence_fling.rs`, which flicks a real +fence in the real transcript screen and reads that fence's own `Scroll` +back out of what was drawn -- found by downcasting through +`UiRenderState::active`, since there is no handle to it from outside and +a bare coordinate would only prove that *something* moved. + +### The workaround list + +`transcript-ui/src/tool.rs` no longer flattens its two `Span`s into one, +so a tool group holds its cards 4dp off its own edge again. "A `Span` of +`Pad`ded children inside another `Span` places those children a slot out +of step" is **not reproducible on 2026-09-08** -- checked with the +headless render that found it (`IRIS_TOOLS_EXPANDED=1 +iris/run-headless.sh transcript --shot -- -p transcript-ui`, cards +correct with the spans nested) and pinned at layer 1 by +`a_span_of_padded_children_inside_a_span_draws_each_where_its_box_is`. +Worth knowing for the next one of these: that layer-1 test does **not** +reproduce the fault outside a `List` — the plain nested-span shape passed +immediately, and it only became a faithful reproduction of the real tree +once the row was placed inside a masked `List`, which is where `place`'s +oversized measurement pass lives. + +The other two entries there are missing capabilities rather than defects +being dodged, and each now carries its diagnosis in `IRIS_TODO.md` +instead of a workaround: **overflow ellipsis** (parley has none, and the +easy implementation breaks every byte-offset consumer of `TextBuffer` -- +spans, `byte_at`, `Selection`, `apply_delta` -- so it needs a displayed +string distinct from the source, plus an `Overflow` enum in place of +`TextAttrs::wrap`), and **selectable tool-card text** (no key collision +to design around, since a tools row has no markdown blocks at all; the +cost is the register/unregister lifecycle across the three routes that +rebuild a card, which is exactly where a stale `Selection` handle +panics).