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

Add more specific scopes to bool, num, double, int, Object, String, dynamic in textmate grammar #4974

Open
2 tasks
Number-3434 opened this issue Feb 3, 2024 · 8 comments
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Milestone

Comments

@Number-3434
Copy link

Number-3434 commented Feb 3, 2024

Treat bool, num, double, and int as keywords.

Note

It is not necessary to change the default colouring for these types.
These could be configured by the user, or by external theme packs.

Before:

Image of Syntax Highlighting Dark
Image of Syntax Highlighting Light

After:

Image of Syntax Highlighting Dark
Image of Syntax Highlighting Light

(Ignore the difference in bracket colours).

This could also apply to String, Map, List, Set, Object, and dynamic.

  • TextMate Grammars
  • Semantic Highlighting
@Number-3434 Number-3434 changed the title Syntax Highlight bool, num, double, int as Keywords Syntax Highlight bool, num, double, int as Keywords Feb 3, 2024
@DanTup
Copy link
Member

DanTup commented Feb 5, 2024

Can you provide some motivation for this? Those things are types so it seems like they should be coloured as such.

Unfortunately we can't change colours conditionally in a textmate grammar, so this isn't something we could control as an option there (we could perhaps have a Semantic Tokens modifier for built-in types like these that would allow you to customise them, but based on your other issues I think you're not using Semantic Tokens and that might not help you).

To change the textmate grammar, I think it would need to be clear that the majority of users would prefer this change.

@DanTup DanTup added the awaiting info Requires more information from the customer to progress label Feb 5, 2024
@Number-3434
Copy link
Author

Number-3434 commented Feb 5, 2024

Can you provide some motivation for this? Those things are types so it seems like they should be coloured as such.

Clarification

I don't need there to be an option to toggle this, I can manually do it in the settings if TextMate grammars are provided.

Motivation

Dart is my second language after C#, and I got used to seeing keywords such as int, bool, etc. highlighted as keywords.

(In fact, the images above are actually from Dart and C#).

In my opinion, since int, bool, etc. in Dart are built-in types (i.e. shipped with the SDK and don't require an import statement), they could technically be interpreted as keywords.

In C#, built-in keyworded types are given the scope keyword.type.<type>.cs (e.g. int would be given keyword.type.int.cs).

Therefore, in Dart, these built-in types could be given scopes such as:

  • keyword.type.int.dart
  • keyword.type.double.dart
  • keyword.type.string.dart
  • keyword.type.num.dart
  • keyword.type.object.dart
  • keyword.type.dynamic.dart
  • keyword.type.set.dart
  • keyword.type.list.dart
  • keyword.type.map.dart
  • keyword.type.symbol.dart
  • keyword.type.bool.dart

@DanTup
Copy link
Member

DanTup commented Feb 5, 2024

One option is that we change support.class.dart to support.class.primitive.dart here (and add String to the list):

{
"match": "(?<!\\$)\\b(bool|num|int|double|dynamic)\\b(?!\\$)",
"name": "support.class.dart"
},

This should keep the same colouring by default, but I think would allow you to customise the colours yourself in the VS Code settings.

I think we should only do it for the items in that existing list though. Types like Set/List/Map/etc. are not really special - they are just classes that live inside the SDK (like hundreds of other classes). The textmate grammar cannot tell the difference between List from the SDK and class List {} that is user-defined.

@Number-3434
Copy link
Author

Number-3434 commented Feb 5, 2024

One option is that we change support.class.dart to support.class.primitive.dart (and add String to the list)

That sounds perfect!

What about Object though? Would that also be included?

@DanTup
Copy link
Member

DanTup commented Feb 5, 2024

Yeah, I think it would be reasonable to include that too.

@DanTup DanTup added this to the v3.84.0 milestone Feb 5, 2024
@DanTup DanTup added in editor Relates to code editing or language features and removed awaiting info Requires more information from the customer to progress labels Feb 5, 2024
@DanTup DanTup changed the title Syntax Highlight bool, num, double, int as Keywords Add more specific scopes to bool, num, double, int, Object, String, dynamic in textmate grammar Feb 5, 2024
@Number-3434
Copy link
Author

Number-3434 commented Feb 5, 2024

Sorry, I didn't fully read your comment before.

To clarify, I use semantic tokens.

I like them as they provide much richer information, but it is sometimes annoying that they override textmate tokens.

Sorry, I tried the fix and I actually do need semantic tokens for this, as they are overriding the TextMate tokens.


With semantic highlighting:

Screenshot 2024-02-05 at 16 24 43

Without semantic highlighting:

Screenshot 2024-02-05 at 16 22 39

@DanTup
Copy link
Member

DanTup commented Feb 5, 2024

I use semantic tokens.

Ah sorry, I misunderstood from comments in #4963 and thought you had semantic tokens disabled.

Semantic Tokens will completely replace the textmate grammar (this is VS Code behaviour), although the grammar will be used immediately when you open a file and then replaced when the tokens arrive.

In that case, supporting this would require server changes to add additional modifiers to those types.

@DanTup DanTup modified the milestones: v3.84.0, On Deck Feb 5, 2024
@DanTup DanTup added the in lsp/analysis server Something to be fixed in the Dart analysis server label Feb 5, 2024
@Number-3434
Copy link
Author

Number-3434 commented Feb 5, 2024

Ah sorry, I misunderstood from comments in #4963 and thought you had semantic tokens disabled.

Sorry, in that issue I meant something like "this syntax highlighting does not need semantic tokens".

Would it be wise to implement both? (I.e. in this v3.84.0, release TextMate grammars, and later release semantic highlighting)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Projects
None yet
Development

No branches or pull requests

2 participants