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

Use different comletion trigger character set for VSCode #11446

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

alexgav
Copy link
Contributor

@alexgav alexgav commented Feb 4, 2025

Summary of the changes

VS Code HTML language service is a lot more aggressive with showing low quality matches than VS HTML language service. We cannot use the same completion trigger characters in VS Code as in VS because it causes unwanted completion list with bad completion non-matches to pop up while user types in plain text.

Fixes:
dotnet/vscode-csharp#7678
dotnet/vscode-csharp#7871

@alexgav alexgav requested a review from a team as a code owner February 4, 2025 22:05

[Export(typeof(CompletionTriggerAndCommitCharacters))]
[method: ImportingConstructor]
class CohostCompletionTriggerAndCommitCharacters(LanguageServerFeatureOptions languageServerFeatureOptions) : CompletionTriggerAndCommitCharacters(languageServerFeatureOptions);
Copy link
Member

Choose a reason for hiding this comment

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

internal sealed

/// <summary>
/// Indicates that VSCode-compatible completion trigger character set should be used
/// </summary>
public abstract bool VsCodeCompletionTriggerCharacters { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Consider adding "Use" to the front of this, to match the existing flags (and make it more verb-y)

…to delegating HTML client if trigger character is not HTML.

Doing so causes issues in VS Code client because it will just return bad matches in that case, so we shouldn't even ask. I don't see any side-effects from not sending it to VS client either, seems logical not to send non-HTML trigger character request to HTML language server.
public required bool SupportsSoftSelectionInCompletion { get; set; }

[JsonPropertyName("vsCodeCompletionTriggerCharacters")]
Copy link
Member

Choose a reason for hiding this comment

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

Since you have to make at least one more commit for the failing test anyway, I'm going to be annoying: Nit, should be "useVSCodeCompletionTriggerCharacters" 😛

@@ -103,7 +103,7 @@ await _provider.GetCompletionListAsync(
}

[Fact]
public async Task HtmlDelegation_UnsupportedTriggerCharacter_TranslatesToInvoked()
public async Task HtmlDelegation_UnsupportedTriggerCharacter_ReturnsNull()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's interesting we had a test for this specific behavior... I don't know if this change would break anything, though I can't see how sending explicit completion invokes to HTML LSP for something that's not an HTML trigger character is a good thing. VS HTML at least behaves decently by returning nothing in inappropriate situation, but VS Code HTML is very aggressive with low quality matches ("better return anything than nothing"?) and that makes typing very annoying. In VS, it seems like a wasted message sent to HTML LSP.

TriggerCharacter = "f",
TriggerKind = RoslynCompletionTriggerKind.TriggerCharacter
TriggerCharacter = null,
TriggerKind = RoslynCompletionTriggerKind.Invoked
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we (probably me) were specifying bad input data. I debugged and confirmed VS actually sends the newly specified data in this change

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants