From 0102ffbfcab7fd6f74474e46b8e8b1746daa1041 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Thu, 1 Feb 2024 14:35:42 +0200 Subject: [PATCH] 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 | 1 + crates/file_finder/Cargo.toml | 1 + crates/file_finder/src/file_finder.rs | 175 ++++++++++---------- crates/file_finder/src/file_finder_tests.rs | 18 +- 4 files changed, 101 insertions(+), 94 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cd632175c1..14affc3281 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2661,6 +2661,7 @@ dependencies = [ "env_logger", "fuzzy", "gpui", + "itertools 0.11.0", "language", "menu", "picker", diff --git a/crates/file_finder/Cargo.toml b/crates/file_finder/Cargo.toml index 56b644e7f8..e67997f97d 100644 --- a/crates/file_finder/Cargo.toml +++ b/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 diff --git a/crates/file_finder/src/file_finder.rs b/crates/file_finder/src/file_finder.rs index a2d856760b..5880ec465b 100644 --- a/crates/file_finder/src/file_finder.rs +++ b/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::>(); 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, + currently_opened: Option<&FoundPath>, query: &PathLikeWithPosition, new_search_matches: impl Iterator, 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, ProjectPanelOrdMatch>>, + history_items: impl IntoIterator + '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::>(), + 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, + currently_opened: Option<&FoundPath>, query: &PathLikeWithPosition, ) -> HashMap, 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, - opened_file: bool, } impl FoundPath { fn new(project: ProjectPath, absolute: Option) -> Self { - Self { - project, - absolute, - opened_file: false, - } - } - - fn new_opened_file(project: ProjectPath, absolute: Option) -> 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(()) diff --git a/crates/file_finder/src/file_finder_tests.rs b/crates/file_finder/src/file_finder_tests.rs index 66d1675227..616437c26e 100644 --- a/crates/file_finder/src/file_finder_tests.rs +++ b/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) -> SearchEntries } } +#[track_caller] fn assert_match_selection( finder: &Picker, 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, 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); }