-
Notifications
You must be signed in to change notification settings - Fork 188
fix: resolve completion BEFORE applying changes #2632
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
base: main
Are you sure you want to change the base?
Conversation
|
The test I can likely fix but there is a potential showstopper - there is noticable flicker due to applying edit after small delay... EDIT: Actually I don't see the flicker in rust-analyzer. I see it in LSP code base where I run LSP-ruff, LSP-pyright and LSP-pylsp (for some reason). The reason for the flicker is the fact that ST removes the prefix when completion doesn't use the |
|
I'm thinking that if we can avoid using |
|
I've added code that manually removes the prefix instead of relying on |
|
Well, that doesn't work because ST doesn't report correct prefix when completing |
|
I have suspicion that ST's logic for inserting plain completions is just to replace the text to the left of the cursor as long as it matches against the completion text. If so then instead of relying on EDIT: No, that doesn't help when completing |
|
I'll switch this to draft since it's not perfect. It is usable when relying on ST's prefix removal but that creates a visible flicker for completions that use it (non-textedit ones). Not using ST's logic for prefix removal would be ideal but then we'd have to reverse-engineer how ST does it. It's probably not that hard but I'm afraid of potential edge cases. I've tried to ask in sublimehq/sublime_text#6766 but I suppose it might be considered a "trade secret" :) |
|
I've updated the logic. The initial comment is updated to reflect the current logic. I'm not sure we want this change because it will create slight delay in applying completions for case nr. 3. And the question is whether correctness is really worth the extra delay. It does fix the rust-analyzer issue though so correctness should in theory win. |
|
Don't like how it works actually... The delay on inserting completion for case nr. 3 is annoying and it also allows one to insert extra text before completion is applied. We could fix it by ignoring completion if document changed but that's not great either. I suppose other editors get away with resolving completion first because they can start resolving when completion is highlighted in the popup. We'd probably need that in ST to make things usable. There is a corresponding ST issue for that. |
Resolve the completion before applying changes to the document. Before we have done it lazily, after applying initial changes to the document, which meant that the server might not resolve the item properly later (it would see the changed document state already).
The current implementation behaves differently depending on the type of completion that is being inserted. It considers those three cases:
additionalTextEditsis already present or session doesn't support resolving) - we apply completion synchronously like before.additionalTextEditsif added during resolving.Case 2 is required due to limitations in ST API - we don't have a way to replicate ST's bespoke prefix removal so can't skip it. See relevant ST issue.
Fixes #2631