JavaKeywordsDemo doesn't update syntax highlighting when multiple lines inserted in succession before each other

[willplatt]

JavaKeywordsDemo doesn't update syntax highlighting after certain changes. This is because it tries to be efficient by only recomputing highlighting for lines that are changed, but it does this incorrectly.

Specifically, when multiple changes come through at once (due to reduceSuccessions()), the first change that was made to line x may now actually apply to some other line y due to changes that happened since. The program does not account for this and hence highlighting is recomputed for line x instead of line y. For example, a change on line 5 may in fact apply to line 8 by the time we compute syntax highlighting because 3 lines have since been inserted before the original line 5. But the current logic will recompute highlighting for line 5 instead of line 8.

The easiest way I have found to see this bug is by deleting a line (with its line ending included), then deleting the line after it, then the line after that. Then quickly undo several times to bring the lines back in under 500ms. Only the top line will receive syntax highlighting. For example, with the default text in the editor, place the cursor just after the / closing the mult-line comment and select down to just after the next { and delete this selection. Then repeat by selecting just after the multi-line comment down to the end of the single-line comment. Then repeat by selecting down to just after the for ... {. After rapidly hitting undo three times, the single-line comment and for line won't be highlighted (see image).

I'm about to submit a pull request to fix this bug.

[Symeon94]

I'm trying to understand the description, but I'm not sure to follow. In the example you gave, you do 3 delete followed by 3 quick undo, could you guide us through what you think the merge will do to these?

In your example you delete line 11 and then line 12, such that 13 becomes 11. Then you quickly undo, wjat will actually happen ?

Thanks for the explanation

[willplatt]

Can you see the image I included? Lines 11 and 12 have black text because syntax highlighting wasn't applied to these lines. This is the bug that is addressed by the pull request.

[Symeon94]

That I saw, what I meant is if you could guide us through what you think happens when the code is executed with the steps you are describing.

[willplatt]

Ah, okay. So With each undo there is an insertion on the lines labelled 9 and 10 (indices 8 and 9)—line 9 for the newline character and line 10 for the other characters. The problem is that the three undos together were being seen as changes to lines 9 and 10 only. But really the three of them together are changes to lines 9, 10, 11, and 12. That's because the first insertion gets pushed down two lines by the later two and ends up being for lines 11 and 12, and similarly the second insertion ends up being lines 10 and 11. Do you understand now?

[Symeon94]

Ok, so the three changes recorded are delete (from the top of my head, I think it's actual replace to empty, but that doesn't matter here) with at line 9-10

• \n public static void main(String[] args) {
• \n // single-line comment
• \n for(String arg: args) {

What you are saying is that there is a merge of the operations but done wrongly as it should be modifying line 9/10/11/12. I don't have enough time to look in details at that today, I'll go look again tomorrow but to me merging the three should be a single change on line 9 with the three content above merged.

Thanks for taking the time to add explanation, I see @Jugen reviewed your PR, I'll have a look at it tomorrow too.

[willplatt]

To be clear, I'm not saying the three insertions are merged into one insertion. I'm saying that the three insertions are performed correctly one after another but that the subscribed listener is only called once with data from all three changes. The lambda in reduceSuccessions() is called twice, but it treats the changes as independent when they aren't.

[Symeon94]

Thanks for the clarification, that is actually very helpful to understand.

[...] it treats the changes as independent when they aren't.

It looks to me as 3 independent changes, for as long as they are executed in the right sequence.

But executing the code I finally get what you mean. This is what happens:

1. Change indexes are collected and merged until a pause of 500ms is seen (that's done in the reduceSuccession)
2. Once a pause in changes is seen, it uses the merged indexes to tell what line it must update the style of

And what you are saying is that the indexes of the updates are not what they will be after the change is applied and that causes the wrong line to be styled after the pause of 500ms is seen.

100% clear now!

I'll have a look at the PR and let you know if I see anything.

---

On another note, can we take that opportunity to cleanup that big subscription code by separating the operations in methods and giving proper names. For instance (a, b) -> { a.addAll(b); return a;} is not very telling. it could be replaced by an external method or by proper naming:

.reduceSuccession(this::mergeRanges, Duration.ofMillis(500))
// ...
private Set<IndexRange> mergeRanges(Set<IndexRange> a, Set<IndexRange> b) {
    a.addAll(b);
    return a;
}

(This is an example. This specific method might be gone).

[willplatt]

I'm glad it makes sense to you now.

I could separate things out into small functions, but I was trying to keep the style of the original code. Personally I don't see much of a problem with the example lambda you gave—it's easy to understand but a little untidy with two statements on one line.