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

feat(pickers): implement group-by. #2771

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

griwes
Copy link

@griwes griwes commented Nov 7, 2023

Description

This PR implements a feature to enable having a grouping of entries have a "header" indicating the common field. This is very similar to what telescope-egrepify does display-wise, but fully configurable to allow grouping by arbitrary entry properties (to be fully honest, I don't right now know when I'd use this with anything than filename, but making this general was easy enough), and to allow the customization of the display of those headers.

Noteworthy points:

  1. This does not impact the order of entries. If it turns out that entries from the same group end up separated by other entries, you'll get two groupings with two separate headers for them. When I was starting this, I was planning to have this affect the order, but gave up on that idea very quickly; it's possible that because of that, the name "group by" is no longer deserved; if that's the case, then I'm open to other name suggestions.
  2. The majority of the small, scattered changes implement a workaround for API: control virt_lines_above, viewport, screen lines, scroll neovim/neovim#16166. In general, the headers are just displayed with an extmark, with virt_lines set to the header text, and virt_lines_above = true. However, for the very first header that does not currently work due to the neovim bug, so as a workaround I insert an additional blank line into the results buffer, and place an overlay extmark there. This complicates some of the math, but after fighting many, many off-by-one errors, I think I've got it right at this point?
  3. There's a drive-by fix for an issue that happens on the master branch of telescope itself, where in some cases setting the ascending sorting direction would make telescope leave garbage entries at the bottom of the buffer (they weren't selectable, but would show up, without any highlights - I guess this is Halloween-appropriate?). It was bothering me a lot, so one of the changes in _clear_extra_rows makes sure that the ascending sorting direction still has its extra rows cleared when it is at max_results.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Resolves #2297

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list relevant details about your configuration

Extensively manually tested a roughly complete cartesian product of:

  • live_grep and lsp_references
  • with the group_by argument being a string, or a table with overrides
  • with and without a default configuration
  • with both sorting strategies
  • with and without devicons (for both the entries and the headers)
  • with and without path_display = "hidden"

Configuration:

  • Neovim version (nvim --version): NVIM v0.10.0-dev-324fad1
  • Operating system and version: Debian unstable

Checklist:

  • My code follows the style guidelines of this project (stylua)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas (I don't think it needs much more in terms of comments, but I'm happy to add comments wherever you deem them to be necessary)
  • I have made corresponding changes to the documentation (lua annotations)

@jugarpeupv
Copy link

@griwes thanks so much for the implementation, i am quite unfamiliar with lua, do you know how can someone try this code in their personal neovim config?

Please @Conni2461 could you review this PR? This feature would be awesome, order the results by filename just like VsCode.

@jamestrew
Copy link
Contributor

I'm a little per-occupied to review bigger features at the moment but rest assured I haven't forgotten about this.

@jugarpeupv probably just replacing nvim-telescope/telescope.nvim with griwes/telescope.nvim and setting branch to group-by in your package manager.

If more people want to give this a test, that'll be appreciated as well.

@jugarpeupv
Copy link

@jamestrew i tried to test this with your indications but an error arises, i do not understand why (see the image)

@griwes thank you for the implementation, I was wondering if you could provide an example on how to invoke the live_grep command, i was reading the code and i guess i would figure it out but i would appreciate it since I am still not quite familiar with how telescope pickers work.

image image

@max397574
Copy link
Contributor

@jugarpeupv you didn't write the correct branch name
you used _ instead of -

@jugarpeupv
Copy link

@max397574 thank you! i managed to get it to work, it is just what i was looking for

@jamestrew hope this get merged soon

image

@jugarpeupv
Copy link

@griwes one thing i would suggest is to hide fileicon on the registries, since we can see the filetype icon on the header. It is just to see less icons which does not supply relevant information

@griwes
Copy link
Author

griwes commented Dec 22, 2023

@jugarpeupv you can also pass in path_display = 'hidden' to the picker. The reason I didn't do anything with that option is that it already exists and seems fairly orthogonal to the feature at hand.

As to extensions, if they accept and pass through the arguments passed into them to the picker, this should also work there. If they don't, there really isn't much that can be done other than modifying the extensions.

@jugarpeupv
Copy link

@griwes I just tested with path_display but i do not see the results :S

image

As to extensions, i deleted the comment because i managed to get it working with the extension live_grep_args, just passing group_by = "filename", so it is fine

Thank you for your quick reply ^^

@jugarpeupv
Copy link

@griwes i just added path_display = "hidden" to the telescope global config and i can see the result, but this is not what i was talking about. I was pointing that the icons shown in the registries might be omitted, but in the registries, not in the header

image

@griwes
Copy link
Author

griwes commented Dec 22, 2023

Ah right, I forgot that path_display doesn't hide the icons; there's a separate option called disable_devicons that you can set to true to hide them. (The group_by object also accepts disable_devicons separately from that, to control just the icons in the header.)

@max397574
Copy link
Contributor

i don't think this should be in the documentation so it doesn't get to big
but perhaps after the pr is merged could you provide an example on how one would write a custom group-by function (assuming that is possible) e.g. for grouping by filetype in find_files

@jugarpeupv
Copy link

@griwes disable_devicons is working as i expected, thank you.

@griwes
Copy link
Author

griwes commented Mar 30, 2024

@jamestrew any chance you could give this a cursory glance soon-ish? I haven't looked at what the current conflicts are yet, but I'd like to rebase this in the near future and would love some initial feedback, specifically as to whether the approach in this PR is generally acceptable or if something fundamental would need to change, ideally before I start rebasing.

@asilvadesigns
Copy link

👀

@max397574
Copy link
Contributor

@griwes @jamestrew any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add grouping by file to "live_grep" picker
5 participants