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: cycle through function signatures/overloads #9974

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

karthago1
Copy link
Contributor

@karthago1 karthago1 commented Mar 23, 2024

implement handle_event to cycle through the function signatures.

by pressin alt + p/n the function signature will be changed. (similar behavior as the completion popup)

fixes #5403

edit:
key bindings are changed to alt + p/n.

asciicast

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Mar 23, 2024
@the-mikedavis the-mikedavis added the S-waiting-on-review Status: Awaiting review from a maintainer. label Mar 24, 2024
@karthago1 karthago1 force-pushed the cycle-function-signature branch 2 times, most recently from d806103 to 831d9ef Compare March 24, 2024 20:55
@karthago1 karthago1 force-pushed the cycle-function-signature branch 4 times, most recently from b2065ac to 39617fc Compare April 2, 2024 21:55
@pascalkuthe
Copy link
Member

This seems like it would be better handled with a custom compoment instead of using an event handler.

The lsp always sends all signatures so you don't need to rerequest the signature help when you want to change signatures. Instead a custom component should just store all signatures and cycle trough them when keys are pressed.

The handlers are only meant to facilitate asynchronous tasks. When so.ethkng can be done synchronously like here that is preferable (both faster and less complex)

@karthago1 karthago1 force-pushed the cycle-function-signature branch 3 times, most recently from 1976d71 to f0e7558 Compare April 5, 2024 21:02
@karthago1
Copy link
Contributor Author

Thank you for the review. Now the signatures are stored inside the SignatureHelp ui. It is faster than the previous implementation as you suggested.

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

changes look mostly good to me now but I am concerned about keybindings. Also want to test this out a bit (altough not sure how much use that will be with rust which thankfully doens't have overloading)

helix-term/src/ui/lsp.rs Outdated Show resolved Hide resolved
@pascalkuthe pascalkuthe added the S-needs-testing Status: Needs to be tested out in order to discover potential bugs. label Apr 5, 2024
@karthago1
Copy link
Contributor Author

This PR should have no side effects if the received signatures are less or equal 1. If more signatures are received, then the signature index is printed and A-n / A-p will be processed/evaluated.

(already Tested with C++ and C#)

@thomasaarholt
Copy link
Contributor

thomasaarholt commented Apr 6, 2024

I just tested it, and it works as advertised. Here's a screen recording of me using it in Python, where I added a really long docstring to ensure that it looked right with that. Gist here of the example.

The PR functionality works well. I actually reveal an unrelated bug in that video with the long output - pressing Ctrl D to scroll doesn't scroll the whole way in my small-ish window. The bug is present on the master branch too. If I use a bigger window, I can see the whole output.

@karthago1
Copy link
Contributor Author

The PR functionality works well. I actually reveal an unrelated bug in that video with the long output - pressing Ctrl D to scroll doesn't scroll the whole way in my small-ish window. The bug is present on the master branch too. If I use a bigger window, I can see the whole output.

Thank you for the report. I create this PR #10181 to fix this issue

pascalkuthe
pascalkuthe previously approved these changes Apr 7, 2024
@pascalkuthe pascalkuthe removed the S-needs-testing Status: Needs to be tested out in order to discover potential bugs. label Apr 9, 2024
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

It'll be nice to have this, thanks for working on it! I have some minor comments about style and cosmetics

helix-term/src/ui/lsp.rs Outdated Show resolved Hide resolved
helix-term/src/ui/lsp.rs Outdated Show resolved Hide resolved
@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-waiting-on-review Status: Awaiting review from a maintainer. labels Apr 16, 2024
implement handle_event to cycle through the function signatures.

To change the signature press alt+p/n .

Signed-off-by: Ben Fekih, Hichem <[email protected]>
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2024
@pascalkuthe pascalkuthe merged commit 69e08d9 into helix-editor:master Apr 16, 2024
6 checks passed
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 20, 2024
implement handle_event to cycle through the function signatures.

To change the signature press alt+p/n .

Signed-off-by: Ben Fekih, Hichem <[email protected]>
Comment on lines +241 to +248
let mut active_signature = old_popup
.as_ref()
.map(|popup| popup.contents().active_signature())
.unwrap_or_else(|| response.active_signature.unwrap_or_default() as usize);

if active_signature >= signatures.len() {
active_signature = signatures.len() - 1;
}
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 that we should prefer the active_signature we get from the language server here instead:

let active_signature = response
    .active_signature
    .map(|n| n as usize)
    .or_else(|| {
        old_popup
            .as_ref()
            .map(|popup| popup.contents().active_signature())
    })
    .unwrap_or_default()
    .min(signatures.len() - 1);

Some language servers update the active_signature they send when you enter enough parameters. For example with Erlang you might be typing maps:get( which is a multiarity function. maps:get(a, b should show the signature of maps:get/2 (takes 2 arguments) but for maps:get(a, b, the language server will show maps:get/3.

We could make this more complicated though and separately track which signature the language server sends and which you navigate to with A-n/A-p, preferring the signature you navigate to until the server changes which signature it considers active.

Copy link
Contributor Author

@karthago1 karthago1 May 1, 2024

Choose a reason for hiding this comment

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

I have played with your code. My c++ LSP somehow doesn't update the active signature at all. it is always 0.
so IMO it is very annoying to switch to the right signature manually, then it switches always back to the first one.

but in the same time using the C# LSP your code brings some improvements, since the suggested LSP signature is in most cases useful.

I created #10655, which implements the bit more complicated solution you have suggested

rcorre added a commit to rcorre/helix that referenced this pull request May 1, 2024
PR helix-editor#9974 added alt-p/alt-n keybindings to scroll through signatures.
This wasn't very discoverable, as it's not in the docs or the command palette.

This also removes a broken link for "comment mode" in the table of contents.
pascalkuthe pushed a commit that referenced this pull request May 2, 2024
* Add completion/signature bindings to keymap.md

PR #9974 added alt-p/alt-n keybindings to scroll through signatures.
This wasn't very discoverable, as it's not in the docs or the command palette.

This also removes a broken link for "comment mode" in the table of contents.

* Update keymap.md
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
implement handle_event to cycle through the function signatures.

To change the signature press alt+p/n .

Signed-off-by: Ben Fekih, Hichem <[email protected]>
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
* Add completion/signature bindings to keymap.md

PR helix-editor#9974 added alt-p/alt-n keybindings to scroll through signatures.
This wasn't very discoverable, as it's not in the docs or the command palette.

This also removes a broken link for "comment mode" in the table of contents.

* Update keymap.md
Chirikumbrah pushed a commit to Chirikumbrah/helix that referenced this pull request Jun 15, 2024
implement handle_event to cycle through the function signatures.

To change the signature press alt+p/n .

Signed-off-by: Ben Fekih, Hichem <[email protected]>
Chirikumbrah pushed a commit to Chirikumbrah/helix that referenced this pull request Jun 15, 2024
* Add completion/signature bindings to keymap.md

PR helix-editor#9974 added alt-p/alt-n keybindings to scroll through signatures.
This wasn't very discoverable, as it's not in the docs or the command palette.

This also removes a broken link for "comment mode" in the table of contents.

* Update keymap.md
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
implement handle_event to cycle through the function signatures.

To change the signature press alt+p/n .

Signed-off-by: Ben Fekih, Hichem <[email protected]>
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
* Add completion/signature bindings to keymap.md

PR helix-editor#9974 added alt-p/alt-n keybindings to scroll through signatures.
This wasn't very discoverable, as it's not in the docs or the command palette.

This also removes a broken link for "comment mode" in the table of contents.

* Update keymap.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to cycle through signature help signatures
5 participants