eclipse-tm4e / tm4e

TextMate support in Eclipse IDE
https://projects.eclipse.org/projects/technology.tm4e
Eclipse Public License 2.0
93 stars 56 forks source link

fix: sync issues between TokenizerThread and TMPresentationReconciler #566

Closed sebthom closed 1 year ago

sebthom commented 1 year ago

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 commented 1 year ago

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

angelozerr commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

See

image

angelozerr commented 1 year ago

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