-
Notifications
You must be signed in to change notification settings - Fork 54
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
Implementation of a semantic tokens reconciler #253
Conversation
b282afe
to
857dec2
Compare
857dec2
to
4547839
Compare
Do you think you can add tests about it? |
f1388aa
to
bca2ff2
Compare
...p4e/src/org/eclipse/lsp4e/operations/semanticTokens/SemanticHighlightReconcilerStrategy.java
Outdated
Show resolved
Hide resolved
...p4e/src/org/eclipse/lsp4e/operations/semanticTokens/SemanticHighlightReconcilerStrategy.java
Outdated
Show resolved
Hide resolved
bca2ff2
to
6816201
Compare
As mentioned earlier, do you think you can add some tests about this change? |
Yes, I will look into writing some test. I have marked the PR as a draft in the meantime |
@mickaelistria , I must admit I am clueless about how to test this reconciler. I have seen that eclipse/tm4e#33 has been open since long. Can we actually write a test for a reconciler? |
0f3a050
to
7304985
Compare
@mickaelistria , the commit cannot be built because do you know what that is? Thanks |
Please try using latest Tycho release (this can fit in a dedicated PR) |
Initial implementation of a semantic tokens reconciler which supports only SemanticTokensFull. The reconciler uses the theme defined for TM4E so that the same styles are used for the same token types. In addition to the tokens defined for TM4E, also the deprecated modifier is supported by marking striking out the regions.
can be unit tested.
6d27a06
to
60ef8b8
Compare
Thanks, that fixed the issue. |
ed9fbe8
to
9c80db6
Compare
Hi @mickaelistria , I have refactored the code so that I could write tests of most parts involved in the reconciler. The code is not building currently but that looks like a problem with the build system, and not the PR. I have also tested this code (a very similar one because I did some minor refactoring today when writing the unit tests) in our setup and it works really nice. So I am putting it again for review, if you think more tests are needed, I am happy to do them if they can be done without testing a full fledge editor setup, I am still not sure how to do that. Regards |
9c80db6
to
ffd155b
Compare
639d686
to
fcace1a
Compare
d45bc6e
to
1348bcf
Compare
@mickaelistria , I have now also written a unit test that shows end-to-end that opening a file in the editor triggers the semantic reconciler and that this one updates the styles in the buffer to the values which are set in the mocked server. I think the code is good, but just in case I have also added a store configuration to disable the reconciler as a workaround, if it would prove to be faulty in such way and makes working with the editor hard, until we can fix it. Is it ok with you to merge it now? I believe it is a very nice feature. |
testCheckIfOtherAnnotationsRemains failed, but that fails constantly in the jobs, I am sure it is not this PR's fault :) |
1348bcf
to
df9262e
Compare
Thanks! |
@mickaelistria @rubenporras I'm currently try to use this but nothing seem to visually change (but I also don't get an error), my method is called and I'm currently just return a single dummy like this:
I use version: code: eclipse-pde/eclipse.pde#821 So I seem to miss something ... or is it required to configure something here? |
hello i'm testing LSP within Eclipse. The semantic highlighting in VSCode is working and lsp4j already working on this topic. |
I can't make this work in Eclipse either so there so it seems I must be something I did wrong, it is not implemented "visually" or some magic setting is required :-) |
Hello, In addition to having TM4E installed, it should be configured for the editor where semantic highlight is needed. At the very least the stylesheet it uses must contain an style for the token the language server returns. Have you done that? Regards |
Likely not as it is nowhere documented (or I didn't found that) :-)
I wounder if there could be some defaults? If I read it correctly here there are some default tokens so at least for them there should be some "fallback" if nothing else is defined? |
Fair enough. Where would you expect such documentation?
I would not know what that would be. How can we know the color (or other style) of a token? |
Good question I searched for
I think it must not be perfect, but for example make some of the bold, others italic or even some random colors what seems suitable to have a different visual appearance. Right now it just stays all the same so I don't even know if my above example has ever worked. |
Implementation of a semantic tokens reconciler which supports only SemanticTokensFull.
The reconciler uses the theme defined for TM4E so that the same styles are used for the same token types.
In addition to the tokens defined for TM4E, also the deprecated modifier is supported by marking striking out the regions.