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

Adjusts csharp semantic token scopes to match 1.26 outputs #6094

Conversation

fuzzlebuck
Copy link
Contributor

@fuzzlebuck fuzzlebuck commented Aug 10, 2023

This PR adjusts the semantic token scopes to match those in earlier versions, 1.26 etc.

Directly relates to Issue #5731

I've compared the foreground outputs using the Inspect Editor Tokens in version 1.26.0 and in this version an confirmed the primary foreground now matches for all of the updated scopes in this PR.

For example the two listed in the original issue:

Namespace in 1.26.0:

image

Namespace in latest:

image

Parameter in 1.26.0:

image

Parameter in latest:

image

@fuzzlebuck

This comment was marked as resolved.

@fuzzlebuck fuzzlebuck changed the title Adjusts csharp scopes to match 1.26 outputs Adjusts csharp semantic token scopes to match 1.26 outputs Aug 10, 2023
@fuzzlebuck
Copy link
Contributor Author

fuzzlebuck commented Aug 10, 2023

I believe the tests are failing for an unrelated reason and I'm unable to restart the pipeline due to lack of access if someone could retry them for me?

@fuzzlebuck fuzzlebuck marked this pull request as ready for review August 10, 2023 10:32
@fuzzlebuck fuzzlebuck requested a review from a team as a code owner August 10, 2023 10:32
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution - one suggestion to try out.

package.json Outdated
@@ -4737,40 +4737,40 @@
"language": "csharp",
"scopes": {
"namespace": [
"entity.name.type.namespace.cs"
"entity.name.namespace"
Copy link
Member

@dibarbet dibarbet Aug 10, 2023

Choose a reason for hiding this comment

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

So looking at the 1.x version, it doesn't specify any fallback textmate scopes for semantic tokens. Which I believe means that it uses the default fallbacks as defined here - https://code.visualstudio.com/api/language-extensions/semantic-highlight-guide#predefined-textmate-scope-mappings .

I think removing the fallbacks for the core LSP semantic token names (aka anything in that table) would have the same affect (as suggested by Joey here - #5731 (comment)). Would you be able to try that? If it works I think that is the direction we should take.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thank you, I've made the change and it looks good!

It's also resolved a 1.x issue I had noticed where local variables had the .cs namespace so looked incorrect in some themes.

@dibarbet
Copy link
Member

So trying it out locally - it definitely doesn't look right (mainly keywords I think?) on Visual Studio 2019 Dark
image

What the colors should look like:
image

It looks like its getting the keyword.control scope which is purple in the theme. But these aren't control keywords. I do see that there isn't a fallback scope listed for keyword in the table - we might need to keep that one?

cc @JoeRobich in case he knows of another idea

"struct": [
"entity.name.type.struct.cs"
],
"typeParameter": [
Copy link
Member

Choose a reason for hiding this comment

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

`typeParameter doesn't list a fallback in the table, we probably need to keep this one

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 one is interesting, in 1.x it maps as entity.name.type.parameter
image

Putting it back in the scopes list for 2.x it comes out as expected entity.name.type.type-parameter.cs, but should it be the more generic one or the .cs specific one?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see in the VS theme's it's using entity.name.type.type-parameter

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should set it to "entity.name.type.type-parameter" in the scopes

Copy link
Member

Choose a reason for hiding this comment

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

This one is interesting, in 1.x it maps as entity.name.type.parameter

Interesting - I don't know where that is coming from.

I think we should set it to "entity.name.type.type-parameter" in the scopes

Yup, I'm good with that. If though entity.name.type.parameter looks better on other themes, and it works with the VS theme, I'm also OK with setting to that. But entity.name.type.type-parameter seems to be what O# used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked this one on a couple of themes and it looks ok to me.

"macro": [
"keyword.preprocessor.cs"
],
"keyword": [
Copy link
Member

Choose a reason for hiding this comment

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

same with keyword here.

Copy link
Member

Choose a reason for hiding this comment

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

If keyword.cs doesn't work on the other themes - try just 'keyword'. It looks like it should have the correct color in the theme json
https://github.com/dotnet/vscode-csharp/blob/main/themes/vs2019_dark.json#L311

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thank you, I will update and will double check them one by one and compare the outputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this back in, looks ok now, please see my latest comment with comparisons.

@fuzzlebuck
Copy link
Contributor Author

Updated with changes and compared each token that was removed against v 1.26.0:

1.26.0:
image

Latest update:
image

A couple of small discrepancies to mention:

  • The preprocessor has a different color now due to it being mapped to entity.name.function.preprocessor whereas in 1.26.0 it was mapped to keyword.preprocessor.cs, let me know if you think I should change it to the .cs one.

  • The type parameter for generics is now mapping to entity.name.type.type-parameter whereas in 1.26.0 it was mapped to entity.name.type.parameter although in the themes I looked at it made no difference aesthetically.

  • I couldn't work out how to match a plain entity.name.type in code.

@dibarbet
Copy link
Member

The preprocessor has a different color now due to it being mapped to entity.name.function.preprocessor whereas in 1.26.0 it was mapped to keyword.preprocessor.cs, let me know if you think I should change it to the .cs one.

I'm OK with this change, it looks to be the same in the new version with or without this change. If we get a ton more feedback we can always switch it later.

The type parameter for generics is now mapping to entity.name.type.type-parameter whereas in 1.26.0 it was mapped to entity.name.type.parameter although in the themes I looked at it made no difference aesthetically.

Yup works for me.

Thanks for the contribution!

@fuzzlebuck
Copy link
Contributor Author

Happy to help, and thank you!

@dibarbet dibarbet merged commit cbf2ba9 into dotnet:main Aug 11, 2023
4 checks passed
@JoeRobich
Copy link
Member

It looks like its getting the keyword.control scope which is purple in the theme

Blame VS Code for that. No idea why they only included one core keyword token type and choose the keyword.control scope to represent it. For O#, we originally didn't utilize any of the built in token types and had a plainKeyword token type. We have since taken changes to use the LSP core token types when possible to work better with other LSP enabled editors.

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.

None yet

3 participants