oak-team/oak-gpui
3
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Preserve selected entry in file_finder (#13452)

Browse Source 
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))
This commit is contained in:
kshokhin
2024-08-27 16:24:06 +03:00
committed by GitHub
parent 60af9dd4b1
commit 5586397f95
4 changed files with 375 additions and 127 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Cargo.lock
Generated
-1
View File
@@ -4126,7 +4126,6 @@ dependencies = [
"futures 0.3.30",
"fuzzy",
"gpui",
"itertools 0.11.0",
"language",
"menu",
"picker",
crates/file_finder/Cargo.toml
-1
View File
@@ -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
crates/file_finder/src/file_finder.rs
+211 -121
View File
@@ -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<AtomicBool>,
history_items: Vec<FoundPath>,
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<ProjectPanelOrdMatch>),
History {
path: FoundPath,
panel_match: Option<ProjectPanelOrdMatch>,
},
Search(ProjectPanelOrdMatch),
}
impl Match {
fn path(&self) -> &Arc<Path> {
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<usize, usize> {
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<Item = &'a FoundPath> + Clone,
@@ -230,88 +276,95 @@ impl Matches {
new_search_matches: impl Iterator<Item = ProjectPanelOrdMatch>,
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))
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 new_history_matches = matching_history_items(history_items, currently_opened, query);
let new_search_matches: Vec<Match> = new_search_matches
.filter(|path_match| !new_history_matches.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 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
}
(Match::History(_, _), Match::Search(_)) if self.separate_history => {
cmp::Ordering::Less
}
(Match::Search(_), Match::History(_, _)) if self.separate_history => {
cmp::Ordering::Greater
}
(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))
}
(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 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();
}
// 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<Match>, 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;
}
}
}
}
}
/// 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<Item = &'a FoundPath>,
currently_opened: Option<&'a FoundPath>,
query: Option<&FileSearchQuery>,
) -> HashMap<Arc<Path>, Option<ProjectPanelOrdMatch>> {
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<Arc<Path>, 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<usize>, String, Vec<usize>) {
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,
);
// 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.selected_index = 0;
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(),
crates/file_finder/src/file_finder_tests.rs
+164 -4
View File
@@ -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<FileFinderDelegate>) -> 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()