-
Notifications
You must be signed in to change notification settings - Fork 143
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 support for semanticTokens #446
Add support for semanticTokens #446
Conversation
Given this, should we remove or deprecate semantic highlights as semantic tokens replace it? |
Can one of the admins verify this patch? |
Looks like you beat me to it! Excited to see this. 👍 |
@spoenemann Could you take a look? I don't know who to ping here 😅 |
I have little time this week, maybe next one. If anyone else would like to review, go for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this huge works ! It's really a good job !
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/SemanticTokenModfiers.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
Thanks for the review, I'll take a look tomorrow! |
Signed-off-by: Eric Dallo <[email protected]>
Signed-off-by: Eric Dallo <[email protected]>
Done @Dufgui |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Two remarks below.
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/SemanticTokenModifiers.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
Could you review it again @spoenemann ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there.
this.range = range | ||
} | ||
|
||
new(@NonNull SemanticTokensLegend legend, SemanticTokensServerFull full, Boolean range, List<DocumentFilter> documentSelector) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specification says it's either a SemanticTokensOptions
(with legend
) or a SemanticTokensRegistrationOptions
(with documentSelector
), but not both.
Please also add a no-arguments constructor. In some contexts it's easier to construct the protocol classes with setters, e.g. with Xtend:
options = new SemanticTokensWithRegistrationOptions => [
legend = ...
range = ...
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it, what do you suggest?
I thought that this comment suggested merging both classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SemanticTokensRegistrationOptions
extends SemanticTokensOptions
in the spec, so legend
would be in both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SemanticTokensRegistrationOptions
extends TextDocumentRegistrationOptions
, which has a documentSelector
, but no legend
.
I thought that this comment suggested merging both classes
Yes, but merging the classes is just a workaround because Java's type system is not as flexible as TypeScript. We use a single class to parse / serialize both alternatives of SemanticTokensOptions | SemanticTokensRegistrationOptions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above, I didn't realize that SemanticTokensRegistrationOptions is actually meant to extend SemanticTokensOptions. Then the current constructors can be kept, but please add one without arguments:
new() {
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done @spoenemann. I didn't add an empty constructor before because both SemanticTokensOptions
and SemanticTokensRegistrationOptions
have non-null legend
filed, so I thought that it didn't make sense to add a constructor that doesn't initialize the legend
field.
Signed-off-by: Eric Dallo <[email protected]>
Signed-off-by: Eric Dallo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
woohoo, good news! gonna implement semanticTokens in next release of our ls. thanks! |
One small question... I can't find any method for |
Yeah, I think I forgot about that @nixel2007 |
* The actual tokens. | ||
*/ | ||
@NonNull | ||
private List<Integer> data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for pinging closed PR, but shouldn't we migrate List<Integer>
to int[]
? It's a huge list/array with a lot of Integer
boxing/unboxing operations. Yes, List
is more comfortable class with all its Collection
methods. But is it fast enough?
@ericdallo @spoenemann WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do some profiling before optimizing something like that. Java ArrayList and Integer boxing/unboxing is super optimized in the JVM, I don't know whether you'll notice any difference for real-world array sizes.
Closes #430
Add support for textDocument/semanticTokens
Signed-off-by: Eric Dallo [email protected]