tab_switcher: Fix bug where selected index after closing tab did not match pane's active item (#44006)

Dino created 1 month ago

Whenever an item is removed using the Tab Switcher, the list of matches
is automatically updated, which can lead to the order of the elements
being updated and changing in comparison to what the user was previously
seeing. Unfortunately this can lead to a situation where the selected
index, since it wasn't being updated, would end up in a different item
than the one that was actually active in the pane.

This Pull Request updates the handling of the `PaneEvent::RemovedItem`
event so that the `TabSwitcherDelegate.selected_index` field is
automatically updated to match the pane's new active item.

Seeing as this is being updated, the
`test_close_preserves_selected_position` test is also removed, as it no
longer makes sense with the current implementation. I believe a better
user experience would be to actually not update the order of the
matches, simply removing the ones that no longer exist, and keep the
selected index position, but will tackle that in a different Pull
Request.

Closes #44005 

Release Notes:

- Fixed a bug with the tab switcher where, after closing a tab, the
selected entry would not match the pane's active item

Change summary

crates/tab_switcher/src/tab_switcher.rs       | 40 +++++++++
crates/tab_switcher/src/tab_switcher_tests.rs | 79 ++++++--------------
2 files changed, 61 insertions(+), 58 deletions(-)

Detailed changes

crates/tab_switcher/src/tab_switcher.rs

@@ -347,11 +347,23 @@ impl TabSwitcherDelegate {
         };
         cx.subscribe_in(&pane, window, |tab_switcher, _, event, window, cx| {
             match event {
-                PaneEvent::AddItem { .. }
-                | PaneEvent::RemovedItem { .. }
-                | PaneEvent::Remove { .. } => tab_switcher.picker.update(cx, |picker, cx| {
+                PaneEvent::AddItem { .. } | PaneEvent::Remove { .. } => {
+                    tab_switcher.picker.update(cx, |picker, cx| {
+                        let query = picker.query(cx);
+                        picker.delegate.update_matches(query, window, cx);
+                        cx.notify();
+                    })
+                }
+                PaneEvent::RemovedItem { .. } => tab_switcher.picker.update(cx, |picker, cx| {
                     let query = picker.query(cx);
                     picker.delegate.update_matches(query, window, cx);
+
+                    // When the Tab Switcher is being used and an item is
+                    // removed, there's a chance that the new selected index
+                    // will not match the actual tab that is now being displayed
+                    // by the pane, as such, the selected index needs to be
+                    // updated to match the pane's state.
+                    picker.delegate.sync_selected_index(cx);
                     cx.notify();
                 }),
                 _ => {}
@@ -540,11 +552,33 @@ impl TabSwitcherDelegate {
         let Some(pane) = tab_match.pane.upgrade() else {
             return;
         };
+
         pane.update(cx, |pane, cx| {
             pane.close_item_by_id(tab_match.item.item_id(), SaveIntent::Close, window, cx)
                 .detach_and_log_err(cx);
         });
     }
+
+    /// Updates the selected index to ensure it matches the pane's active item,
+    /// as the pane's active item can be indirectly updated and this method
+    /// ensures that the picker can react to those changes.
+    fn sync_selected_index(&mut self, cx: &mut Context<Picker<TabSwitcherDelegate>>) {
+        let Ok(Some(item)) = self.pane.read_with(cx, |pane, _cx| pane.active_item()) else {
+            return;
+        };
+
+        let item_id = item.item_id();
+        let Some((index, _tab_match)) = self
+            .matches
+            .iter()
+            .enumerate()
+            .find(|(_index, tab_match)| tab_match.item.item_id() == item_id)
+        else {
+            return;
+        };
+
+        self.selected_index = index;
+    }
 }
 
 impl PickerDelegate for TabSwitcherDelegate {

crates/tab_switcher/src/tab_switcher_tests.rs

@@ -5,7 +5,7 @@ use menu::SelectPrevious;
 use project::{Project, ProjectPath};
 use serde_json::json;
 use util::{path, rel_path::rel_path};
-use workspace::{AppState, Workspace};
+use workspace::{ActivatePreviousItem, AppState, Workspace};
 
 #[ctor::ctor]
 fn init_logger() {
@@ -197,6 +197,8 @@ async fn test_close_selected_item(cx: &mut gpui::TestAppContext) {
             json!({
                 "1.txt": "First file",
                 "2.txt": "Second file",
+                "3.txt": "Third file",
+                "4.txt": "Fourth file",
             }),
         )
         .await;
@@ -206,80 +208,47 @@ async fn test_close_selected_item(cx: &mut gpui::TestAppContext) {
         cx.add_window_view(|window, cx| Workspace::test_new(project.clone(), window, cx));
 
     let tab_1 = open_buffer("1.txt", &workspace, cx).await;
+    let tab_3 = open_buffer("3.txt", &workspace, cx).await;
     let tab_2 = open_buffer("2.txt", &workspace, cx).await;
+    let tab_4 = open_buffer("4.txt", &workspace, cx).await;
+
+    // After opening all buffers, let's navigate to the previous item two times, finishing with:
+    //
+    // 1.txt | [3.txt] | 2.txt | 4.txt
+    //
+    // With 3.txt being the active item in the pane.
+    cx.dispatch_action(ActivatePreviousItem);
+    cx.dispatch_action(ActivatePreviousItem);
+    cx.run_until_parked();
 
     cx.simulate_modifiers_change(Modifiers::control());
     let tab_switcher = open_tab_switcher(false, &workspace, cx);
     tab_switcher.update(cx, |tab_switcher, _| {
-        assert_eq!(tab_switcher.delegate.matches.len(), 2);
-        assert_match_at_position(tab_switcher, 0, tab_2.boxed_clone());
-        assert_match_selection(tab_switcher, 1, tab_1.boxed_clone());
+        assert_eq!(tab_switcher.delegate.matches.len(), 4);
+        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_4.boxed_clone());
+        assert_match_at_position(tab_switcher, 3, tab_1.boxed_clone());
     });
 
     cx.simulate_modifiers_change(Modifiers::control());
     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_2);
+        assert_eq!(tab_switcher.delegate.matches.len(), 3);
+        assert_match_selection(tab_switcher, 0, tab_3);
+        assert_match_at_position(tab_switcher, 1, tab_4);
+        assert_match_at_position(tab_switcher, 2, tab_1);
     });
 
     // Still switches tab on modifiers release
     cx.simulate_modifiers_change(Modifiers::none());
     cx.read(|cx| {
         let active_editor = workspace.read(cx).active_item_as::<Editor>(cx).unwrap();
-        assert_eq!(active_editor.read(cx).title(cx), "2.txt");
+        assert_eq!(active_editor.read(cx).title(cx), "3.txt");
     });
     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(
-            path!("/root"),
-            json!({
-                "1.txt": "First file",
-                "2.txt": "Second file",
-                "3.txt": "Third file",
-            }),
-        )
-        .await;
-
-    let project = Project::test(app_state.fs.clone(), [path!("/root").as_ref()], cx).await;
-    let (workspace, cx) =
-        cx.add_window_view(|window, cx| Workspace::test_new(project.clone(), window, 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);