Added Colon as a Commit Character #1825

TanayParikh · 2020-04-23T22:32:36Z

Before

After

Fixes: https://github.com/dotnet/aspnetcore/issues/21066
Fixes: https://github.com/dotnet/aspnetcore/issues/21485

NTaylorMullen · 2020-04-23T22:45:14Z

We may also want to add : as a trigger character as well so you're immediately prompted with a new completion session when you type :

NTaylorMullen · 2020-04-23T22:46:10Z

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/HtmlCSharp/InitializeHandler.cs

@@ -18,7 +18,7 @@ internal class InitializeHandler : IRequestHandler<InitializeParams, InitializeR
            {
                CompletionProvider = new CompletionOptions()
                {
-                    AllCommitCharacters = new[] { " ", ".", ";", ">", "=" }, // This is necessary to workaround a bug where the commit character in CompletionItem is not respected.
+                    AllCommitCharacters = new[] { " ", ".", ";", ">", "=", ":" }, // This is necessary to workaround a bug where the commit character in CompletionItem is not respected.


@ajaybhargavb why does this impact our Razor language server completion endpoint?

That is a great question. When I suggested this, I didn't realize this was in our endpoint. Now that I think about it, it shouldn't have affected this. Very interesting.

If this is actually impacting our Razor language endpoint this is a pretty awful multi-language server bug and will bite us harddd once we start including more features.

Could it be that our HtmlCSharp server is calling the Razor server manually and that is why this is respected? @TanayParikh, to test this, you can just return null in CompletionHandler and see if you still get the same behavior for directive attributes. Ideally, that should still work because CompletionHandler only takes care of HTML and CSharp completion.

Returned null from https://github.com/dotnet/aspnetcore-tooling/blob/master/src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/HtmlCSharp/CompletionHandler.cs#L63

No longer getting completions.

No longer getting completions.

That's a problem. We should still be getting Tag helper completions from RazorCompletionEndpoint. This could explain why adding a : here impacted that.

I'm surprised. Our projection provider shouldn't be invoking anything in our Razor language server: https://github.com/dotnet/aspnetcore-tooling/blob/b0e378c1626ea7c43d29cc4cbe32ccf3f0d50bcd/src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/HtmlCSharp/DefaultLSPProjectionProvider.cs#L70

Note, we have two onkeypresss.

HTML onkeypress

Razor Directive Attribute @onkeypress

For either to complete with a : commit char, we need to add the commit char here. Adding it to the RazorLanguageServer: https://github.com/dotnet/aspnetcore-tooling/blob/master/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/RazorCompletionEndpoint.cs#L155-L160 does not work.

Created an issue to track this: https://github.com/dotnet/aspnetcore/issues/21346

ajaybhargavb · 2020-04-23T22:49:07Z

We may also want to add : as a trigger character as well so you're immediately prompted with a new completion session when you type :

It already is https://github.com/dotnet/aspnetcore-tooling/blob/master/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/RazorCompletionEndpoint.cs#L159. Don't know why it doesn't prompt again.

TanayParikh · 2020-04-23T22:50:29Z

We may also want to add : as a trigger character as well so you're immediately prompted with a new completion session when you type :

It already is https://github.com/dotnet/aspnetcore-tooling/blob/master/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/RazorCompletionEndpoint.cs#L159. Don't know why it doesn't prompt again.

It does, it's just slow. Will repro.

TanayParikh · 2020-04-23T23:05:47Z

We may also want to add : as a trigger character as well so you're immediately prompted with a new completion session when you type :

It already is https://github.com/dotnet/aspnetcore-tooling/blob/master/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/RazorCompletionEndpoint.cs#L159. Don't know why it doesn't prompt again.

It does, it's just slow. Will repro.

@NTaylorMullen / @ajaybhargavb

NTaylorMullen · 2020-04-23T23:14:49Z

Ya I just checked out the branch and see it too:

@ToddGrun is it expected for the HTML language server to return attribute completions after a :? Also, we currently have a TriggerCharacter for : in our HTML Language Server list of trigger characters. Is that needed?

TanayParikh · 2020-04-29T22:46:17Z

Updated to support completion of directive attributes with : completion character.

ajaybhargavb · 2020-04-29T23:24:16Z

...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs

@@ -153,7 +155,8 @@ public override IReadOnlyList<RazorCompletionItem> GetCompletionItems(RazorSynta
                var razorCompletionItem = new RazorCompletionItem(
                    completion.Key,
                    insertText,
-                    RazorCompletionItemKind.DirectiveAttribute);
+                    RazorCompletionItemKind.DirectiveAttribute,
+                    ElementCommitCharacters);


Can you verify if we're still respecting pressing space or = to complete? If in doubt, check what happens in the old editor (with the LSP preview feature off). We'll need to be consistent with that experience.

still respecting pressing space or = to complete?

No we aren't.

I can add = to ElementCommitCharacters, however the completion would be of the form bind=| (not bind="|").

Likewise, I can add [space] to ElementCommitCharacters, but the completion is bind | (not bind="|").

Would that be something on our end or WTE?

If in doubt, check what happens in the old editor (with the LSP preview feature off). We'll need to be consistent with that experience.

For whatever reason, I'm not getting any directive attribute completion in this case.

Spoke offline. We are going to try and be smart about when to add and = as commit characters.

Updated to contextually support and : commit chars. = is always available.

The @attributes=| is missing the quotes (@attributes="|") because we don't have snippet support enabled:
https://github.com/dotnet/aspnetcore-tooling/blob/master/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/DefaultTagHelperCompletionService.cs#L225

...odeAnalysis.Razor.Workspaces.Test/Completion/DirectiveAttributeCompletionItemProviderTest.cs

TanayParikh · 2020-05-01T01:30:35Z

Added support for taghelper attributes. = is always supported as a commit character & Boolean attributes will be completed with .

NTaylorMullen

Mostly had a few questions.

In the future i'd ask that we add additional features outside of the original issue in separate PRs to ensure the PRs can be easily evaluated. I typically will do everything together and then manually split the code changes to ensure things work together 😄

NTaylorMullen · 2020-05-01T16:57:23Z

...rc/Microsoft.AspNetCore.Razor.LanguageServer/Completion/DefaultTagHelperCompletionService.cs

+        {
+            var commitCharacters = new HashSet<string>() { "=" };
+
+            // IsBooleanProperty isn't set


I'm not entirely sure, but it would explain why we were doing this: https://github.com/dotnet/aspnetcore-tooling/blob/master/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/DefaultTagHelperCompletionService.cs#L231

Hmm, I wonder if it's a deserialization issue where when we deserialize the TagHelpers they don't get that property set

Created a investigation issue for this: https://github.com/dotnet/aspnetcore/issues/21486

...rc/Microsoft.AspNetCore.Razor.LanguageServer/Completion/DefaultTagHelperCompletionService.cs

...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs

TanayParikh · 2020-05-04T22:54:50Z

Merge fail 😆. Please ignore the notifications, fixing up.

PR Changes Added Colon as a Commit Character Added support for ` ` and `:` Commit support for taghelper attributes

TanayParikh · 2020-05-05T00:22:15Z

In the future i'd ask that we add additional features outside of the original issue in separate PRs to ensure the PRs can be easily evaluated. I typically will do everything together and then manually split the code changes to ensure things work together 😄
~ @NTaylorMullen

Will keep that in mind next time, thanks! I created a separate issue to document support for contextual commit chars: https://github.com/dotnet/aspnetcore/issues/21485

TanayParikh · 2020-05-05T00:29:59Z

...rc/Microsoft.AspNetCore.Razor.LanguageServer/Completion/DefaultTagHelperCompletionService.cs

+            }
+            else if (boundAttributes.Any(b => b.TypeName.StartsWith("System.Boolean")))
+            {
+                // Have to use string type because IsBooleanProperty isn't set


https://github.com/dotnet/aspnetcore/issues/21486

ajaybhargavb

👏

pranavkm · 2020-05-05T16:33:20Z

...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs

@@ -150,11 +150,14 @@ public override IReadOnlyList<RazorCompletionItem> GetCompletionItems(RazorSynta
                    insertText = insertText.Substring(1);
                }

+                (var attributeDescriptionInfos, var commitCharacters) = completion.Value;


Suggested change

(var attributeDescriptionInfos, var commitCharacters) = completion.Value;

var (attributeDescriptionInfos, commitCharacters) = completion.Value;

works just as well

Thanks, noted 😄. Will fix this up in my next PR.

#pranavpoints

pranavkm · 2020-05-05T16:34:21Z

...rc/Microsoft.AspNetCore.Razor.LanguageServer/Completion/DefaultTagHelperCompletionService.cs

+            {
+                return NoCommitCharacters;
+            }
+            else if (boundAttributes.Any(b => b.TypeName.StartsWith("System.Boolean")))


Is there a reason this is a StartsWith rather than an exact comparison?

To allow for System.Boolean? nullable bool https://github.com/dotnet/aspnetcore/blob/master/src/Mvc/Mvc.TagHelpers/src/FormTagHelper.cs#L96

TanayParikh requested review from rynowak, ajaybhargavb, NTaylorMullen and ryanbrandenburg April 23, 2020 22:32

NTaylorMullen reviewed Apr 23, 2020

View reviewed changes

TanayParikh force-pushed the taparik/add_colon_commit_char branch from c31d128 to bc89c59 Compare April 29, 2020 22:45

TanayParikh requested a review from NTaylorMullen April 29, 2020 23:17

ajaybhargavb reviewed Apr 29, 2020

View reviewed changes

NTaylorMullen approved these changes Apr 29, 2020

View reviewed changes

ryanbrandenburg approved these changes Apr 29, 2020

View reviewed changes

TanayParikh commented Apr 30, 2020

View reviewed changes

...odeAnalysis.Razor.Workspaces.Test/Completion/DirectiveAttributeCompletionItemProviderTest.cs Outdated Show resolved Hide resolved

TanayParikh requested a review from ajaybhargavb May 1, 2020 01:30

NTaylorMullen reviewed May 1, 2020

View reviewed changes

TanayParikh added 4 commits May 4, 2020 14:46

Added Colon as a Commit Character

dc02ad0

Added support for and :

499b827

Tests

af44e98

Removed commit chars for bind-...

fb897d3

TanayParikh requested review from dougbu and pranavkm as code owners May 4, 2020 22:53

TanayParikh removed request for pranavkm and dougbu May 4, 2020 22:53

Commit support for taghelper attributes

47180e0

PR Changes Added Colon as a Commit Character Added support for ` ` and `:` Commit support for taghelper attributes

TanayParikh force-pushed the taparik/add_colon_commit_char branch from 7787b89 to 47180e0 Compare May 5, 2020 00:02

Fixed tests and build

98ca258

TanayParikh commented May 5, 2020

View reviewed changes

ajaybhargavb approved these changes May 5, 2020

View reviewed changes

TanayParikh merged commit cc6e355 into master May 5, 2020

TanayParikh deleted the taparik/add_colon_commit_char branch May 5, 2020 16:55

pranavkm reviewed May 5, 2020

View reviewed changes

TanayParikh mentioned this pull request Jul 14, 2020

Updates for Multiple Language Server CompletionOptions Support #1948

Merged

TanayParikh mentioned this pull request Oct 4, 2021

BoundAttributeDescriptor.IsBooleanProperty Isn't Set #4575

Open

	(var attributeDescriptionInfos, var commitCharacters) = completion.Value;
	var (attributeDescriptionInfos, commitCharacters) = completion.Value;

Added Colon as a Commit Character #1825

Added Colon as a Commit Character #1825

Conversation

TanayParikh commented Apr 23, 2020 • edited Loading

Before

After

NTaylorMullen commented Apr 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajaybhargavb commented Apr 23, 2020

TanayParikh commented Apr 23, 2020

TanayParikh commented Apr 23, 2020 • edited Loading

NTaylorMullen commented Apr 23, 2020

TanayParikh commented Apr 29, 2020

Choose a reason for hiding this comment

TanayParikh Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TanayParikh May 1, 2020 • edited Loading

Choose a reason for hiding this comment

TanayParikh commented May 1, 2020

NTaylorMullen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TanayParikh commented May 4, 2020

TanayParikh commented May 5, 2020 • edited Loading

Choose a reason for hiding this comment

ajaybhargavb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TanayParikh commented Apr 23, 2020 •

edited

Loading

TanayParikh commented Apr 23, 2020 •

edited

Loading

TanayParikh Apr 30, 2020 •

edited

Loading

TanayParikh May 1, 2020 •

edited

Loading

TanayParikh commented May 5, 2020 •

edited

Loading