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

@@ -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 |

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<usize>, count: usize) -> Option<usize> {
-    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);
     }
 }

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,

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<dyn Fn(&[SharedString], &mut Window, &mut App)>;
 type OnActivate = Rc<dyn Fn(&SharedString, &mut Window, &mut App)>;
 type Picker = Rc<dyn Fn(usize, Pick, &mut Window, &mut App)>;
 
-/// 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<SharedString> = 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<SharedString> =
+                    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)
     }

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<SharedString>, Vec<SharedString>);
+/// A multiple-choice list of four rows, one maybe disabled, maybe reversed, that keeps what it reports and what was opened.
+struct Picks(
+    Vec<SharedString>,
+    Vec<SharedString>,
+    Option<&'static str>,
+    bool,
+);
 
 impl Render for Picks {
     fn render(&mut self, _: &mut Window, cx: &mut Context<Self>) -> 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<Pixels> {
 #[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<String> = 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<String> = 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");
+}

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<Move> {
-    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<Move> {
         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();

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<ElementId>,
         slides: impl IntoIterator<Item = Slide>,
@@ -90,7 +90,7 @@ impl Lightbox {
     ) -> Self {
         let slides: Vec<Slide> = 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()
     }
 }

src/overlays/tests/guides.rs

@@ -33,6 +33,7 @@ struct Guides {
     at: usize,
     log: Vec<String>,
     lower: bool,
+    pictures: usize,
 }
 
 fn target() -> Bounds<gpui::Pixels> {
@@ -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<Guides>, &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);