Reveal the selected item when cycling a picker's selection (#12950)

Max Brunsfeld created 2 years ago

Release Notes:

- Fixed a bug where the selected tab was not always shown when cycling
between tabs with `ctrl-tab`.

Change summary

crates/file_finder/Cargo.toml               |  1 
crates/file_finder/src/file_finder_tests.rs | 39 +++++++++++++++++++++++
crates/gpui/src/elements/uniform_list.rs    |  7 ++++
crates/picker/Cargo.toml                    |  3 +
crates/picker/src/picker.rs                 | 12 ++++++
5 files changed, 61 insertions(+), 1 deletion(-)

Detailed changes

crates/file_finder/Cargo.toml

@@ -37,6 +37,7 @@ editor = { workspace = true, features = ["test-support"] }
 env_logger.workspace = true
 gpui = { workspace = true, features = ["test-support"] }
 language = { workspace = true, features = ["test-support"] }
+picker = { workspace = true, features = ["test-support"] }
 serde_json.workspace = true
 theme = { workspace = true, features = ["test-support"] }
 workspace = { workspace = true, features = ["test-support"] }

crates/file_finder/src/file_finder_tests.rs

@@ -1747,6 +1747,45 @@ async fn test_extending_modifiers_does_not_confirm_selection(cx: &mut gpui::Test
     active_file_picker(&workspace, cx);
 }
 
+#[gpui::test]
+async fn test_repeat_toggle_action(cx: &mut gpui::TestAppContext) {
+    let app_state = init_test(cx);
+    app_state
+        .fs
+        .as_fake()
+        .insert_tree(
+            "/test",
+            json!({
+                "00.txt": "",
+                "01.txt": "",
+                "02.txt": "",
+                "03.txt": "",
+                "04.txt": "",
+                "05.txt": "",
+            }),
+        )
+        .await;
+
+    let project = Project::test(app_state.fs.clone(), ["/test".as_ref()], cx).await;
+    let (workspace, cx) = cx.add_window_view(|cx| Workspace::test_new(project, cx));
+
+    cx.dispatch_action(Toggle::default());
+    let picker = active_file_picker(&workspace, cx);
+    picker.update(cx, |picker, _| {
+        assert_eq!(picker.delegate.selected_index, 0);
+        assert_eq!(picker.logical_scroll_top_index(), 0);
+    });
+
+    // When toggling repeatedly, the picker scrolls to reveal the selected item.
+    cx.dispatch_action(Toggle::default());
+    cx.dispatch_action(Toggle::default());
+    cx.dispatch_action(Toggle::default());
+    picker.update(cx, |picker, _| {
+        assert_eq!(picker.delegate.selected_index, 3);
+        assert_eq!(picker.logical_scroll_top_index(), 3);
+    });
+}
+
 async fn open_close_queried_buffer(
     input: &str,
     expected_matches: usize,

crates/gpui/src/elements/uniform_list.rs

@@ -98,6 +98,13 @@ impl UniformListScrollHandle {
     pub fn scroll_to_item(&mut self, ix: usize) {
         self.deferred_scroll_to_item.replace(Some(ix));
     }
+
+    /// Get the index of the topmost visible child.
+    pub fn logical_scroll_top_index(&self) -> usize {
+        self.deferred_scroll_to_item
+            .borrow()
+            .unwrap_or_else(|| self.base_handle.logical_scroll_top().0)
+    }
 }
 
 impl Styled for UniformList {

crates/picker/Cargo.toml

@@ -12,6 +12,9 @@ workspace = true
 path = "src/picker.rs"
 doctest = false
 
+[features]
+test-support = []
+
 [dependencies]
 anyhow.workspace = true
 editor.workspace = true

crates/picker/src/picker.rs

@@ -310,7 +310,7 @@ impl<D: PickerDelegate> Picker<D> {
         let count = self.delegate.match_count();
         let index = self.delegate.selected_index();
         let new_index = if index + 1 == count { 0 } else { index + 1 };
-        self.set_selected_index(new_index, false, cx);
+        self.set_selected_index(new_index, true, cx);
         cx.notify();
     }
 
@@ -538,6 +538,16 @@ impl<D: PickerDelegate> Picker<D> {
                 .into_any_element(),
         }
     }
+
+    #[cfg(any(test, feature = "test-support"))]
+    pub fn logical_scroll_top_index(&self) -> usize {
+        match &self.element_container {
+            ElementContainer::List(state) => state.logical_scroll_top().item_ix,
+            ElementContainer::UniformList(scroll_handle) => {
+                scroll_handle.logical_scroll_top_index()
+            }
+        }
+    }
 }
 
 impl<D: PickerDelegate> EventEmitter<DismissEvent> for Picker<D> {}