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

Fix semantic tokens offset due to document updates #1632

Merged
merged 1 commit into from
Sep 25, 2020

Conversation

0dinD
Copy link
Contributor

@0dinD 0dinD commented Sep 22, 2020

Fixes #1597, together with eclipse-jdtls/eclipse.jdt.ls#1552

The fix is basically taken straight from the implementation of the TypeScript semantic token provider, see this line.

Copy link
Collaborator

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fbricon
Copy link
Collaborator

fbricon commented Sep 24, 2020

travis chokes on

ERROR: /home/travis/build/redhat-developer/vscode-java/src/semanticTokenProvider.ts[50, 13]: Identifier 'iv' is never reassigned; use 'const' instead of 'let'.

@0dinD
Copy link
Contributor Author

0dinD commented Sep 24, 2020

Oh, sorry, I just copied the code from the TypeScript implementation. I'll fix it later today.

@0dinD 0dinD force-pushed the fix-semantictokens-offset branch from 0abc723 to 44c216c Compare September 24, 2020 12:23
@0dinD
Copy link
Contributor Author

0dinD commented Sep 24, 2020

@fbricon I've fixed up the code now.

@fbricon fbricon merged commit 85941bf into redhat-developer:master Sep 25, 2020
@fbricon
Copy link
Collaborator

fbricon commented Sep 25, 2020

Thanks @0dinD, this seems to make a big difference, even without eclipse-jdtls/eclipse.jdt.ls#1552

@0dinD
Copy link
Contributor Author

0dinD commented Sep 25, 2020

No problem!
I think the reason why this PR also mitigates the issue fixed in eclipse-jdtls/eclipse.jdt.ls#1552 is because the extension now waits until the document changes have settled before actually applying any semantic tokens from the server. But still, there could be an edge case where we do need eclipse-jdtls/eclipse.jdt.ls#1552, so as long as there is no problem with that PR I think it's a good idea to also apply that fix. Better safe than sorry, right?

When the document and compilation unit become out-of-sync on the server side without the client knowing, the token offset will become "stuck" until you change the document again, which is very annoying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Since July update cause java highlight break
3 participants