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.

Sign up for GitHub

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

Jump to bottom

Global search in a background thread #1543

Closed
wants to merge 2 commits into from
Closed

Global search in a background thread #1543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bee2cb9
Global search in a background thread
pppKin Jan 19, 2022
81d7393
refactor
pppKin Jan 24, 2022
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
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion helix-term/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ serde = { version = "1.0", features = ["derive"] }
# ripgrep for global search
grep-regex = "0.1.9"
grep-searcher = "0.1.8"
tokio-stream = "0.1.8"

[target.'cfg(not(windows))'.dependencies] # https://github.com/vorner/signal-hook/issues/100
signal-hook-tokio = { version = "0.3", features = ["futures-v0_3"] }
264 changes: 146 additions & 118 deletions helix-term/src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use helix_core::{
use helix_view::{
clipboard::ClipboardType,
document::{Mode, SCRATCH_BUFFER_NAME},
editor::{Action, Motion},
editor::{Action, FilePickerConfig, Motion},
input::KeyEvent,
keyboard::KeyCode,
view::View,
Expand All @@ -34,14 +34,15 @@ use helix_lsp::{
};
use insert::*;
use movement::Movement;
use tokio::task::JoinHandle;

use crate::{
compositor::{self, Component, Compositor},
ui::{self, FilePicker, Picker, Popup, Prompt, PromptEvent},
};

use crate::job::{self, Job, Jobs};
use futures_util::{FutureExt, StreamExt};
use futures_util::FutureExt;
use std::{collections::HashSet, num::NonZeroUsize};
use std::{fmt, future::Future};

Expand All @@ -53,10 +54,9 @@ use std::{
use once_cell::sync::Lazy;
use serde::de::{self, Deserialize, Deserializer};

use grep_regex::RegexMatcherBuilder;
use grep_regex::{RegexMatcher, RegexMatcherBuilder};
use grep_searcher::{sinks, BinaryDetection, SearcherBuilder};
use ignore::{DirEntry, WalkBuilder, WalkState};
use tokio_stream::wrappers::UnboundedReceiverStream;

pub struct Context<'a> {
pub register: Option<char>,
Expand Down Expand Up @@ -1645,9 +1645,123 @@ fn search_selection(cx: &mut Context) {
cx.editor.set_status(msg);
}

/// search with `matcher` in a background thread, returns a `JoinHandle` to be awaited on
fn global_search_task(
file_picker_config: FilePickerConfig,
matcher: RegexMatcher,
) -> JoinHandle<Vec<(usize, PathBuf)>> {
let searcher = SearcherBuilder::new()
.binary_detection(BinaryDetection::quit(b'\x00'))
.build();
let search_root =
std::env::current_dir().expect("Global search error: Failed to get current dir");
tokio::task::spawn_blocking(move || -> Vec<(usize, PathBuf)> {
pickfire marked this conversation as resolved.
Show resolved Hide resolved
// We're in the background thread now so std::sync::mpsc::channel should suffice
let (all_matches_sx, all_matches_rx) = std::sync::mpsc::channel();
WalkBuilder::new(search_root)
.hidden(file_picker_config.hidden)
.parents(file_picker_config.parents)
.ignore(file_picker_config.ignore)
.git_ignore(file_picker_config.git_ignore)
.git_global(file_picker_config.git_global)
.git_exclude(file_picker_config.git_exclude)
.max_depth(file_picker_config.max_depth)
.build_parallel()
.run(|| {
let mut searcher_cl = searcher.clone();
let matcher_cl = matcher.clone();
let all_matches_sx_cl = all_matches_sx.clone();
Box::new(move |dent: Result<DirEntry, ignore::Error>| -> WalkState {
let dent = match dent {
Ok(dent) => dent,
Err(_) => return WalkState::Continue,
};

match dent.file_type() {
Some(fi) => {
if !fi.is_file() {
return WalkState::Continue;
}
}
None => return WalkState::Continue,
}

let result_sink = sinks::UTF8(|line_num, _| {
match all_matches_sx_cl
.send((line_num as usize - 1, dent.path().to_path_buf()))
{
Ok(_) => Ok(true),
Err(_) => Ok(false),
}
});
let result = searcher_cl.search_path(&matcher_cl, dent.path(), result_sink);

if let Err(err) = result {
log::error!("Global search error: {}, {}", dent.path().display(), err);
}
WalkState::Continue
})
});

all_matches_rx.try_iter().collect()
})
}

/// returns a callback to display global search results
fn global_search_callback(
all_matches: Vec<(usize, PathBuf)>,
current_path: Option<PathBuf>,
) -> job::Callback {
Box::new(move |editor: &mut Editor, compositor: &mut Compositor| {
if all_matches.is_empty() {
editor.set_status("No matches found".to_string());
return;
}

let picker = FilePicker::new(
all_matches,
move |(_line_num, path)| {
let relative_path = helix_core::path::get_relative_path(path)
.to_str()
.unwrap()
.to_owned();
if current_path.as_ref().map(|p| p == path).unwrap_or(false) {
format!("{} (*)", relative_path).into()
} else {
relative_path.into()
}
},
move |editor: &mut Editor, (line_num, path), action| {
match editor.open(path.into(), action) {
Ok(_) => {}
Err(e) => {
editor.set_error(format!(
"Failed to open file '{}': {}",
path.display(),
e
));
return;
}
}

let line_num = *line_num;
let (view, doc) = current!(editor);
let text = doc.text();
let start = text.line_to_char(line_num);
let end = text.line_to_char((line_num + 1).min(text.len_lines()));

doc.set_selection(view.id, Selection::single(start, end));
align_view(doc, view, Align::Center);
},
|_editor, (line_num, path)| Some((path.clone(), Some((*line_num, *line_num)))),
);
compositor.push(Box::new(picker));
})
}

fn global_search(cx: &mut Context) {
let (all_matches_sx, all_matches_rx) =
tokio::sync::mpsc::unbounded_channel::<(usize, PathBuf)>();
let (regex_sx, mut regex_rx) = tokio::sync::mpsc::channel(1);
Copy link
Member

Choose a reason for hiding this comment

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

You want to use tokio::sync::oneshot::channel instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

regex is sent from the callback of Prompt ( move |_view, _doc, regex, event| { ... regex_sx.send() ...}), hence capturing regex_sx, I have just discovered that oneshot::Sender does not implement Copy trait and compiler would refuse to compile here.

Copy link
Member

Choose a reason for hiding this comment

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

You can use a move closure to move it into the callback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have this error:

error[E0507]: cannot move out of regex_sx, a captured variable in an Fn closure

I think the compiler cannot guarantee that we would call regex_sx only once, so it refuse to compile here.

Copy link
Member

Choose a reason for hiding this comment

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

Right, using a channel seems like a hack to me anyway. I'd rather pass cx through the regex_prompt callback as a first parameter and manually call let (doc, view) = current!(cx). Then you can create the search picker instance inside the callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think having a dedicated FnOnce callback for PromptEvent::Validate might clear things up a bit and we can use oneshot channel, but I really don't want to introduce any breaking change to the API, hence the hack.
About passing cx through regex_prompt, wouldn't it mess up the lifetime of cx because we still need to await the search results (just my naive thought)?


let smart_case = cx.editor.config.smart_case;
let file_picker_config = cx.editor.config.file_picker.clone();

Expand All @@ -1668,126 +1782,40 @@ fn global_search(cx: &mut Context) {
return;
}

if let Ok(matcher) = RegexMatcherBuilder::new()
.case_smart(smart_case)
.build(regex.as_str())
{
let searcher = SearcherBuilder::new()
.binary_detection(BinaryDetection::quit(b'\x00'))
.build();

let search_root = std::env::current_dir()
.expect("Global search error: Failed to get current dir");
WalkBuilder::new(search_root)
.hidden(file_picker_config.hidden)
.parents(file_picker_config.parents)
.ignore(file_picker_config.ignore)
.git_ignore(file_picker_config.git_ignore)
.git_global(file_picker_config.git_global)
.git_exclude(file_picker_config.git_exclude)
.max_depth(file_picker_config.max_depth)
.build_parallel()
.run(|| {
let mut searcher_cl = searcher.clone();
let matcher_cl = matcher.clone();
let all_matches_sx_cl = all_matches_sx.clone();
Box::new(move |dent: Result<DirEntry, ignore::Error>| -> WalkState {
let dent = match dent {
Ok(dent) => dent,
Err(_) => return WalkState::Continue,
};

match dent.file_type() {
Some(fi) => {
if !fi.is_file() {
return WalkState::Continue;
}
}
None => return WalkState::Continue,
}

let result_sink = sinks::UTF8(|line_num, _| {
match all_matches_sx_cl
.send((line_num as usize - 1, dent.path().to_path_buf()))
{
Ok(_) => Ok(true),
Err(_) => Ok(false),
}
});
let result =
searcher_cl.search_path(&matcher_cl, dent.path(), result_sink);

if let Err(err) = result {
log::error!(
"Global search error: {}, {}",
dent.path().display(),
err
);
}
WalkState::Continue
})
});
} else {
// Otherwise do nothing
// log::warn!("Global Search Invalid Pattern")
}
match block_on(regex_sx.send(regex)) {
Copy link
Member

Choose a reason for hiding this comment

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

If you use oneshot then the sender isn't async and you don't need to block_on here.

Copy link
Member

Choose a reason for hiding this comment

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

Why is this using a channel rather than creating the picker and the global_search_task here? Is it because you have no access to cx or editor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check these out and work on improvements as soon as I can (a bit occupied at the moment:) )

Ok(_) => {}
Err(e) => {
// most likely because regex_rx has been dropped
log::error!("Global search error: {}", e);
}
};
},
);

cx.push_layer(Box::new(prompt));

let current_path = doc_mut!(cx.editor).path().cloned();

let show_picker = async move {
let all_matches: Vec<(usize, PathBuf)> =
UnboundedReceiverStream::new(all_matches_rx).collect().await;
let call: job::Callback =
Box::new(move |editor: &mut Editor, compositor: &mut Compositor| {
if all_matches.is_empty() {
editor.set_status("No matches found".to_string());
return;
}

let picker = FilePicker::new(
all_matches,
move |(_line_num, path)| {
let relative_path = helix_core::path::get_relative_path(path)
.to_str()
.unwrap()
.to_owned();
if current_path.as_ref().map(|p| p == path).unwrap_or(false) {
format!("{} (*)", relative_path).into()
} else {
relative_path.into()
}
},
move |editor: &mut Editor, (line_num, path), action| {
match editor.open(path.into(), action) {
Ok(_) => {}
Err(e) => {
editor.set_error(format!(
"Failed to open file '{}': {}",
path.display(),
e
));
return;
}
}

let line_num = *line_num;
let (view, doc) = current!(editor);
let text = doc.text();
let start = text.line_to_char(line_num);
let end = text.line_to_char((line_num + 1).min(text.len_lines()));
let regex = regex_rx.recv().await;
if let Some(regex) = regex {
if let Ok(matcher) = RegexMatcherBuilder::new()
.case_smart(smart_case)
.build(regex.as_str())
{
match global_search_task(file_picker_config, matcher).await {
Ok(all_matches) => {
let call: job::Callback = global_search_callback(all_matches, current_path);
return Ok(call);
}
Err(e) => {
log::error!("Global search error: {}", e);
}
};
}
}

doc.set_selection(view.id, Selection::single(start, end));
align_view(doc, view, Align::Center);
},
|_editor, (line_num, path)| Some((path.clone(), Some((*line_num, *line_num)))),
);
compositor.push(Box::new(picker));
});
Ok(call)
let empty_call: job::Callback = Box::new(|_, _| {});
Ok(empty_call)
Comment on lines 1798 to +1818
Copy link
Member

Choose a reason for hiding this comment

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

You probably want

cx.jobs.spawn(future);

That way you're not returning an empty job::Callback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry it took so long but I'm confused here; With job::Callback I have access to Editor and Compositor so I can set_status or push the picker to the compositor. How should I access Editor and Compositor if I'm using cx.jobs.spawn(future);, perhaps I misunderstand how jobs and everything work here?

};
cx.jobs.callback(show_picker);
}
Expand Down