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

Show hover results for multiple LSPs #9744

Closed

Conversation

kyfanc
Copy link
Contributor

@kyfanc kyfanc commented Feb 28, 2024

This PR attempts to address #8985.

Currently, only the first configured LSP is used for handling hover feature. This PR supports multiple configured LSPs and merged the results by appending in the popup markdown.

Result:

with single LSP configured

  • effectively same behaviour as before this PR.
single-lsp

with multiple LSP configured

  • results are joined in LSP response order, with LSP name prefixed to identify which LSP the result came from.
multiple-lsp

Testing:

  1. modify languages.toml to add multiple LSPs
# rust
[[language]]
name = "rust"
# multiple lsp
language-servers = [ "rust-analyzer", "rust-analyzer-dup"]

# secondary rust-analyzer
[language-server.rust-analyzer-dup]
command = "rust-analyzer"
  1. open a codebase, and trigger LSP hover commands

Discussion

Ordering of results

  • Since the LSP hover requests are dispatched in async callback style, the ordering of results is not guaranteed. So the ordering in popup markdown will follow to the LSP response order.
  • The LSP name prefix would help identify which LSP the response is coming from.

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Feb 28, 2024
@the-mikedavis the-mikedavis added A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer. and removed A-helix-term Area: Helix term improvements labels Feb 28, 2024
@kyfanc
Copy link
Contributor Author

kyfanc commented Mar 1, 2024

Updated to address some issues in previous commits.

  • now hover contents shall follow the lsp configure order more reliably
  • deduplication is performed for same LSP configured repeatedly
  • fix async race condition of slow hover response and hover popup interactions

@woojiq
Copy link
Contributor

woojiq commented Mar 5, 2024

suggestion (ui): a more visible separator between multiple results. Maybe some kind of bold horizontal line or even a double horizontal line.

@kyfanc
Copy link
Contributor Author

kyfanc commented Mar 6, 2024

Thank you for the suggestion.

Since the hover contents are rendering in markdown, I think it is better to stay with markdown horizontal rule --- as separator between LSPs results, so that future changes to markdown rendering will be compatible.

And I noticed that depending on the LSP and languages, the hover result from LSP may include --- as well, making it harder to identify our separators from a glance. And the LSP names varies, making it hard to spot the names especially jumping between languages.

I think adding a prefix to LSP name may help, as it provides a more consistent visual "anchor".

rust

Screenshot 2024-03-06 at 11 14 12 AM
  • rust-analyser may include ---
Screenshot 2024-03-06 at 11 12 57 AM
  • default theme
Screenshot 2024-03-06 at 11 10 42 AM

typescript

  • typescript-language-server looks cleaner
Screenshot 2024-03-06 at 11 24 09 AM

Any thoughts?

@gabydd
Copy link
Member

gabydd commented Mar 23, 2024

I wonder if we want to use the ui from #9974 instead

@kyfanc
Copy link
Contributor Author

kyfanc commented Mar 25, 2024

The UI from in that PR looks cool!

With a brief look, it seems that the code for signature help feature has been refactored with the new event system recently, while the hover feature is still using the old way. The event system helps dealing with the interactivity of cycling through async results.

For the key binding for cycling, intuitively, I think it would be great to share the same keybindings with any popup feature, like in that PR, but I am not sure if it is possible under the current architecture.

Considering the scale of refactoring and discussion of keybinding for cycling through "popup pages", maybe that could be another PR?

Anyway, I have pushed the prefix improvement mentioned previously for now, so it is ready to merged as is.
And I will look into the event system/ cycle UI if I have time.

@kyfanc
Copy link
Contributor Author

kyfanc commented Apr 3, 2024

I opened another PR #10122 for the new approach with event system and cycling pages UX.
This PR can be closed if that is more promising.

@pascalkuthe
Copy link
Member

pascalkuthe commented Apr 9, 2024

I like the cycle approach better, let's close tgjsn in favor of #10122 👍

@pascalkuthe pascalkuthe closed this Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants