From 2b844f5cb537bb1fb08e26054906f98db974faa5 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Thu, 25 Jan 2024 15:25:33 +0200 Subject: [PATCH] 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 --- crates/file_finder/src/file_finder.rs | 228 ++++++++++++++++---- crates/file_finder/src/file_finder_tests.rs | 143 +++++++++--- 2 files changed, 301 insertions(+), 70 deletions(-) diff --git a/crates/file_finder/src/file_finder.rs b/crates/file_finder/src/file_finder.rs index 672ed6272e..63a1966fbe 100644 --- a/crates/file_finder/src/file_finder.rs +++ b/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, } +/// 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 { + 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)>, - search: Vec, + history: Vec<(FoundPath, Option)>, + search: Vec, } #[derive(Debug)] enum Match<'a> { - History(&'a FoundPath, Option<&'a PathMatch>), - Search(&'a PathMatch), + History(&'a FoundPath, Option<&'a ProjectPanelOrdMatch>), + Search(&'a ProjectPanelOrdMatch), } impl Matches { @@ -176,45 +212,44 @@ impl Matches { &mut self, history_items: &Vec, query: &PathLikeWithPosition, - mut new_search_matches: Vec, + new_search_matches: impl Iterator, extend_old_matches: bool, ) { let matching_history_paths = matching_history_item_paths(history_items, query); - new_search_matches - .retain(|path_match| !matching_history_paths.contains_key(&path_match.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(), - ), - )) - }) - .collect::>(); - self.history = history_items_to_show; + 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, + |(_, a), (_, b)| b.cmp(a), + ); + if extend_old_matches { self.search - .retain(|path_match| !matching_history_paths.contains_key(&path_match.path)); - util::extend_sorted( - &mut self.search, - new_search_matches.into_iter(), - 100, - |a, b| b.cmp(a), - ) + .retain(|path_match| !matching_history_paths.contains_key(&path_match.0.path)); } 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, query: &PathLikeWithPosition, -) -> HashMap, PathMatch> { +) -> HashMap, 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, - matches: Vec, + matches: impl IntoIterator, cx: &mut ViewContext>, ) { 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 - .push_new_matches(&self.history_items, &query, matches, extend_old_matches); + self.matches.push_new_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 { - score: 0.0, + path_matches.push(ProjectPanelOrdMatch(PathMatch { + 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), - path: m.path.clone(), + worktree_id: WorktreeId::from_usize(m.0.worktree_id), + 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, + }), + ] + ); + } +} diff --git a/crates/file_finder/src/file_finder_tests.rs b/crates/file_finder/src/file_finder_tests.rs index 107fe891be..ca07cbf083 100644 --- a/crates/file_finder/src/file_finder_tests.rs +++ b/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::::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(); - assert_eq!(matches[0].path.as_ref(), Path::new("dir2/a.txt")); - assert_eq!(matches[1].path.as_ref(), Path::new("dir1/a.txt")); + let matches = &delegate.matches.search; + assert_eq!(matches[0].0.path.as_ref(), Path::new("dir2/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::>(); 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::>(); 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) -> Vec { - let matches = &picker.delegate.matches; - assert!( - matches.history.is_empty(), - "Should have no history matches, but got: {:?}", - matches.history - ); - let mut results = matches - .search - .iter() - .map(|path_match| Path::new(path_match.path_prefix.as_ref()).join(&path_match.path)) - .collect::>(); - results.sort(); - results +#[derive(Debug)] +struct SearchEntries { + history: Vec, + search: Vec, +} + +impl SearchEntries { + #[track_caller] + fn search_only(self) -> Vec { + assert!( + self.history.is_empty(), + "Should have no history matches, but got: {:?}", + self.history + ); + self.search + } +} + +fn collect_search_matches(picker: &Picker) -> 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(), + } }