Sort file finder matched entries better (#6704)

* applicable history items were sorted by latest opened order, now
sorted by match score as the search matches
* adjust the match sorting to show paths in the alphanumerical order (in
case of a tie on other params)

Release Notes:

- Improved file finder entries ordering

---------

Co-authored-by: Piotr <piotr@zed.dev>

crates/file_finder/src/file_finder.rs

@@ -11,6 +11,7 @@ use gpui::{
 use picker::{Picker, PickerDelegate};
 use project::{PathMatchCandidateSet, Project, ProjectPath, WorktreeId};
 use std::{
+    cmp,
     path::{Path, PathBuf},
     sync::{
         atomic::{self, AtomicBool},
@@ -143,16 +144,51 @@ pub struct FileFinderDelegate {
     history_items: Vec<FoundPath>,
 }
 
+/// Use a custom ordering for file finder: the regular one
+/// defines max element with the highest score and the latest alphanumerical path (in case of a tie on other params), e.g:
+/// `[{score: 0.5, path = "c/d" }, { score: 0.5, path = "/a/b" }]`
+///
+/// In the file finder, we would prefer to have the max element with the highest score and the earliest alphanumerical path, e.g:
+/// `[{ score: 0.5, path = "/a/b" }, {score: 0.5, path = "c/d" }]`
+/// as the files are shown in the project panel lists.
+#[derive(Debug, Clone, PartialEq, Eq)]
+struct ProjectPanelOrdMatch(PathMatch);
+
+impl Ord for ProjectPanelOrdMatch {
+    fn cmp(&self, other: &Self) -> cmp::Ordering {
+        self.partial_cmp(other).unwrap()
+    }
+}
+
+impl PartialOrd for ProjectPanelOrdMatch {
+    fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> {
+        Some(
+            self.0
+                .score
+                .partial_cmp(&other.0.score)
+                .unwrap_or(cmp::Ordering::Equal)
+                .then_with(|| self.0.worktree_id.cmp(&other.0.worktree_id))
+                .then_with(|| {
+                    other
+                        .0
+                        .distance_to_relative_ancestor
+                        .cmp(&self.0.distance_to_relative_ancestor)
+                })
+                .then_with(|| self.0.path.cmp(&other.0.path).reverse()),
+        )
+    }
+}
+
 #[derive(Debug, Default)]
 struct Matches {
-    history: Vec<(FoundPath, Option<PathMatch>)>,
+    history: Vec<(FoundPath, Option<ProjectPanelOrdMatch>)>,
-    search: Vec<PathMatch>,
+    search: Vec<ProjectPanelOrdMatch>,
 }
 
 #[derive(Debug)]
 enum Match<'a> {
-    History(&'a FoundPath, Option<&'a PathMatch>),
+    History(&'a FoundPath, Option<&'a ProjectPanelOrdMatch>),
-    Search(&'a PathMatch),
+    Search(&'a ProjectPanelOrdMatch),
 }
 
 impl Matches {
@@ -176,45 +212,44 @@ impl Matches {
         &mut self,
         history_items: &Vec<FoundPath>,
         query: &PathLikeWithPosition<FileSearchQuery>,
-        mut new_search_matches: Vec<PathMatch>,
+        new_search_matches: impl Iterator<Item = ProjectPanelOrdMatch>,
         extend_old_matches: bool,
     ) {
         let matching_history_paths = matching_history_item_paths(history_items, query);
-        new_search_matches
+        let new_search_matches = new_search_matches
-            .retain(|path_match| !matching_history_paths.contains_key(&path_match.path));
+            .filter(|path_match| !matching_history_paths.contains_key(&path_match.0.path));
-        let history_items_to_show = history_items
+        let history_items_to_show = history_items.iter().filter_map(|history_item| {
-            .iter()
+            Some((
-            .filter_map(|history_item| {
+                history_item.clone(),
-                Some((
+                Some(
-                    history_item.clone(),
+                    matching_history_paths
-                    Some(
+                        .get(&history_item.project.path)?
-                        matching_history_paths
+                        .clone(),
-                            .get(&history_item.project.path)?
+                ),
-                            .clone(),
+            ))
-                    ),
+        });
-                ))
+        self.history.clear();
-            })
+        util::extend_sorted(
-            .collect::<Vec<_>>();
+            &mut self.history,
-        self.history = history_items_to_show;
+            history_items_to_show,
+            100,
+            |(_, a), (_, b)| b.cmp(a),
+        );
+
         if extend_old_matches {
             self.search
-                .retain(|path_match| !matching_history_paths.contains_key(&path_match.path));
+                .retain(|path_match| !matching_history_paths.contains_key(&path_match.0.path));
-            util::extend_sorted(
-                &mut self.search,
-                new_search_matches.into_iter(),
-                100,
-                |a, b| b.cmp(a),
-            )
         } else {
-            self.search = new_search_matches;
+            self.search.clear();
         }
+        util::extend_sorted(&mut self.search, new_search_matches, 100, |a, b| b.cmp(a));
     }
 }
 
 fn matching_history_item_paths(
     history_items: &Vec<FoundPath>,
     query: &PathLikeWithPosition<FileSearchQuery>,
-) -> HashMap<Arc<Path>, PathMatch> {
+) -> HashMap<Arc<Path>, ProjectPanelOrdMatch> {
     let history_items_by_worktrees = history_items
         .iter()
         .filter_map(|found_path| {
@@ -257,7 +292,12 @@ fn matching_history_item_paths(
                 max_results,
             )
             .into_iter()
-            .map(|path_match| (Arc::clone(&path_match.path), path_match)),
+            .map(|path_match| {
+                (
+                    Arc::clone(&path_match.path),
+                    ProjectPanelOrdMatch(path_match),
+                )
+            }),
         );
     }
     matching_history_paths
@@ -383,7 +423,9 @@ impl FileFinderDelegate {
                 &cancel_flag,
                 cx.background_executor().clone(),
             )
-            .await;
+            .await
+            .into_iter()
+            .map(ProjectPanelOrdMatch);
             let did_cancel = cancel_flag.load(atomic::Ordering::Relaxed);
             picker
                 .update(&mut cx, |picker, cx| {
@@ -401,7 +443,7 @@ impl FileFinderDelegate {
         search_id: usize,
         did_cancel: bool,
         query: PathLikeWithPosition<FileSearchQuery>,
-        matches: Vec<PathMatch>,
+        matches: impl IntoIterator<Item = ProjectPanelOrdMatch>,
         cx: &mut ViewContext<Picker<Self>>,
     ) {
         if search_id >= self.latest_search_id {
@@ -412,8 +454,12 @@ impl FileFinderDelegate {
                         .latest_search_query
                         .as_ref()
                         .map(|query| query.path_like.path_query());
-            self.matches
+            self.matches.push_new_matches(
-                .push_new_matches(&self.history_items, &query, matches, extend_old_matches);
+                &self.history_items,
+                &query,
+                matches.into_iter(),
+                extend_old_matches,
+            );
             self.latest_search_query = Some(query);
             self.latest_search_did_cancel = did_cancel;
             cx.notify();
@@ -471,12 +517,12 @@ impl FileFinderDelegate {
                 if let Some(found_path_match) = found_path_match {
                     path_match
                         .positions
-                        .extend(found_path_match.positions.iter())
+                        .extend(found_path_match.0.positions.iter())
                 }
 
                 self.labels_for_path_match(&path_match)
             }
-            Match::Search(path_match) => self.labels_for_path_match(path_match),
+            Match::Search(path_match) => self.labels_for_path_match(&path_match.0),
         };
 
         if file_name_positions.is_empty() {
@@ -556,14 +602,14 @@ impl FileFinderDelegate {
                             if let Some((worktree, relative_path)) =
                                 project.find_local_worktree(query_path, cx)
                             {
-                                path_matches.push(PathMatch {
+                                path_matches.push(ProjectPanelOrdMatch(PathMatch {
-                                    score: 0.0,
+                                    score: 1.0,
                                     positions: Vec::new(),
                                     worktree_id: worktree.read(cx).id().to_usize(),
                                     path: Arc::from(relative_path),
                                     path_prefix: "".into(),
                                     distance_to_relative_ancestor: usize::MAX,
-                                });
+                                }));
                             }
                         })
                         .log_err();
@@ -724,8 +770,8 @@ impl PickerDelegate for FileFinderDelegate {
                         Match::Search(m) => split_or_open(
                             workspace,
                             ProjectPath {
-                                worktree_id: WorktreeId::from_usize(m.worktree_id),
+                                worktree_id: WorktreeId::from_usize(m.0.worktree_id),
-                                path: m.path.clone(),
+                                path: m.0.path.clone(),
                             },
                             cx,
                         ),
@@ -805,3 +851,101 @@ impl PickerDelegate for FileFinderDelegate {
         )
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_custom_project_search_ordering_in_file_finder() {
+        let mut file_finder_sorted_output = vec![
+            ProjectPanelOrdMatch(PathMatch {
+                score: 0.5,
+                positions: Vec::new(),
+                worktree_id: 0,
+                path: Arc::from(Path::new("b0.5")),
+                path_prefix: Arc::from(""),
+                distance_to_relative_ancestor: 0,
+            }),
+            ProjectPanelOrdMatch(PathMatch {
+                score: 1.0,
+                positions: Vec::new(),
+                worktree_id: 0,
+                path: Arc::from(Path::new("c1.0")),
+                path_prefix: Arc::from(""),
+                distance_to_relative_ancestor: 0,
+            }),
+            ProjectPanelOrdMatch(PathMatch {
+                score: 1.0,
+                positions: Vec::new(),
+                worktree_id: 0,
+                path: Arc::from(Path::new("a1.0")),
+                path_prefix: Arc::from(""),
+                distance_to_relative_ancestor: 0,
+            }),
+            ProjectPanelOrdMatch(PathMatch {
+                score: 0.5,
+                positions: Vec::new(),
+                worktree_id: 0,
+                path: Arc::from(Path::new("a0.5")),
+                path_prefix: Arc::from(""),
+                distance_to_relative_ancestor: 0,
+            }),
+            ProjectPanelOrdMatch(PathMatch {
+                score: 1.0,
+                positions: Vec::new(),
+                worktree_id: 0,
+                path: Arc::from(Path::new("b1.0")),
+                path_prefix: Arc::from(""),
+                distance_to_relative_ancestor: 0,
+            }),
+        ];
+        file_finder_sorted_output.sort_by(|a, b| b.cmp(a));
+
+        assert_eq!(
+            file_finder_sorted_output,
+            vec![
+                ProjectPanelOrdMatch(PathMatch {
+                    score: 1.0,
+                    positions: Vec::new(),
+                    worktree_id: 0,
+                    path: Arc::from(Path::new("a1.0")),
+                    path_prefix: Arc::from(""),
+                    distance_to_relative_ancestor: 0,
+                }),
+                ProjectPanelOrdMatch(PathMatch {
+                    score: 1.0,
+                    positions: Vec::new(),
+                    worktree_id: 0,
+                    path: Arc::from(Path::new("b1.0")),
+                    path_prefix: Arc::from(""),
+                    distance_to_relative_ancestor: 0,
+                }),
+                ProjectPanelOrdMatch(PathMatch {
+                    score: 1.0,
+                    positions: Vec::new(),
+                    worktree_id: 0,
+                    path: Arc::from(Path::new("c1.0")),
+                    path_prefix: Arc::from(""),
+                    distance_to_relative_ancestor: 0,
+                }),
+                ProjectPanelOrdMatch(PathMatch {
+                    score: 0.5,
+                    positions: Vec::new(),
+                    worktree_id: 0,
+                    path: Arc::from(Path::new("a0.5")),
+                    path_prefix: Arc::from(""),
+                    distance_to_relative_ancestor: 0,
+                }),
+                ProjectPanelOrdMatch(PathMatch {
+                    score: 0.5,
+                    positions: Vec::new(),
+                    worktree_id: 0,
+                    path: Arc::from(Path::new("b0.5")),
+                    path_prefix: Arc::from(""),
+                    distance_to_relative_ancestor: 0,
+                }),
+            ]
+        );
+    }
+}

crates/file_finder/src/file_finder_tests.rs

@@ -114,7 +114,7 @@ async fn test_absolute_paths(cx: &mut TestAppContext) {
         .await;
     picker.update(cx, |picker, _| {
         assert_eq!(
-            collect_search_results(picker),
+            collect_search_matches(picker).search_only(),
             vec![PathBuf::from("a/b/file2.txt")],
             "Matching abs path should be the only match"
         )
@@ -136,7 +136,7 @@ async fn test_absolute_paths(cx: &mut TestAppContext) {
         .await;
     picker.update(cx, |picker, _| {
         assert_eq!(
-            collect_search_results(picker),
+            collect_search_matches(picker).search_only(),
             Vec::<PathBuf>::new(),
             "Mismatching abs path should produce no matches"
         )
@@ -169,7 +169,7 @@ async fn test_complex_path(cx: &mut TestAppContext) {
     picker.update(cx, |picker, _| {
         assert_eq!(picker.delegate.matches.len(), 1);
         assert_eq!(
-            collect_search_results(picker),
+            collect_search_matches(picker).search_only(),
             vec![PathBuf::from("其他/S数据表格/task.xlsx")],
         )
     });
@@ -486,7 +486,7 @@ async fn test_single_file_worktrees(cx: &mut TestAppContext) {
         assert_eq!(matches.len(), 1);
 
         let (file_name, file_name_positions, full_path, full_path_positions) =
-            delegate.labels_for_path_match(&matches[0]);
+            delegate.labels_for_path_match(&matches[0].0);
         assert_eq!(file_name, "the-file");
         assert_eq!(file_name_positions, &[0, 1, 4]);
         assert_eq!(full_path, "the-file");
@@ -556,9 +556,9 @@ async fn test_path_distance_ordering(cx: &mut TestAppContext) {
             delegate.matches.history.is_empty(),
             "Search matches expected"
         );
-        let matches = delegate.matches.search.clone();
+        let matches = &delegate.matches.search;
-        assert_eq!(matches[0].path.as_ref(), Path::new("dir2/a.txt"));
+        assert_eq!(matches[0].0.path.as_ref(), Path::new("dir2/a.txt"));
-        assert_eq!(matches[1].path.as_ref(), Path::new("dir1/a.txt"));
+        assert_eq!(matches[1].0.path.as_ref(), Path::new("dir1/a.txt"));
     });
 }
 
@@ -957,7 +957,7 @@ async fn test_search_preserves_history_items(cx: &mut gpui::TestAppContext) {
                 Some(PathBuf::from("/src/test/first.rs"))
             ));
             assert_eq!(delegate.matches.search.len(), 1, "Only one non-history item contains {first_query}, it should be present");
-            assert_eq!(delegate.matches.search.first().unwrap().path.as_ref(), Path::new("test/fourth.rs"));
+            assert_eq!(delegate.matches.search.first().unwrap().0.path.as_ref(), Path::new("test/fourth.rs"));
         });
 
     let second_query = "fsdasdsa";
@@ -1002,10 +1002,65 @@ async fn test_search_preserves_history_items(cx: &mut gpui::TestAppContext) {
                 Some(PathBuf::from("/src/test/first.rs"))
             ));
             assert_eq!(delegate.matches.search.len(), 1, "Only one non-history item contains {first_query_again}, it should be present, even after non-matching query");
-            assert_eq!(delegate.matches.search.first().unwrap().path.as_ref(), Path::new("test/fourth.rs"));
+            assert_eq!(delegate.matches.search.first().unwrap().0.path.as_ref(), Path::new("test/fourth.rs"));
         });
 }
 
+#[gpui::test]
+async fn test_search_sorts_history_items(cx: &mut gpui::TestAppContext) {
+    let app_state = init_test(cx);
+
+    app_state
+        .fs
+        .as_fake()
+        .insert_tree(
+            "/root",
+            json!({
+                "test": {
+                    "1_qw": "// First file that matches the query",
+                    "2_second": "// Second file",
+                    "3_third": "// Third file",
+                    "4_fourth": "// Fourth file",
+                    "5_qwqwqw": "// A file with 3 more matches than the first one",
+                    "6_qwqwqw": "// Same query matches as above, but closer to the end of the list due to the name",
+                    "7_qwqwqw": "// One more, same amount of query matches as above",
+                }
+            }),
+        )
+        .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));
+    // generate some history to select from
+    open_close_queried_buffer("1", 1, "1_qw", &workspace, cx).await;
+    open_close_queried_buffer("2", 1, "2_second", &workspace, cx).await;
+    open_close_queried_buffer("3", 1, "3_third", &workspace, cx).await;
+    open_close_queried_buffer("2", 1, "2_second", &workspace, cx).await;
+    open_close_queried_buffer("6", 1, "6_qwqwqw", &workspace, cx).await;
+
+    let finder = open_file_picker(&workspace, cx);
+    let query = "qw";
+    finder
+        .update(cx, |finder, cx| {
+            finder.delegate.update_matches(query.to_string(), cx)
+        })
+        .await;
+    finder.update(cx, |finder, _| {
+        let search_matches = collect_search_matches(finder);
+        assert_eq!(
+            search_matches.history,
+            vec![PathBuf::from("test/1_qw"), PathBuf::from("test/6_qwqwqw"),],
+        );
+        assert_eq!(
+            search_matches.search,
+            vec![
+                PathBuf::from("test/5_qwqwqw"),
+                PathBuf::from("test/7_qwqwqw"),
+            ],
+        );
+    });
+}
+
 #[gpui::test]
 async fn test_history_items_vs_very_good_external_match(cx: &mut gpui::TestAppContext) {
     let app_state = init_test(cx);
@@ -1048,14 +1103,14 @@ async fn test_history_items_vs_very_good_external_match(cx: &mut gpui::TestAppCo
                 .matches
                 .search
                 .iter()
-                .map(|path_match| path_match.path.to_path_buf())
+                .map(|path_match| path_match.0.path.to_path_buf())
                 .collect::<Vec<_>>();
             assert_eq!(
                 search_entries,
                 vec![
                     PathBuf::from("collab_ui/collab_ui.rs"),
-                    PathBuf::from("collab_ui/third.rs"),
                     PathBuf::from("collab_ui/first.rs"),
+                    PathBuf::from("collab_ui/third.rs"),
                     PathBuf::from("collab_ui/second.rs"),
                 ],
                 "Despite all search results having the same directory name, the most matching one should be on top"
@@ -1097,7 +1152,7 @@ async fn test_nonexistent_history_items_not_shown(cx: &mut gpui::TestAppContext)
                 .matches
                 .history
                 .iter()
-                .map(|(_, path_match)| path_match.as_ref().expect("should have a path match").path.to_path_buf())
+                .map(|(_, path_match)| path_match.as_ref().expect("should have a path match").0.path.to_path_buf())
                 .collect::<Vec<_>>();
             assert_eq!(
                 history_entries,
@@ -1124,7 +1179,8 @@ async fn open_close_queried_buffer(
         assert_eq!(
             finder.delegate.matches.len(),
             expected_matches,
-            "Unexpected number of matches found for query {input}"
+            "Unexpected number of matches found for query `{input}`, matches: {:?}",
+            finder.delegate.matches
         );
         finder.delegate.history_items.clone()
     });
@@ -1137,7 +1193,7 @@ async fn open_close_queried_buffer(
         let active_editor_title = active_editor.read(cx).title(cx);
         assert_eq!(
             expected_editor_title, active_editor_title,
-            "Unexpected editor title for query {input}"
+            "Unexpected editor title for query `{input}`"
         );
     });
 
@@ -1210,18 +1266,49 @@ fn active_file_picker(
     })
 }
 
-fn collect_search_results(picker: &Picker<FileFinderDelegate>) -> Vec<PathBuf> {
+#[derive(Debug)]
-    let matches = &picker.delegate.matches;
+struct SearchEntries {
-    assert!(
+    history: Vec<PathBuf>,
-        matches.history.is_empty(),
+    search: Vec<PathBuf>,
-        "Should have no history matches, but got: {:?}",
+}
-        matches.history
+
-    );
+impl SearchEntries {
-    let mut results = matches
+    #[track_caller]
-        .search
+    fn search_only(self) -> Vec<PathBuf> {
-        .iter()
+        assert!(
-        .map(|path_match| Path::new(path_match.path_prefix.as_ref()).join(&path_match.path))
+            self.history.is_empty(),
-        .collect::<Vec<_>>();
+            "Should have no history matches, but got: {:?}",
-    results.sort();
+            self.history
-    results
+        );
+        self.search
+    }
+}
+
+fn collect_search_matches(picker: &Picker<FileFinderDelegate>) -> SearchEntries {
+    let matches = &picker.delegate.matches;
+    SearchEntries {
+        history: matches
+            .history
+            .iter()
+            .map(|(history_path, path_match)| {
+                path_match
+                    .as_ref()
+                    .map(|path_match| {
+                        Path::new(path_match.0.path_prefix.as_ref()).join(&path_match.0.path)
+                    })
+                    .unwrap_or_else(|| {
+                        history_path
+                            .absolute
+                            .as_deref()
+                            .unwrap_or_else(|| &history_path.project.path)
+                            .to_path_buf()
+                    })
+            })
+            .collect(),
+        search: matches
+            .search
+            .iter()
+            .map(|path_match| Path::new(path_match.0.path_prefix.as_ref()).join(&path_match.0.path))
+            .collect(),
+    }
 }