diff --git a/crates/moon-gpui/src/elements/uniform_list.rs b/crates/moon-gpui/src/elements/uniform_list.rs index 0a33145..fb77183 100644 --- a/crates/moon-gpui/src/elements/uniform_list.rs +++ b/crates/moon-gpui/src/elements/uniform_list.rs @@ -42,6 +42,7 @@ where item_count, item_to_measure_index: 0, render_items: Box::new(render_range), + on_visible_range: None, decorations: Vec::new(), interactivity: Interactivity { element_id: Some(id), @@ -61,6 +62,7 @@ pub struct UniformList { render_items: Box< dyn for<'a> Fn(Range, &'a mut Window, &'a mut App) -> SmallVec<[AnyElement; 64]>, >, + on_visible_range: Option Fn(Range, &'a mut Window, &'a mut App)>>, decorations: Vec>, interactivity: Interactivity, scroll_handle: Option, @@ -479,14 +481,38 @@ impl Element for UniformList { let visible_range = first_visible_element_ix ..cmp::min(last_visible_element_ix, self.item_count); + let rendered_range = if y_flipped { + self.item_count.saturating_sub(visible_range.end) + ..self.item_count.saturating_sub(visible_range.start) + } else { + visible_range.clone() + }; + + // Report exactly what is on screen. Showing nothing while rows EXIST is not + // "no rows" — it is a collapsed panel, a splitter mid-drag, or a first frame + // before sizing; saying `0..0` there would be indistinguishable from the + // emptied-list report below, and a consumer evicting state outside the range + // would drop the focused row every frame its container is squeezed shut. + // + // Two conditions, because neither alone is that question. The rendered range + // can be non-empty at zero height whenever the scroll offset is not a whole + // multiple of a row (`floor`/`ceil` then straddle one row that has no pixels), + // and the PADDED box is zero for a merely padded list whose row is plainly + // visible — Taffy floors the element at its own padding, so `bounds` is the + // height that says whether anything is shown at all. + if bounds.size.height > Pixels::ZERO + && !rendered_range.is_empty() + && let Some(on_visible_range) = self.on_visible_range.as_ref() + { + on_visible_range(rendered_range.clone(), window, cx); + } + let items = if y_flipped { - let flipped_range = self.item_count.saturating_sub(visible_range.end) - ..self.item_count.saturating_sub(visible_range.start); - let mut items = (self.render_items)(flipped_range, window, cx); + let mut items = (self.render_items)(rendered_range, window, cx); items.reverse(); items } else { - (self.render_items)(visible_range.clone(), window, cx) + (self.render_items)(rendered_range, window, cx) }; let content_mask = ContentMask { bounds }; @@ -531,6 +557,11 @@ impl Element for UniformList { frame_state.decorations.push(decoration); } }); + } else if let Some(on_visible_range) = self.on_visible_range.as_ref() { + // An emptied list still reports, with the empty range it now draws. A consumer + // that evicts state for rows outside the reported range needs this frame most + // of all: it is the frame where every row it was tracking stopped existing. + on_visible_range(0..0, window, cx); } hitbox @@ -649,6 +680,42 @@ impl UniformList { self } + /// Observes the item range this list draws. + /// + /// The observer is handed the very range that goes to the item renderer — flipped indices + /// included when `y_flipped` is set, so it always speaks the renderer's index space, while + /// [`UniformListDecoration::compute`] keeps receiving unflipped positions. A list with no + /// items reports `0..0` rather than staying silent; a list that HAS items but draws none of + /// them — no room, mid-collapse, not yet sized — stays silent instead, so "there is nothing + /// to show" and "there is no room to show it in" never arrive as the same range. + /// + /// It is deliberately a channel of its own rather than a hook inside the item renderer: that + /// closure is ALSO invoked to measure a single item — `measure_item` renders + /// `item_to_measure_index..+1` from both `request_layout` and `prepaint`, before the real + /// range exists — so an observer wired through the renderer sees a phantom one-item range + /// twice before every real one. An observer that evicts state outside the range it is given + /// (focus, an open popup) would then evict everything below the measured item on every frame. + /// + /// What the runtime does NOT promise, because the observer runs from `prepaint`: + /// + /// - It fires once per prepaint of this element, not once per drawn frame. A frame that never + /// prepaints the list — an ancestor `AnyView::cached` hit — reports nothing, and a prepaint + /// pass that is retried and discarded (`Window::transact`) reports more than once. + /// - `cx.notify()` is silent for an entity held by THIS window while it draws + /// (`Invalidator::invalidate_view` bails unless `draw_phase == None`), and `Window::refresh` + /// is silent on its own `not_drawing()` guard, so state the observer changes reaches the + /// screen on the next frame something else schedules. `Window::blur` still takes effect, and + /// an entity with no drawing window of its own still notifies. + /// - It runs after the deferred `scroll_to_item` for this frame has been applied, so a scroll + /// the observer requests lands on the next frame. + pub fn on_visible_range( + mut self, + on_visible_range: impl 'static + for<'a> Fn(Range, &'a mut Window, &'a mut App), + ) -> Self { + self.on_visible_range = Some(Box::new(on_visible_range)); + self + } + /// Adds a decoration element to the list. pub fn with_decoration(mut self, decoration: impl UniformListDecoration + 'static) -> Self { self.decorations.push(Box::new(decoration)); @@ -862,4 +929,52 @@ mod test { }) } } + + /// Catches wiring an `on_visible_range` observer through the item renderer instead of its own + /// channel. The renderer is also called to MEASURE one item, twice per frame, so an observer + /// living there sees a phantom `0..1` before every real range — and a consumer that evicts row + /// state outside the reported range (keyboard focus, an open popup) then evicts every row but + /// the measured one, on every frame. + #[gpui::test] + fn visible_range_observer_skips_the_measured_item(cx: &mut TestAppContext) { + use crate::{Context, IntoElement, Render, Window, div, prelude::*, px, uniform_list}; + use std::{cell::RefCell, ops::Range, rc::Rc}; + + struct TestView { + ranges: Rc>>>, + } + + impl Render for TestView { + fn render( + &mut self, + _window: &mut Window, + _cx: &mut Context, + ) -> impl IntoElement { + let sink = self.ranges.clone(); + div().w(px(100.0)).h(px(200.0)).child( + uniform_list("probe", 50, |range: Range, _window, _cx| { + range + .map(|ix| div().h(px(20.0)).child(format!("row {ix}"))) + .collect::>() + }) + .size_full() + .on_visible_range(move |range, _window, _cx| sink.borrow_mut().push(range)), + ) + } + } + + let ranges = Rc::new(RefCell::new(Vec::new())); + let sink = ranges.clone(); + let window = cx.add_window(move |_, _| TestView { ranges: sink }); + // Opening the window already drew a frame; measure exactly the one drawn below. + ranges.borrow_mut().clear(); + cx.update_window(window.into(), |_, window, cx| { + window.draw(cx).clear(); + }) + .unwrap(); + + // 200 px of viewport over 20 px rows, unscrolled: rows 0..10 are drawn, and that is the + // only thing the observer may hear about. + assert_eq!(*ranges.borrow(), vec![0..10]); + } } diff --git a/crates/moon-ui-components/component-manifest.json b/crates/moon-ui-components/component-manifest.json index 7f03be6..bc637d9 100644 --- a/crates/moon-ui-components/component-manifest.json +++ b/crates/moon-ui-components/component-manifest.json @@ -736,7 +736,10 @@ "public_path": "moon_ui::MoonVirtualList", "upstream_ref": null, "fork_reason": "Moon virtualized terminal list styling", - "contracts": ["gallery.visual_coverage"] + "contracts": [ + "virtual_list.visible_range_reporting", + "gallery.visual_coverage" + ] }, { "concept": "window_frame", diff --git a/crates/moon-ui-components/src/moon/virtual_list.rs b/crates/moon-ui-components/src/moon/virtual_list.rs index 60d69bf..18c10e1 100644 --- a/crates/moon-ui-components/src/moon/virtual_list.rs +++ b/crates/moon-ui-components/src/moon/virtual_list.rs @@ -77,6 +77,19 @@ impl MoonVirtualList { self } + /// Observe the item range the list actually draws. + /// + /// Every reported range is a range the list renders: never the single row it measured to get + /// a row height, and, for a flipped list, always in the item renderer's own index space. A + /// list holding no rows reports `0..0`, while a list that holds rows but shows none of them — + /// a collapsed panel, a frame before sizing — reports nothing at all, so a squeezed list is + /// never mistaken for an empty one. An observer may therefore evict state belonging to rows + /// outside the range (keyboard focus, an open popup) without first proving the range is real. + /// + /// It reports per prepaint of the list, which is not the same as per frame: a cached ancestor + /// view can skip it, and a retried prepaint pass can repeat it. Keep the observer an + /// idempotent assignment, and note that a repaint it asks for (`cx.notify`, `Window::refresh`) + /// is dropped mid-draw — see [`gpui::UniformList::on_visible_range`] for the full contract. pub fn on_visible_range( mut self, on_visible_range: impl 'static + Fn(Range, &mut Window, &mut App), @@ -191,15 +204,18 @@ impl RenderOnce for MoonVirtualList { let list_id = ElementId::from(SharedString::from(format!("{}:list", id))); let mut list = uniform_list(list_id, self.item_count, move |range, window, cx| { - if let Some(on_visible_range) = &on_visible_range { - on_visible_range(range.clone(), window, cx); - } Self::render_range(&render_item, item_height, range, window, cx) }) .size_full() .p(px(self.padding)) .track_scroll(&scroll_handle) .y_flipped(self.y_flipped); + // Through the list's own observer channel, NOT from the item renderer above: that closure + // is also used to measure one item, so an observer inside it fires on a phantom + // single-item range twice per frame before the real one ever arrives. + if let Some(on_visible_range) = on_visible_range { + list = list.on_visible_range(on_visible_range); + } if self.surface { list = self.background_policy.apply(list, p.shell_high, 0.98); } @@ -257,3 +273,6 @@ impl UniformListDecoration for MoonVirtualListTailFill { .into_any_element() } } + +#[cfg(test)] +mod tests; diff --git a/crates/moon-ui-components/src/moon/virtual_list/tests.rs b/crates/moon-ui-components/src/moon/virtual_list/tests.rs new file mode 100644 index 0000000..70f1c3a --- /dev/null +++ b/crates/moon-ui-components/src/moon/virtual_list/tests.rs @@ -0,0 +1,219 @@ +//! Regression coverage for the range `MoonVirtualList` reports to `on_visible_range`. + +use std::cell::RefCell; +use std::ops::Range; +use std::rc::Rc; + +use super::MoonVirtualList; + +/// Catches wiring the observer back into the item renderer in `virtual_list.rs`, the way it was +/// wired before: `uniform_list` renders `item_to_measure_index..+1` to MEASURE a row, from both +/// `request_layout` and `prepaint`, so an observer inside the renderer receives `0..1` twice per +/// frame before the real range. A consumer that evicts row state outside the reported range then +/// evicts every row but the first on every frame — in MoonTerminal's Connections tab that blurred +/// the core-name field one frame after the click, so the row could not be renamed at all. +#[gpui::test] +fn visible_range_observer_never_sees_the_measured_row(cx: &mut gpui::TestAppContext) { + let ranges = observed_ranges(cx, 50, 200.0, 0.0, false, None, None); + + assert_eq!( + ranges.len(), + 1, + "the observer must run once per drawn frame, got {ranges:?}" + ); + // 200 px of viewport over 20 px rows: rows 0..10 are the ones actually drawn. + assert_eq!(ranges[0], 0..10); +} + +/// Catches reporting `visible_range` instead of the range handed to the item renderer when the +/// list is flipped: with 50 rows the flipped list draws items `0..10` while its unflipped window +/// is `40..50`, so a consumer indexing its own data by the reported range would address the wrong +/// end of the list entirely. +#[gpui::test] +fn flipped_list_reports_the_range_it_renders(cx: &mut gpui::TestAppContext) { + let ranges = observed_ranges(cx, 50, 200.0, 0.0, true, None, None); + + assert_eq!( + ranges.len(), + 1, + "the observer must run once per drawn frame, got {ranges:?}" + ); + assert_eq!(ranges[0], 0..10); +} + +/// Catches reporting the first N rows instead of the range under the scroll offset — the case the +/// two unscrolled tests above cannot separate. A consumer evicting row state outside the report +/// would then evict every row the user has actually scrolled to. +#[gpui::test] +fn scrolled_list_reports_the_rows_under_the_offset(cx: &mut gpui::TestAppContext) { + let ranges = observed_ranges(cx, 50, 200.0, 0.0, false, Some(12), None); + + assert_eq!( + ranges.len(), + 1, + "the observer must report once per prepaint, got {ranges:?}" + ); + // Row 12 pulled to the top of a 200 px viewport of 20 px rows: rows 12..22 are drawn. + assert_eq!(ranges[0], 12..22); +} + +/// Catches leaving the report inside the `item_count > 0` branch of `uniform_list.rs`: a list that +/// drops to zero rows would then never report, and a consumer would keep focus and open popups +/// pinned to rows that no longer exist. Drives the real transition — a populated frame, then the +/// frame that empties it — because that second frame is the whole reason the branch exists. +#[gpui::test] +fn emptied_list_still_reports_its_empty_range(cx: &mut gpui::TestAppContext) { + use gpui::AppContext as _; + + cx.update(crate::init); + let ranges = Rc::new(RefCell::new(Vec::new())); + let sink = ranges.clone(); + let window = cx.add_window(move |_, _| ListHarness { + item_count: 50, + height: 200.0, + padding: 0.0, + y_flipped: false, + scroll: super::MoonVirtualListScrollHandle::new(), + ranges: sink, + }); + cx.update_window(window.into(), |_, window, cx| { + window.draw(cx).clear(); + }) + .unwrap(); + assert_eq!( + ranges.borrow().last().cloned(), + Some(0..10), + "the populated frame must report its rows first" + ); + + ranges.borrow_mut().clear(); + window + .update(cx, |view, _window, _cx| view.item_count = 0) + .unwrap(); + cx.update_window(window.into(), |_, window, cx| { + window.draw(cx).clear(); + }) + .unwrap(); + + assert_eq!(*ranges.borrow(), vec![0..0]); +} + +/// Catches gating the report on the PADDED viewport height instead of on the element's own. +/// Taffy floors the list at its own padding, so a padded list is 40 px tall with a padded box of +/// exactly zero — the boundary a `> 0` test on that box fails — while the content mask still +/// shows a row, and a consumer would be told nothing while the user looks straight at it. +#[gpui::test] +fn padded_list_still_reports_the_row_it_draws(cx: &mut gpui::TestAppContext) { + // 20 px of padding floors the element at 40 px and its padded box at 0, yet scrolling row 12 + // to the top leaves first = floor((240 - 20) / 20) = 11 and last = ceil((240 + 0) / 20) = 12. + let ranges = observed_ranges(cx, 50, 30.0, 20.0, false, Some(12), None); + + assert_eq!(ranges, vec![11..12]); +} + +/// Catches reporting `0..0` for a list that still holds rows but is laid out at zero height — a +/// collapsed dock panel, a splitter mid-drag, the frame before first sizing. That report is +/// indistinguishable from the emptied-list one above, so a consumer would blur the row the user is +/// typing into every frame its container is squeezed shut. +#[gpui::test] +fn collapsed_list_reports_nothing_at_all(cx: &mut gpui::TestAppContext) { + let aligned = observed_ranges(cx, 50, 0.0, 0.0, false, None, None); + assert!(aligned.is_empty(), "expected silence, got {aligned:?}"); + + // Off a row boundary the computed range is NOT empty — floor(250/20) = 12 and + // ceil(250/20) = 13 straddle a row that has no pixels on screen — so an emptiness test alone + // would let a collapsed list report `12..13` and evict every other row. + let straddling = observed_ranges(cx, 50, 0.0, 0.0, false, None, Some(-250.0)); + assert!( + straddling.is_empty(), + "expected silence off a row boundary, got {straddling:?}" + ); +} + +/// Root view drawing one virtual list of 20-pixel rows and recording every reported range. +struct ListHarness { + item_count: usize, + height: f32, + padding: f32, + y_flipped: bool, + scroll: super::MoonVirtualListScrollHandle, + ranges: Rc>>>, +} + +impl gpui::Render for ListHarness { + /// Draw the list inside a fixed box so the visible row count is a property of the test, not of + /// the test window's size. + fn render( + &mut self, + _window: &mut gpui::Window, + _cx: &mut gpui::Context, + ) -> impl gpui::IntoElement { + use gpui::{ParentElement as _, Styled as _}; + + let sink = self.ranges.clone(); + gpui::div() + .w(gpui::px(300.0)) + .h(gpui::px(self.height)) + .flex() + .child( + MoonVirtualList::new("probe", self.item_count, 20.0, |ix, _window, _cx| { + gpui::div().child(format!("row {ix}")) + }) + // No border and no surface: an unpadded caller's viewport is then exactly the + // height it passes in, instead of that height minus a one-pixel outline. + .border(false) + .surface(false) + .padding(self.padding) + .track_scroll(&self.scroll) + .y_flipped(self.y_flipped) + .on_visible_range(move |range, _window, _cx| sink.borrow_mut().push(range)), + ) + } +} + +/// Draw one frame of the harness and return the ranges its observer was handed. +fn observed_ranges( + cx: &mut gpui::TestAppContext, + item_count: usize, + height: f32, + padding: f32, + y_flipped: bool, + scroll_to_row: Option, + scroll_offset_y: Option, +) -> Vec> { + use gpui::AppContext as _; + + cx.update(crate::init); + let ranges = Rc::new(RefCell::new(Vec::new())); + let sink = ranges.clone(); + let scroll = super::MoonVirtualListScrollHandle::new(); + let scroll_for_view = scroll.clone(); + let window = cx.add_window(move |_, _| ListHarness { + item_count, + height, + padding, + y_flipped, + scroll: scroll_for_view, + ranges: sink, + }); + if let Some(first_row) = scroll_to_row { + scroll.scroll_to_item(first_row, gpui::ScrollStrategy::Top); + } + if let Some(offset_y) = scroll_offset_y { + scroll + .0 + .borrow() + .base_handle + .set_offset(gpui::point(gpui::px(0.0), gpui::px(offset_y))); + } + // Opening the window already draws a frame; drop what it reported so the assertions below + // describe exactly one frame. + ranges.borrow_mut().clear(); + cx.update_window(window.into(), |_, window, cx| { + window.draw(cx).clear(); + }) + .unwrap(); + + let observed = ranges.borrow().clone(); + observed +} diff --git a/docs/MOON_PATCH_QUEUE.md b/docs/MOON_PATCH_QUEUE.md index 25004c0..7e08f2a 100644 --- a/docs/MOON_PATCH_QUEUE.md +++ b/docs/MOON_PATCH_QUEUE.md @@ -47,6 +47,14 @@ cargo xtask transform --zed-tag v0.0.0 --zed-path R:\test\_zed_gpui_base_84b753 - regular pointer tooltips require 800 ms of continuous hover, expire after five visible seconds, and stay suppressed until pointer re-entry; hoverable tooltips remain persistent while either their trigger or content is hovered. + - `UniformList::on_visible_range`: a channel for observing the item range the + list draws. The renderer closure cannot serve as one — `measure_item` runs + it on a single item, twice per frame, before the real range exists — so a + consumer wired through the renderer sees a phantom one-item range. A + re-sync drops the field, the builder and its call in `prepaint`; restore + all three, and with them both halves of the reporting rule: an empty list + reports `0..0`, while a list that holds rows but renders none of them + stays silent. 2. Zed bugfix candidates kept separate from `gpu_canvas` when possible: - Windows DPI/restore-bounds behavior diff --git a/xtask/src/component_audit/contracts.rs b/xtask/src/component_audit/contracts.rs index 0f2848d..df21ac2 100644 --- a/xtask/src/component_audit/contracts.rs +++ b/xtask/src/component_audit/contracts.rs @@ -397,6 +397,20 @@ pub(super) fn contract_checks(root: &Path) -> Result> { &tests, "toggle click handling must respect disabled and controlled state", ), + test_contract( + "virtual_list.visible_range_reporting", + ContractSeverity::Guardrail, + &[ + "visible_range_observer_never_sees_the_measured_row", + "flipped_list_reports_the_range_it_renders", + "scrolled_list_reports_the_rows_under_the_offset", + "emptied_list_still_reports_its_empty_range", + "collapsed_list_reports_nothing_at_all", + "padded_list_still_reports_the_row_it_draws", + ], + &tests, + "the virtual list must report only ranges it renders, never the row it measured, and must still report once it holds no rows", + ), pass_if( "window_frame.visual_types", ContractSeverity::Critical,