`Picker`s "v2"

[the-mikedavis]

I was hoping to make these picker changes as separate PRs but they layer in a way that makes reviewing them independently awkward: one commit changes a block and the next changes it again, so I think it's easier to review altogether. This PR is a kind of "version 2" for the Picker component that resolves a bunch of things we've wanted to change about Pickers:

• Lay out items in a table format so that we can filter on columns independently (previously https://github.com/helix-editor/helix/pull/7265)
• Control whether certain columns are used for Nucleo-based filtering or not (useful for DynamicPickers, see #5714)
• Replace the ad-hoc IdleTimeout approach for syntax highlighting the preview with an event-system hook (https://github.com/helix-editor/helix/issues/9629)
• Use DynamicPicker for global_search (https://github.com/helix-editor/helix/issues/196)
• Use an event-system hook for DynamicPicker rather than IdleTimeout. As a consequence, we can merge the DynamicPicker type into Picker, a refactor that joyously removes this unfortunate block.

Shout out to @pascalkuthe's excellent work with the event system (#8021) and Nucleo (#7814) that make this change possible.

The table format and filtering by columns (originally posted in #7265):

Dynamic global search running against a very large directory (originally posted in #196):

Related to #9629 Closes #196 Closes #5714 Closes #5446 Closes #3543 Closes #4956 Closes #2109

[archseer]

For pickers with a single column, could we just hide the header in that case? That way it ends up looking like the original picker

[pascalkuthe]

Just a couple things I noticed I haven't read trough all of it yet

[the-mikedavis]

For pickers with a single column, could we just hide the header in that case? That way it ends up looking like the original picker

Yep actually we don't render the header if columns.len() isn't > 1: https://github.com/helix-editor/helix/blob/e93685bd27e8102ee687fdb05ce5e969984ad036/helix-term/src/ui/picker.rs#L753-L758 https://github.com/helix-editor/helix/blob/e93685bd27e8102ee687fdb05ce5e969984ad036/helix-term/src/ui/picker.rs#L476-L484

So the file picker doesn't look or behave any differently for example

[sicher]

Very nice!

For a slightly more compact look the column headers could be rendered with a different background - instead of having the line below.

[the-mikedavis]

Yeah the separator lines were a little bulky. I removed the separators and added a theme key only for the header text

[univerz]

Yeah the separator lines were a little bulky. I removed the separators and added a theme key only for the header text

or even move them into the separator below the input, similar to dbg mockups?

[the-mikedavis]

or even move them into the separator below the input, similar to dbg mockups?

That's much more complicated implementation-wise - we would probably need to change the table rendering in helix-tui. I'll leave that for future work

[thiagomajesk]

Wow, we have to get this on master! This solves my main issue with Helix. Brilliant work @the-mikedavis!!!

[Philipp-M]

Just started testing this.

First of all, great work!

One suggestion: Make debounce timeouts configurable.

I have reduced them significantly (highlight: 150 -> 33 (I have a key-repeat synced to frame-rate, reason why it's so low while still debouncing effectively when scrolling), dynamic query: 275 -> 75). I think everyone has a (slightly) different preference here, since no one types the same speed (but as a fast typer + impatiance, the current ones just feel way too slow to me). I could imagine that flashes (when not typing that fast) could annoy others, which is why I think it makes sense to make this configurable (with sane defaults).

I have noticed flashes when using the DynamicQueryHandler via the global search, although items look the same and there's picker.matcher.restart(false) already. It's still slightly less (but still) noticeable when picker.matcher.restart(true) (paradoxically). So maybe worth looking into?

[the-mikedavis]

What do you mean by flashes? Can you capture it in a gif / video / asciinema cast?

W.r.t. configurable timeouts see https://github.com/helix-editor/helix/pull/9668#discussion_r1498270629

[Philipp-M]

Sure, here's an asciinema

(edit: Just to confirm, since this was tested on a branch merged with all kinds of things, it also occurs on this one)

[jscarrott]

Something I have noticed is that the global search no longer reads from the search selection. Which is a minor annoyance for something that completely replaces my use of fzf/rg.

[the-mikedavis]

Yeah with the switch away from the prompt for searching we lose the ability to move the search history (/ register). You can use the latest search with <C-r>/ (inserts the latest contents of / into the prompt). I'd also like to have "/<space>/ open global search with the latest / contents as the line, I'll push some changes for that

[pascalkuthe]

Yeah with the switch away from the prompt for searching we lose the ability to move the search history (/ register). You can use the latest search with <C-r>/ (inserts the latest contents of / into the prompt). I'd also like to have "/<space>/ open global search with the latest / contents as the line, I'll push some changes for that

Could we maybe do things a little differently: Keep the prompt (and the nice preselect/history functionality which is something that I use a ton paricularly in combination with *) and one confirmed start the picker with that inserted.

This would also be somewhat more efficient as it would only crawl the fs once by default unless you actually interactively change the search term.

[the-mikedavis]

@pascalkuthe and I talked about this a little privately. I like the behavior of global_search opening the picker as it feels faster / more responsive (though really it isn't) so I'd like to avoid having a prompt open the picker. But using the latest value you have on / is a super useful common-case.

I've been trying a change locally that is a compromise: it adds the Prompt's history function to the Picker only when the picker is empty, and only for global search. So like a regular prompt you will see the latest value of / as a suggestion and you can use Enter to accept the suggestion, or type anything else to dismiss it.

:point_up: Here I'm using * to add "center" to the / register and then Enter within the picker to accept it as a suggestion.

[daedroza]

Hi @the-mikedavis

Can we additional column in the beginning for selector? i.e the selection symbol, the primary ask for this is to extend it in near future to allow picker to select multiple files at once to open.

What do you think of such functionality?

[godalming123]

@daedroza, that's a good idea, but personally, I don't see the point in adding UI elements that do not serve a purpose yet, and just take up space. I also think that if this feature is added, the row to show which files are selected should only appear when the user begins to select files in order to save space.

[daedroza]

@daedroza, that's a good idea, but personally, I don't see the point in adding UI elements that do not serve a purpose yet, and just take up space. I also think that if this feature is added, the row to show which files are selected should only appear when the user begins to select files in order to save space.

Thanks for feedback,

In the screenshot/GIF, there seems to be enough space for selection symbol ">" so i think my request can be accomodated the issue is this column shouldnt have a header but I'm unsure how it could be fit...

[godalming123]

@the-mikedavis, in the dynamic global search would it be possible to not just highlight the line where the search query exists, but also highlight each occurrence of the search query in a more vibrant color like blue. This would make long lines with several occurrences of a search query much easier to scan. If you don't want to distract the user with lots of color, then maybe the blue highlight could only be shown for the selected line.

[the-mikedavis]

Can we [add] additional column in the beginning for selector?

That would be unrelated to these changes: these changes also don't touch the highlight symbol for example (the >). There's prior art for changing the picker like this in https://github.com/helix-editor/helix/pull/4837. It's an interesting change but I'm not totaly convinced it's necessary since we have the ability to open items in the background and reopen the last picker.

[the-mikedavis]

in the dynamic global search would it be possible to not just highlight the line where the search query exists, but also highlight each occurrence of the search query in a more vibrant color like blue

Technically this is harder than it seems because grep-searcher exposes the bytes of the lines that matched rather than the byte indices of the exact match(es) within the lines. (I assume this is because it considers a line matched after the first occurrence of the pattern and stops searching the line.) We could run the input regex over the matched lines again to find the match indices and highlight those in the preview with a new theme key. That could be follow-up work to this PR. I'm not sure how expensive that could be though - we would probably not want to run those regexes per render - and it would require some changes to the picker preview so we could attach virtual text or extra theming.

[pascalkuthe]

in the dynamic global search would it be possible to not just highlight the line where the search query exists, but also highlight each occurrence of the search query in a more vibrant color like blue

Technically this is harder than it seems because grep-searcher exposes the bytes of the lines that matched rather than the byte indices of the exact match(es) within the lines. (I assume this is because it considers a line matched after the first occurrence of the pattern and stops searching the line.) We could run the input regex over the matched lines again to find the match indices and highlight those in the preview with a new theme key. That could be follow-up work to this PR. I'm not sure how expensive that could be though - we would probably not want to run those regexes per render - and it would require some changes to the picker preview so we could attach virtual text or extra theming.

Regex cursor should make the cost pretty negligible. We discussed a while ago tonallow very simple regex highlights (for thing like comment inections). This usecase could be simply.covered by regex highlighting with the search pattern.

[pascalkuthe]

(Definitly a followup pr tough)

[Iorvethe]

Thank you for this PR! I found a tiny bug with the latest version where after writing one character (at least) and deleting it, the search results aren’t updated. Here’s a screencast of it:

https://asciinema.org/a/sXcoYawSecUUGfGMJSOz1Elkv

[the-mikedavis]

Looks like I left a stray clear() of the old query around, should be fixed in the latest push

[kirawi]

Regarding interactive search, I actually find it annoying and in the way since I have to specify %p: (my finger has to overextend to reach %) per each path query whereas I never actually use the interactivity. I'd ideally like to disable it altogether but I'd assume that it would be nontrivial. Any ideas? Maybe swap it so that the regex query is specified by e.g. %s instead?

[the-mikedavis]

I think it would be awkward to have the dynamic column not also be the primary column (the one that doesn't need a %name: specifier). Maybe we could make it easier to switch to the path input by adding some kind of completion for columns? Or maybe it would be enough to switch out % for an easier to type character

[kirawi]

Or maybe a way to group them together? e.g. %p:{rs$ filename foldername} etc.?

[pascalkuthe]

Or maybe a way to group them together? e.g. %p:{rs$ filename foldername} etc.?

you can do that with %p:"fr$ filneam foldername" (last quote is optional if nothing follows). I do have the slight grivance that this requires r specical characters to type everytime I open the global search (I never do anything else). I am perfectly fine with typing %p once the :" is a bit unfortunate.I wonder if we could make %p" behave the same as %p:"? That would at-least improve this somewhat. The quoting syntax also feel unopimal since its only needed because of default columns. If every column just had a specifc name we could just use %regex word1 word2 %path path1 path2 %regex word1 where column name automatically ends at first space and the column stays active until the next (non-escaped) %.

Or maybe that could even work with the current syntax just that the default column would only matter at the start of the picker and then if you want to go back to the default column you would need to use the explicit name. For exmaple word1 word2 %p path1 path2 %regex word3. I like this last idea quite a bit since it cuts down on the numer of specical chars a lot, keeps the "start typing and it just works" experience

[useche]

I think this patch may have a regression: it is not searching in the buffer contents as was done in #5652. I opened a new buffer with no file as backing (:new) and then searched with global search. It didn't find the text I wrote in the new buffer.