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.

Sign up for GitHub

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

Jump to bottom

Re-request workspace symbols on keypress in picker #3110

Closed
wants to merge 4 commits into from
Closed

Re-request workspace symbols on keypress in picker #3110

wants to merge 4 commits into from

Conversation

sudormrfbin
Copy link
Member

@sudormrfbin sudormrfbin commented Jul 19, 2022 •
edited

Fixes #1437. Most language servers limit the number of workspace symbols that are returned with an empty query even though all symbols are supposed to be returned, according to the spec (for perfomance reasons). This patch adds a workspace symbol picker based on a dynamic picker that allows re-requesting the symbols on every keypress (i.e. when the picker query text changes). The old behavior has been completely replaced, and I have only tested with rust-analyzer so far.

Before After
helix-old-workspace-symbols helix-dynamic-workspace-symbols

Notice how in the old picker the format_and_save function was not being picked up. This is because the picker only filtered the list of symbols that was initially given by rust-analyzer which does not include all symbols. In the dynamic picker a specific query of the current prompt text is sent to the server and we get accurate results from the server.

The newly added dynamic picker can also be used for interactive global search (#196), which is next on my list :)

TODO

  • Figure out a way to do debouncing (sending a request on every keypress is expensive and might cause screen jitters now and then).
  • Do not score the picker before replacing with new data, which causes old contents to be re-sorted and displayed only to be replaced by new content. Caused by reusing Picker inside DynamicPicker.

@sudormrfbin
Add force_score() for scoring picker items without optimizations
7599724
@sudormrfbin
Add DynamicPicker for updating options on every key
fbaca6e
@sudormrfbin
Re-request workspace symbols on keypress in picker
2d86d10 
Most language servers limit the number of workspace symbols that
are returned with an empty query even though all symbols are
supposed to be returned, according to the spec (for perfomance
reasons). This patch adds a workspace symbol picker based on a
dynamic picker that allows re-requesting the symbols on every
keypress (i.e. when the picker query text changes). The old behavior
has been completely replaced, and I have only tested with
rust-analyzer so far.
@sudormrfbin sudormrfbin marked this pull request as ready for review July 19, 2022 18:22
@archseer
Copy link
Member

Debounce: you could use the idle timer event that's used for completion, I can't remember if the change is there or not but I imagined it would trigger a generic event on the compositor so the components can handle idle timeout the way they want

sudormrfbin pushed a commit to sudormrfbin/helix that referenced this pull request Jul 23, 2022
@archseer @sudormrfbin
Add custom event type replacing crossterm's Event
bcdd44f 
Ported over from 61365df in the `gui` branch. This will allow
adding our own events, most notably an idle timer event (useful
for adding debounced input in [dynamic pickers][1] used by interactive
global search and workspace symbols).

[1]: helix-editor#3110
@sudormrfbin sudormrfbin mentioned this pull request Jul 23, 2022
Merged
@poliorcetics
Copy link
Contributor

Very much interested in this, thank you for working on it !

archseer added a commit to sudormrfbin/helix that referenced this pull request Aug 9, 2022
@archseer
Add custom event type replacing crossterm's Event
98809b9 
Ported over from 61365df in the `gui` branch. This will allow
adding our own events, most notably an idle timer event (useful
for adding debounced input in [dynamic pickers][1] used by interactive
global search and workspace symbols).

[1]: helix-editor#3110
archseer added a commit that referenced this pull request Aug 9, 2022
@sudormrfbin @archseer
Add custom event type replacing crossterm's Event (#3169)
634b6d4 
Ported over from 61365df in the `gui` branch. This will allow
adding our own events, most notably an idle timer event (useful
for adding debounced input in [dynamic pickers][1] used by interactive
global search and workspace symbols).

[1]: #3110

Co-authored-by: Blaž Hrastnik <blaz@mxxn.io>
@the-mikedavis the-mikedavis mentioned this pull request Aug 31, 2022
Closed
@sudormrfbin sudormrfbin linked an issue Sep 3, 2022 that may be closed by this pull request
Workspace symbol picker doesn't seem to work with Intelephense #3619
Closed
thomasskk pushed a commit to thomasskk/helix that referenced this pull request Sep 9, 2022
@sudormrfbin @archseer @thomasskk
Add custom event type replacing crossterm's Event (helix-editor#3169)
2b2368c 
Ported over from 61365df in the `gui` branch. This will allow
adding our own events, most notably an idle timer event (useful
for adding debounced input in [dynamic pickers][1] used by interactive
global search and workspace symbols).

[1]: helix-editor#3110

Co-authored-by: Blaž Hrastnik <blaz@mxxn.io>
@archseer archseer mentioned this pull request Sep 13, 2022
Open
@kirawi kirawi added A-language-server Area: Language server client A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Sep 13, 2022
@papertigers
Copy link
Contributor

Is there anything we can do to help move this forward?
Currently I am maintaining this patch in a local branch that I rebase onto. I have a few larger rust projects where I hit the symbol limit, making this PR really useful.

@johnrichardrinehart
Copy link

@the-mikedavis @archseer @sudormrfbin Anything blocking this from a merge? cc/ @papertigers

@sudormrfbin
Copy link
Member Author

It requires #3172 (which has some unresolved comments) and then a proper mechanism for debouncing.

@archseer
Copy link
Member

I think just #3172 is enough on it's own for now. It'll slightly lag since we will only send the request on idle and not per keypress but that at least gets it working.

@johnrichardrinehart
Copy link

johnrichardrinehart commented Oct 11, 2022 •
edited

I think merging this now in light of #3172 having had been merged makes sense. If per-key operations are excessively laggy then someone can file an issue and we can introduce a PR for debouncing separately.

This would unlock #196 🥺.

@the-mikedavis the-mikedavis mentioned this pull request Oct 16, 2022
Closed
@pppKin pppKin mentioned this pull request Oct 18, 2022
Closed
@the-mikedavis the-mikedavis mentioned this pull request Nov 9, 2022
Closed
@creikey creikey mentioned this pull request Nov 27, 2022
Closed
@the-mikedavis the-mikedavis mentioned this pull request Nov 28, 2022
Closed
@the-mikedavis the-mikedavis linked an issue Nov 28, 2022 that may be closed by this pull request
Workspace symbol picker is empty on a Dart project #4910
Closed
@the-mikedavis the-mikedavis mentioned this pull request Nov 28, 2022
Closed
@the-mikedavis the-mikedavis linked an issue Nov 28, 2022 that may be closed by this pull request
Workspace symbol picker is empty on a Typescript project #4916
Closed
@sgoodluck
Copy link

sgoodluck commented Nov 28, 2022 •
edited

#4916 is related.

I pulled down this PR and ran it successfully.

Please note that nothing appears until after I start typing (which may be by design?)

See asciinema below:
https://asciinema.org/a/uXMGy7yVQP4CfDgL4KTdhNCTS

@pascalkuthe pascalkuthe mentioned this pull request Dec 4, 2022
Closed
@the-mikedavis the-mikedavis mentioned this pull request Dec 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-helix-term Area: Helix term improvements A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
No milestone
7 participants
@sudormrfbin @archseer @poliorcetics @papertigers @johnrichardrinehart @sgoodluck @kirawi