Fix completion on part of existing string #1941
Conversation
|
The completion does not handle correctly the case when completion keeps a bit of the original text. It works with This will skip this condition and instead use the one the is more complicated, that allows overwriting the existing code, but that path does not support this case correctly. I was able to naively fix it by just taking the start of the typedText and prepending it to the proposed change. But this seems like oversimplifying the problem. |
|
can you add some tests that reproduce this issue? |
|
|
||
| (insertText, insertTextFormat) = getAdjustedInsertTextWithPosition(change, position, newOffset: 0); | ||
|
|
||
| insertText = prefix + insertText; |
There was a problem hiding this comment.
what about a situation where insertText should correct the letter casing of the typed text?
There was a problem hiding this comment.
Would that be triggered in this case? I don't think it will because that is covered by the if above this one, then then typedSpan is replaced as a whole. In this case the part of the span is already correct so the suggestion only contains the part that needs to be added. At least that is what I saw when trying one<tab> vs One<tab>. In the first case one gets OnEnabled() ... suggestion, but One gets Enabled() ... suggestion. The span of the first suggestion is identical to the typedSpan, while the span of the second suggestion starts 2 characters after th typedSpan, because you already have On in the text.
Yeah, sure. I could not find where this is tested originally, because I was looking for CompletionService, but now I see there is CompletionFacts. |
|
@333fred Maybe you have some thoughts on how we could handle this. |
|
I think the correct way to handle this is to use a TextEdit for the change, rather than use insert text. When I implemented this originally, I missed the part of the LSP spec that stated |
Because I misread the LSP spec when imlementing completion, I didn't realize that InsertTextFormat applied to both InsertText, and to the NewText property of the main TextEdit of a completion item. This latter version is much more robust to casing issues than using InsertText directly, and ensures that matching of completion items in the completion list works better as well. Fixes dotnet/vscode-csharp#4063.
This is the vscode side of OmniSharp/omnisharp-roslyn#1941. This is written in such a fashion as to be mergeable now, as it will fall back to InsertText if TextEdit is not present in the completion response. After some appropriate period of time, we can remove the InsertText component.
…ment with examples on vscode behavior for replacing ranges.
Use text edit for main insert text
| // completion and typing `EQ` will bring up the Equals override, but then dismissing and | ||
| // reinvoking completion will have a range that just replaces the Q. Vscode will then consider | ||
| // that capital Q to be the start of the filter text, and filter out the Equals overload | ||
| // leaving the user with no completion and no explanation |
There was a problem hiding this comment.
Technically, vscode does have an API to deal with this inconsistency. But it's not in the lsp spec, only in the native vscode completion handler api, so we can't use it here.
|
@333fred Thanks, that's perfect. |
This is the vscode side of OmniSharp/omnisharp-roslyn#1941. This is written in such a fashion as to be mergeable now, as it will fall back to InsertText if TextEdit is not present in the completion response. After some appropriate period of time, we can remove the InsertText component.
When completion re-uses part of existing string the proposed change should re-use the part of the string in the original span and commit the change.
Fix dotnet/vscode-csharp#4063