From 492578a67c11f59542710cdca00583213fd0263f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Fri, 25 Sep 2026 21:21:33 -0400 Subject: [PATCH] T12c, T13 review fixes: an emptied lightbox closes through its close path, keys pass over disabled and loading rows, the list cursor follows its row through a re-sort --- TASKS.md | 16 +++++--- src/data_display/gallery.rs | 6 +-- src/lists/item.rs | 4 ++ src/lists/select.rs | 62 ++++++++++++++++++++++------- src/lists/tests.rs | 75 ++++++++++++++++++++++++++++++++---- src/lists/tree/model.rs | 40 +++++++++++++++---- src/overlays/lightbox.rs | 13 +++++-- src/overlays/tests/guides.rs | 21 +++++++++- 8 files changed, 196 insertions(+), 41 deletions(-) diff --git a/TASKS.md b/TASKS.md index 8a34707..0faaba8 100644 --- a/TASKS.md +++ b/TASKS.md @@ -42,10 +42,10 @@ Per-component lines live in `tasks/`. Tags there: - [x] T11b Loading · Progress & states - [x] T11c Motion · Presence & order - [x] T11d Motion · Effects -- [ ] T12a Data Display · People & numbers -- [ ] T12b Data Display · Records +- [x] T12a Data Display · People & numbers +- [x] T12b Data Display · Records - [ ] T12c Data Display · Media -- [ ] T12d Data Display · Codes & measures +- [x] T12d Data Display · Codes & measures - [ ] T13a Lists & Trees · Lists - [ ] T13b Lists & Trees · Trees - [ ] T13c Lists & Trees · Files @@ -187,12 +187,16 @@ Per-component lines live in `tasks/`. Tags there: | T11d | 2 | FAIL | Marquee's doc still said its content is drawn twice | | T11d | 3 | PASS | | | T12a | 1 | FAIL | the avatar upload took HEIC, which gpui cannot decode, an accent badge's dot matched its fill, and a square avatar's upload veil stayed round | +| T12a | 2 | PASS | | | T12b | 1 | FAIL | the gpui test module compiled without test-support, and a long description value overflowed a narrow column | +| T12b | 2 | PASS | | | T12c | 1 | FAIL | the gallery kept an open index past a shrunk list, and the lightbox asserted on it | +| T12c | 2 | FAIL | emptying the list removed the open lightbox without handing focus back | | T12d | 1 | FAIL | Code 128 took a set-switch character, a usage bar's float sum tripped its assert, and a dense QR floored its modules to nothing | -| T13a | 1 | — | pending: codex still logged out | -| T13b | 1 | — | pending: codex still logged out | -| T13c | 1 | — | pending: codex still logged out | +| T12d | 2 | PASS | | +| T13a | 1 | FAIL | Down and Cmd-A picked disabled rows, and Enter opened them | +| T13b | 1 | FAIL | Down stuck on a loading row and never reached the nodes past it | +| T13c | 1 | FAIL | the cursor kept its index through a re-sort, so Enter opened another file | | T14a | 1 | — | pending: codex still logged out | | T14b | 1 | — | pending: codex still logged out | | T14c | 1 | — | pending review | diff --git a/src/data_display/gallery.rs b/src/data_display/gallery.rs index e032bd7..ccb6362 100644 --- a/src/data_display/gallery.rs +++ b/src/data_display/gallery.rs @@ -9,9 +9,9 @@ use crate::{ primitives::{FocusRing, Image, tab_stop}, }; -/// The open picture if it is still there, the last when it went past the end, none when all are gone. +/// The open picture, or the last when it went past the end; with none left the lightbox closes itself. fn clamp(open: Option, count: usize) -> Option { - open.and_then(|at| count.checked_sub(1).map(|last| at.min(last))) + open.map(|at| at.min(count.saturating_sub(1))) } /// Pictures in square tiles. A press, or Enter on a focused tile, opens the lightbox at that picture. @@ -132,7 +132,7 @@ mod tests { fn an_open_picture_clamps_when_pictures_go_away() { assert_eq!(clamp(Some(1), 3), Some(1)); assert_eq!(clamp(Some(2), 1), Some(0), "past the end opens the last"); - assert_eq!(clamp(Some(0), 0), None, "no pictures closes it"); + assert_eq!(clamp(Some(2), 0), Some(0), "the lightbox closes itself"); assert_eq!(clamp(None, 3), None); } } diff --git a/src/lists/item.rs b/src/lists/item.rs index e169f9b..d18341f 100644 --- a/src/lists/item.rs +++ b/src/lists/item.rs @@ -77,6 +77,10 @@ impl ListItem { self } + pub(crate) fn is_disabled(&self) -> bool { + self.disabled + } + /// Makes the row pressable; the press keeps focus where it is. pub fn on_click( mut self, diff --git a/src/lists/select.rs b/src/lists/select.rs index ec60f1f..899f749 100644 --- a/src/lists/select.rs +++ b/src/lists/select.rs @@ -38,11 +38,12 @@ pub(crate) fn picked( .collect() } -/// The row the keyboard is on, where a Shift range starts, and the scroll that follows them. +/// The row the keyboard is on and where a Shift range starts, by index and by key so they follow their rows when the owner reorders them, and the scroll that follows them. #[derive(Default)] struct Cursor { at: usize, anchor: usize, + keys: Option<(SharedString, SharedString)>, scroll: ScrollHandle, } @@ -50,7 +51,7 @@ type OnSelect = Rc; type OnActivate = Rc; type Picker = Rc; -/// Rows you select. A press picks one; with `multiple`, Cmd-press adds or drops one and Shift-press takes the range from the last pick. Up and Down move, Shift with them extends, Space toggles, Cmd-A takes all. +/// Rows you select. A press picks one; with `multiple`, Cmd-press adds or drops one and Shift-press takes the range from the last pick. Up and Down move, Shift with them extends, Space toggles, Cmd-A takes all. Keys pass over disabled rows, and no pick adds one. #[derive(IntoElement)] pub struct SelectableList { id: ElementId, @@ -123,14 +124,34 @@ impl RenderOnce for SelectableList { let focused = focus.is_focused(window); let cursor = window.use_keyed_state((self.id.clone(), "cursor"), cx, |_, _| Cursor::default()); - let (at, scroll) = { + let keys: Rc<[SharedString]> = self.rows.iter().map(|(key, _)| key.clone()).collect(); + let (at, anchor, scroll) = { let cursor = cursor.read(cx); + let place = |key: Option<&SharedString>, ix: usize| { + key.and_then(|key| keys.iter().position(|row| row == key)) + .unwrap_or(ix.min(count.saturating_sub(1))) + }; + let held = cursor.keys.as_ref(); ( - cursor.at.min(count.saturating_sub(1)), + place(held.map(|keys| &keys.0), cursor.at), + place(held.map(|keys| &keys.1), cursor.anchor), cursor.scroll.clone(), ) }; - let keys: Rc<[SharedString]> = self.rows.iter().map(|(key, _)| key.clone()).collect(); + if (at, anchor) != (cursor.read(cx).at, cursor.read(cx).anchor) { + cursor.update(cx, |cursor, _| (cursor.at, cursor.anchor) = (at, anchor)); + } + let off: Rc<[bool]> = self + .rows + .iter() + .map(|(_, item)| item.is_disabled()) + .collect(); + let barred: Vec = self + .rows + .iter() + .filter(|(_, item)| item.is_disabled()) + .map(|(key, _)| key.clone()) + .collect(); let selected = self.selected; let multiple = self.multiple; let pick: Picker = { @@ -147,12 +168,20 @@ impl RenderOnce for SelectableList { (false, Pick::All) => return, (false, _) => Pick::One, }; - let next = picked(&keys, &selected, cursor.read(cx).anchor, ix, pick); + let next: Vec = + picked(&keys, &selected, cursor.read(cx).anchor, ix, pick) + .into_iter() + .filter(|key| !barred.contains(key) || selected.contains(key)) + .collect(); cursor.update(cx, |cursor, cx| { cursor.at = ix; if pick != Pick::Range { cursor.anchor = ix; } + cursor.keys = Some(( + keys[ix].clone(), + keys[cursor.anchor.min(keys.len() - 1)].clone(), + )); cursor.scroll.scroll_to_item(ix); cx.notify(); }); @@ -209,22 +238,27 @@ impl RenderOnce for SelectableList { let held = &event.keystroke.modifiers; let at = cursor.read(cx).at.min(count - 1); let extend = if held.shift { Pick::Range } else { Pick::One }; + let open = |ix: &usize| !off[*ix]; let (to, how) = match event.keystroke.key.as_str() { - "down" => ((at + 1).min(count - 1), extend), - "up" => (at.saturating_sub(1), extend), - "home" => (0, extend), - "end" => (count - 1, extend), - "space" => (at, Pick::Toggle), + "down" => ((at + 1..count).find(open), extend), + "up" => ((0..at).rev().find(open), extend), + "home" => ((0..count).find(open), extend), + "end" => ((0..count).rev().find(open), extend), + "space" => (Some(at).filter(open), Pick::Toggle), "enter" => { cx.stop_propagation(); - activate(&keys[at], window, cx); + if open(&at) { + activate(&keys[at], window, cx); + } return; } - "a" if held.platform => (at, Pick::All), + "a" if held.platform => (Some(at), Pick::All), _ => return, }; cx.stop_propagation(); - pick(to, how, window, cx); + if let Some(to) = to { + pick(to, how, window, cx); + } }) .children(rows) } diff --git a/src/lists/tests.rs b/src/lists/tests.rs index 1f46545..7b68cdd 100644 --- a/src/lists/tests.rs +++ b/src/lists/tests.rs @@ -36,16 +36,29 @@ fn settle(cx: &mut VisualTestContext) { const NAMES: [&str; 4] = ["a", "b", "c", "d"]; -/// A multiple-choice list of four rows that keeps what it reports, and what was opened. -struct Picks(Vec, Vec); +/// A multiple-choice list of four rows, one maybe disabled, maybe reversed, that keeps what it reports and what was opened. +struct Picks( + Vec, + Vec, + Option<&'static str>, + bool, +); impl Render for Picks { fn render(&mut self, _: &mut Window, cx: &mut Context) -> impl IntoElement { let (view, opened) = (cx.entity(), cx.entity()); - NAMES - .iter() + let names: Vec<&&str> = if self.3 { + NAMES.iter().rev().collect() + } else { + NAMES.iter().collect() + }; + names + .into_iter() .fold(SelectableList::new("picks").multiple(), |list, name| { - list.row(*name, ListItem::new(*name, *name)) + list.row( + *name, + ListItem::new(*name, *name).disabled(self.2 == Some(*name)), + ) }) .selected(self.0.clone()) .on_change(move |keys, _, cx| view.update(cx, |picks, _| picks.0 = keys.to_vec())) @@ -69,7 +82,7 @@ fn row(ix: usize) -> Point { #[gpui::test] fn presses_pick_one_toggle_with_cmd_and_range_with_shift(cx: &mut TestAppContext) { setup(cx); - let (view, cx) = cx.add_window_view(|_, _| Picks(Vec::new(), Vec::new())); + let (view, cx) = cx.add_window_view(|_, _| Picks(Vec::new(), Vec::new(), None, false)); settle(cx); cx.simulate_click(row(0), Modifiers::none()); settle(cx); @@ -277,7 +290,7 @@ fn a_long_list_builds_only_what_is_near_the_view(cx: &mut TestAppContext) { #[gpui::test] fn enter_or_a_double_press_opens_a_row(cx: &mut TestAppContext) { setup(cx); - let (view, cx) = cx.add_window_view(|_, _| Picks(Vec::new(), Vec::new())); + let (view, cx) = cx.add_window_view(|_, _| Picks(Vec::new(), Vec::new(), None, false)); settle(cx); cx.simulate_click(row(1), Modifiers::none()); settle(cx); @@ -304,3 +317,51 @@ fn enter_or_a_double_press_opens_a_row(cx: &mut TestAppContext) { }); assert_eq!(opened, ["b", "d"]); } + +#[gpui::test] +fn keys_pass_over_a_disabled_row_and_never_pick_or_open_it(cx: &mut TestAppContext) { + setup(cx); + let (view, cx) = cx.add_window_view(|_, _| Picks(Vec::new(), Vec::new(), Some("b"), false)); + settle(cx); + cx.simulate_click(row(0), Modifiers::none()); + settle(cx); + cx.simulate_keystrokes("down"); + settle(cx); + assert_eq!(picks(&view, cx), ["c"], "down steps over the disabled row"); + cx.simulate_keystrokes("up enter cmd-a"); + settle(cx); + assert_eq!( + picks(&view, cx), + ["a", "c", "d"], + "all leaves the disabled row out" + ); + let opened: Vec = view.read_with(cx, |picks, _| { + picks.1.iter().map(|key| key.to_string()).collect() + }); + assert_eq!( + opened, + ["a"], + "up landed on the first row, not the disabled one" + ); +} + +#[gpui::test] +fn the_cursor_stays_on_its_row_when_the_rows_reorder(cx: &mut TestAppContext) { + setup(cx); + let (view, cx) = cx.add_window_view(|_, _| Picks(Vec::new(), Vec::new(), None, false)); + settle(cx); + cx.simulate_click(row(0), Modifiers::none()); + settle(cx); + view.update(cx, |picks, cx| { + picks.3 = true; + cx.notify(); + }); + settle(cx); + cx.simulate_keystrokes("enter up"); + settle(cx); + let opened: Vec = view.read_with(cx, |picks, _| { + picks.1.iter().map(|key| key.to_string()).collect() + }); + assert_eq!(opened, ["a"], "enter opens the row the cursor was on"); + assert_eq!(picks(&view, cx), ["b"], "up moves from where that row went"); +} diff --git a/src/lists/tree/model.rs b/src/lists/tree/model.rs index a930466..1a950a4 100644 --- a/src/lists/tree/model.rs +++ b/src/lists/tree/model.rs @@ -181,10 +181,10 @@ pub(crate) enum Move { Close(SharedString), } -/// Up, Down, Home and End walk the rows; Right opens a node or enters it, Left closes it or goes to its parent. +/// Up, Down, Home and End walk the rows past any still loading; Right opens a node or enters it, Left closes it or goes to its parent. pub(crate) fn step(rows: &[Row], at: usize, key: &str) -> Option { - let last = rows.len().checked_sub(1)?; let row = rows.get(at)?; + let node = |ix: &usize| rows[*ix].shown != Shown::Loading; let (opens, open, node_key) = match &row.shown { Shown::Node { key, opens, open, .. @@ -192,12 +192,15 @@ pub(crate) fn step(rows: &[Row], at: usize, key: &str) -> Option { Shown::Loading => (false, false, None), }; match key { - "down" => (at < last).then(|| Move::To(at + 1)), - "up" => at.checked_sub(1).map(Move::To), - "home" => Some(Move::To(0)), - "end" => Some(Move::To(last)), + "down" => (at + 1..rows.len()).find(node).map(Move::To), + "up" => (0..at).rev().find(node).map(Move::To), + "home" => (0..rows.len()).find(node).map(Move::To), + "end" => (0..rows.len()).rev().find(node).map(Move::To), "right" if opens && !open => node_key.map(Move::Open), - "right" if open => (at < last).then(|| Move::To(at + 1)), + "right" if open => Some(at + 1) + .filter(|next| rows.get(*next).is_some_and(|child| child.depth > row.depth)) + .filter(node) + .map(Move::To), "left" if open => node_key.map(Move::Close), "left" => row.parent.map(Move::To), _ => None, @@ -342,6 +345,29 @@ mod tests { assert_eq!(step(&shown, 3, "end"), Some(Move::To(4))); } + #[test] + fn keys_pass_over_a_loading_row() { + let nodes = tree(); + let shown = rows(&nodes, &open(&["docs"])); + assert_eq!( + step(&shown, 1, "down"), + Some(Move::To(3)), + "down skips the loading row" + ); + assert_eq!(step(&shown, 3, "up"), Some(Move::To(1))); + assert_eq!( + step(&shown, 1, "right"), + None, + "nothing to enter while it loads" + ); + let waiting = rows(&[TreeNode::new("docs", "docs").pending()], &open(&["docs"])); + assert_eq!( + step(&waiting, 0, "end"), + Some(Move::To(0)), + "end stops on the last node" + ); + } + #[test] fn boxes_mix_and_fill_by_their_leaves() { let nodes = tree(); diff --git a/src/overlays/lightbox.rs b/src/overlays/lightbox.rs index 18e102e..a857468 100644 --- a/src/overlays/lightbox.rs +++ b/src/overlays/lightbox.rs @@ -81,7 +81,7 @@ pub struct Lightbox { } impl Lightbox { - /// Render it while open; the owner keeps `at` and moves it in `on_step`. + /// Render it while open; the owner keeps `at` and moves it in `on_step`. With no pictures left it closes, handing focus back. pub fn new( id: impl Into, slides: impl IntoIterator, @@ -90,7 +90,7 @@ impl Lightbox { ) -> Self { let slides: Vec = slides.into_iter().collect(); assert!( - at < slides.len(), + slides.is_empty() || at < slides.len(), "slide {at} is past the last of {}", slides.len() ); @@ -127,6 +127,11 @@ impl RenderOnce for Lightbox { on_close(window, cx) }) }; + if count == 0 { + log::info!("lightbox {:?}: no pictures left", self.id); + window.defer(cx, move |window, cx| close(window, cx)); + return div().into_any_element(); + } let step: OnStep = { let (id, on_step) = (self.id.clone(), self.on_step); Rc::new(move |to, window, cx| { @@ -276,6 +281,8 @@ impl RenderOnce for Lightbox { ), ), ); - deferred(anchored().position(Point::default()).child(stage)).with_priority(1) + deferred(anchored().position(Point::default()).child(stage)) + .with_priority(1) + .into_any_element() } } diff --git a/src/overlays/tests/guides.rs b/src/overlays/tests/guides.rs index d31b08f..4a9b2b1 100644 --- a/src/overlays/tests/guides.rs +++ b/src/overlays/tests/guides.rs @@ -33,6 +33,7 @@ struct Guides { at: usize, log: Vec, lower: bool, + pictures: usize, } fn target() -> Bounds { @@ -60,7 +61,10 @@ impl Render for Guides { cx.notify(); }) }; - let slides = ["a", "b", "c"].map(|name| Slide::new(Path::new("/missing/ely.png"), name)); + let slides = ["a", "b", "c"] + .into_iter() + .take(self.pictures) + .map(|name| Slide::new(Path::new("/missing/ely.png"), name)); let steps = [ TourStep::new(target(), "Here", "The first stop"), TourStep::new( @@ -129,6 +133,7 @@ fn guides(cx: &mut TestAppContext) -> (Entity, &mut VisualTestContext) { at: 0, log: Vec::new(), lower: false, + pictures: 3, }); settle(cx); (view, cx) @@ -167,6 +172,20 @@ fn the_lightbox_steps_with_arrows_and_escape_hands_focus_back(cx: &mut TestAppCo assert!(before_focused(&view, cx)); } +#[gpui::test] +fn a_lightbox_whose_pictures_go_away_closes_and_hands_focus_back(cx: &mut TestAppContext) { + let (view, cx) = guides(cx); + open(&view, Open::Lightbox, cx); + assert!(!before_focused(&view, cx), "the lightbox took focus"); + view.update(cx, |guides, cx| { + guides.pictures = 0; + cx.notify(); + }); + settle(cx); + assert!(state(&view, cx).0.is_none(), "it closed"); + assert!(before_focused(&view, cx), "focus went back"); +} + #[gpui::test] fn the_dark_closes_the_lightbox_and_the_picture_does_not(cx: &mut TestAppContext) { let (view, cx) = guides(cx);