Add basic semantic highlighting#84
Conversation
|
I have identified a few issues that I am looking into. It seems to be possible to correctly identify variables, but class and function references, as well as methods and properties, are hard to differentiate, where I’d say that the latter is more annoying. Since the current parser is in P-code, I can’t make any adjustments there on my own. If the current parser would be updated, the information could easily be used in the highlighter. Otherwise, the best option could be to update the textmate scopes so the highlighting is at least not actively misleading. |
|
Hi @Gustaf-C , thanks for working on this! Overall, this approach looks good to me. I agree that class, function, method, and property references are going to be difficult to differentiate at this time. That being said, I think adding simple variable semantic highlighting is a good start which adds definite value! I will take a closer look at the code itself when this is transferred out of draft state. The parsing logic currently uses some undocumented MATLAB APIs which are very difficult to work with, so we are trying to move away from their usage. We are currently waiting for a new API to become available in MATLAB which should provide us easier-to-use and more detailed info about the file contents. At that point, it should be possible to add more semantic token types. |
|
I am reasonably happy with the changes now. A small example: 
The previous behaviour can be seen below: I would however suggest that we remove the highlighting for methods/properties in textmate since the current behaviour is more confusing than helping. |
dklilley
left a comment
dklilley
left a comment
There was a problem hiding this comment.
Thanks for working on this! Overall this is looking good - I just let a few comments with suggestions.
It would also be great to add some unit tests for SemanticTokensProvider. You should be able to use either FormatSupportProvider.test.ts or HighlightSymbolProvider.test.ts as starting points.
| // Ensures that semantic tokens are refreshed after indexing, | ||
| // so highlighting is updated after opening the editor. | ||
| documentIndexer.setOnIndexed(() => { | ||
| scheduleSemanticRefresh() | ||
| }) | ||
|
|
||
| let refreshTimer: NodeJS.Timeout | undefined | ||
|
|
||
| function scheduleSemanticRefresh (): void { | ||
| if (refreshTimer != null) clearTimeout(refreshTimer) | ||
|
|
||
| // Delay sending the refresh notification to batch multiple indexing updates together | ||
| refreshTimer = setTimeout(() => { | ||
| void connection.sendRequest('workspace/semanticTokens/refresh') | ||
| }, 150) | ||
| } |
There was a problem hiding this comment.
Can this logic be moved within the SemanticTokenProvider? I think this can be set up in the constructor there, which should help encapsulate this logic.
There was a problem hiding this comment.
I have moved the logic into a separate function, which hides the logic from the server but keeps the provider focused on computing the tokens. I feel like that's a decent enough solution that doesn't mix responsibilities.
|
Thanks for your comments, I have made some tweaks accordingly. I'll add some basic unit tests. |
|
Thanks for working on this! |
2 participants
Fixes mathworks/MATLAB-extension-for-vscode#45
This adds basic support for semantic highlighting. Currently only variables is implemented, but I plan to do some more work before it is ready to be merged. I am opening a draft PR to allow for some initial feedback.
Results can be seen below where indexed vectors are now correctly identified as variables:
Changes
Note
Depends on plumbing in mathworks/MATLAB-extension-for-vscode#321