-
Notifications
You must be signed in to change notification settings - Fork 206
Partial fix for https://github.com/dotnet/vscode-csharp/issues/7678 #11681
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?
Changes from 1 commit
a3adeec
c14fac9
5246955
2213da9
cb48e2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,7 +69,13 @@ public CompletionTriggerAndCommitCharacters(LanguageServerFeatureOptions languag | |
allTriggerCharacters.UnionWith(delegationTriggerCharacters); | ||
|
||
var commitCharacters = new HashSet<char>(); | ||
commitCharacters.UnionWith(s_commitCharacters); | ||
// We shouldn't specify commit characters for VSCode. | ||
// It doesn't appear to need them and they interfere with normal item commit. | ||
// E.g. see https://github.com/dotnet/vscode-csharp/issues/7678 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, this is kind of a point-in-time comment that might be confusing when somebody else is reading it in a year. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would make it clearer? Or do you suggest dropping the comment completely? |
||
if (!languageServerFeatureOptions.UseVsCodeCompletionTriggerCharacters) | ||
{ | ||
commitCharacters.UnionWith(s_commitCharacters); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CommitElementsWithSpace is already broken without this change. We are currently always committing elements with space in VS Code even though that setting is "false". That happens because specifying commit characters via capabilities ends up bypassing the code we use to remove Space from the specified commit characters. That code works only when Space is specified by the specific instance of the completion list or specific completion items in that list. We should discuss if that option makes sense in VS Code given behavior differences in how aggressively completion list is shown with low quality matches in VS Code. I would suggest we don't combine that work with this fix because once again, that options is broken already, without this PR. I am glad you brought it up though, thank you. |
||
} | ||
|
||
_csharpTriggerCharacters = csharpTriggerCharacters; | ||
_htmlTriggerCharacters = htmlTriggerCharacters; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
languageServerFEatureOptions.UseVsCodeCompletionTriggerCharacters
isn't set, there isn't reason to createcommitCharacters
, since it'll just end up settingAllCommitCharacters
to[]
below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, pushed a commit to fix this. Thanks!