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

Pickers "v2" #9647

Merged
merged 20 commits into from
Jul 15, 2024
Merged

Pickers "v2" #9647

merged 20 commits into from
Jul 15, 2024

Conversation

the-mikedavis
Copy link
Member

@the-mikedavis the-mikedavis commented Feb 17, 2024

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:

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

NOTE: the syntax in the demos is out of date. Docs are being worked on in #11218.

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

table-picker demo

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

global search demo

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

@the-mikedavis the-mikedavis added C-enhancement Category: Improvements A-helix-term Area: Helix term improvements E-hard Call for participation: Experience needed to fix: Hard / a lot S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 17, 2024
@archseer
Copy link
Member

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

helix-term/src/ui/picker/handlers.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker/handlers.rs Show resolved Hide resolved
@pascalkuthe
Copy link
Member

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

@the-mikedavis
Copy link
Member Author

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:

// -- 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(
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
Copy link

sicher commented Feb 17, 2024

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
Copy link
Member Author

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

@univerz
Copy link

univerz commented Feb 17, 2024

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 the-mikedavis force-pushed the pickers-v2 branch 2 times, most recently from 02554fb to 71e1d2f Compare February 17, 2024 15:46
@the-mikedavis
Copy link
Member Author

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

helix-term/src/ui/picker/handlers.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker/handlers.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker/handlers.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker/query.rs Outdated Show resolved Hide resolved
@thiagomajesk
Copy link

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

@Philipp-M
Copy link
Contributor

Philipp-M commented Mar 2, 2024

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
Copy link
Member Author

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
Copy link
Contributor

Philipp-M commented Mar 2, 2024

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
Copy link
Contributor

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
Copy link
Member Author

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

archseer
archseer previously approved these changes Jul 15, 2024
We can track the ranges in the input text that correspond to each column
and use this information during rendering to apply a new theme key that
makes the "active column" stand out. This makes it easier to tell at
a glance which column you're entering.
@archseer archseer merged commit 08ee8b9 into master Jul 15, 2024
6 checks passed
@archseer archseer deleted the pickers-v2 branch July 15, 2024 14:31
@lemontheme
Copy link
Contributor

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:

asciicast

  1. 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.

asciicast


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

@the-mikedavis
Copy link
Member Author

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
Copy link
Contributor

rcorre commented Jul 16, 2024

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
Copy link
Contributor

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

rcorre added a commit to rcorre/helix that referenced this pull request Jul 18, 2024
Filtering on columns was implemented in helix-editor#9647.
The only documentation I could find on this feature
was the PR itself, and the video demo used a different syntax.
pascalkuthe pushed a commit that referenced this pull request Jul 24, 2024
* Document use of filter columns in pickers.

Filtering on columns was implemented in #9647.
The only documentation I could find on this feature
was the PR itself, and the video demo used a different syntax.

* Note that column filters are space-separated.

* Note that picker filters can be abbreviated.

* Specify correct picker in docs.

* Clarify picker filter prefix shortenting.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Move picker docs to their own section.

* Update book/src/pickers.md

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Improve docs on picker registers, keybinds, and syntax.

* Clarify wording around picker queries.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

---------

Co-authored-by: Ryan Roden-Corrent <ryan@rcorre.net>
Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
@peteringram0
Copy link

Im missing the underline's on the table headings. Does it only work with some fonts or something?

@the-mikedavis
Copy link
Member Author

There are new theme keys you can add to your theme to style the headers. In my theme I have basically:

"ui.picker.header.column" = { underline.style = "line" }
"ui.picker.header.column.active" = { modifiers = ["bold"], underline.style = "line" }

Also see #11343

SofusA pushed a commit to SofusA/helix-pull-diagnostics that referenced this pull request Aug 4, 2024
* Document use of filter columns in pickers.

Filtering on columns was implemented in helix-editor#9647.
The only documentation I could find on this feature
was the PR itself, and the video demo used a different syntax.

* Note that column filters are space-separated.

* Note that picker filters can be abbreviated.

* Specify correct picker in docs.

* Clarify picker filter prefix shortenting.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Move picker docs to their own section.

* Update book/src/pickers.md

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Improve docs on picker registers, keybinds, and syntax.

* Clarify wording around picker queries.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

---------

Co-authored-by: Ryan Roden-Corrent <ryan@rcorre.net>
Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
mxxntype pushed a commit to mxxntype/helix that referenced this pull request Aug 14, 2024
* Document use of filter columns in pickers.

Filtering on columns was implemented in helix-editor#9647.
The only documentation I could find on this feature
was the PR itself, and the video demo used a different syntax.

* Note that column filters are space-separated.

* Note that picker filters can be abbreviated.

* Specify correct picker in docs.

* Clarify picker filter prefix shortenting.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Move picker docs to their own section.

* Update book/src/pickers.md

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Improve docs on picker registers, keybinds, and syntax.

* Clarify wording around picker queries.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

---------

Co-authored-by: Ryan Roden-Corrent <ryan@rcorre.net>
Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
stackotter pushed a commit to stackotter/helix that referenced this pull request Aug 28, 2024
* Document use of filter columns in pickers.

Filtering on columns was implemented in helix-editor#9647.
The only documentation I could find on this feature
was the PR itself, and the video demo used a different syntax.

* Note that column filters are space-separated.

* Note that picker filters can be abbreviated.

* Specify correct picker in docs.

* Clarify picker filter prefix shortenting.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Move picker docs to their own section.

* Update book/src/pickers.md

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Improve docs on picker registers, keybinds, and syntax.

* Clarify wording around picker queries.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

---------

Co-authored-by: Ryan Roden-Corrent <ryan@rcorre.net>
Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
kyruzic pushed a commit to kyruzic/helix that referenced this pull request Sep 27, 2024
* Document use of filter columns in pickers.

Filtering on columns was implemented in helix-editor#9647.
The only documentation I could find on this feature
was the PR itself, and the video demo used a different syntax.

* Note that column filters are space-separated.

* Note that picker filters can be abbreviated.

* Specify correct picker in docs.

* Clarify picker filter prefix shortenting.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Move picker docs to their own section.

* Update book/src/pickers.md

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Improve docs on picker registers, keybinds, and syntax.

* Clarify wording around picker queries.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

---------

Co-authored-by: Ryan Roden-Corrent <ryan@rcorre.net>
Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
plul pushed a commit to plul/helix that referenced this pull request Oct 13, 2024
* Document use of filter columns in pickers.

Filtering on columns was implemented in helix-editor#9647.
The only documentation I could find on this feature
was the PR itself, and the video demo used a different syntax.

* Note that column filters are space-separated.

* Note that picker filters can be abbreviated.

* Specify correct picker in docs.

* Clarify picker filter prefix shortenting.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Move picker docs to their own section.

* Update book/src/pickers.md

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Improve docs on picker registers, keybinds, and syntax.

* Clarify wording around picker queries.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

---------

Co-authored-by: Ryan Roden-Corrent <ryan@rcorre.net>
Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
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 E-hard Call for participation: Experience needed to fix: Hard / a lot S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet