Pickers "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 Refactor Picker as a table with multiple columns/filters #7265)
• Control whether certain columns are used for Nucleo-based filtering or not (useful for DynamicPickers, see Workspace symbol search does not support special characters #5714)
• Replace the ad-hoc IdleTimeout approach for syntax highlighting the preview with an event-system hook (Refactor ad-hoc hooks to use the event system #9629)
• Use DynamicPicker for global_search (interactive global search #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:

helix/helix-term/src/ui/picker.rs

Lines 753 to 758 in e93685b

	// -- Header
	if self.columns.len() > 1 {
	let header_text_style = cx.editor.theme.get("ui.picker.header.text");
	let header_separator_style = cx.editor.theme.get("ui.picker.header.separator");
	table = table.header(

helix/helix-term/src/ui/picker.rs

Lines 476 to 484 in e93685b

	fn header_height(&self) -> u16 {
	if self.columns.len() > 1 {
	// The header is one line for the column names and
	// one line for the separator bars.
	2
	} else {
	0
	}
	}

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 #9668 (comment)

[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]

The issue in #9647 (comment) was solved with the last force-push

[lemontheme]

Just built from master. What a happy surprise to discover that this has been merged! :)

So far, so (mostly) good. Rather than open a bunch of issues, I'll quickly share some feedback here. I hope that's okay, seeing as this PR is closed.

1. In the command palette (cmd+?), the 'bindings' column expands in width to fit the widest binding(s), but it doesn't shrink once the wide entries are filtered out. The 'doc' column is forced to a narrow width and stays there. Only short command descriptions are readable. Anything longer than 5 words is truncated from the left. As a user, my impression is that the old command palette was more useful. Here's an asciinema recording to illustrate:

2. The previous global search (cmd+/) was instant, whereas with the new global search I need to wait 1–2s after the popup displays to execute a search, or else nothing happens. Quickly typing<cmd+/><query><enter>, then inspecting the results, is what I'm used to after almost a year of using Helix full-time. By contrast, pressing enter now selects the first result. I keep forgetting this, so now I'm continually closing files I didn't mean to open. So my feedback here consists of two subpoints: a) the new search feels less immediate; b) it breaks muscle memory somewhat.

---

Hope this is useful! Let me know if you'd prefer me to open separate issues instead :)

[the-mikedavis]

For 1 see #9647 (comment). The width handling is naive and improvements can be made as follow-up PRs.

For 2 there is a debounce now since global_search reruns as you change the query. It's a fairly expensive operation so we could turn down the debounce but the lower it goes the more global search will chip at your battery. I'm aware this breaks muscle memory but I'd rather have a better interface even if it means people have to retrain their muscle memory.

[rcorre]

This is awesome, thanks for all the hard work!
Is it expected that filters work right now, or is that going to be a follow-up?
I'm trying to type filters as shown in the video in the PR description (e.g. %path:.yml) and nothing is changing in the picker.
I wasn't sure if I should file an issue or if that's an upcoming feature.

[lazytanuki]

This is awesome, thanks for all the hard work! Is it expected that filters work right now, or is that going to be a follow-up? I'm trying to type filters as shown in the video in the PR description (e.g. %path:.yml) and nothing is changing in the picker. I wasn't sure if I should file an issue or if that's an upcoming feature.

The correct syntax would be %path .yml