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

Fix some Razor colorization issues #6502

Merged

Conversation

allisonchou
Copy link
Contributor

Partially fixes dotnet/razor#9375 (rest of issue in progress)

Examples of scenarios this PR fixes:

Before:
image
After (the parens problem is going to require a more complicated fix that I'm actively working on):
image

Before:
image
After (same comment as above as parens fix):
image

@allisonchou allisonchou requested a review from a team October 6, 2023 20:55
@allisonchou allisonchou requested a review from a team as a code owner October 6, 2023 20:55
@@ -4813,7 +4813,7 @@
"description": "A Razor component element"
},
{
"id": "razorComponentAttribute",
"id": "RazorComponentAttribute",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is weird, but somehow the Razor language server defines RazorComponentAttribute as uppercase but all other tokens as lowercase: https://github.com/dotnet/razor/blob/main/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Models/RazorSemanticTokensLegend.cs#L31C13-L31C13

We probably want to fix this in the future, although I'm not sure of the implications of making it lowercase/what other things may break

Copy link
Member

Choose a reason for hiding this comment

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

Ah you added this as soon as I hit submit review. IMHO we should modify the server side. It would only break people that have added specific theme customization for this token. But I'm not sure that is super common, and it is easily fixable.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to making a server fix here, unless there's some timeline issues with flowing the bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So changing the casing on the server side breaks VS:
image

I think this is because we have to make a change to the VS LSP client as well: https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient?path=/src/product/RemoteLanguage/Impl/Features/SemanticTokens/Tagger/SemanticTokensTaggerBase.cs&version=GBdevelop&line=564&lineEnd=565&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents

I'm going to file a follow up issue for this, but since the fix requires multiple repos I think we could still take this as a temporary solution

@@ -4864,6 +4864,10 @@
"id": "markupAttribute",
"description": "The name of a Markup attribute."
},
{
"id": "markupAttributeQuote",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were missing this semantic token type

"markupComment": [
"comment.block.html"
],
"markupCommentPunctuation": [
"punctuation.definition.comment.html",
"comment.block.html"
],
"markupTagDelimiter": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We defined this token earlier in the file but failed to set a scope for it

@@ -4813,7 +4813,7 @@
"description": "A Razor component element"
},
{
"id": "razorComponentAttribute",
"id": "RazorComponentAttribute",
Copy link
Member

Choose a reason for hiding this comment

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

why is only this one caps?

@@ -5119,7 +5123,7 @@
"razorComponentElement": [
"entity.name.class.element.component"
],
"razorComponentAttribute": [
"RazorComponentAttribute": [
Copy link
Member

Choose a reason for hiding this comment

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

same here - this feels really inconsistent. Is it possible to make these all uniform

Copy link
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

Signing off but would love to see a server fix of "RazorComponentAttribute" to "razorComponentAttribute"

@allisonchou
Copy link
Contributor Author

allisonchou commented Oct 6, 2023

Signing off but would love to see a server fix of "RazorComponentAttribute" to "razorComponentAttribute"

Filed dotnet/razor#9376. The fix is going to involve multi repos and the flow may take a while, so I think we can still take this fix in the meantime

@allisonchou allisonchou merged commit 149948b into dotnet:main Oct 6, 2023
@allisonchou allisonchou deleted the dev/allichou/TryFixRazorColors4 branch October 6, 2023 22:02
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.

Textmate colorization issues in VS Code
4 participants