Adjust SemanticTokensService to LSP specification #2753
Conversation
|
fixes #2750 |
|
One thing not working yet is the "multilineTokenSupport". VS Code for example does not support multiline tokens but this implementation uses multiline tokens by default. |
...eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/semantictokens/SemanticTokensService.java
Show resolved
Hide resolved
| data.add(positionTokenType); // token type | ||
| data.add(positionTokenModifiers); // token modifiers | ||
| } | ||
| int deltaOffset = lightweightPosition.getOffset() - lastOffset; |
There was a problem hiding this comment.
After this change, I wonder how you'd indicate a position with an ID that is specifically not compatible to the LSP token types? E.g. another UI might support more token types.
I think that's why 0 was used as a return type for an ID that's semantic but not supported by the LSP protocol.
There was a problem hiding this comment.
the problem is, that 0 is a valid tokenTypeIndex in LSP. I now changed it to return -1 if no valid TokenType is found. So the TokenType with index 0 can be used and still we can detect unsupported types.
| // delta start relative to previous token if on the same line or to 0 | ||
| data.add(deltaLine == 0 ? deltaOffset : position.getCharacter()); | ||
| data.add(lightweightPosition.getLength()); // length | ||
| data.add(positionTokenType); // token type |
There was a problem hiding this comment.
I think here is where we missed a positionTokenType - 1 to compensate for the chosen communication channel 0 == not contained in list
There was a problem hiding this comment.
to use 0 as an indicator for not supported tokenTypes we would need to add a placeholder tokenType in the list of supported types. Otherwise we would not be able to use the first token Type in the list of supported types. That's why I changed the indicator to -1
.../org/eclipse/xtext/ide/tests/testlanguage/syntaxcoloring/SemanticHighlightingCalculator.java
Outdated
Show resolved
Hide resolved
|
@jnt0r @szarnekow what is the state of this pr? |
|
I currently don't have much time. Will have to take a look at it in a week. |
|
There are still a few adjustments needed. |
|
@jnt0r can you please rebase and sequash commits into 1 |
jnt0r
force-pushed
the
semantic-tokens-service
branch
from
0b6cd73 to
d640553
Compare
|
@cdietrich I don't quite understand why the build failed. Looks like some build setup and no code issue... |
|
As I said you need to rebase |
jnt0r
force-pushed
the
semantic-tokens-service
branch
from
d640553 to
c9ea8af
Compare
|
Okay. Thought I already did it. Seems to work now. |
|
Thank you all for working on this problem - my research project will definitely benefit from it. I hope this can be merged soon. |
|
I'll approve but probably hold off the merge until after the release since we are in the last week essentially. It will be discussed next week wether we want to merge this nevertheless or immediately after the build. |
|
What's the state of this? |
TokenType is 0 based and represents the index in the list of supported token types. 0 is valid for TokenType and TokenModifiers so we do not need to check for 0 here. Signed-off-by: Jonathan Pollert <[email protected]>
jnt0r
force-pushed
the
semantic-tokens-service
branch
from
c9ea8af to
bbbf392
Compare
|
@szarnekow can this be merged? |
|
ping @cdietrich @szarnekow |
|
Thanks again. I merged the changes @jnt0r |
TokenType is 0 based and represents the index in the list of supported token types.
0 is valid for TokenType and TokenModifiers, so we do not need to check for 0 here.
I added all supported TokenTypes and TokenModifiers from the specification to the List of possible types and modifiers.