diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 1930c0d..50ec9bf 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -5,6 +5,21 @@ 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 (later still: the list's overscroll clamp, in frame) + +Finishes the item the previous entry deferred. IRIS.md has the account. + +- **`List` lays out a second time within the frame** when its walk lands + off the end of the content, instead of writing the correction to the + anchor and asking for another frame. The extra walk is paid only on an + overscrolled frame, and it is mostly O(1) moves. +- **`Painter::draw_again` is removed**, `List` having been its only + caller -- so the framework no longer offers a way to ask for a + corrective frame at all. +- **`List::place`'s top-known and bottom-known cases are one path** + (`Placement::edges`), which is the "write the logic once" rule applied + to two symmetric directions rather than a behaviour change. + ## 2026-09-08 (later: a scroll area measures and places in one frame) From Iris's phone report about the composer's padding while typing diff --git a/docs/IRIS.md b/docs/IRIS.md index a54e1a8..16235d2 100644 --- a/docs/IRIS.md +++ b/docs/IRIS.md @@ -12,6 +12,56 @@ 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 (later still): a `List` clamps its overscroll in the same frame, and `draw_again` is gone + +The last place in iris that corrected itself on a later frame. `List`'s +walk outward from its anchor could end up off the end of its content -- +a fling stops wherever the spline's last step left it, and a `scroll` is +deliberately unclamped because nothing at the moment of the call knows +where the content ends. `clamp_to_content` measured that gap from the +edges the walk had just placed, wrote it to the anchor, and asked for +another frame. So one frame was drawn with the content past its own end, +and on Iris's phone a hard fling to the top left the whole screen blank +until something asked for that frame -- which a fling that has stopped no +longer does. + +It is the same shape as `Scroll`'s fix. The walk is now `List::lay_out`, +and `List::draw` runs it, asks `overscroll_gap` whether the layout landed +off the end, and on a gap moves the anchor and runs the walk **again, +inside the same frame**. `overscroll_gap` is a pure measurement -- no +painter, no redraw handle -- and the decision to lay out again is `draw`'s. + +Three properties make the second pass cheap and correct: + +- **It runs only on a frame that actually overscrolled.** An ordinary + scroll tick still walks once. +- **One further pass always settles it.** The gap is measured from the + edges the first walk placed, so moving the anchor by it puts that edge + exactly on the viewport's; the opposite end can only open a new gap if + the content is shorter than the viewport, which `overscroll_gap` + declines to touch at all (a short list is bottom-anchored on purpose). +- **The second walk is mostly moves.** Every row keeps the box its cached + height gives it and only its offset changes, which is `draw_inner`'s + O(1) `mov` path. + +**Public surface: `Painter::draw_again` is removed.** `List` was its only +caller, so with this there is no "ask for a corrective frame" mechanism in +the framework -- which is the point, since reaching for one is the sign a +placement should have been redone inside the draw that discovered the +problem. + +`List::place` also lost half its body to the same simplification the rule +suggests: a placement is one pinned edge plus a height, so `Placement:: +edges(height)` gives the box and the top-known and bottom-known cases stop +being two copies of the same arithmetic. + +Tests that draw no settling frame on purpose, and fail without the change: +`fling_toward_the_start_stops_at_the_first_row` and the new +`scrolling_past_the_start_is_given_back_in_the_same_frame` in `list.rs`, +and `scrolling_past_the_first_row_settles_on_it` / +`scrolling_past_the_last_row_settles_on_it` at layer 1 +(`transcript-fixture/tests/top_edge.rs`). + ## 2026-09-08 (later): a `Scroll` measures and places its content in one frame Iris's phone: "when typing with the keyboard up and entering enough diff --git a/docs/IRIS_TODO.md b/docs/IRIS_TODO.md index b4722ae..da2e28d 100644 --- a/docs/IRIS_TODO.md +++ b/docs/IRIS_TODO.md @@ -7,7 +7,7 @@ order and what "done" looks like. Tick and date them in place. ## Fix -- [ ] **`List::clamp_to_content` still corrects on the next frame +- [x] **`List::clamp_to_content` still corrects on the next frame (2026-09-08).** Iris's rule, stated while the composer's caret was being fixed: "nothing in the framework should ever self heal because it should not be drawn incorrectly in the first place. If you need 2 @@ -15,15 +15,26 @@ order and what "done" looks like. Tick and date them in place. happen within the same frame. Layout should never be frame dependent, it should be a pure function of the state." `Scroll::draw` was brought to that rule the same day (it measures its content and places it - again in the one frame, IRIS.md's entry). `List::clamp_to_content` is - the one place left that has not been: it discovers a fling has run - past the content's end, calls `scroll(gap)` and `Painter::draw_again`, - and asks its own `RequestRedraw` handle for a frame -- so one frame is - drawn with the content past its end and the next one snaps it back. - The fix is the same shape as `Scroll`'s: re-place inside the draw that - found the gap. Not done in the same change because `List::place` is a - larger piece of machinery than `Scroll::draw` and this deserves its - own before/after on the phone. + again in the one frame, IRIS.md's entry). Done for `List` later the + same day: the walk outward from the anchor is now `List::lay_out`, and + `draw` runs it, asks `overscroll_gap` (a pure measurement, no painter + and no redraw handle) whether the layout landed off the end of the + content, and on a gap moves the anchor and runs the walk a second time + **inside the same frame**. `Painter::draw_again` had no other caller + and is gone with it, so there is now no "ask for a corrective frame" + mechanism in the framework at all. One further pass always settles it: + the gap is measured from the edges the walk actually placed, so moving + the anchor by it puts that edge exactly on the viewport's, and the + opposite end cannot open a new gap without the content being shorter + than the viewport, which `overscroll_gap` declines to touch. The extra + walk is paid only on an overscrolled frame and re-offers every row the + same box at a new offset, which `draw_inner` dispatches as an O(1) + move. Three tests draw no settling frame on purpose and fail without + the change: `fling_toward_the_start_stops_at_the_first_row`, + `scrolling_past_the_start_is_given_back_in_the_same_frame` (both in + `list.rs`) and `scrolling_past_the_first_row_settles_on_it` / + `scrolling_past_the_last_row_settles_on_it` (layer 1, + `transcript-fixture/tests/top_edge.rs`). - [x] **`request_device` asked for compute-shader limits it never uses (2026-09-05).** `Limits::default()` (both `iris/src/android/render.rs` @@ -1124,7 +1135,7 @@ do not duplicate it there. **past its own first row** (`fling_toward_the_start_stops_at_the_ first_row` was leaving it 1398px below a 600px viewport, a blank screen, and that test's own assertion could not see it). - `clamp_to_content` gives the gap back. Both ends: + the overscroll clamp gives the gap back. Both ends: `scrolling_past_the_first_row_settles_on_it`, `scrolling_past_the_last_row_settles_on_it`. This is also the first item of the later report below. @@ -1140,7 +1151,7 @@ do not duplicate it there. ## From the phone, 2026-09-07, later (build from 4274b8b, ai-app-bench b47eb73) - [x] **"You shouldn't be able to scroll below the bottom (or above - top)."** Done in e922b73, as `List::clamp_to_content` rather than as a + top)."** Done in e922b73, as a clamp in `List::draw` rather than as a clamp inside the scroll setter: nothing at the moment of a `scroll` call knows where the content ends (that is what walking the rows finds out), so the correction is measured from the ends the layout walk diff --git a/docs/RUST.md b/docs/RUST.md index 4796b78..0f851e5 100644 --- a/docs/RUST.md +++ b/docs/RUST.md @@ -654,7 +654,7 @@ a change landed the way it did. `fonts.xml` monospace declaration against fontique's actually-scanned families, Android-only, verified `mono=Some("Droid Sans Mono")` on this checkout's emulator. -- [x] Scroll clamped at both ends (e922b73, `List::clamp_to_content`) +- [x] Scroll clamped at both ends (e922b73, `List`'s overscroll clamp) and Compose's velocity estimator (docs/IRIS_TODO.md, 2026-09-07 later). Ticked 2026-09-08 against those entries, which were already `[x]` while this box was not. Two things this box's own wording had diff --git a/iris/core/src/ui/painter.rs b/iris/core/src/ui/painter.rs index b2091f4..816d8bf 100644 --- a/iris/core/src/ui/painter.rs +++ b/iris/core/src/ui/painter.rs @@ -166,21 +166,6 @@ impl<'a> Painter<'a> { self.mask = self.own_mask; } - /// Ask for this widget to be drawn again on the next frame, from - /// inside its own `draw` -- for a layout that can only discover a - /// correction to itself by laying out once (`List::clamp_to_content`, - /// which learns how far past its content the list is from the walk it - /// has just done). The mark is the same one `Widgets::get_dyn_mut` - /// sets, so `UiRenderState::update` picks it up exactly as it does any - /// other dirty widget; it does **not** by itself ask the platform for - /// a frame, which is the caller's own `RequestRedraw` handle. - /// - /// The correction it asks for must converge, or this is a widget that - /// redraws forever. - pub fn draw_again(&mut self) { - self.rsc.widgets_mut().needs_redraw.insert(self.id); - } - /// Whether anything is clipping what this widget draws -- its own /// [`Self::set_mask`], or one an ancestor set that it inherited. What /// a widget whose contents may legitimately extend past its own box diff --git a/iris/src/widget/list.rs b/iris/src/widget/list.rs index 286c4ff..8bf0052 100644 --- a/iris/src/widget/list.rs +++ b/iris/src/widget/list.rs @@ -104,12 +104,14 @@ //! that "whatever lies between" stays empty however far the list is //! panned. //! -//! **Overscroll is taken back on the next frame, never rubber-banded.** A -//! `scroll()` or a fling past the first or last row leaves a gap for one -//! frame; `clamp_to_content` measures it from the ends the walk already -//! placed and gives it back (the same one-frame-lag `Scroll`'s content -//! length has, per LAYOUT.md). A list shorter than its viewport is not -//! overscrolled and is left alone, still bottom-anchored. +//! **Overscroll is taken back within the frame, never rubber-banded.** A +//! `scroll()` or a fling past the first or last row leaves a gap; +//! `overscroll_gap` measures it from the ends the walk already placed, and +//! `draw` moves the anchor by it and walks a second time before the frame +//! ends -- layout is a pure function of the state, not of how many frames +//! have been drawn (Iris, 2026-09-08), the same rule `Scroll::draw` +//! follows. A list shorter than its viewport is not overscrolled and is +//! left alone, still bottom-anchored. use crate::prelude::*; use iris_core::util::HashMap; @@ -180,17 +182,31 @@ struct RowExtent { bottom: f32, } +/// Which of a row's edges the walk already knows, and where it is. The +/// two are symmetric -- one pinned edge plus a height gives the box -- +/// so `place` parameterises over them through [`Self::edges`] rather +/// than carrying a copy of the same logic per direction. +#[derive(Clone, Copy)] enum Placement { /// This row's leading edge is known; its trailing edge is wherever its - /// natural size puts it. Placed directly with `widget_within`. + /// own height puts it. Top(f32), - /// This row's trailing edge is known; its leading edge depends on a - /// size not yet learned. Placed by measuring at the full offered - /// region first, then `reposition`ing -- the same two-step `Aligned` - /// already uses for the identical problem. + /// This row's trailing edge is known; its leading edge is that height + /// back from it. Bottom(f32), } +impl Placement { + /// The `(leading, trailing)` edges this placement implies for a row + /// of `height`. + fn edges(self, height: f32) -> (f32, f32) { + match self { + Placement::Top(top) => (top, top + height), + Placement::Bottom(bottom) => (bottom - height, bottom), + } + } +} + /// A virtualised, bottom-anchored list of variable-height rows. See the /// module doc for the design. pub struct List { @@ -406,8 +422,9 @@ impl List { /// Move the anchor's edge by `amt` pixels. Positive moves later /// content into view (mirrors `Scroll::scroll`'s sign convention). /// Unclamped here, on purpose: it is one write, and there is nothing - /// at this point that knows where the content ends. The next `draw` - /// gives back whatever this moved past ([`Self::clamp_to_content`]). + /// at this point that knows where the content ends. The `draw` that + /// follows gives back whatever this moved past, in that same frame + /// ([`Self::overscroll_gap`]). pub fn scroll(&mut self, amt: f32) { if let Some(a) = &mut self.anchor { a.offset -= amt; @@ -804,14 +821,15 @@ impl List { }); } - /// Take back an empty band at one edge that content on the other side - /// of the viewport could fill -- the correction that makes a `scroll` - /// or a fling past the end of the content settle *on* the end rather - /// than beyond it. + /// The empty band at one edge that content on the other side of the + /// viewport could fill -- positive to move content toward the leading + /// edge -- or `None` when the layout already sits on its content. + /// This is what makes a `scroll` or a fling past the end of the + /// content settle *on* the end rather than beyond it. /// - /// `top`/`bottom` are the extreme edges this frame's walk actually - /// placed, so the gap is already measured: `at_start` means nothing is - /// above `top`, and if `top` is nevertheless below the viewport's own + /// `top`/`bottom` are the extreme edges the walk actually placed, so + /// the gap is already measured: `at_start` means nothing is above + /// `top`, and if `top` is nevertheless below the viewport's own /// leading edge then those pixels are empty and always will be. This /// is the whole of what the module doc used to list as deliberately /// unsolved ("no overscroll clamping ... nothing to measure how much @@ -829,19 +847,9 @@ impl List { /// the space is not overscroll at all -- it is a bottom-anchored list /// with three rows in it, and pulling those to the top would be this /// widget rejecting its own default (`repair_anchor`). - /// - /// Applied to the anchor, so it lands on the *next* frame rather than - /// re-running this one -- which means one frame is drawn with the - /// content past its own end, and **that is a deviation from the rule - /// that layout is a pure function of the state rather than of how - /// many frames have been drawn** (Iris, 2026-09-08; `Scroll::draw` - /// used to lag the same way and no longer does, so this is now the - /// only place left). docs/IRIS_TODO.md carries it; the fix is the - /// same shape as `Scroll`'s -- re-place within this frame instead of - /// marking the next one. - fn clamp_to_content(&mut self, painter: &mut Painter, top: f32, bottom: f32) { + fn overscroll_gap(&self, top: f32, bottom: f32) -> Option { if self.at_start == self.at_end { - return; + return None; } // `at_start`/`at_end` already carry the sign of their own gap // (`top >= 0.0`, `bottom <= viewport_len`), so this is the gap @@ -852,21 +860,76 @@ impl List { bottom - self.viewport_len }; // Sub-pixel gaps are what floating-point row heights leave behind - // every frame; correcting one would ask for another frame, which - // would leave another, and the list would never stop redrawing. - if gap.abs() < 0.5 { - return; + // every frame; laying out again for one would leave another, and + // the list would never settle. + (gap.abs() >= 0.5).then_some(gap) + } + + /// Place every row that reaches the viewport, outward from the + /// anchor, and return the extreme `(leading, trailing)` edges the walk + /// reached. Rebuilds `extents` and `at_start`/`at_end` from what it + /// placed; the caller clears `extents` first, since `reanchor_at_tap` + /// reads the previous frame's copy. + /// + /// Called a second time in the same `draw` when the first pass lands + /// off the end of the content -- see [`Self::overscroll_gap`] and + /// `draw`. + fn lay_out(&mut self, painter: &mut Painter) -> (f32, f32) { + let anchor = self + .anchor + .expect("lay_out with no anchor: `draw` returns before this without one"); + let placement = match anchor.edge { + Edge::Top => Placement::Top(anchor.offset), + Edge::Bottom => Placement::Bottom(anchor.offset), + }; + let (mut top, mut bottom) = self.place(painter, anchor.slot, placement); + + let mut idx_top = anchor.slot; + while top > 0.0 { + let Some(prev) = self.prev_slot(idx_top) else { + break; + }; + let (t, _) = self.place(painter, prev, Placement::Bottom(top)); + top = t; + idx_top = prev; } - self.scroll(gap); - // Nothing else will ask: the frame this correction was discovered - // in has already been laid out, and a fling that ran out at an end - // (`tick_fling`'s `hit_bound`) has stopped requesting frames -- - // which is exactly the case that left the list parked past its own - // first row. - painter.draw_again(); - if let Some(redraw) = &self.redraw { - redraw.request_redraw(); + + let mut idx_bottom = anchor.slot; + while bottom < self.viewport_len { + let Some(next) = self.next_slot(idx_bottom) else { + break; + }; + let (_, b) = self.place(painter, next, Placement::Top(bottom)); + bottom = b; + idx_bottom = next; } + + // What `tick_fling` clamps a fling against -- see `at_start`'s + // field doc. `top`/`bottom` are the extreme edges actually placed + // this frame, and `prev_slot`/`next_slot` returning `None` is what + // "no more content" means everywhere else in this widget. + self.at_start = self.prev_slot(idx_top).is_none() && top >= 0.0; + self.at_end = self.next_slot(idx_bottom).is_none() && bottom <= self.viewport_len; + + // Both halves of `intersects_viewport`'s rule, checked where they + // are cheap to check: what this pass put on screen is exactly what + // overlaps the viewport, and nothing above or below it can be + // seen. The first failed silently for a whole build -- an + // off-screen row draws correctly, it is just in the wrong place. + // `assert!` for R1's reason: it walks the rows *on screen*, a + // handful, once per draw, and a release build is the only build + // this fault has ever been seen in. + assert!( + self.extents + .values() + .all(|e| self.intersects_viewport(e.top, e.bottom)), + "a row outside the viewport (0..{}) is recorded as on screen: {:?}", + self.viewport_len, + self.extents + .values() + .find(|e| !self.intersects_viewport(e.top, e.bottom)), + ); + (top, bottom) } fn update_snap_end(&mut self) { @@ -985,71 +1048,60 @@ impl List { // below is recorded from the intersection test rather than from // "was this drawn". if let Some(h) = cached { - let (top, bottom) = match placement { - Placement::Top(top) => (top, top + h), - Placement::Bottom(bottom) => (bottom - h, bottom), - }; + let (top, bottom) = placement.edges(h); if !self.intersects_viewport(top, bottom) { return (top, bottom); } } - let (top, bottom, height) = match (placement, cached) { - (Placement::Top(top), Some(h)) => { - // Offered a box sized to the *cached* height (cheap to - // compare against last frame's offer, see `place`'s doc), - // but the returned/recorded height comes from what this - // draw actually reported -- if the row's real content grew - // since it was cached (and was therefore redrawn: an - // unchanged widget never disagrees with its own cache), - // this frame already reflects the new size rather than - // waiting a frame to self-correct. - let region = Self::abs_region(axis, top, top + h); - let used = painter.widget_within(self.slot_widget(slot), region); + let widget = self.slot_widget(slot); + let height = match cached { + // Offered a box sized to the *cached* height (cheap to compare + // against last frame's offer, see `place`'s doc), but the + // height kept is what this draw actually reported -- if the + // row's real content grew since it was cached (and was + // therefore redrawn: an unchanged widget never disagrees with + // its own cache), it is drawn again here, this frame, at the + // box its own height implies rather than waiting a frame to + // self-correct. + Some(h) => { + let (top, bottom) = placement.edges(h); + let used = painter.widget_within(widget, Self::abs_region(axis, top, bottom)); let height = resolve(used); if height != h { - let corrected = Self::abs_region(axis, top, top + height); - painter.widget_within(self.slot_widget(slot), corrected); + let (top, bottom) = placement.edges(height); + painter.widget_within(widget, Self::abs_region(axis, top, bottom)); } - (top, top + height, height) + height } - (Placement::Top(top), None) => { - let first = Self::abs_region(axis, top, top + GENEROUS_PADDING); + // Never measured, so there is no height to place it at: it is + // measured at an oversized region first and drawn again at the + // box that measurement implies (`draw_twice`, not + // `reposition`, which writes an offset and never a size). + // + // A bottom-known row measures at a *zero-anchored* region + // rather than at its own box: using the real box would make + // the measurement's offered size track this list's own height, + // so a sibling growing taller (the input-box case) would look + // like a resize to every bottom-known row and force a full + // redraw of each -- despite a row's content depending only on + // width. + None => { + let measure_from = match placement { + Placement::Top(top) => top, + Placement::Bottom(_) => 0.0, + }; + let first = Self::abs_region(axis, measure_from, measure_from + GENEROUS_PADDING); let mut height = 0.0; - painter.draw_twice(self.slot_widget(slot), first, |used| { + painter.draw_twice(widget, first, |used| { height = resolve(used); - Self::abs_region(axis, top, top + height) + let (top, bottom) = placement.edges(height); + Self::abs_region(axis, top, bottom) }); - (top, top + height, height) - } - (Placement::Bottom(bottom), Some(h)) => { - let region = Self::abs_region(axis, bottom - h, bottom); - let used = painter.widget_within(self.slot_widget(slot), region); - let height = resolve(used); - if height != h { - let corrected = Self::abs_region(axis, bottom - height, bottom); - painter.widget_within(self.slot_widget(slot), corrected); - } - (bottom - height, bottom, height) - } - (Placement::Bottom(bottom), None) => { - // Measured at a fixed, zero-anchored region rather than - // `painter.region()` (the list's *actual* offered box): - // using the real box would make the measurement's offered - // *size* track this list's own height, so a sibling - // growing taller (the input-box case) would look like a - // resize to every bottom-known row and force a full - // redraw of each -- despite a row's content depending - // only on width. - let first = Self::abs_region(axis, 0.0, GENEROUS_PADDING); - let mut height = 0.0; - painter.draw_twice(self.slot_widget(slot), first, |used| { - height = resolve(used); - Self::abs_region(axis, bottom - height, bottom) - }); - (bottom - height, bottom, height) + height } }; + let (top, bottom) = placement.edges(height); if let Some(k) = key { self.heights.insert(k, height); // `extents` is what is *on screen* (`key_at`'s doc, and @@ -1115,10 +1167,10 @@ impl Widget for List { self.viewport_len = painter.region().axis(axis).len().to_abs(output_len); self.repair_anchor(); - let Some(mut anchor) = self.anchor else { + if self.anchor.is_none() { self.extents.clear(); return Size::REST; - }; + } // `reanchor_at_tap` reads `extents` as they stood after the // *previous* frame's layout -- the last on-screen box for each @@ -1126,63 +1178,38 @@ impl Widget for List { // frame has to wait until after this call, not before. if let Some(tap) = self.pending_tap.take() { self.reanchor_at_tap(tap); - anchor = self.anchor.unwrap(); } self.extents.clear(); - let placement = match anchor.edge { - Edge::Top => Placement::Top(anchor.offset), - Edge::Bottom => Placement::Bottom(anchor.offset), - }; - let (mut top, mut bottom) = self.place(painter, anchor.slot, placement); + let (top, bottom) = self.lay_out(painter); - let mut idx_top = anchor.slot; - while top > 0.0 { - let Some(prev) = self.prev_slot(idx_top) else { - break; - }; - let (t, _) = self.place(painter, prev, Placement::Bottom(top)); - top = t; - idx_top = prev; + // **The clamp is applied inside the frame that found it**, not + // marked for the next one: layout is a pure function of the state + // rather than of how many frames have been drawn (Iris, + // 2026-09-08), and a correction that lands next frame is a frame + // drawn wrong -- with nothing guaranteed to ask for that next + // frame, since a fling that ran out at an end has already stopped + // requesting them, which is exactly what left the list parked past + // its own first row. Same shape as `Scroll::draw`, which measures + // its content and places it again in the one frame. + // + // One further pass settles it, always: the gap is measured from + // the edges this walk actually placed, so moving the anchor by it + // puts that edge exactly on the viewport's, and the rows the + // second walk brings into view are placed outward from there. The + // opposite end cannot open a new gap -- that would mean the + // content is shorter than the viewport, which `overscroll_gap` + // already declines to touch. The extra walk is paid only on a + // frame that was overscrolled, and it re-offers every row the same + // box at a new offset, which `draw_inner` dispatches as an O(1) + // move. + if let Some(gap) = self.overscroll_gap(top, bottom) { + self.scroll(gap); + self.extents.clear(); + self.lay_out(painter); } - let mut idx_bottom = anchor.slot; - while bottom < self.viewport_len { - let Some(next) = self.next_slot(idx_bottom) else { - break; - }; - let (_, b) = self.place(painter, next, Placement::Top(bottom)); - bottom = b; - idx_bottom = next; - } - - // What `tick_fling` clamps a fling against -- see `at_start`'s - // field doc. `top`/`bottom` are the extreme edges actually placed - // this frame, and `prev_slot`/`next_slot` returning `None` is what - // "no more content" means everywhere else in this widget. - self.at_start = self.prev_slot(idx_top).is_none() && top >= 0.0; - self.at_end = self.next_slot(idx_bottom).is_none() && bottom <= self.viewport_len; - - // Both halves of `intersects_viewport`'s rule, checked where they - // are cheap to check: what this frame put on screen is exactly - // what overlaps the viewport, and nothing above or below it can - // be seen. The first failed silently for a whole build -- an - // off-screen row draws correctly, it is just in the wrong place. - // `assert!` for R1's reason: it walks the rows *on screen*, a - // handful, once per draw, and a release build is the only build - // this fault has ever been seen in. - assert!( - self.extents - .values() - .all(|e| self.intersects_viewport(e.top, e.bottom)), - "a row outside the viewport (0..{}) is recorded as on screen: {:?}", - self.viewport_len, - self.extents - .values() - .find(|e| !self.intersects_viewport(e.top, e.bottom)), - ); self.rehome_anchor(); - self.clamp_to_content(painter, top, bottom); self.update_snap_end(); Size::REST } @@ -1256,7 +1283,7 @@ mod tests { /// The case the top-edge cull and the overscroll clamp both had no /// reason to touch: fewer rows than fit. Every one of them is drawn - /// (nothing here is outside the viewport), and `clamp_to_content` + /// (nothing here is outside the viewport), and `overscroll_gap` /// leaves the list bottom-anchored -- the gap above the first row is /// not overscroll, it is where this widget puts a short list, and /// pulling it to the top would be the clamp overriding @@ -1605,8 +1632,9 @@ mod tests { // Backwards, into content that exists: a list opens flush with // its newest end, so scrolling *forward* from there is - // overscroll, and `clamp_to_content` lays out a second time to - // give it back -- a correct extra pass, but not the ordinary + // overscroll, and the clamp lays out a second time within the + // frame to give it back -- a correct extra pass, but not the + // ordinary // scroll tick whose cost this test is about. rsc.ui.widgets.get_mut(&list_weak).unwrap().scroll(-5.0); render.update(&root, &mut rsc); @@ -2146,6 +2174,34 @@ mod tests { assert_eq!((before.top, before.bottom), (after.top, after.bottom)); } + /// The clamp on the frame that discovers it, for an ordinary + /// `scroll` rather than a fling: one `update` after moving 100,000px + /// past the first row, and the list is already flush with the top. + /// The old code left that frame drawn with the whole screen blank and + /// snapped back on the next one, so this fails on it -- it draws no + /// settling frame on purpose (the same shape as + /// `phone_screen.rs`'s composer test). + #[test] + fn scrolling_past_the_start_is_given_back_in_the_same_frame() { + let mut rsc = TestRsc { + ui: UiData::default(), + }; + let (list_weak, root, mut render) = build_flingable_list(&mut rsc); + rsc.ui + .widgets + .get_mut(&list_weak) + .unwrap() + .scroll(-100_000.0); + render.update(&root, &mut rsc); + let list_ref = rsc.ui.widgets.get(&list_weak).unwrap(); + let first = list_ref.extents[&0]; + assert!( + first.top.abs() < 0.5, + "the frame that overscrolled should end flush with the first row, not {}px from it", + first.top, + ); + } + #[test] fn fling_toward_the_start_stops_at_the_first_row() { let mut rsc = TestRsc { @@ -2165,10 +2221,11 @@ mod tests { break; } } - // The frame that gives back whatever the fling's last step spent - // past the first row -- `clamp_to_content` writes the anchor at - // the end of a draw, so it lands on the next one. - render.update(&root, &mut rsc); + // No settling frame: the draw that discovers the fling ran past + // the first row gives those pixels back inside that same frame + // (`overscroll_gap`), so the last frame the loop above drew is + // already flush with the top. Drawing one more here would hide a + // regression to the old next-frame correction. let list_ref = rsc.ui.widgets.get(&list_weak).unwrap(); assert!( list_ref.at_start, diff --git a/iris/transcript-fixture/tests/top_edge.rs b/iris/transcript-fixture/tests/top_edge.rs index d1d34b7..e71035f 100644 --- a/iris/transcript-fixture/tests/top_edge.rs +++ b/iris/transcript-fixture/tests/top_edge.rs @@ -279,8 +279,8 @@ fn the_row_across_the_bottom_edge_is_drawn() { /// Panning past the first row settles *on* it rather than beyond it. The /// list is scrolled far further back than the fixture is long, which is -/// what a hard fling toward the top does; before `List::clamp_to_content` -/// it stayed wherever that left it -- the phone's "black from the header +/// what a hard fling toward the top does; before the clamp existed it +/// stayed wherever that left it -- the phone's "black from the header /// down", and a whole blank screen in `iris`'s own /// `fling_toward_the_start_stops_at_the_first_row`. #[test] @@ -292,10 +292,10 @@ fn scrolling_past_the_first_row_settles_on_it() { for _ in 0..60 { t = scrolled(&mut h, &screen, -100_000.0, t); } - // The correction is written at the end of a draw and lands on the - // next one. - h.frame(t); - + // No settling frame on purpose: the draw that discovers the gap gives + // it back inside that same frame (`List::overscroll_gap`), so the last + // frame `scrolled` drew is already flush with the first row. Adding + // one here would hide a regression to the old next-frame correction. let rows = drawn_rows(&h, &screen); let first = *rows.first().expect("the first row is on screen"); assert!( @@ -318,7 +318,6 @@ fn scrolling_past_the_last_row_settles_on_it() { for _ in 0..20 { t = scrolled(&mut h, &screen, 100_000.0, t); } - h.frame(t); let rows = drawn_rows(&h, &screen); let last = *rows.last().expect("the last row is on screen");