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 #1552

Merged

Conversation

0dinD
Copy link
Contributor

@0dinD 0dinD commented Sep 21, 2020

Fixes part of redhat-developer/vscode-java#1597.
See redhat-developer/vscode-java#1632 for the second part of the fix.

This is done by getting the token line and column number directly from its compilation unit, rather than the document, which for whatever reason gets out-of-sync during frequent updates.

Signed-off-by: 0dinD <zerodind@gmail.com>
return new SemanticTokens(Collections.emptyList());
}

SemanticTokensVisitor collector = new SemanticTokensVisitor(root, SemanticTokenManager.getInstance());
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. But not sure whether it can fix the problem completely.

As my understanding, both document and CompilationUnit are a snapshot of the document at some point. The difference is CompilationUnit is the AST tree parsed from the document at some point. I'm curious how the fix can guarantee it's not out-of-sync.

Copy link
Contributor Author

@0dinD 0dinD Sep 22, 2020

Choose a reason for hiding this comment

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

Yes, the compilation unit might get out-of-sync with the real document at some points. But the problem before this change was that the semantic token visitor used both a document and AST nodes from the compilation unit, and the document was not in sync with the compilation unit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I just submitted redhat-developer/vscode-java#1632, which should fix the problem that you've pointed out. It turns out that there were two separate issues causing the offset, see my comment.

@Eskibear
Copy link
Contributor

@0dinD This PR looks promising, good job! As you know it's not easy for me to reproduce the issue, so I'd like to use it in my working projects for a while, and see if we do get rid of the issue. I'll let you know my feedback then.

@0dinD
Copy link
Contributor Author

0dinD commented Sep 24, 2020

@Eskibear sounds like a good idea! Don't forget to also use redhat-developer/vscode-java#1632.

@0dinD
Copy link
Contributor Author

0dinD commented Sep 24, 2020

@Eskibear if it helps, here are some gifs showing the best way I can reproduce the issue:

Part one of the issue: (fixed by this PR)
reproduce-1597-part-1
Note that I have the EditorConfig extension installed and configured to trim whitespace on Ctrl + S here. Strangely enough, I can't reproduce the issue as easily by just undo/redo or similar, probably because EditorConfig gives a different timing between document edits. You're going to have to experiment a bit until you find the right timing between keypresses.

Part two of the issue: (fixed by redhat-developer/vscode-java#1632)
reproduce-1597-part-2

If you're wondering what I mean by "part one" and "part two", see my issue comment.

@fbricon
Copy link
Contributor

fbricon commented Sep 25, 2020

PR LGTM. I'll let @Eskibear merge it once he's done testing it.

Copy link
Contributor

@Eskibear Eskibear left a comment

Choose a reason for hiding this comment

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

LGTM.
I've played it for in my project for one day or two, and so far it worked well. And I also tried the way metioned in #1552 (comment) , I cannot reproduce it now. 👍

@testforstephen testforstephen merged commit 8b7e12d into eclipse-jdtls:master Sep 27, 2020
@testforstephen
Copy link
Contributor

Thanks @0dinD.

Looks like @Eskibear doesn't have committer permission to this github repo. CC: @fbricon

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

Successfully merging this pull request may close these issues.

4 participants