Select the second item in the file finder by default (#6947)

This PR completes the first task of the Tabless editing feature (#6424).
It makes file finder select the previously opened file by default which
allows the user to quickly switch between two last opened files by
clicking `Cmd-P + Enter`.

This feature was also requested in #4663 comments.

Release Notes:
* Improved file finder selection: currently opened item is not selected now

crates/file_finder/src/file_finder.rs

@@ -61,7 +61,7 @@ impl FileFinder {
                 let abs_path = project
                     .worktree_for_id(project_path.worktree_id, cx)
                     .map(|worktree| worktree.read(cx).abs_path().join(&project_path.path));
-                FoundPath::new(project_path, abs_path)
+                FoundPath::new_opened_file(project_path, abs_path)
             });
 
         // if exists, bubble the currently opened path to the top
@@ -139,7 +139,7 @@ pub struct FileFinderDelegate {
     latest_search_query: Option<PathLikeWithPosition<FileSearchQuery>>,
     currently_opened_path: Option<FoundPath>,
     matches: Matches,
-    selected_index: Option<usize>,
+    selected_index: usize,
     cancel_flag: Arc<AtomicBool>,
     history_items: Vec<FoundPath>,
 }
@@ -231,7 +231,15 @@ impl Matches {
             &mut self.history,
             history_items_to_show,
             100,
-            |(_, a), (_, b)| b.cmp(a),
+            |(ap, a), (bp, b)| {
+                if ap.opened_file {
+                    return cmp::Ordering::Less;
+                }
+                if bp.opened_file {
+                    return cmp::Ordering::Greater;
+                }
+                b.cmp(a)
+            },
         );
 
         if extend_old_matches {
@@ -305,11 +313,24 @@ fn matching_history_item_paths(
 struct FoundPath {
     project: ProjectPath,
     absolute: Option<PathBuf>,
+    opened_file: bool,
 }
 
 impl FoundPath {
     fn new(project: ProjectPath, absolute: Option<PathBuf>) -> Self {
-        Self { project, absolute }
+        Self {
+            project,
+            absolute,
+            opened_file: false,
+        }
+    }
+
+    fn new_opened_file(project: ProjectPath, absolute: Option<PathBuf>) -> Self {
+        Self {
+            project,
+            absolute,
+            opened_file: true,
+        }
     }
 }
 
@@ -372,7 +393,7 @@ impl FileFinderDelegate {
             latest_search_query: None,
             currently_opened_path,
             matches: Matches::default(),
-            selected_index: None,
+            selected_index: 0,
             cancel_flag: Arc::new(AtomicBool::new(false)),
             history_items,
         }
@@ -427,7 +448,6 @@ impl FileFinderDelegate {
             let did_cancel = cancel_flag.load(atomic::Ordering::Relaxed);
             picker
                 .update(&mut cx, |picker, cx| {
-                    picker.delegate.selected_index.take();
                     picker
                         .delegate
                         .set_search_matches(search_id, did_cancel, query, matches, cx)
@@ -460,6 +480,7 @@ impl FileFinderDelegate {
             );
             self.latest_search_query = Some(query);
             self.latest_search_did_cancel = did_cancel;
+            self.selected_index = self.calculate_selected_index();
             cx.notify();
         }
     }
@@ -630,6 +651,20 @@ impl FileFinderDelegate {
                 .log_err();
         })
     }
+
+    /// Calculates selection index after the user performed search.
+    /// Prefers to return 1 if the top visible item corresponds to the currently opened file, otherwise returns 0.
+    fn calculate_selected_index(&self) -> usize {
+        let first = self.matches.history.get(0);
+        if let Some(first) = first {
+            if !first.0.opened_file {
+                return 0;
+            }
+        } else {
+            return 0;
+        }
+        (self.matches.len() - 1).min(1)
+    }
 }
 
 impl PickerDelegate for FileFinderDelegate {
@@ -644,11 +679,11 @@ impl PickerDelegate for FileFinderDelegate {
     }
 
     fn selected_index(&self) -> usize {
-        self.selected_index.unwrap_or(0)
+        self.selected_index
     }
 
     fn set_selected_index(&mut self, ix: usize, cx: &mut ViewContext<Picker<Self>>) {
-        self.selected_index = Some(ix);
+        self.selected_index = ix;
         cx.notify();
     }
 
@@ -671,7 +706,6 @@ impl PickerDelegate for FileFinderDelegate {
         if raw_query.is_empty() {
             let project = self.project.read(cx);
             self.latest_search_id = post_inc(&mut self.search_count);
-            self.selected_index.take();
             self.matches = Matches {
                 history: self
                     .history_items
@@ -687,6 +721,7 @@ impl PickerDelegate for FileFinderDelegate {
                     .collect(),
                 search: Vec::new(),
             };
+            self.selected_index = self.calculate_selected_index();
             cx.notify();
             Task::ready(())
         } else {

crates/file_finder/src/file_finder_tests.rs

@@ -1062,6 +1062,120 @@ async fn test_search_sorts_history_items(cx: &mut gpui::TestAppContext) {
     });
 }
 
+#[gpui::test]
+async fn test_select_current_open_file_when_no_history(cx: &mut gpui::TestAppContext) {
+    let app_state = init_test(cx);
+
+    app_state
+        .fs
+        .as_fake()
+        .insert_tree(
+            "/root",
+            json!({
+                "test": {
+                    "1_qw": "",
+                }
+            }),
+        )
+        .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, cx));
+    // Open new buffer
+    open_queried_buffer("1", 1, "1_qw", &workspace, cx).await;
+
+    let picker = open_file_picker(&workspace, cx);
+    picker.update(cx, |finder, _| {
+        assert_match_selection(&finder, 0, "1_qw");
+    });
+}
+
+#[gpui::test]
+async fn test_keep_opened_file_on_top_of_search_results_and_select_next_one(
+    cx: &mut TestAppContext,
+) {
+    let app_state = init_test(cx);
+
+    app_state
+        .fs
+        .as_fake()
+        .insert_tree(
+            "/src",
+            json!({
+                "test": {
+                    "bar.rs": "// Bar file",
+                    "lib.rs": "// Lib file",
+                    "maaa.rs": "// Maaaaaaa",
+                    "main.rs": "// Main file",
+                    "moo.rs": "// Moooooo",
+                }
+            }),
+        )
+        .await;
+
+    let project = Project::test(app_state.fs.clone(), ["/src".as_ref()], cx).await;
+    let (workspace, cx) = cx.add_window_view(|cx| Workspace::test_new(project, cx));
+
+    open_close_queried_buffer("bar", 1, "bar.rs", &workspace, cx).await;
+    open_close_queried_buffer("lib", 1, "lib.rs", &workspace, cx).await;
+    open_queried_buffer("main", 1, "main.rs", &workspace, cx).await;
+
+    // main.rs is on top, previously used is selected
+    let picker = open_file_picker(&workspace, cx);
+    picker.update(cx, |finder, _| {
+        assert_eq!(finder.delegate.matches.len(), 3);
+        assert_match_at_position(finder, 0, "main.rs");
+        assert_match_selection(finder, 1, "lib.rs");
+    });
+
+    // all files match, main.rs is still on top
+    picker
+        .update(cx, |finder, cx| {
+            finder.delegate.update_matches(".rs".to_string(), cx)
+        })
+        .await;
+    picker.update(cx, |finder, _| {
+        assert_eq!(finder.delegate.matches.len(), 5);
+        assert_match_at_position(finder, 0, "main.rs");
+        assert_match_selection(finder, 1, "bar.rs");
+    });
+
+    // main.rs is not among matches, select top item
+    picker
+        .update(cx, |finder, cx| {
+            finder.delegate.update_matches("b".to_string(), cx)
+        })
+        .await;
+    picker.update(cx, |finder, _| {
+        assert_eq!(finder.delegate.matches.len(), 2);
+        assert_match_at_position(finder, 0, "bar.rs");
+    });
+
+    // main.rs is back, put it on top and select next item
+    picker
+        .update(cx, |finder, cx| {
+            finder.delegate.update_matches("m".to_string(), cx)
+        })
+        .await;
+    picker.update(cx, |finder, _| {
+        assert_eq!(finder.delegate.matches.len(), 3);
+        assert_match_at_position(finder, 0, "main.rs");
+        assert_match_selection(finder, 1, "moo.rs");
+    });
+
+    // get back to the initial state
+    picker
+        .update(cx, |finder, cx| {
+            finder.delegate.update_matches("".to_string(), cx)
+        })
+        .await;
+    picker.update(cx, |finder, _| {
+        assert_eq!(finder.delegate.matches.len(), 3);
+        assert_match_at_position(finder, 0, "main.rs");
+        assert_match_selection(finder, 1, "lib.rs");
+    });
+}
+
 #[gpui::test]
 async fn test_history_items_vs_very_good_external_match(cx: &mut gpui::TestAppContext) {
     let app_state = init_test(cx);
@@ -1172,6 +1286,27 @@ async fn open_close_queried_buffer(
     expected_editor_title: &str,
     workspace: &View<Workspace>,
     cx: &mut gpui::VisualTestContext,
+) -> Vec<FoundPath> {
+    let history_items = open_queried_buffer(
+        input,
+        expected_matches,
+        expected_editor_title,
+        workspace,
+        cx,
+    )
+    .await;
+
+    cx.dispatch_action(workspace::CloseActiveItem { save_intent: None });
+
+    history_items
+}
+
+async fn open_queried_buffer(
+    input: &str,
+    expected_matches: usize,
+    expected_editor_title: &str,
+    workspace: &View<Workspace>,
+    cx: &mut gpui::VisualTestContext,
 ) -> Vec<FoundPath> {
     let picker = open_file_picker(&workspace, cx);
     cx.simulate_input(input);
@@ -1186,7 +1321,6 @@ async fn open_close_queried_buffer(
         finder.delegate.history_items.clone()
     });
 
-    cx.dispatch_action(SelectNext);
     cx.dispatch_action(Confirm);
 
     cx.read(|cx| {
@@ -1198,8 +1332,6 @@ async fn open_close_queried_buffer(
         );
     });
 
-    cx.dispatch_action(workspace::CloseActiveItem { save_intent: None });
-
     history_items
 }
 
@@ -1313,3 +1445,30 @@ fn collect_search_matches(picker: &Picker<FileFinderDelegate>) -> SearchEntries
             .collect(),
     }
 }
+
+fn assert_match_selection(
+    finder: &Picker<FileFinderDelegate>,
+    expected_selection_index: usize,
+    expected_file_name: &str,
+) {
+    assert_eq!(finder.delegate.selected_index(), expected_selection_index);
+    assert_match_at_position(finder, expected_selection_index, expected_file_name);
+}
+
+fn assert_match_at_position(
+    finder: &Picker<FileFinderDelegate>,
+    match_index: usize,
+    expected_file_name: &str,
+) {
+    let match_item = finder
+        .delegate
+        .matches
+        .get(match_index)
+        .expect("Finder should have a match item with the given index");
+    let match_file_name = match match_item {
+        Match::History(found_path, _) => found_path.absolute.as_deref().unwrap().file_name(),
+        Match::Search(path_match) => path_match.0.path.file_name(),
+    };
+    let match_file_name = match_file_name.unwrap().to_string_lossy().to_string();
+    assert_eq!(match_file_name.as_str(), expected_file_name);
+}