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

Add lsp signature help #1755

Merged
merged 21 commits into from
Jul 19, 2022
Merged

Conversation

sudormrfbin
Copy link
Member

@sudormrfbin sudormrfbin commented Mar 5, 2022

Implements signature help, aka parameter hints:

command

TODO

  • Obstructs the completion popup
  • Bar cursor in insert mode can be seen through the popup if popup overlays the cursor
  • Popup is not updated on backspace
  • editor.lsp.signature-help config
  • Keybind for manual triggering (Ctrl-S in insert mode)

Future Work

  1. Show all signatures for a method (useful for overloaded methods)

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Obstructs the completion popup

There's a TODO item in Popup or about adding a Bias::Above/Below that would specify a preference for popup placement around the cursor (This is only a preference because you still want to place it below if there's no space above)

// if there's a orientation preference, use that
// if we're on the top part of the screen, do below
// if we're on the bottom part, do above
// -- make sure frame doesn't stick out of bounds
let mut rel_x = position.col as u16;
let mut rel_y = position.row as u16;
if viewport.width <= rel_x + width {
rel_x = rel_x.saturating_sub((rel_x + width).saturating_sub(viewport.width));
}
// TODO: be able to specify orientation preference. We want above for most popups, below
// for menus/autocomplete.
if viewport.height > rel_y + height {
rel_y += 1 // position below point
} else {
rel_y = rel_y.saturating_sub(height) // position above point
}
.

I'd add this as a builder method similar to margin:

pub fn margin(mut self, margin: Margin) -> Self {
self.margin = margin;
self
}

Then you can specify the signature help to be displayed above, and completion below.

We also want to add a editor.lsp.signature-help config item that would allow configuring / disabling this since I can imagine some users find it distracting.

helix-term/src/ui/popup.rs Outdated Show resolved Hide resolved
helix-term/src/commands/lsp.rs Show resolved Hide resolved
@archseer
Copy link
Member

archseer commented Mar 7, 2022

helix-term/src/commands.rs Outdated Show resolved Hide resolved
@sudormrfbin sudormrfbin linked an issue Mar 10, 2022 that may be closed by this pull request
@sudormrfbin sudormrfbin force-pushed the signature-help-popup branch from a09d948 to 4a48370 Compare March 12, 2022 16:39
@sudormrfbin sudormrfbin force-pushed the signature-help-popup branch from a23536e to 87efd55 Compare March 22, 2022 16:50
sudormrfbin added a commit to sudormrfbin/helix that referenced this pull request Mar 26, 2022
This is mainly done to accomodate the new lsp.signature-help config
option that will be introduced in helix-editor#1755
which will have to be accessed by commands. The top level config
struct is split and moved to different places, making the relocation
necessary
sudormrfbin added a commit to sudormrfbin/helix that referenced this pull request Mar 26, 2022
This is mainly done to accomodate the new lsp.signature-help config
option that will be introduced in helix-editor#1755
which will have to be accessed by commands. The top level config
struct is split and moved to different places, making the relocation
necessary
archseer pushed a commit that referenced this pull request Mar 28, 2022
* Move top level lsp config to editor.lsp

This is mainly done to accomodate the new lsp.signature-help config
option that will be introduced in #1755
which will have to be accessed by commands. The top level config
struct is split and moved to different places, making the relocation
necessary

* Revert rebase slipup
@sudormrfbin sudormrfbin force-pushed the signature-help-popup branch from 87efd55 to 3000bf4 Compare March 28, 2022 10:43
@sudormrfbin
Copy link
Member Author

I added Ctrl-s as a keybind for invoking signature help in insert mode just because, any other suggestions ?

@the-mikedavis
Copy link
Member

It might be easier to remember something like CtrlShiftx but OTOH that's a lot of keystrokes. I think Ctrls is ok. Ctrlh could be another good option but we'd have to kill (haha) the readline/emacs binding that's already there.

@sudormrfbin
Copy link
Member Author

ctrl-h would be a better keybind but it'll probably clash with backspace on a lot of terminal emulators. Switching to termwiz might be beneficial for it ability to distinguish between these too.

@archseer
Copy link
Member

Switching to termwiz might be beneficial for it ability to distinguish between these too.

Even then we probably shouldn't rely on https://sw.kovidgoyal.net/kitty/keyboard-protocol/ by default since not every terminal supports it.

@David-Else
Copy link
Contributor

ctrl-s is not a good shortcut as it means save in most editors. ctrl-space triggers autocomplete in VS Code, how about that?

@the-mikedavis
Copy link
Member

the-mikedavis commented Apr 25, 2022

That's a bad reason to avoid a key. I think it's even a weak reason to prefer a key (e.g. ctrl-w that deletes a word backwords in vim in insert mode). Helix's keymap shouldn't be bound by the keys other editors choose.

Ctrl-Space could be interesting though as long as the terminal accepts the inputs (iirc it gives a keycode of 0x00. idk if that gets confused with another key)

@the-mikedavis
Copy link
Member

I noticed testing this that Backspace in insert mode will pop up the "Language server not active for current buffer" status message for buffers like scratch or text. Probably not a big deal but it does grab the eye a bit.

book/src/keymap.md Outdated Show resolved Hide resolved
match &param.label {
lsp::ParameterLabel::Simple(string) => {
let start = signature.label.find(string.as_str())?;
Some((start, start + string.len()))
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't look closely on this. Does this also work with double width characters like CJK? This seemed to be characters length after looking at the code (wasn't mentioned in the comment whether it is character length or terminal width length) and we don't seemed to be checking the width of required_size.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think it should if the intent is to use byte length, or do we need to use string.width() ?

Copy link
Member Author

@sudormrfbin sudormrfbin Jun 25, 2022

Choose a reason for hiding this comment

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

(This actually turned out to be a bug, see #3154.)


Syntax highlighting uses char byte offsets so that is what we need as the final (start, end) value. Now, LSP sends the label offsets in utf-16 according to the configured OffsetEncoding (but we have it hardcoded to utf8) and the same is used by the LSP for ParameterLabel::LabelOffsets, so we would have to convert this to char byte offsets:

                    lsp::ParameterLabel::LabelOffsets([start, end]) => {
                        Some((*start as usize, *end as usize))
                    }

str.find() reports the byte index of the start of the pattern, so that would have to be changed too.

But strangely, with a naive test on a bit of CJK and non ASCII text, the parameter highlighting seems to work okay:

helix-signature-help-cjk

Details
fn main() {}

/// Documentation with doc tests:
///
/// ```
/// test
/// ```
fn func(_par老am0: char, _lö老虎wy̆: usize, _paröam1: &str) {
    return;
}

@archseer
Copy link
Member

Found one issue: We need to check for the signature help capability before sending the request. If the language server doesn't support it then "Async job failed: Unhandled method textDocument/signatureHelp" is emitted to the statusline.

@sudormrfbin
Copy link
Member Author

A lesser pain point is that with larger font (have weak eyes) it is often that full doc does not fit above, and it starts to try fit below. My suggestion would be to keep it simple and to just show function signature. Full doc also obstructs code when i would like to see the names of variables i defined above.

@Houkime I'll add a config option to control the display of docs 👍🏾

@sudormrfbin
Copy link
Member Author

Escape now closes the popup and exits insert mode, similar to the completion popup. Closing the popup only can be done with ctrl-c. Also added a config option editor.lsp.display-signature-help-docs that controls the docs display.

@lazytanuki
Copy link
Contributor

Hi and thanks for this feature !

Been playing with it a little bit, found something that looks like an issue: when writing a closure and hitting Backspace, the signature help shows up for the function for which the closure is an argument (may not be clear, see screenshot below). You should be able to reproduce it by editing helix-term/src/commands/lsp.rs as shown below:

image

Also, a keybinding suggestion: how about mapping C-u and C-d (which are already quite present in different parts of the editor) to put the signature help popup above or below current line (when it's already being displayed, not to trigger it) ? User case is when editing code in the middle of a function, signature help pops up below the current line, but you want to see what's there so you just press C-u and it makes the signature help switch to being above the current line.

@the-mikedavis
Copy link
Member

Backspace triggers the LSP call - showing the signature for the function in that case is up to rust-analyzer. It can be a bit odd to show the signature of the outer call when you're in a closure - maybe we should open an issue in rust-analyzer?

@lazytanuki
Copy link
Contributor

lazytanuki commented Jun 28, 2022

I am not sure about that, it is not doing it in neovim with the lsp signature plugin. When specifically asking for the signature help when inside the closure, this plugin returns "No signature help available". I'll try to see how this is handled by that plugin. Maybe a treesitter scope check ?

Edit: Welp, I am on a different setup right now and the issue seems to be gone, maybe it was a bug in RA and I'm using a different version at the moment, I'll check when I get back home.
Edit2: Yup, was just me running an old version of RA, cause this was fixed 3 months ago

@sudormrfbin
Copy link
Member Author

Also, a keybinding suggestion: how about mapping C-u and C-d (which are already quite present in different parts of the editor) to put the signature help popup above or below current line (when it's already being displayed, not to trigger it)

C-u and C-d are already taken for scrolling the popup, and I think having a single keybind that toggles top and bottom orientations would be better anyway. And it would fit better as a generic keybind added to any popup rather than only the signature help, so a separate PR will be better (after this one gets merged, since position biasing is introduced here).

Comment on lines +170 to +171
/// Enable automatic pop up of signature help (parameter hints)
pub auto_signature_help: bool,
Copy link
Contributor

@pickfire pickfire Jun 29, 2022

Choose a reason for hiding this comment

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

Is there a way to manually trigger signature help if this is disabled? We can manually trigger completion with ctrl-x, maybe once we trigger that it automatically triggers signature help as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a keybind earlier but removed it due to a conflict. Adding a keybind is as easy as binding it to signature_help command, and we definitely need a built-in insert mode keybind too. Any suggestions?

We can manually trigger completion with ctrl-x, maybe once we trigger that it automatically triggers signature help as well?

I'd rather have a separate key that can toggle just the signature help popup since that allows exclusive control.

@pickfire
Copy link
Contributor

I was trying this branch out and noticed that the editor.auto-completion = false no longer works as expected, even when I did not manually trigger the auto completion it still shows up.

@sudormrfbin
Copy link
Member Author

I was trying this branch out and noticed that the editor.auto-completion = false no longer works as expected, even when I did not manually trigger the auto completion it still shows up.

I can't reproduce this; I tried both :set auto-completion false inside the editor and editor.auto-completion = false in the TOML config and neither of them show the completion popup unless manually triggered. The signature help pops up regardless of the auto completion setting, which is as expected.

@archseer archseer merged commit 791bf7e into helix-editor:master Jul 19, 2022
@sudormrfbin sudormrfbin deleted the signature-help-popup branch July 19, 2022 04:39
sudormrfbin added a commit to sudormrfbin/helix that referenced this pull request Jul 22, 2022
The language server sends a char offset range within the
signature help label text to highlight as the current parameter,
but helix uses byte offset ranges for rendering highlights. This
was brought up in the [review of the original signature help PR][1],
but the ranges were being highlighted correctly, and there were no
out of bound or indexing panics. Turns out rust-analyzer was
[incorrectly sending byte offsets] instead of char offsets and this
made it seem like all was well and good with offsets in helix during
initial testing.

[1]: helix-editor#1755 (comment)
[2]: rust-lang/rust-analyzer#12272
sudormrfbin added a commit to sudormrfbin/helix that referenced this pull request Jul 23, 2022
The language server sends a char offset range within the
signature help label text to highlight as the current parameter,
but helix uses byte offset ranges for rendering highlights. This
was brought up in the [review of the original signature help PR][1],
but the ranges were being highlighted correctly, and there were no
out of bound or indexing panics. Turns out rust-analyzer was
[incorrectly sending byte offsets][2] instead of char offsets and this
made it seem like all was well and good with offsets in helix during
initial testing.

[1]: helix-editor#1755 (comment)
[2]: rust-lang/rust-analyzer#12272
archseer pushed a commit that referenced this pull request Jul 29, 2022
The language server sends a char offset range within the
signature help label text to highlight as the current parameter,
but helix uses byte offset ranges for rendering highlights. This
was brought up in the [review of the original signature help PR][1],
but the ranges were being highlighted correctly, and there were no
out of bound or indexing panics. Turns out rust-analyzer was
[incorrectly sending byte offsets] instead of char offsets and this
made it seem like all was well and good with offsets in helix during
initial testing.

[1]: #1755 (comment)
[2]: rust-lang/rust-analyzer#12272
GreasySlug pushed a commit to GreasySlug/helix that referenced this pull request Aug 2, 2022
The language server sends a char offset range within the
signature help label text to highlight as the current parameter,
but helix uses byte offset ranges for rendering highlights. This
was brought up in the [review of the original signature help PR][1],
but the ranges were being highlighted correctly, and there were no
out of bound or indexing panics. Turns out rust-analyzer was
[incorrectly sending byte offsets] instead of char offsets and this
made it seem like all was well and good with offsets in helix during
initial testing.

[1]: helix-editor#1755 (comment)
[2]: rust-lang/rust-analyzer#12272
thomasskk pushed a commit to thomasskk/helix that referenced this pull request Sep 9, 2022
* Add lsp signature help

* Do not move signature help popup on multiple triggers

* Highlight current parameter in signature help

* Auto close signature help

* Position signature help above to not block completion

* Update signature help on backspace/insert mode delete

* Add lsp.auto-signature-help config option

* Add serde default annotation for LspConfig

* Show LSP inactive message only if signature help is invoked manually

* Do not assume valid signature help response from LSP

Malformed LSP responses are common, and these should not crash the
editor.

* Check signature help capability before sending request

* Reuse Open enum for PositionBias in popup

* Close signature popup and exit insert mode on escape

* Add config to control signature help docs display

* Use new Margin api in signature help

* Invoke signature help on changing to insert mode
thomasskk pushed a commit to thomasskk/helix that referenced this pull request Sep 9, 2022
The language server sends a char offset range within the
signature help label text to highlight as the current parameter,
but helix uses byte offset ranges for rendering highlights. This
was brought up in the [review of the original signature help PR][1],
but the ranges were being highlighted correctly, and there were no
out of bound or indexing panics. Turns out rust-analyzer was
[incorrectly sending byte offsets] instead of char offsets and this
made it seem like all was well and good with offsets in helix during
initial testing.

[1]: helix-editor#1755 (comment)
[2]: rust-lang/rust-analyzer#12272
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.

Signature help
8 participants