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: sync issues between TokenizerThread and TMPresentationReconciler #566

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

sebthom
Copy link
Member

@sebthom sebthom commented Jun 20, 2023

This PR is a major rewrite of how async line tokenization/colorizing is performed. It addresses the broken syntax highlighting #544 after reformatting XML files via the XML Language Server. It also further improves rendering performance and UI responsiveness issues as reported by #540 and #561.


There is one minor rendering issue left which I am unsure how to address. When multiline XML comments are reformatted, some of the lines that don't change due to reformatting loose their coloring. This is however not an issue caused by the tokenization model being out-of-sync or the TMPresentationReconciler not applying coloring to damaged regions but by the fact that these regions don't seem to be reported as damaged by the TextViewer. For performance reasons I am reluctant to simply re-colorize all lines of a document in case anything changed.

Here is an example:
image
Lines with a line number with white background did not change during the reformatting. Some of them loose their styling but not all.

@sebthom sebthom changed the title fix sync issues between TokenizerThread and TMPresentationReconciler fix: sync issues between TokenizerThread and TMPresentationReconciler Jun 20, 2023
this commit also fixes UI freezes caused by tm4e when working with very
large documents
@sebthom
Copy link
Member Author

sebthom commented Jun 20, 2023

@angelozerr do you have a chance to give this a try before I merge it?

@angelozerr
Copy link
Contributor

angelozerr commented Jun 20, 2023

I'm sorry @sebthom it wil be hard for me to find time to test your PR.

It seems that you have add some colorize logic with DocumentListener, it means that colorization will be done on backround and with document listener both?

@sebthom
Copy link
Member Author

sebthom commented Jun 20, 2023

It seems that you have add some colorize logic with DocumentListener, it means that colorization will be done on backround and with document listener both?

I actually did not change this part. In the UI package I mainly did refactoring, like separating the colorization code into a dedicated separate class.

The main difference is that now when a lot of changes are performed in very fast succession (like done by the formatter), these changes are processed in a batch instead of separately and then firing a ModelTokensChangedEvent for each change. This confused the colorizer as it received line numbers that were outdated.

The other difference is that the managing of the TMModels internal list of lines is now completely down by the tokenizer thread whereas before the UI thread would effectively insert/remove lines which resulted in UI freezes for very large files. Now the UI thread only adds "edit" events to a non-blocking queue.

@angelozerr
Copy link
Contributor

Thanks so much for your clarification. It looks great! Congrats!

I wonder if we could add a test with TextEdit like we have for XML formatter?

@sebthom
Copy link
Member Author

sebthom commented Jun 20, 2023

@angelozerr I don't have much experience with Eclipse UI tests. Is it possible to start a UI test directly from within the Eclipse Workspace via Run As -> JUnit Test? So far I can only get the UI tests working when running them on the command line via maven which I find extremely cumbersome during development.

For example when I try to execute TMinGenericEditorTest or the TMEditorColorTest from within Eclipse all I get is java.lang.NoClassDefFoundError: org/eclipse/e4/ui/workbench/IWorkbench

Running the RegistryTest I get java.lang.NullPointerException: Cannot invoke "org.eclipse.core.runtime.content.IContentTypeManager.getContentType(String)" because the return value of "org.eclipse.core.runtime.Platform.getContentTypeManager()" is null and so on...

@angelozerr
Copy link
Contributor

See

image

@angelozerr
Copy link
Contributor

My idea was to write a test with MultiTextEdit like https://github.com/eclipse/lsp4e/blob/2a404e96ab462a1929a156b604745def685a166a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LSPEclipseUtils.java#L497

@sebthom sebthom merged commit 9b03a29 into eclipse-tm4e:main Jun 21, 2023
@sebthom sebthom deleted the sync-fix-final branch June 21, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants