Fix ugly semantic coloring on CSL languages while editing by lkishalmi · Pull Request #8011 · apache/netbeans

lkishalmi · 2024-12-01T21:52:54Z

This one is bugging me for years.

Whenever I insert or write text to a CSL based editor the semantic coloring does not keep track with the document change
and some text elements get discolored as you type. The next parsing run would then set the colors right.

See the discoloration of permissions before:
Screencast From 2024-12-01 13-04-47.webm

After this fix:
Screencast From 2024-12-01 13-07-58.webm

Fortunately or Unfortunately , I had to change the code here and there in order to be able to understand what's going on.

So this could be a one -liner, removing the Math.min form:

netbeans/ide/csl.api/src/org/netbeans/modules/csl/editor/semantic/GsfSemanticLayer.java

Line 322 in c2ec6e4

? Math.min(layer.getShiftedPos(element.range.getEnd()), nextElementStartOffset)

I submitted my whole work on this though, hoping that it would simplify that part of CSL and makes it less cryptic.

So my changes in brief (read it with the code changes):

Updated the language level to Java 17
Made SequenceElement as a record. The ComparisonItem got me a lot of headache, at the end it was only used to get the tailSet method work. Interestingly that subset just used as an optimization, the editor works even if the highlight sequence return the whole sequence all the time. (Tested with a 3500+ lines yaml, the performance overhead was barely noticeable.) Yet, I've copied the SortedSet into a list, where a simple binary search could be used to get a partial iterator.
Using just the iterator on GsfHighlightSequence made it a lot simpler.
Initially, I was thinking that the issue is due to some failed/garbage collected weak listener creation/registration, so I moved a few things to lambdas there.
Removed a lot of commented/unused code inherited from Retouche (I think that was the code name of the Java Editor rewrite)

matthiasblaesing

Looks generally sane to me (only eyeballed). My comments are mostly stylistic (you opened the can of worms ;-)).

The one place I'd really like to see improved is documentation, what firstSequenceElement should be doing.

matthiasblaesing · 2024-12-03T19:19:19Z

ide/csl.api/src/org/netbeans/modules/csl/editor/semantic/GsfSemanticLayer.java

        }
    }
+
+    private static <T> int firstSequenceElement(List<SequenceElement> l, int offset) {


A comment what this method should do would be nice. I have a feeling, but knowing would be nice.

matthiasblaesing · 2024-12-03T19:22:28Z

ide/csl.api/src/org/netbeans/modules/csl/editor/semantic/GsfSemanticLayer.java

+public final class GsfSemanticLayer extends AbstractHighlightsContainer implements DocumentListener {

    // -J-Dorg.netbeans.modules.csl.editor.semantic.GsfSemanticLayer.level=FINE
    private static final Logger LOG = Logger.getLogger(GsfSemanticLayer.class.getName());


LOG is unused and can be removed (reduce warnings).

matthiasblaesing · 2024-12-03T19:23:05Z

ide/csl.api/src/org/netbeans/modules/csl/editor/semantic/GsfSemanticLayer.java

+    private final List<Lookup.Result> coloringResults = new ArrayList<>(3);
+    private final List<LookupListener> coloringListeners = new ArrayList<>(3);


My take on the reason for this:

Suggested change

private final List<Lookup.Result> coloringResults = new ArrayList<>(3);

private final List<LookupListener> coloringListeners = new ArrayList<>(3);

// Write only - used to keep strong reference to Lookup.Result/LookupListener

@SuppressWarnings("MismatchedQueryAndUpdateOfCollection")

private final List<Lookup.Result> coloringResults = new ArrayList<>(3);

@SuppressWarnings("MismatchedQueryAndUpdateOfCollection")

private final List<LookupListener> coloringListeners = new ArrayList<>(3);

matthiasblaesing · 2024-12-03T19:28:07Z

ide/csl.api/src/org/netbeans/modules/csl/editor/semantic/HighlightImpl.java

Suggested by NB:

make all fields final

use StringBuilder in coloringsToString instead of StringBuffer

modifiy coloringsAttributesOrder to private static final Collection<ColoringAttributes> coloringsAttributesOrder = List.of(

in parse drop the reassignment of a and move the a.trim() into the valueOf call

matthiasblaesing · 2024-12-03T19:32:22Z

ide/csl.api/src/org/netbeans/modules/csl/editor/semantic/SemanticHighlighter.java

+        if (language != null) {
+            ColoringManager manager = language.getColoringManager();
+            SemanticAnalyzer task = language.getSemanticAnalyzer();
+            if (manager != null && task != null) {
+                Parser.Result r = resultIterator.getParserResult();
+                if (r instanceof ParserResult) {
+                    process(language, (ParserResult) r, newColoring);
+                }
+            }
+        }


I think this can be simplified to (manager and task can't be null, at least that is my reading from the code and assured by api annotations).

Suggested change

if (language != null) {

ColoringManager manager = language.getColoringManager();

SemanticAnalyzer task = language.getSemanticAnalyzer();

if (manager != null && task != null) {

Parser.Result r = resultIterator.getParserResult();

if (r instanceof ParserResult) {

process(language, (ParserResult) r, newColoring);

}

}

}

if (language != null) {

Parser.Result r = resultIterator.getParserResult();

if (r instanceof ParserResult parserResult) {

process(language, parserResult, newColoring);

}

}

mbien

Code looks good. Matthias made some good suggestions though.

I vaguely remember the old "schlieman" API impl having a similar problem. The bug could be potentially quite old. Good job hunting it down!

mbien · 2024-12-04T08:05:54Z

ide/csl.api/src/org/netbeans/modules/csl/editor/semantic/GsfSemanticLayer.java

        public int getEndOffset() {
-            return (element != null)
-                    ? Math.min(layer.getShiftedPos(element.range.getEnd()), nextElementStartOffset)
-                    : Integer.MAX_VALUE;
+            return getShiftedPos(element.range().getEnd());
        }


wondering if you could move the fix into its own commit? Even if its just the one liner which removes Math.min.

It would get lost among the other improvements. We do also have a rule in the reviewer guide which states to try to avoid obfuscating fixes with code refactoring. (I know I did break it myself too a few times, but I believe this case here is easy to split still)

Well, I probably close this PR and create two new ones. One with the one-line fix, the other is with the code cleanups.

While fixing the issues on HighlightImp, it turned out that it's not used at all.
I guess it is just an old implementation copied over from ...java.semantic package (that one is used), and later it turned out CSL would not use token based Highlighting.

your choice. Commit messages is all what is left in the repo history once merged. If they describe what they do -> all good. But if you want to split this into two PRs -> also good of course.

lkishalmi · 2024-12-04T22:25:40Z

Closing in favor of #8018

Fix ugly semantic coloring on CSL languages

d7abcf6

lkishalmi added the Editor label Dec 1, 2024

lkishalmi requested review from matthiasblaesing and mbien December 1, 2024 21:52

lkishalmi changed the title ~~Fix ugly semantic coloring on CSL languages~~ Fix ugly semantic coloring on CSL languages while editing Dec 1, 2024

mbien added this to the NB25 milestone Dec 1, 2024

matthiasblaesing reviewed Dec 3, 2024

View reviewed changes

mbien added the Code cleanup Label for cleanup done on the Netbeans IDE label Dec 4, 2024

mbien approved these changes Dec 4, 2024

View reviewed changes

mbien added the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Dec 4, 2024

lkishalmi mentioned this pull request Dec 4, 2024

Fix ugly semantic coloring on CSL languages while editing #8018

Merged

mbien added the CSL [ci] enable web job label Dec 4, 2024

lkishalmi closed this Dec 4, 2024

lkishalmi mentioned this pull request Dec 8, 2024

Code cleanup for csl.editor.semantic package #8028

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ugly semantic coloring on CSL languages while editing#8011

Fix ugly semantic coloring on CSL languages while editing#8011
lkishalmi wants to merge 1 commit intoapache:masterfrom
lkishalmi:csl-highlighter-fix

lkishalmi commented Dec 1, 2024

Uh oh!

matthiasblaesing left a comment

Uh oh!

matthiasblaesing Dec 3, 2024

Uh oh!

matthiasblaesing Dec 3, 2024

Uh oh!

matthiasblaesing Dec 3, 2024

Uh oh!

matthiasblaesing Dec 3, 2024

Uh oh!

matthiasblaesing Dec 3, 2024

Uh oh!

mbien left a comment

Uh oh!

mbien Dec 4, 2024

Uh oh!

lkishalmi Dec 4, 2024

Uh oh!

mbien Dec 4, 2024

Uh oh!

lkishalmi commented Dec 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		private final List<Lookup.Result> coloringResults = new ArrayList<>(3);
		private final List<LookupListener> coloringListeners = new ArrayList<>(3);

Conversation

lkishalmi commented Dec 1, 2024

Uh oh!

matthiasblaesing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkishalmi commented Dec 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants