From 5586397f952bdf34590b2d1a1097071dbd987a7d Mon Sep 17 00:00:00 2001 From: kshokhin Date: Tue, 27 Aug 2024 16:24:06 +0300 Subject: [PATCH] Preserve selected entry in file_finder (#13452) Closes #11737 If the query has not changed and entry is still in the matches list keep it selected Release Notes: - Fixes file finder jumping during background updates ([#11737](https://github.com/zed-industries/zed/issues/11737)) --- Cargo.lock | 1 - crates/file_finder/Cargo.toml | 1 - crates/file_finder/src/file_finder.rs | 328 +++++++++++++------- crates/file_finder/src/file_finder_tests.rs | 168 +++++++++- 4 files changed, 373 insertions(+), 125 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 03e7630ab2acd96329d21ca6acf11a94d1032b81..ddfdc4d6932b88c5c049426f696a5f132742a104 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4126,7 +4126,6 @@ dependencies = [ "futures 0.3.30", "fuzzy", "gpui", - "itertools 0.11.0", "language", "menu", "picker", diff --git a/crates/file_finder/Cargo.toml b/crates/file_finder/Cargo.toml index 72ac5e556257c5aa5ca5fafed41d32cbfcaf6254..8f17b191a530730bac2f5678e6dc3cee94667dc5 100644 --- a/crates/file_finder/Cargo.toml +++ b/crates/file_finder/Cargo.toml @@ -19,7 +19,6 @@ editor.workspace = true futures.workspace = true fuzzy.workspace = true gpui.workspace = true -itertools = "0.11" menu.workspace = true picker.workspace = true project.workspace = true diff --git a/crates/file_finder/src/file_finder.rs b/crates/file_finder/src/file_finder.rs index f74866292fe276f3f7d2174da0783044044d8a4f..c44da084ced3470af89bc84f2c009d85a5db46b4 100644 --- a/crates/file_finder/src/file_finder.rs +++ b/crates/file_finder/src/file_finder.rs @@ -4,7 +4,7 @@ mod file_finder_tests; mod new_path_prompt; mod open_path_prompt; -use collections::{BTreeSet, HashMap}; +use collections::HashMap; use editor::{scroll::Autoscroll, Bias, Editor}; use fuzzy::{CharBag, PathMatch, PathMatchCandidate}; use gpui::{ @@ -12,7 +12,6 @@ use gpui::{ FocusableView, Model, Modifiers, ModifiersChangedEvent, ParentElement, Render, Styled, Task, View, ViewContext, VisualContext, WeakView, }; -use itertools::Itertools; use new_path_prompt::NewPathPrompt; use open_path_prompt::OpenPathPrompt; use picker::{Picker, PickerDelegate}; @@ -166,6 +165,7 @@ pub struct FileFinderDelegate { cancel_flag: Arc, history_items: Vec, separate_history: bool, + first_update: bool, } /// Use a custom ordering for file finder: the regular one @@ -209,10 +209,29 @@ struct Matches { #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone)] enum Match { - History(FoundPath, Option), + History { + path: FoundPath, + panel_match: Option, + }, Search(ProjectPanelOrdMatch), } +impl Match { + fn path(&self) -> &Arc { + match self { + Match::History { path, .. } => &path.project.path, + Match::Search(panel_match) => &panel_match.0.path, + } + } + + fn panel_match(&self) -> Option<&ProjectPanelOrdMatch> { + match self { + Match::History { panel_match, .. } => panel_match.as_ref(), + Match::Search(panel_match) => Some(&panel_match), + } + } +} + impl Matches { fn len(&self) -> usize { self.matches.len() @@ -222,6 +241,33 @@ impl Matches { self.matches.get(index) } + fn position( + &self, + entry: &Match, + currently_opened: Option<&FoundPath>, + ) -> Result { + if let Match::History { + path, + panel_match: None, + } = entry + { + // Slow case: linear search by path. Should not happen actually, + // since we call `position` only if matches set changed, but the query has not changed. + // And History entries do not have panel_match if query is empty, so there's no + // reason for the matches set to change. + self.matches + .iter() + .position(|m| path.project.path == *m.path()) + .ok_or(0) + } else { + self.matches.binary_search_by(|m| { + // `reverse()` since if cmp_matches(a, b) == Ordering::Greater, then a is better than b. + // And we want the better entries go first. + Self::cmp_matches(self.separate_history, currently_opened, &m, &entry).reverse() + }) + } + } + fn push_new_matches<'a>( &'a mut self, history_items: impl IntoIterator + Clone, @@ -230,88 +276,95 @@ impl Matches { new_search_matches: impl Iterator, extend_old_matches: bool, ) { - let no_history_score = 0; - let matching_history_paths = - matching_history_item_paths(history_items.clone(), currently_opened, query); - let new_search_matches = new_search_matches - .filter(|path_match| !matching_history_paths.contains_key(&path_match.0.path)) - .map(Match::Search) - .map(|m| (no_history_score, m)); - let old_search_matches = self - .matches - .drain(..) - .filter(|_| extend_old_matches) - .filter(|m| matches!(m, Match::Search(_))) - .map(|m| (no_history_score, m)); - let history_matches = history_items - .into_iter() - .chain(currently_opened) - .enumerate() - .filter_map(|(i, history_item)| { - let query_match = matching_history_paths - .get(&history_item.project.path) - .cloned(); - let query_match = if query.is_some() { - query_match? - } else { - query_match.flatten() - }; - Some((i + 1, Match::History(history_item.clone(), query_match))) - }); + let Some(query) = query else { + // assuming that if there's no query, then there's no search matches. + self.matches.clear(); + let path_to_entry = |found_path: &FoundPath| Match::History { + path: found_path.clone(), + panel_match: None, + }; + self.matches + .extend(currently_opened.into_iter().map(path_to_entry)); + + self.matches.extend( + history_items + .into_iter() + .filter(|found_path| Some(*found_path) != currently_opened) + .map(path_to_entry), + ); + return; + }; - let mut unique_matches = BTreeSet::new(); - self.matches = old_search_matches - .chain(history_matches) - .chain(new_search_matches) - .filter(|(_, m)| unique_matches.insert(m.clone())) - .sorted_by(|(history_score_a, a), (history_score_b, b)| { - match (a, b) { - // bubble currently opened files to the top - (Match::History(path, _), _) if Some(path) == currently_opened => { - cmp::Ordering::Less - } - (_, Match::History(path, _)) if Some(path) == currently_opened => { - cmp::Ordering::Greater - } + let new_history_matches = matching_history_items(history_items, currently_opened, query); + let new_search_matches: Vec = new_search_matches + .filter(|path_match| !new_history_matches.contains_key(&path_match.0.path)) + .map(Match::Search) + .collect(); - (Match::History(_, _), Match::Search(_)) if self.separate_history => { - cmp::Ordering::Less - } - (Match::Search(_), Match::History(_, _)) if self.separate_history => { - cmp::Ordering::Greater - } + if extend_old_matches { + // since we take history matches instead of new search matches + // and history matches has not changed(since the query has not changed and we do not extend old matches otherwise), + // old matches can't contain paths present in history_matches as well. + self.matches.retain(|m| matches!(m, Match::Search(_))); + } else { + self.matches.clear(); + } - (Match::History(_, match_a), Match::History(_, match_b)) => { - match_b.cmp(match_a) - } - (Match::History(_, match_a), Match::Search(match_b)) => { - Some(match_b).cmp(&match_a.as_ref()) - } - (Match::Search(match_a), Match::History(_, match_b)) => { - match_b.as_ref().cmp(&Some(match_a)) + // At this point we have an unsorted set of new history matches, an unsorted set of new search matches + // and a sorted set of old search matches. + // It is possible that the new search matches' paths contain some of the old search matches' paths. + // History matches' paths are unique, since store in a HashMap by path. + // We build a sorted Vec, eliminating duplicate search matches. + // Search matches with the same paths should have equal `ProjectPanelOrdMatch`, so we should + // not have any duplicates after building the final list. + for new_match in new_history_matches + .into_values() + .chain(new_search_matches.into_iter()) + { + match self.position(&new_match, currently_opened) { + Ok(_duplicate) => continue, + Err(i) => { + self.matches.insert(i, new_match); + if self.matches.len() == 100 { + break; } - (Match::Search(match_a), Match::Search(match_b)) => match_b.cmp(match_a), } - .then(history_score_a.cmp(history_score_b)) - }) - .take(100) - .map(|(_, m)| m) - .collect(); + } + } + } + + /// If a < b, then a is a worse match, aligning with the `ProjectPanelOrdMatch` ordering. + fn cmp_matches( + separate_history: bool, + currently_opened: Option<&FoundPath>, + a: &Match, + b: &Match, + ) -> cmp::Ordering { + debug_assert!(a.panel_match().is_some() && b.panel_match().is_some()); + + match (&a, &b) { + // bubble currently opened files to the top + (Match::History { path, .. }, _) if Some(path) == currently_opened => { + cmp::Ordering::Greater + } + (_, Match::History { path, .. }) if Some(path) == currently_opened => { + cmp::Ordering::Less + } + + (Match::History { .. }, Match::Search(_)) if separate_history => cmp::Ordering::Greater, + (Match::Search(_), Match::History { .. }) if separate_history => cmp::Ordering::Less, + + _ => a.panel_match().cmp(&b.panel_match()), + } } } -fn matching_history_item_paths<'a>( +fn matching_history_items<'a>( history_items: impl IntoIterator, currently_opened: Option<&'a FoundPath>, - query: Option<&FileSearchQuery>, -) -> HashMap, Option> { - let Some(query) = query else { - return history_items - .into_iter() - .chain(currently_opened) - .map(|found_path| (Arc::clone(&found_path.project.path), None)) - .collect(); - }; + query: &FileSearchQuery, +) -> HashMap, Match> { + let mut candidates_paths = HashMap::default(); let history_items_by_worktrees = history_items .into_iter() @@ -333,6 +386,7 @@ fn matching_history_item_paths<'a>( .chars(), ), }; + candidates_paths.insert(Arc::clone(&found_path.project.path), found_path); Some((found_path.project.worktree_id, candidate)) }) .fold( @@ -358,9 +412,15 @@ fn matching_history_item_paths<'a>( ) .into_iter() .map(|path_match| { + let (_, found_path) = candidates_paths + .remove_entry(&path_match.path) + .expect("candidate info not found"); ( Arc::clone(&path_match.path), - Some(ProjectPanelOrdMatch(path_match)), + Match::History { + path: found_path.clone(), + panel_match: Some(ProjectPanelOrdMatch(path_match)), + }, ) }), ); @@ -439,6 +499,7 @@ impl FileFinderDelegate { cancel_flag: Arc::new(AtomicBool::new(false)), history_items, separate_history, + first_update: true, } } @@ -524,12 +585,19 @@ impl FileFinderDelegate { ) { if search_id >= self.latest_search_id { self.latest_search_id = search_id; - let extend_old_matches = self.latest_search_did_cancel - && Some(query.path_query()) - == self - .latest_search_query - .as_ref() - .map(|query| query.path_query()); + let query_changed = Some(query.path_query()) + != self + .latest_search_query + .as_ref() + .map(|query| query.path_query()); + let extend_old_matches = self.latest_search_did_cancel && !query_changed; + + let selected_match = if query_changed { + None + } else { + self.matches.get(self.selected_index).cloned() + }; + self.matches.push_new_matches( &self.history_items, self.currently_opened_path.as_ref(), @@ -537,9 +605,19 @@ impl FileFinderDelegate { matches.into_iter(), extend_old_matches, ); + + self.selected_index = selected_match.map_or_else( + || self.calculate_selected_index(), + |m| { + self.matches + .position(&m, self.currently_opened_path.as_ref()) + .unwrap_or(0) + }, + ); + self.latest_search_query = Some(query); self.latest_search_did_cancel = did_cancel; - self.selected_index = self.calculate_selected_index(); + cx.notify(); } } @@ -550,10 +628,13 @@ impl FileFinderDelegate { cx: &AppContext, ix: usize, ) -> (String, Vec, String, Vec) { - let (file_name, file_name_positions, full_path, full_path_positions) = match path_match { - Match::History(found_path, found_path_match) => { - let worktree_id = found_path.project.worktree_id; - let project_relative_path = &found_path.project.path; + let (file_name, file_name_positions, full_path, full_path_positions) = match &path_match { + Match::History { + path: entry_path, + panel_match, + } => { + let worktree_id = entry_path.project.worktree_id; + let project_relative_path = &entry_path.project.path; let has_worktree = self .project .read(cx) @@ -561,7 +642,7 @@ impl FileFinderDelegate { .is_some(); if !has_worktree { - if let Some(absolute_path) = &found_path.absolute { + if let Some(absolute_path) = &entry_path.absolute { return ( absolute_path .file_name() @@ -579,7 +660,7 @@ impl FileFinderDelegate { let mut path = Arc::clone(project_relative_path); if project_relative_path.as_ref() == Path::new("") { - if let Some(absolute_path) = &found_path.absolute { + if let Some(absolute_path) = &entry_path.absolute { path = Arc::from(absolute_path.as_path()); } } @@ -593,7 +674,7 @@ impl FileFinderDelegate { path_prefix: "".into(), distance_to_relative_ancestor: usize::MAX, }; - if let Some(found_path_match) = found_path_match { + if let Some(found_path_match) = &panel_match { path_match .positions .extend(found_path_match.0.positions.iter()) @@ -718,7 +799,7 @@ impl FileFinderDelegate { /// Skips first history match (that is displayed topmost) if it's currently opened. fn calculate_selected_index(&self) -> usize { - if let Some(Match::History(path, _)) = self.matches.get(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 { @@ -726,6 +807,7 @@ impl FileFinderDelegate { } } } + 0 } } @@ -758,7 +840,7 @@ impl PickerDelegate for FileFinderDelegate { .matches .iter() .enumerate() - .find(|(_, m)| !matches!(m, Match::History(_, _))) + .find(|(_, m)| !matches!(m, Match::History { .. })) .map(|(i, _)| i); if let Some(first_non_history_index) = first_non_history_index { if first_non_history_index > 0 { @@ -777,26 +859,34 @@ impl PickerDelegate for FileFinderDelegate { let raw_query = raw_query.replace(' ', ""); let raw_query = raw_query.trim(); if raw_query.is_empty() { - let project = self.project.read(cx); - self.latest_search_id = post_inc(&mut self.search_count); - self.matches = Matches { - separate_history: self.separate_history, - ..Matches::default() - }; - self.matches.push_new_matches( - self.history_items.iter().filter(|history_item| { - project - .worktree_for_id(history_item.project.worktree_id, cx) - .is_some() - || (project.is_local_or_ssh() && history_item.absolute.is_some()) - }), - self.currently_opened_path.as_ref(), - None, - None.into_iter(), - false, - ); - - self.selected_index = 0; + // if there was no query before, and we already have some (history) matches + // there's no need to update anything, since nothing has changed. + // We also want to populate matches set from history entries on the first update. + if self.latest_search_query.is_some() || self.first_update { + let project = self.project.read(cx); + + self.latest_search_id = post_inc(&mut self.search_count); + self.latest_search_query = None; + self.matches = Matches { + separate_history: self.separate_history, + ..Matches::default() + }; + self.matches.push_new_matches( + self.history_items.iter().filter(|history_item| { + project + .worktree_for_id(history_item.project.worktree_id, cx) + .is_some() + || (project.is_local_or_ssh() && history_item.absolute.is_some()) + }), + self.currently_opened_path.as_ref(), + None, + None.into_iter(), + false, + ); + + self.first_update = false; + self.selected_index = 0; + } cx.notify(); Task::ready(()) } else { @@ -843,9 +933,9 @@ impl PickerDelegate for FileFinderDelegate { ) } }; - match m { - Match::History(history_match, _) => { - let worktree_id = history_match.project.worktree_id; + match &m { + Match::History { path, .. } => { + let worktree_id = path.project.worktree_id; if workspace .project() .read(cx) @@ -856,12 +946,12 @@ impl PickerDelegate for FileFinderDelegate { workspace, ProjectPath { worktree_id, - path: Arc::clone(&history_match.project.path), + path: Arc::clone(&path.project.path), }, cx, ) } else { - match history_match.absolute.as_ref() { + match path.absolute.as_ref() { Some(abs_path) => { if secondary { workspace.split_abs_path( @@ -881,7 +971,7 @@ impl PickerDelegate for FileFinderDelegate { workspace, ProjectPath { worktree_id, - path: Arc::clone(&history_match.project.path), + path: Arc::clone(&path.project.path), }, cx, ), @@ -957,7 +1047,7 @@ impl PickerDelegate for FileFinderDelegate { .expect("Invalid matches state: no element for index {ix}"); let icon = match &path_match { - Match::History(_, _) => Icon::new(IconName::HistoryRerun) + Match::History { .. } => Icon::new(IconName::HistoryRerun) .color(Color::Muted) .size(IconSize::Small) .into_any_element(), diff --git a/crates/file_finder/src/file_finder_tests.rs b/crates/file_finder/src/file_finder_tests.rs index c6ce27d4f45bc0ba63028a5a1f7ecbb08fffec83..25ed9fcd39907af8167c202ad66e8bbf918f9729 100644 --- a/crates/file_finder/src/file_finder_tests.rs +++ b/crates/file_finder/src/file_finder_tests.rs @@ -1323,6 +1323,62 @@ async fn test_history_items_shown_in_order_of_open(cx: &mut TestAppContext) { }); } +#[gpui::test] +async fn test_selected_history_item_stays_selected_on_worktree_updated(cx: &mut TestAppContext) { + let app_state = init_test(cx); + + app_state + .fs + .as_fake() + .insert_tree( + "/test", + json!({ + "test": { + "1.txt": "// One", + "2.txt": "// Two", + "3.txt": "// Three", + } + }), + ) + .await; + + let project = Project::test(app_state.fs.clone(), ["/test".as_ref()], cx).await; + let (workspace, cx) = cx.add_window_view(|cx| Workspace::test_new(project, cx)); + + open_close_queried_buffer("1", 1, "1.txt", &workspace, cx).await; + open_close_queried_buffer("2", 1, "2.txt", &workspace, cx).await; + open_close_queried_buffer("3", 1, "3.txt", &workspace, cx).await; + + let picker = open_file_picker(&workspace, cx); + picker.update(cx, |finder, _| { + assert_eq!(finder.delegate.matches.len(), 3); + assert_match_selection(finder, 0, "3.txt"); + assert_match_at_position(finder, 1, "2.txt"); + assert_match_at_position(finder, 2, "1.txt"); + }); + + cx.dispatch_action(SelectNext); + + // Add more files to the worktree to trigger update matches + for i in 0..5 { + let filename = format!("/test/{}.txt", 4 + i); + app_state + .fs + .create_file(Path::new(&filename), Default::default()) + .await + .expect("unable to create file"); + } + + cx.executor().advance_clock(FS_WATCH_LATENCY); + + picker.update(cx, |finder, _| { + assert_eq!(finder.delegate.matches.len(), 3); + assert_match_at_position(finder, 0, "3.txt"); + assert_match_selection(finder, 1, "2.txt"); + assert_match_at_position(finder, 2, "1.txt"); + }); +} + #[gpui::test] async fn test_history_items_vs_very_good_external_match(cx: &mut gpui::TestAppContext) { let app_state = init_test(cx); @@ -1541,6 +1597,107 @@ async fn test_search_results_refreshed_on_adding_and_removing_worktrees( }); } +#[gpui::test] +async fn test_selected_match_stays_selected_after_matches_refreshed(cx: &mut gpui::TestAppContext) { + let app_state = init_test(cx); + + app_state.fs.as_fake().insert_tree("/src", json!({})).await; + + app_state + .fs + .create_dir("/src/even".as_ref()) + .await + .expect("unable to create dir"); + + let initial_files_num = 5; + for i in 0..initial_files_num { + let filename = format!("/src/even/file_{}.txt", 10 + i); + app_state + .fs + .create_file(Path::new(&filename), Default::default()) + .await + .expect("unable to create file"); + } + + 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.clone(), cx)); + + // Initial state + let picker = open_file_picker(&workspace, cx); + cx.simulate_input("file"); + let selected_index = 3; + // Checking only the filename, not the whole path + let selected_file = format!("file_{}.txt", 10 + selected_index); + // Select even/file_13.txt + for _ in 0..selected_index { + cx.dispatch_action(SelectNext); + } + + picker.update(cx, |finder, _| { + assert_match_selection(finder, selected_index, &selected_file) + }); + + // Add more matches to the search results + let files_to_add = 10; + for i in 0..files_to_add { + let filename = format!("/src/file_{}.txt", 20 + i); + app_state + .fs + .create_file(Path::new(&filename), Default::default()) + .await + .expect("unable to create file"); + } + cx.executor().advance_clock(FS_WATCH_LATENCY); + + // file_13.txt is still selected + picker.update(cx, |finder, _| { + let expected_selected_index = selected_index + files_to_add; + assert_match_selection(finder, expected_selected_index, &selected_file); + }); +} + +#[gpui::test] +async fn test_first_match_selected_if_previous_one_is_not_in_the_match_list( + cx: &mut gpui::TestAppContext, +) { + let app_state = init_test(cx); + + app_state + .fs + .as_fake() + .insert_tree( + "/src", + json!({ + "file_1.txt": "// file_1", + "file_2.txt": "// file_2", + "file_3.txt": "// file_3", + }), + ) + .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.clone(), cx)); + + // Initial state + let picker = open_file_picker(&workspace, cx); + cx.simulate_input("file"); + // Select even/file_2.txt + cx.dispatch_action(SelectNext); + + // Remove the selected entry + app_state + .fs + .remove_file("/src/file_2.txt".as_ref(), Default::default()) + .await + .expect("unable to remove file"); + cx.executor().advance_clock(FS_WATCH_LATENCY); + + // file_1.txt is now selected + picker.update(cx, |finder, _| { + assert_match_selection(finder, 0, "file_1.txt"); + }); +} + #[gpui::test] async fn test_keeps_file_finder_open_after_modifier_keys_release(cx: &mut gpui::TestAppContext) { let app_state = init_test(cx); @@ -1940,8 +2097,11 @@ impl SearchEntries { fn collect_search_matches(picker: &Picker) -> SearchEntries { let mut search_entries = SearchEntries::default(); for m in &picker.delegate.matches.matches { - match m { - Match::History(history_path, path_match) => { + match &m { + Match::History { + path: history_path, + panel_match: path_match, + } => { search_entries.history.push( path_match .as_ref() @@ -1996,8 +2156,8 @@ fn assert_match_at_position( .matches .get(match_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(), + let match_file_name = match &match_item { + Match::History { path, .. } => path.absolute.as_deref().unwrap().file_name(), Match::Search(path_match) => path_match.0.path.file_name(), } .unwrap()