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

Interactive global search #4687

Closed
wants to merge 4 commits into from
Closed

Conversation

the-mikedavis
Copy link
Member

@the-mikedavis the-mikedavis commented Nov 9, 2022

This refactors global search (<space>/) to be interactive. The idle timeout triggers an async callback which refills the picker with new search results.

https://asciinema.org/a/536431

Closes #196

@the-mikedavis the-mikedavis added C-enhancement Category: Improvements A-helix-term Area: Helix term improvements S-waiting-on-pr Status: This is waiting on another PR to be merged first labels Nov 9, 2022
@the-mikedavis the-mikedavis changed the title Refactor global_search as a DynPicker Interactive global search Nov 10, 2022
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-pr Status: This is waiting on another PR to be merged first labels Dec 24, 2022
@the-mikedavis
Copy link
Member Author

I have been using this for a while and it's nice to be able to change the query quickly but there are some usability regressions:

  • You can only search using the regex query, so you can't filter on filenames as you can with master's global_search. For really large searches it's very useful to be able to filter on filenames.
  • The picker filters options using the search text, so the picker and the dynamic picker fight over showing results when using regular expression syntax like escapes (for example, search for \{self: there are matches but the picker filters them out).

It would also be useful to search in other pickers with multiple criteria (for example you could filter the symbol picker by symbol kind), so maybe we can come up with an improvement to the picker functionality that will benefit all pickers. I also wonder how telescope handles this?

@pascalkuthe
Copy link
Member

Another example where I also ran into a similar problem would.be a git commit picker. You could filter:

  • Author
  • Date
  • commit message
  • checksum

Irrespective of whether we want something like a commit picker in core I think this example also shows potential for filtering multiple queries . I think the easiest route to implement this would be to create some kind of cycle_picker_filter keybinding that would change what you are filtering (while keeping the previous filters intact). Maybe we could reuse the old binding here

@matoous
Copy link
Contributor

matoous commented Dec 25, 2022

Just to add one more usecase: in symbol picker it would be nice to be able to filter by the symbol type. What I have also seen elsewhere (which is imho inferior to what @pascalkuthe suggested) is having custom strings in the filter to filter for various things, e.g. in the example of symbol picker one could do function:XX to filter only for functions, in the case of commits this would be author:XX, and in the case of global search one could do file:XX. But.. this is way more verbose and fragily in my opinion. I really like the idea of being able to cycle through the filters. Also also, very far from the scope of this PR: maybe another option (or complementary to the cycle filter) would be to have shortcuts for picking different filters, e.g. (for commits):

prompt: XXX
---
1: message 2: author 3: date 4: checksum
---
commit 1
commit 2
...

and a key binding (e.g. alt+X) to select one of the filters.

@pascalkuthe
Copy link
Member

Just to add one more usecase: in symbol picker it would be nice to be able to filter by the symbol type. What I have also seen elsewhere (which is imho inferior to what @pascalkuthe suggested) is having custom strings in the filter to filter for various things, e.g. in the example of symbol picker one could do function:XX to filter only for functions, in the case of commits this would be author:XX, and in the case of global search one could do file:XX. But.. this is way more verbose and fragily in my opinion. I really like the idea of being able to cycle through the filters. Also also, very far from the scope of this PR: maybe another option (or complementary to the cycle filter) would be to have shortcuts for picking different filters, e.g. (for commits):

prompt: XXX
---
1: message 2: author 3: date 4: checksum
---
commit 1
commit 2
...

and a key binding (e.g. alt+X) to select one of the filters.

I think alt-X might be preferable to a cycle keybding. We could show something similar to a buffer line on top to make this intutive:

|1: message | 2: author | 3: date | 4: sha |
————————————————————————————————————————————

Another nice thing to imorove the UI would be to display the full item while only filtering parts of it. For.example again for commits (but I can think of many other examoles) it would.be nice to print the full commit (sha author date message) in the picker even when filtering by author so you don't loose all context of what is being filtered. That will require some adjustment to to way we handle fuzzy indecies (currently if the display_text and filter_text missmatch in widthnit doesn't display correctly and we can get rid of sort_text I believe).

That said all that won't be necessary for global_search. I think the basic mechanism of having multiple filters could be implemented in this PR and usednin global_search and more advanced UI left to future PRs.

@otavioschwanck
Copy link

i have no idea if this code already but would be awesome to chain multiple searches based on previous searchs like telescope do now (with S-RET). Also, doom-emacs find-files has the best implementation of that i ever seen. Example of that:

Press SPC / and search for - , returns:

  • banana_good
  • kiwi_good
  • others

change the completion to - #good, returns:

  • banana_good
  • kiwi_good

change the completion to - #good#ba, returns:

  • banana_good

Every # picks the previous result and filters again. This is so powerful and awesome, the thing i most miss on emacs.

@the-mikedavis
Copy link
Member Author

I think what you're describing can be done by adding spaces between searches. That's specific to filtering pickers though which is not really related to this PR.

I think the basic mechanism of having multiple filters could be implemented in this PR and used in global_search and more advanced UI left to future PRs

It might be best to start with the symbol picker for hacking on this idea - that picker only needs to be sorted by symbol type and name. Then the workspace symbol picker would be a good next step since that also uses a DynamicPicker. Plus the diff for this PR is already pretty ugly with how the callback was refactored.

I think the extra filtering feature might also need #3053

sudormrfbin added a commit to sudormrfbin/helix that referenced this pull request Feb 3, 2023
Merges the code for the Picker and FilePicker into a single Picker that
can show a file preview if a preview callback is provided. This change
was mainly made to facilitate refactoring out a simple skeleton of a
picker that does not do any filtering to be reused in a normal Picker
and a DynamicPicker (see helix-editor#5714; in particular [mikes-comment] and
[gokuls-comment]).

The crux of the issue is that a picker maintains a list of predefined
options (eg. list of files in the directory) and (re-)filters them every
time the picker prompt changes, while a dynamic picker (eg. interactive
global search, helix-editor#4687) recalculates the full list of options on every
prompt change. Using a filtering picker to drive a dynamic picker hence
does duplicate work of filtering thousands of matches for no reason. It
could also cause problems like interfering with the regex pattern in the
global search.

I tried to directly extract a PickerBase to be reused in Picker and
FilePicker and DynamicPicker, but the problem is that DynamicPicker is
actually a DynamicFilePicker (i.e. it can preview file contents) which
means we would need PickerBase, Picker, FilePicker, DynamicPicker and
DynamicFilePicker and then another way of sharing the previewing code
between a FilePicker and a DynamicFilePicker. By merging Picker and
FilePicker into Picker, we only need PickerBase, Picker and
DynamicPicker.

[gokuls-comment]: helix-editor#5714 (comment)
[mikes-comment]: helix-editor#5714 (comment)
sudormrfbin added a commit to sudormrfbin/helix that referenced this pull request Feb 3, 2023
Merges the code for the Picker and FilePicker into a single Picker that
can show a file preview if a preview callback is provided. This change
was mainly made to facilitate refactoring out a simple skeleton of a
picker that does not do any filtering to be reused in a normal Picker
and a DynamicPicker (see helix-editor#5714; in particular [mikes-comment] and
[gokuls-comment]).

The crux of the issue is that a picker maintains a list of predefined
options (eg. list of files in the directory) and (re-)filters them every
time the picker prompt changes, while a dynamic picker (eg. interactive
global search, helix-editor#4687) recalculates the full list of options on every
prompt change. Using a filtering picker to drive a dynamic picker hence
does duplicate work of filtering thousands of matches for no reason. It
could also cause problems like interfering with the regex pattern in the
global search.

I tried to directly extract a PickerBase to be reused in Picker and
FilePicker and DynamicPicker, but the problem is that DynamicPicker is
actually a DynamicFilePicker (i.e. it can preview file contents) which
means we would need PickerBase, Picker, FilePicker, DynamicPicker and
DynamicFilePicker and then another way of sharing the previewing code
between a FilePicker and a DynamicFilePicker. By merging Picker and
FilePicker into Picker, we only need PickerBase, Picker and
DynamicPicker.

[gokuls-comment]: helix-editor#5714 (comment)
[mikes-comment]: helix-editor#5714 (comment)
@pascalkuthe pascalkuthe mentioned this pull request Feb 4, 2023
@rcorre
Copy link
Contributor

rcorre commented Feb 4, 2023

This is great, I've really missed the behavior of :Rg from fzf.vim.

With regards to cycling filters, :Rg seems to keep searches where either the filename or the text match each search term, and then orders them by some criteria. I'm not quite sure how it works, but it feels like magic. I can just type some combination of text/filename critiera, and it does a great job of finding the file I want, without ever having to be explicit about what field I'm searching:

1675518991

@pascalkuthe
Copy link
Member

This is great, I've really missed the behavior of :Rg from fzf.vim.

With regards to cycling filters, :Rg seems to keep searches where either the filename or the text match each search term, and then orders them by some criteria. I'm not quite sure how it works, but it feels like magic. I can just type some combination of text/filename critiera, and it does a great job of finding the file I want, without ever having to be explicit about what field I'm searching:

1675518991

:Rg works completly differently. It runs ripgrep with a search regex once and then performs fuzzy matching on the concatenation of the filepath and the matched line. This is very similar to how helix currently works with the only difference that we only allow fuzzy matching on the path and not on the line.

By comparison this PR reruns the ripgrep based search automatically (we use ripgrep internals since it's written in rust) so you can change the regex and automatically get updated results.

@rcorre
Copy link
Contributor

rcorre commented Feb 5, 2023

:Rg works completly differently. It runs ripgrep with a search regex once and then performs fuzzy matching on the concatenation of the filepath and the matched line

That is how :Rg <pattern> works.

By comparison this PR reruns the ripgrep based search automatically (we use ripgrep internals since it's written in rust) so you can change the regex and automatically get updated results.

This is how :Rg (without arguments) works. In my screenshot, I've run :Rg without arguments. "cursor" matched lines with "cursor", while "tutor" matched the file "tutor". I can see how explicitly limiting terms to specific fields would be useful too, but sometimes it's nice to not have to think about whether you're filtering the filename/line/etc.

@pascalkuthe
Copy link
Member

pascalkuthe commented Feb 5, 2023

:Rg works completly differently. It runs ripgrep with a search regex once and then performs fuzzy matching on the concatenation of the filepath and the matched line

That is how :Rg <pattern> works.

By comparison this PR reruns the ripgrep based search automatically (we use ripgrep internals since it's written in rust) so you can change the regex and automatically get updated results.

This is how :Rg (without arguments) works. In my screenshot, I've run :Rg without arguments. "cursor" matched lines with "cursor", while "tutor" matched the file "tutor". I can see how explicitly limiting terms to specific fields would be useful too, but sometimes it's nice to not have to think about whether you're filtering the filename/line/etc.

:Rg without arguments does the same thing. It just returns all lines in that case without any regex filtering on which you then fuzzy match.

You can configure it to behave the same as this PR but in that case you are not matching on filenames anymore either. From the README of fzf.vim:

In the default implementation of Rg, ripgrep process starts only once with the initial query (e.g. :Rg foo) and fzf filters the output of the process.

This is okay in most cases because fzf is quite performant even with millions of lines, but we can make fzf completely delegate its search responsibliity to ripgrep process by making it restart ripgrep whenever the query string is updated. In this scenario, fzf becomes a simple selector interface rather than a "fuzzy finder".

We will name the new command all-uppercase RG so we can still access the default version. --bind 'change:reload:rg ... {q}' will make fzf restart ripgrep process whenever the query string, denoted by {q}, is changed. With --disabled option, fzf will no longer perform search. The query string you type on fzf prompt is only used for restarting ripgrep process. Also note that we enabled previewer with fzf#vim#with_preview. The last argument to the function, ctrl-/, is the key to toggle the preview window.

This will be used in a child commit to setup an async job on the
compositor context within global_search's callback. The compositor
context was already available at the callback's callsite but was being
reduced to just the editor field.
This can be used to set an initial query from a register. Global
search will use this and the file picker could also consider allowing
an initial query using this helper.
This commit refactors global search to use a DynamicPicker rather than
a prompt plus a picker based on the prompt entry. There aren't any
actual changes: the display format is the same (changed in the parent
commit) and the regex-matcher-builder and async collection are
essentially unchanged. Now these same blocks are used in the dynamic
picker's query callback.

The only novel code here is in the Err case for
RegexMatcherBuilder::build. We now open up a popup showing the failure
message for regexs that fail to compile.
@the-mikedavis the-mikedavis added the S-needs-discussion Status: Needs discussion or design. label Jun 6, 2023
@the-mikedavis
Copy link
Member Author

This is now part of #7265: it fixes the usability problems I mentioned in #4687 (comment)

@the-mikedavis the-mikedavis deleted the interactive-global-search branch June 18, 2023 15:40
the-mikedavis pushed a commit that referenced this pull request Jun 18, 2023
Merges the code for the Picker and FilePicker into a single Picker that
can show a file preview if a preview callback is provided. This change
was mainly made to facilitate refactoring out a simple skeleton of a
picker that does not do any filtering to be reused in a normal Picker
and a DynamicPicker (see #5714; in particular [mikes-comment] and
[gokuls-comment]).

The crux of the issue is that a picker maintains a list of predefined
options (eg. list of files in the directory) and (re-)filters them every
time the picker prompt changes, while a dynamic picker (eg. interactive
global search, #4687) recalculates the full list of options on every
prompt change. Using a filtering picker to drive a dynamic picker hence
does duplicate work of filtering thousands of matches for no reason. It
could also cause problems like interfering with the regex pattern in the
global search.

I tried to directly extract a PickerBase to be reused in Picker and
FilePicker and DynamicPicker, but the problem is that DynamicPicker is
actually a DynamicFilePicker (i.e. it can preview file contents) which
means we would need PickerBase, Picker, FilePicker, DynamicPicker and
DynamicFilePicker and then another way of sharing the previewing code
between a FilePicker and a DynamicFilePicker. By merging Picker and
FilePicker into Picker, we only need PickerBase, Picker and
DynamicPicker.

[gokuls-comment]: #5714 (comment)
[mikes-comment]: #5714 (comment)
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
Merges the code for the Picker and FilePicker into a single Picker that
can show a file preview if a preview callback is provided. This change
was mainly made to facilitate refactoring out a simple skeleton of a
picker that does not do any filtering to be reused in a normal Picker
and a DynamicPicker (see helix-editor#5714; in particular [mikes-comment] and
[gokuls-comment]).

The crux of the issue is that a picker maintains a list of predefined
options (eg. list of files in the directory) and (re-)filters them every
time the picker prompt changes, while a dynamic picker (eg. interactive
global search, helix-editor#4687) recalculates the full list of options on every
prompt change. Using a filtering picker to drive a dynamic picker hence
does duplicate work of filtering thousands of matches for no reason. It
could also cause problems like interfering with the regex pattern in the
global search.

I tried to directly extract a PickerBase to be reused in Picker and
FilePicker and DynamicPicker, but the problem is that DynamicPicker is
actually a DynamicFilePicker (i.e. it can preview file contents) which
means we would need PickerBase, Picker, FilePicker, DynamicPicker and
DynamicFilePicker and then another way of sharing the previewing code
between a FilePicker and a DynamicFilePicker. By merging Picker and
FilePicker into Picker, we only need PickerBase, Picker and
DynamicPicker.

[gokuls-comment]: helix-editor#5714 (comment)
[mikes-comment]: helix-editor#5714 (comment)
@pascalkuthe pascalkuthe mentioned this pull request Apr 6, 2024
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
Merges the code for the Picker and FilePicker into a single Picker that
can show a file preview if a preview callback is provided. This change
was mainly made to facilitate refactoring out a simple skeleton of a
picker that does not do any filtering to be reused in a normal Picker
and a DynamicPicker (see helix-editor#5714; in particular [mikes-comment] and
[gokuls-comment]).

The crux of the issue is that a picker maintains a list of predefined
options (eg. list of files in the directory) and (re-)filters them every
time the picker prompt changes, while a dynamic picker (eg. interactive
global search, helix-editor#4687) recalculates the full list of options on every
prompt change. Using a filtering picker to drive a dynamic picker hence
does duplicate work of filtering thousands of matches for no reason. It
could also cause problems like interfering with the regex pattern in the
global search.

I tried to directly extract a PickerBase to be reused in Picker and
FilePicker and DynamicPicker, but the problem is that DynamicPicker is
actually a DynamicFilePicker (i.e. it can preview file contents) which
means we would need PickerBase, Picker, FilePicker, DynamicPicker and
DynamicFilePicker and then another way of sharing the previewing code
between a FilePicker and a DynamicFilePicker. By merging Picker and
FilePicker into Picker, we only need PickerBase, Picker and
DynamicPicker.

[gokuls-comment]: helix-editor#5714 (comment)
[mikes-comment]: helix-editor#5714 (comment)
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Merges the code for the Picker and FilePicker into a single Picker that
can show a file preview if a preview callback is provided. This change
was mainly made to facilitate refactoring out a simple skeleton of a
picker that does not do any filtering to be reused in a normal Picker
and a DynamicPicker (see helix-editor#5714; in particular [mikes-comment] and
[gokuls-comment]).

The crux of the issue is that a picker maintains a list of predefined
options (eg. list of files in the directory) and (re-)filters them every
time the picker prompt changes, while a dynamic picker (eg. interactive
global search, helix-editor#4687) recalculates the full list of options on every
prompt change. Using a filtering picker to drive a dynamic picker hence
does duplicate work of filtering thousands of matches for no reason. It
could also cause problems like interfering with the regex pattern in the
global search.

I tried to directly extract a PickerBase to be reused in Picker and
FilePicker and DynamicPicker, but the problem is that DynamicPicker is
actually a DynamicFilePicker (i.e. it can preview file contents) which
means we would need PickerBase, Picker, FilePicker, DynamicPicker and
DynamicFilePicker and then another way of sharing the previewing code
between a FilePicker and a DynamicFilePicker. By merging Picker and
FilePicker into Picker, we only need PickerBase, Picker and
DynamicPicker.

[gokuls-comment]: helix-editor#5714 (comment)
[mikes-comment]: helix-editor#5714 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements S-needs-discussion Status: Needs discussion or design. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

interactive global search
5 participants