Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve sorting for the file picker #1066

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion helix-term/src/application.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,11 @@ impl Application {
if first.is_dir() {
std::env::set_current_dir(&first)?;
editor.new_file(Action::VerticalSplit);
compositor.push(Box::new(ui::file_picker(".".into(), &config.editor)));
compositor.push(Box::new(ui::file_picker(
".".into(),
Some(".".into()),
&config.editor,
)));
} else {
let nr_of_files = args.files.len();
editor.open(first.to_path_buf(), Action::VerticalSplit)?;
Expand Down
9 changes: 8 additions & 1 deletion helix-term/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1575,6 +1575,7 @@ fn global_search(cx: &mut Context) {
align_view(doc, view, Align::Center);
},
|_editor, (line_num, path)| Some((path.clone(), Some((*line_num, *line_num)))),
|_, l_score, _, r_score| l_score.cmp(&r_score).reverse(),
);
compositor.push(Box::new(picker));
});
Expand Down Expand Up @@ -2757,7 +2758,8 @@ fn command_mode(cx: &mut Context) {

fn file_picker(cx: &mut Context) {
let root = find_root(None).unwrap_or_else(|| PathBuf::from("./"));
let picker = ui::file_picker(root, &cx.editor.config);
let current_dir = std::env::current_dir().ok();
let picker = ui::file_picker(root, current_dir, &cx.editor.config);
cx.push_layer(Box::new(picker));
}

Expand Down Expand Up @@ -2825,6 +2827,7 @@ fn buffer_picker(cx: &mut Context) {
.cursor_line(doc.text().slice(..));
Some((meta.path.clone()?, Some((line, line))))
},
|_, l_score, _, r_score| l_score.cmp(&r_score).reverse(),
);
cx.push_layer(Box::new(picker));
}
Expand Down Expand Up @@ -2902,6 +2905,7 @@ fn symbol_picker(cx: &mut Context) {
));
Some((path, line))
},
|_, l_score, _, r_score| l_score.cmp(&r_score).reverse(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to do this everywhere? Why not just have it sorted in the order it will be displayed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sort function specifies the order it is displayed in. If we'd replace this with a different sort function, the matches would no longer be sorted by their matching score, so we need this line to mimick the old behavior.
I thought about making the sort function parameter an Option and using this as a default if None is given. I don't think that's a great idea though, especially because most pickers can probably be improved by adding additional criteria to their sort functions.

);
picker.truncate_start = false;
compositor.push(Box::new(picker))
Expand Down Expand Up @@ -2964,6 +2968,7 @@ fn workspace_symbol_picker(cx: &mut Context) {
));
Some((path, line))
},
|_, l_score, _, r_score| l_score.cmp(&r_score).reverse(),
);
picker.truncate_start = false;
compositor.push(Box::new(picker))
Expand Down Expand Up @@ -3016,6 +3021,7 @@ pub fn code_action(cx: &mut Context) {
}
}
},
|_, l_score, _, r_score| l_score.cmp(&r_score).reverse(),
);
compositor.push(Box::new(picker))
}
Expand Down Expand Up @@ -3530,6 +3536,7 @@ fn goto_impl(
));
Some((path, line))
},
|_, l_score, _, r_score| l_score.cmp(&r_score).reverse(),
);
compositor.push(Box::new(picker));
}
Expand Down
43 changes: 33 additions & 10 deletions helix-term/src/ui/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ use helix_core::regex::RegexBuilder;
use helix_view::{Document, Editor, View};

use std::path::PathBuf;
use std::time::SystemTime;

/// The bonus that is given to the score of files that are contained in the current directory in the file picker.
const FILE_CURRENT_DIR_BONUS: i64 = 5;

pub fn regex_prompt(
cx: &mut crate::commands::Context,
Expand Down Expand Up @@ -93,7 +97,11 @@ pub fn regex_prompt(
)
}

pub fn file_picker(root: PathBuf, config: &helix_view::editor::Config) -> FilePicker<PathBuf> {
pub fn file_picker(
root: PathBuf,
current_dir: Option<PathBuf>,
config: &helix_view::editor::Config,
) -> FilePicker<(PathBuf, SystemTime, bool)> {
use ignore::{types::TypesBuilder, WalkBuilder};
use std::time;

Expand Down Expand Up @@ -136,37 +144,52 @@ pub fn file_picker(root: PathBuf, config: &helix_view::editor::Config) -> FilePi
.or_else(|_| metadata.created())
.unwrap_or(time::UNIX_EPOCH)
});
let in_current_dir = match &current_dir {
Some(current_dir) => entry.path().starts_with(current_dir),
None => false,
};

Some((entry.into_path(), time))
Some((entry.into_path(), time, in_current_dir))
});

let mut files: Vec<_> = if root.join(".git").is_dir() {
let files: Vec<_> = if root.join(".git").is_dir() {
files.collect()
} else {
const MAX: usize = 8192;
files.take(MAX).collect()
};

files.sort_by_key(|file| std::cmp::Reverse(file.1));

let files = files.into_iter().map(|(path, _)| path).collect();

FilePicker::new(
files,
move |path: &PathBuf| {
move |(path, _, _)| {
// format_fn
path.strip_prefix(&root)
.unwrap_or(path)
.to_str()
.unwrap()
.into()
},
move |editor: &mut Editor, path: &PathBuf, action| {
move |editor: &mut Editor, (path, _, _), action| {
editor
.open(path.into(), action)
.expect("editor.open failed");
},
|_editor, path| Some((path.clone(), None)),
|_editor, (path, _, _)| Some((path.clone(), None)),
|(_, l_time, l_in_current_dir), mut l_score, (_, r_time, r_in_current_dir), mut r_score| {
// Sort the matches of the picker by:
// * The closeness of the file name to the search with a small boost being given to files in the current directory
// * As a tie-breaker, use the timestamp of the file
if *l_in_current_dir {
l_score += FILE_CURRENT_DIR_BONUS;
}
if *r_in_current_dir {
r_score += FILE_CURRENT_DIR_BONUS;
}
l_score
.cmp(&r_score)
.reverse()
.then_with(|| l_time.cmp(r_time).reverse())
},
)
}

Expand Down
21 changes: 18 additions & 3 deletions helix-term/src/ui/picker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use tui::widgets::Widget;

use std::{
borrow::Cow,
cmp::Ordering,
collections::HashMap,
io::Read,
path::{Path, PathBuf},
Expand Down Expand Up @@ -70,7 +71,7 @@ impl Preview<'_, '_> {

/// Alternate text to show for the preview.
fn placeholder(&self) -> &str {
match *self {
match self {
Self::EditorDocument(_) => "<File preview>",
Self::Cached(preview) => match preview {
CachedPreview::Document(_) => "<File preview>",
Expand All @@ -88,9 +89,10 @@ impl<T> FilePicker<T> {
format_fn: impl Fn(&T) -> Cow<str> + 'static,
callback_fn: impl Fn(&mut Editor, &T, Action) + 'static,
preview_fn: impl Fn(&Editor, &T) -> Option<FileLocation> + 'static,
sort_fn: impl Fn(&T, i64, &T, i64) -> Ordering + 'static,
) -> Self {
Self {
picker: Picker::new(false, options, format_fn, callback_fn),
picker: Picker::new(false, options, format_fn, callback_fn, sort_fn),
truncate_start: true,
preview_cache: HashMap::new(),
read_buffer: Vec::with_capacity(1024),
Expand Down Expand Up @@ -285,6 +287,9 @@ pub struct Picker<T> {

format_fn: Box<dyn Fn(&T) -> Cow<str>>,
callback_fn: Box<dyn Fn(&mut Editor, &T, Action)>,
/// This function specifies the ordering of any 2 matches given as (l_element, l_matching_score, r_element, r_matching_score).
/// The matches are sorted in ascending order, so the highest priority match should be the smallest element in terms of this ordering.
sort_fn: Box<dyn Fn(&T, i64, &T, i64) -> Ordering>,
}

impl<T> Picker<T> {
Expand All @@ -293,6 +298,7 @@ impl<T> Picker<T> {
options: Vec<T>,
format_fn: impl Fn(&T) -> Cow<str> + 'static,
callback_fn: impl Fn(&mut Editor, &T, Action) + 'static,
sort_fn: impl Fn(&T, i64, &T, i64) -> Ordering + 'static,
) -> Self {
let prompt = Prompt::new(
"".into(),
Expand All @@ -314,6 +320,7 @@ impl<T> Picker<T> {
truncate_start: true,
format_fn: Box::new(format_fn),
callback_fn: Box::new(callback_fn),
sort_fn: Box::new(sort_fn),
};

// TODO: scoring on empty input should just use a fastpath
Expand Down Expand Up @@ -344,7 +351,15 @@ impl<T> Picker<T> {
.map(|score| (index, score))
}),
);
self.matches.sort_unstable_by_key(|(_, score)| -score);
self.matches
.sort_unstable_by(|(l_idx, l_score), (r_idx, r_score)| {
(self.sort_fn)(
&self.options[*l_idx],
*l_score,
&self.options[*r_idx],
*r_score,
)
});

// reset cursor position
self.cursor = 0;
Expand Down