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

Refactor Picker as a table with multiple columns/filters #7265

Closed
wants to merge 3 commits into from

Conversation

the-mikedavis
Copy link
Member

@the-mikedavis the-mikedavis commented Jun 7, 2023

The Menu-like portion of the Picker is replaced with a table. Picker::new takes a set of Columns which each contain custom logic for rendering and optionally sorting and filtering an option. (Previously ui::menu::Item implemented that logic.) This allows filtering a picker by more than one value.

Here's a small example showing changes to the symbol picker (<space>s) and the buffer picker (<space>b):

demo

Here I open up the symbol picker (<space>s) which has two columns: symbol kind (function, enum, module, class, etc.) and name. By default the search works on the primary column which is configured as the symbol name. I can also limit the search to just %kind:field though. The buffer picker can filter by buffer ID, modification/focus flags and path. The diagnostics pickers can filter by severity, code, message, and path (for the workspace-wide diagnostics picker).

The workspace symbol picker can filter separately by symbol name, filename, and symbol kind. The symbol name is no longer passed to the fuzzy matcher which fixes some minor issues when using language-server-specific query syntaxes (see #5714). This paves the way for dynamic global search but I have left that off of this PR as the change will be large on its own.

Closes #7040
Closes #5714
Closes #5446
Closes #5110
Closes #2452
Closes #3543

There are a few TODOs:

  • Allow quoting in the picker's field syntax
  • Figure out if this needs theming changes (adding a theme key for the primary column?)
  • Improve width constraint calculations so large columns share the space better

@the-mikedavis the-mikedavis added C-enhancement Category: Improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-needs-discussion Status: Needs discussion or design. labels Jun 7, 2023
@pascalkuthe
Copy link
Member

pascalkuthe commented Jun 7, 2023

This is awesome! I wanted to implement this sometime too but never got around to it. I think this has a ton of potential 👍

I dont have time to review right now but it's great to see that you started on this

@the-mikedavis
Copy link
Member Author

Separate prompts for each field feels a little clunky. Instead I think we should have only one prompt, drop the "active column" concept, and use a lucene-like syntax for filtering by fields. For example the symbol picker could take some queries:

  • func mysym: fuzzy-match by "func mysym" across all fields
  • kind:func mysym: fuzzy-match by "func" in the "type" field and "mysym" in the remaining fields (symbol name)
  • func name:mysym: fuzzy-match by "mysym" in the name field and "func" in the remaining fields (symbol kind)

(The ordering in these examples shouldn't matter.) Values with spaces could be wrapped in quotes, for example kind:"enum member". The matches would have an implicit AND relationship but we could support AND/OR as well if necessary.

Other examples:

  • flags:+ in the buffer picker would filter down to modified files
  • severity:error code:34 path:src/ my error message could be used in the diagnostics picker

@pascalkuthe
Copy link
Member

pascalkuthe commented Jun 8, 2023

I like the general idea of having a single field. The reason I suggested split columns before is that:

  • Not all columns may fit on screen at once
  • potential false positives (solved by the explicit kind: syntax but makes matching for multiple words in the same column (which I do quite a lot) without false positives quite awkward For example: file:syntax.rs file:helix-core file:src instead of syntax.rs helix-core src` on master.
  • Most importantly: How does this work with dynamic pickers. Fuzzy matching across all columns doesn't work for regex queries
  • one Small thing : is too common to require escaping we should use a less common char

My idea to resolve these is the following:

  • %kind will treat all following words until the next word starting with % (or EOL) as queries for the kind column. While typing in such an area that column is called active
  • It the last column name is unknown (or empty) all (non-dynamic} columns are active at once (like you said fuzzy match across everything)
  • When there is not enough space to display all columns, priority is given to the active column
  • Since dynamic columns always need to be manually activated we can prefill the picker when it opens to make that more convenient. For example for interactive global search, we prefill %query foo+bar % (typing immediately fuzzy matches all columns which would just be path in this case while keeping the qury from the prompt ). For the workspace picker, we just prefill %query and the user can start filtering immediately.

A distant future idea: we could create typable command versions of the pickers that accept a prefill as an argument. This would allow binding: open function picker to a key without adding a dedicated command.

It doesn't look as sleek as your variant but it should work quite well. What do you think?

@the-mikedavis
Copy link
Member Author

For dynamic pickers, I have some local changes that allow disabling filtering/sorting on a column. That will resolve #5714 and allow entering regexes for the global search dyn picker. I also have a change locally that allows specifying an input register for global search to set an initial query.

For prefilling pickers, I think specifying a register would be a nice way to do it. I have some local changes building on #7277 that allow a sequence like "/<space>/ to open global search using the / register contents as the initial query. That could be done for other pickers as well.

I like the active column idea you describe for a priority in rendering. For matches with long lines the global search picker can get very cramped. I think there's room for improvement with truncating columns so that outlier long matches don't push the other columns out, and columns share the available space more evenly.

I'd like to start with a proof-of-concept using the : syntax but try it both ways. I think : might be sufficient: we may be able to make smart decisions while parsing to ignore :s not used in fields. And about the file:syntax.rs file:helix-core file:src example, I think that could be the behavior of file:"syntax.rs helix-core src".

@pascalkuthe
Copy link
Member

pascalkuthe commented Jun 8, 2023

For dynamic pickers, I have some local changes that allow disabling filtering/sorting on a column. That will resolve #5714 and allow entering regexes for the global search dyn picker. I also have a change locally that allows specifying an input register for global search to set an initial query.

For prefilling pickers, I think specifying a register would be a nice way to do it. I have some local changes building on #7277 that allow a sequence like "/<space>/ to open global search using the / register contents as the initial query. That could be done for other pickers as well.

I like the active column idea you describe for a priority in rendering. For matches with long lines the global search picker can get very cramped. I think there's room for improvement with truncating columns so that outlier long matches don't push the other columns out, and columns share the available space more evenly.

I'd like to start with a proof-of-concept using the : syntax but try it both ways. I think : might be sufficient: we may be able to make smart decisions while parsing to ignore :s not used in fields. And about the file:syntax.rs file:helix-core file:src example, I think that could be the behavior of file:"syntax.rs helix-core src".

What I mean about the dynamic picker is that when you enter a string without specifying a column it wouldn't work well to fuzzy match that across all column (I would iamgine that works by ocmbining all colums into a single string that is then fuzzyatched on so you can't also modify the query that way). I think for dynamic pickers you always have to specify the column. Hence the idea to have an easy syntax for specifying queries with spaces. I think something more convenient that quotes might be nice for that but we can always bikeshed that later.

Yeah for prototyping the syntax probably doesn't matter that much 👍 It should be easy to change/experiment with later. If we use column: we could indeed make that only active if such a column actually exists (removing the need for escape in most cases). That could.be a nice variant too.

Using a register for prefill is nice. That syntax looks neat

@the-mikedavis
Copy link
Member Author

Ah yeah I see what you mean now. For the time being I have made the behavior different in dynamic pickers and regular pickers: in dynamic pickers, the "common" text (text without a field specified) automatically acts as the dynamic column's input any other fields need to be specified explicitly. It feels intuitive but I'm not sure I like having split behaviors.

@the-mikedavis the-mikedavis added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-needs-discussion Status: Needs discussion or design. labels Jun 14, 2023
@jhugs
Copy link

jhugs commented Jun 30, 2023

When you get a chance to rebase again I'd love to test this out

@the-mikedavis the-mikedavis force-pushed the table-picker branch 2 times, most recently from f61360f to 8af595c Compare September 3, 2023 21:23
@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-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Sep 3, 2023
@the-mikedavis
Copy link
Member Author

I rebased this on top of master with the nucleo changes. Kudos to @pascalkuthe: the column support in nucleo makes the changes to the fuzzy matching parts very clean! 🎉

I have reduced the scope of this PR a little so that dynamic global search is left for the future. global_search now includes the matched line's contents but isn't a DynamicPicker. That change can be done after this PR and I'd like to base it on #8021 so that we have better control over DynamicPicker's debounce behavior. (IME running this branch we want a longer debounce for global search.)

There are a few more changes I'd like to make to this branch but I think it's nearly ready for review.

@pascalkuthe
Copy link
Member

Github doesn't let me link this properly (the ui is super broken) but this also addresses #3543

@matuusu
Copy link

matuusu commented Jan 10, 2024

Are there any plans to move this PR forward?

@the-mikedavis
Copy link
Member Author

Yep, I keep this branch up to date locally and build on it for other changes (see #196 (comment)). I'm waiting on #8021 for some of the changes to global search but the table refactor could be done independently. I'll rebase this branch and clean up the commits when I get a chance.

There are two small changes this branch needs too:

  • The table header needs to be accounted for when paging up/down in the results - currently we ignore the header so we're off by a few lines when switching pages
  • @pascalkuthe mentioned a good idea that you should be able to use prefixes of column names and have that select the correct column, so you could say %p:syntax.rs instead of %path:syntax.rs for example

@ItsEthra
Copy link
Contributor

@pascalkuthe mentioned a good idea that you should be able to use prefixes of column names and have that select the correct column, so you could say %p:syntax.rs instead of %path:syntax.rs for example

I have a branch that implements it, should I make a pr to this branch for that? I made it so if multiple columns match %prefix, then it selects the columns with the shortest name(because it would fit prefix the most) or the first column of all matches.

@the-mikedavis
Copy link
Member Author

I will probably recreate this branch from scratch rather than deal with the conflicts. If you link your branch I will pick up the commit from it when I revisit this

@ItsEthra
Copy link
Contributor

ItsEthra commented Jan 16, 2024

My branch is here, I also rebased it on master, there was just one conflict and it seems alright.

EDIT: Search results are dupped on that branch, ig conflicts were not easy after all. fixed

the-mikedavis and others added 3 commits January 23, 2024 11:15
`menu::Item` is replaced with column configurations for each picker
which control how a column is displayed and whether it is passed to
nucleo for filtering. (This is used for dynamic pickers so that we can
filter those items with the dynamic picker callback rather than nucleo.)

The picker has a new lucene-like syntax that can be used to filter the
picker only on certain criteria. If a filter is not specified, the text
in the prompt applies to the picker's configured "primary" column.

Adding column configurations for each picker is left for the child
commit.
This removes the menu::Item implementations for picker item types and
adds `Vec<Column<T, D>>` configurations.
So you can select a field by a substring of its name, like
`%p:syntax.rs` to specify the `path` field as "syntax.rs".

Co-authored-by: ItsEthra <107059409+ItsEthra@users.noreply.github.com>
// select a column that fits key the most.
field = column_names
.iter()
.filter(|col| col.starts_with(&text))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pascalkuthe what do you think about using fuzzy matching for this instead? It would be more forgiving, so you could use %pth:syntax.rs for path for example. We could even highlight the parts of the column name that matched, like we do for the parts of the query that match items

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this and fuzzy matching seems like overkill - the prefix is more straightforward. Highlighting the matching parts could also be done for prefix strings as well and would be simpler. (Could be a follow-up to #9647)

This was referenced Feb 10, 2024
@the-mikedavis
Copy link
Member Author

Closing in favor of #9647 which includes these commits

@the-mikedavis the-mikedavis deleted the table-picker branch February 17, 2024 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
5 participants