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

Fix LSP ComplitionTriggerKind value for TriggerKind::Auto #9660

Conversation

nkitsaini
Copy link
Contributor

@nkitsaini nkitsaini commented Feb 18, 2024

@the-mikedavis the-mikedavis added A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 18, 2024
the-mikedavis
the-mikedavis previously approved these changes Feb 18, 2024
@pascalkuthe
Copy link
Member

#9656 (comment)

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.

ok the vscode code is pretty opaque but you are right that indeed we should be sending invoked for identifier triggered auto completions. I find that a bit odd but oh well. The implementation is not correct tough. The completion debouncer runs in a background task/thread. Even if the completion was originally caused by autocompletion we could now be at a trigger char (or not anymore).

So we should not be looking at trigger.kind beyond checking if it is set to manual. You should determine whether to use INVOKED or TRIGGER_CHARACTER by checking if trigger_char.is_some()

@nkitsaini
Copy link
Contributor Author

ok the vscode code is pretty opaque

Yeah, I tried to go through it yesterday night but wasn't very confident. But I was to able to confirm the behavior by enabling debug logs for both svelte and rust-analyzer plugin in vscode and looking at LSP message logs.

So we should not be looking at trigger.kind beyond checking if it is set to manual. You should determine whether to use INVOKED or TRIGGER_CHARACTER by checking if trigger_char.is_some()

Makes sense, updated the PR.

@pascalkuthe pascalkuthe merged commit 787cc36 into helix-editor:master Feb 19, 2024
6 checks passed
uek-1 pushed a commit to uek-1/helix that referenced this pull request Feb 24, 2024
shortc pushed a commit to shortc/helix that referenced this pull request Mar 31, 2024
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 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.

TriggerKind::Auto violates lsp spec
3 participants