Refactor file_finder send element open code (#7210)

Follow-up of https://github.com/zed-industries/zed/pull/6947 (cc
@alygin) that fixes a few style nits and refactors the code around:
* use already stored `currently_opened_path` to decide what to do with
the history item sorting
* use the same method to set history items, encapsulate the bubbling up
logic there
* ensure history elements are properly sorted before populating

The main reason to change all that is the new comparator in the previous
version:
https://github.com/zed-industries/zed/pull/6947/files#diff-eac7c8c99856f77cee39117708cd1467fd5bbc8805da2564f851951638020842R234
that almost violated `util::extend_sorted` contract, requiring both
collections to be sorted the same way as the comparator would be: it did
work, because we bubbled currently open item up in the history items
list manually, and that we have only one such item.

Release Notes:
- N/A

Cargo.lock

@@ -2661,6 +2661,7 @@ dependencies = [
  "env_logger",
  "fuzzy",
  "gpui",
+ "itertools 0.11.0",
  "language",
  "menu",
  "picker",

crates/file_finder/Cargo.toml

@@ -15,6 +15,7 @@ collections = { path = "../collections" }
 editor = { path = "../editor" }
 fuzzy = {  path = "../fuzzy" }
 gpui = { path = "../gpui" }
+itertools = "0.11"
 menu = { path = "../menu" }
 picker = { path = "../picker" }
 postage.workspace = true

crates/file_finder/src/file_finder.rs

@@ -8,6 +8,7 @@ use gpui::{
     actions, rems, AppContext, DismissEvent, EventEmitter, FocusHandle, FocusableView, Model,
     ParentElement, Render, Styled, Task, View, ViewContext, VisualContext, WeakView,
 };
+use itertools::Itertools;
 use picker::{Picker, PickerDelegate};
 use project::{PathMatchCandidateSet, Project, ProjectPath, WorktreeId};
 use std::{
@@ -61,36 +62,18 @@ 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_opened_file(project_path, abs_path)
+                FoundPath::new(project_path, abs_path)
             });
 
-        // if exists, bubble the currently opened path to the top
-        let history_items = currently_opened_path
-            .clone()
+        let history_items = workspace
+            .recent_navigation_history(Some(MAX_RECENT_SELECTIONS), cx)
             .into_iter()
-            .chain(
-                workspace
-                    .recent_navigation_history(Some(MAX_RECENT_SELECTIONS), cx)
-                    .into_iter()
-                    .filter(|(history_path, _)| {
-                        Some(history_path)
-                            != currently_opened_path
-                                .as_ref()
-                                .map(|found_path| &found_path.project)
-                    })
-                    .filter(|(_, history_abs_path)| {
-                        history_abs_path.as_ref()
-                            != currently_opened_path
-                                .as_ref()
-                                .and_then(|found_path| found_path.absolute.as_ref())
-                    })
-                    .filter(|(_, history_abs_path)| match history_abs_path {
-                        Some(abs_path) => history_file_exists(abs_path),
-                        None => true,
-                    })
-                    .map(|(history_path, abs_path)| FoundPath::new(history_path, abs_path)),
-            )
-            .collect();
+            .filter(|(_, history_abs_path)| match history_abs_path {
+                Some(abs_path) => history_file_exists(abs_path),
+                None => true,
+            })
+            .map(|(history_path, abs_path)| FoundPath::new(history_path, abs_path))
+            .collect::<Vec<_>>();
 
         let project = workspace.project().clone();
         let weak_workspace = cx.view().downgrade();
@@ -209,39 +192,21 @@ impl Matches {
     fn push_new_matches(
         &mut self,
         history_items: &Vec<FoundPath>,
+        currently_opened: Option<&FoundPath>,
         query: &PathLikeWithPosition<FileSearchQuery>,
         new_search_matches: impl Iterator<Item = ProjectPanelOrdMatch>,
         extend_old_matches: bool,
     ) {
-        let matching_history_paths = matching_history_item_paths(history_items, query);
+        let matching_history_paths =
+            matching_history_item_paths(history_items, currently_opened, query);
         let new_search_matches = new_search_matches
             .filter(|path_match| !matching_history_paths.contains_key(&path_match.0.path));
-        let history_items_to_show = history_items.iter().filter_map(|history_item| {
-            Some((
-                history_item.clone(),
-                Some(
-                    matching_history_paths
-                        .get(&history_item.project.path)?
-                        .clone(),
-                ),
-            ))
-        });
-        self.history.clear();
-        util::extend_sorted(
-            &mut self.history,
-            history_items_to_show,
-            100,
-            |(ap, a), (bp, b)| {
-                if ap.opened_file {
-                    return cmp::Ordering::Less;
-                }
-                if bp.opened_file {
-                    return cmp::Ordering::Greater;
-                }
-                b.cmp(a)
-            },
-        );
 
+        self.set_new_history(
+            currently_opened,
+            Some(&matching_history_paths),
+            history_items,
+        );
         if extend_old_matches {
             self.search
                 .retain(|path_match| !matching_history_paths.contains_key(&path_match.0.path));
@@ -250,14 +215,59 @@ impl Matches {
         }
         util::extend_sorted(&mut self.search, new_search_matches, 100, |a, b| b.cmp(a));
     }
+
+    fn set_new_history<'a>(
+        &mut self,
+        currently_opened: Option<&'a FoundPath>,
+        query_matches: Option<&'a HashMap<Arc<Path>, ProjectPanelOrdMatch>>,
+        history_items: impl IntoIterator<Item = &'a FoundPath> + 'a,
+    ) {
+        let history_items_to_show = history_items
+            .into_iter()
+            .chain(currently_opened)
+            .filter_map(|history_item| match &query_matches {
+                Some(query_matches) => Some((
+                    history_item.clone(),
+                    Some(query_matches.get(&history_item.project.path)?.clone()),
+                )),
+                None => Some((history_item.clone(), None)),
+            })
+            .sorted_by(|(path_a, match_a), (path_b, match_b)| {
+                match (
+                    Some(path_a) == currently_opened,
+                    Some(path_b) == currently_opened,
+                ) {
+                    (true, false) => cmp::Ordering::Less,
+                    (false, true) => cmp::Ordering::Greater,
+                    _ => match_b.cmp(match_a),
+                }
+            });
+
+        self.history.clear();
+        util::extend_sorted(
+            &mut self.history,
+            history_items_to_show.collect::<Vec<_>>(),
+            100,
+            |(path_a, match_a), (path_b, match_b)| match (
+                Some(path_a) == currently_opened,
+                Some(path_b) == currently_opened,
+            ) {
+                (true, false) => cmp::Ordering::Less,
+                (false, true) => cmp::Ordering::Greater,
+                _ => match_b.cmp(match_a).then(path_b.cmp(path_a)),
+            },
+        );
+    }
 }
 
 fn matching_history_item_paths(
     history_items: &Vec<FoundPath>,
+    currently_opened: Option<&FoundPath>,
     query: &PathLikeWithPosition<FileSearchQuery>,
 ) -> HashMap<Arc<Path>, ProjectPanelOrdMatch> {
     let history_items_by_worktrees = history_items
         .iter()
+        .chain(currently_opened)
         .filter_map(|found_path| {
             let candidate = PathMatchCandidate {
                 path: &found_path.project.path,
@@ -309,28 +319,15 @@ fn matching_history_item_paths(
     matching_history_paths
 }
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
 struct FoundPath {
     project: ProjectPath,
     absolute: Option<PathBuf>,
-    opened_file: bool,
 }
 
 impl FoundPath {
     fn new(project: ProjectPath, absolute: Option<PathBuf>) -> Self {
-        Self {
-            project,
-            absolute,
-            opened_file: false,
-        }
-    }
-
-    fn new_opened_file(project: ProjectPath, absolute: Option<PathBuf>) -> Self {
-        Self {
-            project,
-            absolute,
-            opened_file: true,
-        }
+        Self { project, absolute }
     }
 }
 
@@ -474,6 +471,7 @@ impl FileFinderDelegate {
                         .map(|query| query.path_like.path_query());
             self.matches.push_new_matches(
                 &self.history_items,
+                self.currently_opened_path.as_ref(),
                 &query,
                 matches.into_iter(),
                 extend_old_matches,
@@ -652,18 +650,17 @@ impl FileFinderDelegate {
         })
     }
 
-    /// 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.
+    /// Skips first history match (that is displayed topmost) if it's currently opened.
     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;
+        if let Some(Match::History(path, _)) = self.matches.get(0) {
+            if Some(path) == self.currently_opened_path.as_ref() {
+                let elements_after_first = self.matches.len() - 1;
+                if elements_after_first > 0 {
+                    return 1;
+                }
             }
-        } else {
-            return 0;
         }
-        (self.matches.len() - 1).min(1)
+        0
     }
 }
 
@@ -707,20 +704,20 @@ impl PickerDelegate for FileFinderDelegate {
             let project = self.project.read(cx);
             self.latest_search_id = post_inc(&mut self.search_count);
             self.matches = Matches {
-                history: self
-                    .history_items
-                    .iter()
-                    .filter(|history_item| {
-                        project
-                            .worktree_for_id(history_item.project.worktree_id, cx)
-                            .is_some()
-                            || (project.is_local() && history_item.absolute.is_some())
-                    })
-                    .cloned()
-                    .map(|p| (p, None))
-                    .collect(),
+                history: Vec::new(),
                 search: Vec::new(),
             };
+            self.matches.set_new_history(
+                self.currently_opened_path.as_ref(),
+                None,
+                self.history_items.iter().filter(|history_item| {
+                    project
+                        .worktree_for_id(history_item.project.worktree_id, cx)
+                        .is_some()
+                        || (project.is_local() && history_item.absolute.is_some())
+                }),
+            );
+
             self.selected_index = self.calculate_selected_index();
             cx.notify();
             Task::ready(())

crates/file_finder/src/file_finder_tests.rs

@@ -1126,6 +1126,7 @@ async fn test_keep_opened_file_on_top_of_search_results_and_select_next_one(
         assert_eq!(finder.delegate.matches.len(), 3);
         assert_match_at_position(finder, 0, "main.rs");
         assert_match_selection(finder, 1, "lib.rs");
+        assert_match_at_position(finder, 2, "bar.rs");
     });
 
     // all files match, main.rs is still on top
@@ -1446,15 +1447,21 @@ fn collect_search_matches(picker: &Picker<FileFinderDelegate>) -> SearchEntries
     }
 }
 
+#[track_caller]
 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_eq!(
+        finder.delegate.selected_index(),
+        expected_selection_index,
+        "Match is not selected"
+    );
     assert_match_at_position(finder, expected_selection_index, expected_file_name);
 }
 
+#[track_caller]
 fn assert_match_at_position(
     finder: &Picker<FileFinderDelegate>,
     match_index: usize,
@@ -1464,11 +1471,12 @@ fn assert_match_at_position(
         .delegate
         .matches
         .get(match_index)
-        .expect("Finder should have a match item with the given index");
+        .unwrap_or_else(|| panic!("Finder has no match for index {match_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);
+    }
+    .unwrap()
+    .to_string_lossy();
+    assert_eq!(match_file_name, expected_file_name);
 }