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]

In the latest push I switched the query syntax to be the one mentioned at the end of #9647 (comment). So now it's %path file.rs instead. Each column is fuzzy matched.

[everestmz]

In the latest push I switched the query syntax to be the one mentioned at the end of #9647 (comment). So now it's %path file.rs instead. Each column is fuzzy matched.

Ah, I should have read more closely. Can confirm this works, thank you for these updates. Search is awesome now!

[the-mikedavis]

I had originally intended for this to be a follow-up but it was pretty straightforward to add with the new syntax. We can highlight the column that is being entered with a ui.picker.header.active theme key. As a basic set of highlights I would recommend something like:

"ui.picker.header" = { modifiers = ["underlined"] }
"ui.picker.header.active" = { modifiers = ["underlined", "bold"] }

After merging we may want to add those keys to the existing themes in the repo as a baseline and then theme authors can customize from there.

[daedroza]

The original patch also contained the "contents" column (in the very first gif), can we please add that back and hide it behind default flag? I don't use the preview often and rely on the original search contents to help myself narrow down to the result.

Usecase is something like jump to a function call using gw, *, global_search then use %c boolean, to search for function definition instead of other function calls

[daedroza]

Hmm, so I found the hidden function, I made it's constructor hidden as false and filter as true

diff --git a/helix-term/src/ui/picker.rs b/helix-term/src/ui/picker.rs
index f87c18bb..38e6c66c 100644
--- a/helix-term/src/ui/picker.rs
+++ b/helix-term/src/ui/picker.rs
@@ -217,8 +217,8 @@ pub fn hidden(name: impl Into<Arc<str>>) -> Self {
         Self {
             name: name.into(),
             format,
-            filter: false,
-            hidden: true,
+            filter: true,
+            hidden: false,
         }
     }

But helix crashes...
thread 'tokio-runtime-worker' panicked at 'called Result::unwrap() on an Err value: Any { .. }', /home/kalpaj/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ignore-0.4.22/src/walk.rs:1301:31
stack backtrace:8afd686d - std::backtrace_rs::backtrace::libunwind::trace::h9a6b80bbf328ba5d

[the-mikedavis]

That panic is because hidden uses unreachable! for its format function - it's asserting that hidden columns shouldn't have text. If you want to restore how that column looked you will need to use a regular column (Column::new).

@pascalkuthe and I discussed it (also see above: #9647 (comment)) and we think it'd be nice to be able to change how the preview works, so you can use the preview as it is now or alternatively show the matching line and hide the preview. That should be future work though and in the meantime I'm not interested in adding the column back to this PR since it takes up a lot of space and almost always needs to be truncated. Being able to change the preview from showing the context of the currently selected item to being the line contents of each item could be useful for other pickers in the future as well.

[rockboynton]

Is scrolling in the picker covered in this PR or is that still an open issue? #5686

[the-mikedavis]

No this PR doesn't cover that, it's left for other PRs (I believe there is one or two open for it)

[davxy]

for global search would be nice to show line number at the end of the path (much like what you get when you use gr over a symbol)

[useche]

for global search would be nice to show line number at the end of the path (much like what you get when you use gr over a symbol)

Yes, I would like that too. I had this implemented with a separate column in the global search picker:

diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs
index 1c91510d..428d7d0c 100644
--- a/helix-term/src/commands.rs
+++ b/helix-term/src/commands.rs
@@ -2265,6 +2265,9 @@ struct GlobalSearchConfig {
     };

     let columns = [
+        PickerColumn::new("line#", |item: &FileResult, _| {
+            (item.line_num + 1).to_string().into()
+        }),
         PickerColumn::new("path", |item: &FileResult, _| {
             helix_stdx::path::get_relative_path(&item.path)
                 .to_string_lossy()
@@ -2404,7 +2407,7 @@ struct GlobalSearchConfig {

     let picker = Picker::new(
         columns,
-        1, // contents
+        2, // contents
         [],
         config,
         move |cx, FileResult { path, line_num, .. }, action| {

But for uniformity with the references picker, this would be better:

diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs
index 1c91510d..592e26a2 100644
--- a/helix-term/src/commands.rs
+++ b/helix-term/src/commands.rs
@@ -2266,10 +2266,12 @@ struct GlobalSearchConfig {

     let columns = [
         PickerColumn::new("path", |item: &FileResult, _| {
-            helix_stdx::path::get_relative_path(&item.path)
-                .to_string_lossy()
-                .into_owned()
-                .into()
+            format!(
+                "{}:{}",
+                helix_stdx::path::get_relative_path(&item.path).to_string_lossy(),
+                item.line_num + 1
+            )
+            .into()
         }),
         PickerColumn::hidden("contents"),
     ];

[the-mikedavis]

I think we should avoid adding an extra column for it - I don't think you would want to filter on the line number very often. We could use it in the path column so we show filename.ext:123 for line 123

[krish-r]

I noticed that the column width can only grow in an active picker, due to the existing logic -

// helix/helix-term/src/ui/picker.rs
if width as u16 > *max_width {
    *max_width = width as u16;
}

(Noticeable only in a couple of pickers with multiple columns, especially if one of them contains lengthy text.)

Do you think it would be better to have default column widths (perhaps not in this PR)?
This way, if the contents aren't too long, it can shrink / fallback to the defaults instead.

for ex. workspace symbol picker - current vs with default widths

current:

with default widths:

I also tried resetting it to the max width of current contents instead, but the constant change in widths made it difficult to follow the results.

[the-mikedavis]

The current width handling is pretty naive. I was thinking that improving it could be a follow-up. What we have now can take up a lot of space but minimizes how much the columns move around

[hakan-demirli]

Small Feedback:
Global search is updated/performed after I stop writing. There is a small delay between when I stop writing and global search is updated. It feels slow/less responsive compared to both neovim telescope and current helix workaround tmux+fzf+rg combo which updates global search instantly after each key press.
Ok. this has been addressed here.

Small Question:
Is fuzzy global word(s) search disabled by default?
When I am searching for a text file content lets say clipboard_tts. Searching with clipboard tts or cliptts returns nothing. I have to type exactly clipboard_tts.

On the other hand, when I am filtering/searching paths using %path It is fuzzy by default. So, like path I expect fuzzy search for word(s) by default. Or a config option to enable it.

[pascalkuthe]

Global search is not a fuzzy search its an interactive regex/grep search. It's the same as the current global search just that the results are updated as you type.