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

Initial implementation of global search #651

Merged
merged 5 commits into from
Sep 21, 2021
Merged

Initial implementation of global search #651

merged 5 commits into from
Sep 21, 2021

Conversation

pppKin
Copy link
Contributor

@pppKin pppKin commented Aug 25, 2021

Since we haven't figure out how to properly implement a global search feature yet #196 , I hope this could at least be an inspiration of an potential better design:)
This implementation bind space + / to display a prompt (like the search prompt), and on Enter, start the search, gathers the result and pop up a FilePicker. A few necessary changes are made, such as regex_prompt, which now accepts an extra callback that would be invoked once Enter is pressed.
Feel free to close this if we figure out something better later.

@pppKin
Copy link
Contributor Author

pppKin commented Aug 25, 2021

Some improvements that can be made:

  1. Support cancellation. It appears that if Esc is pressed, the show_picker future placed on jobs list will not be dropped.
  2. Async search. Currently the search runs on the main thread which blocks the user from doing anything before the search is completed.

@kirawi
Copy link
Member

kirawi commented Aug 25, 2021

That's weird, why am I shown in the first commit?

@pppKin
Copy link
Contributor Author

pppKin commented Aug 25, 2021

@kirawi Sorry but I have been scratching my head too. Perhaps I made a mistake when rebasing on master branch. Git did not complain anything though so I have no idea where I went wrong:\

@kirawi
Copy link
Member

kirawi commented Aug 25, 2021

Not sure, I'm no git whiz 😅 Maybe it's possible to remove me as the author?

@pppKin
Copy link
Contributor Author

pppKin commented Aug 26, 2021

I did another rebase and force pushed to solve the issue.

helix-term/src/commands.rs Outdated Show resolved Hide resolved
@pppKin pppKin requested a review from archseer August 26, 2021 06:54
@kraem kraem mentioned this pull request Sep 15, 2021
@cossonleo
Copy link
Contributor

What's status of this pr?

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I finally did a full review here. I think it's a good first implementation, I'd still like to add a live search prompt in the future though 👍🏻

helix-term/src/ui/mod.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
Comment on lines +1232 to +1239
let mut searcher_cl = searcher.clone();
let matcher_cl = matcher.clone();
let all_matches_sx_cl = all_matches_sx.clone();
Copy link
Member

Choose a reason for hiding this comment

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

Why clone here? Can't you make it a move closure? .run(move || {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WalkParallel::run takes a callback that returns a Box<dyn FnMut(Result<DirEntry, Error>) -> WalkState + Send + 's>, which would capture searcher, matcher, all_matches_sx. So I think the clone is necessary here.

helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/keymap.rs Show resolved Hide resolved
@pppKin
Copy link
Contributor Author

pppKin commented Sep 17, 2021

Thanks for the solid advice! I'd love to keep working on this PR and I will work on these changes and rebase onto latest master as soon as I can. (I am currently occupied so maybe have to wait till this weekend)

@pppKin
Copy link
Contributor Author

pppKin commented Sep 19, 2021

All changes are pushed and rebased onto latest master now. Let me know if you need me to rebase and squash the commits:)

In the future I would like to bring up the picker as soon as Enter is pressed, move the search to a worker thread, and dynamically append results to the picker. This particular picker then could, should we wished, have 2 input fields, one for modifying search pattern, one for fuzzy matching search results. Unfortunately this is not trivial to implement at the moment(for me at least), so it's just a thought now.

@archseer
Copy link
Member

Great work!

@archseer archseer merged commit 9456d5c into helix-editor:master Sep 21, 2021
@pppKin pppKin deleted the global_search_1 branch October 11, 2021 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants