tab_switcher: Preserve selected position on closed tabs (#22861)

Andrew Borg (Kashin) created 1 year ago

When the user closes a tab, the tab switcher will now select the tab at
the same position. This feature is especially relevant for keyboard
users when you want to close multiple consecutive tabs with
`<Ctrl-Backspace>`.

Please see the discussion at
https://github.com/zed-industries/zed/discussions/22791 for full
motivation and the quick demo.

Release Notes:

- tab_switcher: Preserve selected position when tab is closed

Change summary

crates/tab_switcher/src/tab_switcher.rs       | 52 ++++++++++++--------
crates/tab_switcher/src/tab_switcher_tests.rs | 47 ++++++++++++++++++
2 files changed, 77 insertions(+), 22 deletions(-)

Detailed changes

crates/tab_switcher/src/tab_switcher.rs

@@ -185,11 +185,7 @@ impl TabSwitcherDelegate {
                 PaneEvent::AddItem { .. }
                 | PaneEvent::RemovedItem { .. }
                 | PaneEvent::Remove { .. } => tab_switcher.picker.update(cx, |picker, cx| {
-                    let selected_item_id = picker.delegate.selected_item_id();
                     picker.delegate.update_matches(cx);
-                    if let Some(item_id) = selected_item_id {
-                        picker.delegate.select_item(item_id, cx);
-                    }
                     cx.notify();
                 }),
                 _ => {}
@@ -199,6 +195,7 @@ impl TabSwitcherDelegate {
     }
 
     fn update_matches(&mut self, cx: &mut WindowContext) {
+        let selected_item_id = self.selected_item_id();
         self.matches.clear();
         let Some(pane) = self.pane.upgrade() else {
             return;
@@ -236,13 +233,7 @@ impl TabSwitcherDelegate {
             a_score.cmp(&b_score)
         });
 
-        if self.matches.len() > 1 {
-            if self.select_last {
-                self.selected_index = self.matches.len() - 1;
-            } else {
-                self.selected_index = 1;
-            }
-        }
+        self.selected_index = self.compute_selected_index(selected_item_id);
     }
 
     fn selected_item_id(&self) -> Option<EntityId> {
@@ -251,17 +242,34 @@ impl TabSwitcherDelegate {
             .map(|tab_match| tab_match.item.item_id())
     }
 
-    fn select_item(
-        &mut self,
-        item_id: EntityId,
-        cx: &mut ViewContext<Picker<TabSwitcherDelegate>>,
-    ) {
-        let selected_idx = self
-            .matches
-            .iter()
-            .position(|tab_match| tab_match.item.item_id() == item_id)
-            .unwrap_or(0);
-        self.set_selected_index(selected_idx, cx);
+    fn compute_selected_index(&mut self, prev_selected_item_id: Option<EntityId>) -> usize {
+        if self.matches.is_empty() {
+            return 0;
+        }
+
+        if let Some(selected_item_id) = prev_selected_item_id {
+            // If the previously selected item is still in the list, select its new position.
+            if let Some(item_index) = self
+                .matches
+                .iter()
+                .position(|tab_match| tab_match.item.item_id() == selected_item_id)
+            {
+                return item_index;
+            }
+            // Otherwise, try to preserve the previously selected index.
+            return self.selected_index.min(self.matches.len() - 1);
+        }
+
+        if self.select_last {
+            return self.matches.len() - 1;
+        }
+
+        if self.matches.len() > 1 {
+            // Index 0 is active, so don't preselect it for switching.
+            return 1;
+        }
+
+        0
     }
 
     fn close_item_at(&mut self, ix: usize, cx: &mut ViewContext<Picker<TabSwitcherDelegate>>) {

crates/tab_switcher/src/tab_switcher_tests.rs

@@ -228,6 +228,53 @@ async fn test_close_selected_item(cx: &mut gpui::TestAppContext) {
     assert_tab_switcher_is_closed(workspace, cx);
 }
 
+#[gpui::test]
+async fn test_close_preserves_selected_position(cx: &mut gpui::TestAppContext) {
+    let app_state = init_test(cx);
+    app_state
+        .fs
+        .as_fake()
+        .insert_tree(
+            "/root",
+            json!({
+                "1.txt": "First file",
+                "2.txt": "Second file",
+                "3.txt": "Third file",
+            }),
+        )
+        .await;
+
+    let project = Project::test(app_state.fs.clone(), ["/root".as_ref()], cx).await;
+    let (workspace, cx) = cx.add_window_view(|cx| Workspace::test_new(project.clone(), cx));
+
+    let tab_1 = open_buffer("1.txt", &workspace, cx).await;
+    let tab_2 = open_buffer("2.txt", &workspace, cx).await;
+    let tab_3 = open_buffer("3.txt", &workspace, cx).await;
+
+    let tab_switcher = open_tab_switcher(false, &workspace, cx);
+    tab_switcher.update(cx, |tab_switcher, _| {
+        assert_eq!(tab_switcher.delegate.matches.len(), 3);
+        assert_match_at_position(tab_switcher, 0, tab_3.boxed_clone());
+        assert_match_selection(tab_switcher, 1, tab_2.boxed_clone());
+        assert_match_at_position(tab_switcher, 2, tab_1.boxed_clone());
+    });
+
+    // Verify that if the selected tab was closed, tab at the same position is selected.
+    cx.dispatch_action(CloseSelectedItem);
+    tab_switcher.update(cx, |tab_switcher, _| {
+        assert_eq!(tab_switcher.delegate.matches.len(), 2);
+        assert_match_at_position(tab_switcher, 0, tab_3.boxed_clone());
+        assert_match_selection(tab_switcher, 1, tab_1.boxed_clone());
+    });
+
+    // But if the position is no longer valid, fall back to the position above.
+    cx.dispatch_action(CloseSelectedItem);
+    tab_switcher.update(cx, |tab_switcher, _| {
+        assert_eq!(tab_switcher.delegate.matches.len(), 1);
+        assert_match_selection(tab_switcher, 0, tab_3.boxed_clone());
+    });
+}
+
 fn init_test(cx: &mut TestAppContext) -> Arc<AppState> {
     cx.update(|cx| {
         let state = AppState::test(cx);