Richtextfx: Restyling large portions of the document causes area to scroll when it shouldn't.

Created on 10 Nov 2016 · 18Comments · Source: FXMisc/RichTextFX

I am using this code:
codeArea.clearStyle(0, codeArea.getLength() - 1);

Before / After Clear and style
sem titulo

If the area is already clean, the problem does not happen (it is inside a VirtualizedScrollPane)

Obs.: After some tests, I noticed that this happens when the font type is italic

bug dependency has workaround

Source

RodrigoSantiago

All 18 comments

Additional information that may help finding the cause:
I noticed similar problems in the process of implementing a "mark occurrences" feature (set the background colour on occurrences of the word that is currently at the cursor). My initial simple approach was to re-calculate all StyleSpans and then re-apply those with setStyleSpans(0, newSpans). That method then sometimes caused the line with the caret to scroll to the top of the text-viewport. I then managed to eliminate the problem by iterating over the occurrences to mark and using the setStyle(start, end, styleString) method to apply them one after another.

I also noticed that setting the background colour results in a tiny (unexpected) change of the text width - and maybe is part of the problem?

Environment of my observations: Windows 7, Java 8u102

synth3 on 24 Dec 2016

I modified the java Keywords demo for easy reproduction of the described problem: JavaKeywordsAsync.txt

A Button for clearStyle(0, codeArea.getText().length() - 1) was added ("reset")
some line breaks where added to the sample content
the comment-regex was deactivated

Reproduction:

replace the JavaKeywordsAsync demo with the contents of teh attached file
start the demo
scroll to the bottom
press the "reset" button
Result for 0.7-M3:
the area scrolls to the beginning of the content
Result for 0.7-M2:
the area scrolls so that the line with the caret is at the beginning of the viewport (when the caret position allows that)

Conclusions and sidenotes:

The problem is independent of "italic" font
The behavior in M3 is worse compared to M2
Since StyledTextArea.clearStyle(int from, int to) calls StyledTextAreaModel.setStyle(from, to, initialTextStyle) the problem is not limited to clearing the styles. Actually I stumble over it very often when re-styling large contents

Where could I look to eventually find the cause of the problem?

synth3 on 10 Jan 2017

Here is a work/hack-around:

Double scrollBefore = imTextArea.getEstimatedScrollY();

// set the styles here
textArea.setStyleSpans(...)

if (!scrollBefore.equals(textArea.getEstimatedScrollY()))
{
    textArea.setEstimatedScrollY(scrollBefore);
}

synth3 on 10 Jan 2017

@synth3 I think it'd be easier to find the problem in M2 than in M3 because of the added complexity of M3.
Here's my guess. Since modifying the style makes a large portion of the currently displayed paragraphs invalid and the individual cells are not reusable, the virtualFlow's cellFactory has to remove the old ParagraphBox, recreate a new ParagraphBox for each item, and add those boxes to the viewport. Since it does not store the current position of the viewport before restyling those lines, when it lays out the paragraphboxes again, it has no way of knowing how much to offset the view so that it appears that nothing has changed.

Before you say let's implement @synth3's hack into the code by default, let's say that we weren't clearing the style but were changing the font size to be bigger. Would we still want that code to run? It might be better to just override the method in your individual case so that you store and set the method before and after that occurs.

JordanMartinez on 11 Jan 2017

We've started using RichTextFX for our language project, Avail, at https://github.com/AvailLang/Avail. Because of the complexity of the language, we can't use typical regex-based IDE's for a true Avail environment. We've been using RichTextFX to build out an Avail editor that we hope to make into a full blown IDE. Unfortunately we've bumped up against this bug. It is a show stopper for us as we can't keep the screen from jumping when you start typing. Even with synth3's proposed work-around, we can't keep the screen from visibly reseting itself.

We've extended the class CodeArea for use in our IDE project. Because we have in-depth knowledge of how the file is tokenized from our scanner when we open it, we don't need to run a pattern matcher to apply styles to text; we already know the ranges and how the text should be style. We are using StyleClassedTextArea.setStyleClass(int,int,String) to apply the styles to the various ranges. Because of our in-depth knowledge of the source, we are working towards localized style changes without having to restyle the entire document, but we're not quite there yet. If we achieve that, it might be that the screen won't jump? I can't be sure of that though...

Are there any thoughts on when this bug might be addressed? Is there an earlier version that has CodeArea that we maybe able to use that might not have this bug? Do you have any thoughts on what we might do on our end to try and address the bug? I tried to dig into ParagraphBox and VirtualFlow a bit to see if I could determine how one might capture and restore the position, but I've yet to puzzle that out. Any thoughts or help would be much appreciated. Thanks!

richATavail on 23 Mar 2017

Here's one idea. I have no idea whether it'll work. It attempts to determine how far away the first visible line is offset in the viewport before the style change occurs and then seeks to offset the viewport by that same amount.

Get estimatedScrollY's value and store it in estY
Determine the index of the topmost fully visible line and store in topIdx
Call showAtTop with topIdx
Get estimatedScrollY's value; store the difference between it and estY in offset
do your style change
Call showAtTop with topIdx
Call scrollBy(new Point2D(0, offset)

JordanMartinez on 23 Mar 2017

I added a change listener to estimatedScrollY and this is what it logged when I ran the above demo according to its instructions:
Original value before pressing button: 120.0 Changed to: 88.0 Changed to: 120.0 Changed to: 8.0 Changed to: 0.0 Changed to: 8.0 Changed to: 0.0 Changed to: 8.0
The third change is what causes the issue.

JordanMartinez on 23 Mar 2017

After making ESD the model in #463, the code I ran now displays:
Original value before pressing button: 120.0 Par changes pushing change Changed to: 88.0 Changed to: 120.0 Changed to: 8.0 Changed to: 0.0 Changed to: 8.0 Par changes pushing change Changed to: 0.0 Changed to: 8.0 Par changes pushing change

JordanMartinez on 25 Mar 2017

@richATavail @synth3 I'll be looking into this issue this week. I won't make any guarantees that it'll be fixed by the end of the week. I simply plan on identifying what's causing the problem.

JordanMartinez on 27 Mar 2017

@JordanMartinez Thanks! I understand what it is do this sort of work on your free time, so I do greatly appreciate it!

richATavail on 27 Mar 2017

I've cleaned up the demo to reproduce the code automatically once run:
````java
import java.time.Duration;

import javafx.application.Application;
import javafx.application.Platform;
import javafx.beans.property.SimpleIntegerProperty;
import javafx.scene.Scene;
import javafx.stage.Stage;

import org.fxmisc.flowless.VirtualizedScrollPane;
import org.fxmisc.richtext.StyleClassedTextArea;
import org.reactfx.util.FxTimer;

public class JavaKeywordsAsync extends Application {

public static void main(String[] args) {
    launch(args);
}

private StyleClassedTextArea area;

private class IntProp extends SimpleIntegerProperty {

    IntProp(int val) {
        super(val);
    }

    public int plusPlus() {
        int v = get();
        set(v + 1);
        return v;
    }

}

private void log(String message) {
    System.out.println(message);
}

@Override
public void start(Stage primaryStage) {
    area = new StyleClassedTextArea();

    IntProp scrollChangeCount = new IntProp(0);
    area.widthProperty().addListener((obs, ov, nv) -> log("Width changed to: " + nv));
    area.heightProperty().addListener((obs, ov, nv) -> log("Height changed to: " + nv));
    area.estimatedScrollYProperty().addListener((obs, ov, nv) -> log("Scroll count: " + scrollChangeCount.plusPlus() + " | Changed to: " + nv));
    area.totalHeightEstimateProperty().addListener((obs, ov, nv) -> log("Height Estimate changed to: " + nv));

    primaryStage.setScene(new Scene(new VirtualizedScrollPane<>(area), 600, 400));
    primaryStage.setTitle("Bug Demo");
    primaryStage.show();

    StringBuilder sb = new StringBuilder();
    for (int i = 0; i < 200; i++) {
        sb.append("a\n");
    }
    log("\nReplacing text with sample code");
    area.replaceText(sb.toString());

    FxTimer.runLater(Duration.ofMillis(1500), () -> {
        log("\nRestyling entire document");
        area.setStyleClass(0, area.getLength() - 1, "red");
        Platform.exit();
    });
}

}
which (along with some other change listeners in RTFX) produces:
:richtextfx-demos:run JavaKeywordsAsync
Width changed to: 600.0
Height changed to: 400.0
Layout call count: 0
Height Estimate changed to: 16.0

Restyling entire document
parChanges is now pushing a mod
Finished subscribing to par change
Scroll count: 73 | Changed to: 3200.0
Scroll count: 74 | Changed to: 2816.0
Layout call count: 2
Scroll count: 75 | Changed to: 0.0
````

JordanMartinez on 28 Mar 2017

Found the bug. The code doesn't account for the possibility that a deletion that occurs before the first visible cell in the viewport also deletes part of the visible cells in the viewport.

The issue lies in StartOffStart#transformChange:
````java
@Override
public TargetPosition transformByChange(
int pos, int removedSize, int addedSize) {
// itemIndex = 176; offset =
// pos = 0; removedSize = 200; addedSize = 200

System.out.println(String.format("Transforming change with parameters pos=%s removedSize=%s addedSize=%s", pos, removedSize, addedSize));
if(itemIndex >= pos + removedSize) {
    System.out.println("change before the target item, just update item index");
    return new StartOffStart(itemIndex - removedSize + addedSize, offsetFromStart);

// doesn't check whether this deletion may occur
// pos  itemIndex (1st visible cell)   pos + removedSize
//   | ------- | --------------------------------- |
} else if(itemIndex >= pos) {
    // so this code gets run, which forces the viewport to display the cell whose index == pos
    // or 0 in this case.
    System.out.println("target item deleted, show the first inserted at the target offset");
    return new StartOffStart(pos, offsetFromStart);
} else {
    System.out.println("change after the target item, target position not affected");
    return this;
}

}
````

I combined Flowless and RichTextFX into one project so it would be easier to add println statements into Flowless to see what was going on. The key part here is surrounded by ///:
````
Restyling entire document
parChanges is now pushing a mod: org.reactfx.collection.MaterializedListModificationImpl@6217104f
Finished subscribing to mod: org.reactfx.collection.MaterializedListModificationImpl@6217104f
Scroll count: 73 | Changed to: 3200.0

Items were changed
Navigator's target position before transformations is: StartOffStart(itemIndex=176 offsetFromStart=0.0)
Transforming Navigator's target position with mod: org.reactfx.collection.MappedList$1$1@2b1eb25d
Before mod transformation, Navigator's target position is now: StartOffStart(itemIndex=176 offsetFromStart=0.0)
Transforming change with parameters pos=0 removedSize=200 addedSize=200
///
target item deleted, show the first inserted at the target offset
///
After mod transformation, Navigator's target position is now: StartOffStart(itemIndex=0 offsetFromStart=0.0)

Scroll count: 74 | Changed to: 2816.0
Layout call count: 2
Beginning Virtual Flow layout
using orientation helper to resize navigator
Navigating to the target position and filling viewport
Starting navigator layout
Number of memoized cells count: 1
Cells is not empty, clamping target position and accepthing this as visitor
Navigator#visit StartOffStart - target position is: StartOffStart(itemIndex=0 offsetFromStart=0.0)
Place start at 0 and may crop at 0.0
positioner. crop to (itemIndex - itemsBefore) [0 - 25 = -25], itemIndex + 1 + itemsAfter [0 + 1 + 25 = 26]
cropping. Forgetting from 0 to 0
Cropping. forgetting from 26 to 201
Scroll count: 75 | Changed to: 0.0
Positioner. place start at item index & start offstart
Fill viewport from item index: 0
cropping. Forgetting from 0 to 0
Cropping. forgetting from 25 to 201
Navigator's current target position is: StartOffStart(itemIndex=0 offsetFromStart=0.0)
Checking if old layout breadth == current layout breadth
update finished
Relocating navigator
Ending virtual flow layout
````

JordanMartinez on 28 Mar 2017

@richATavail @synth3 You can get around this issue if you compile a local copy of my PR for Flowless and use it as a dependency in your project because I'm not sure how long it'll be before Tomas merges the code.

JordanMartinez on 28 Mar 2017

👍1

@JordanMartinez Thanks a lot for your work! I'm glad to hear that you found the issue. Until now I didn't manage to get enough understanding of the codebase for being able to find the cause.

synth3 on 28 Mar 2017

@synth3 It was fun and frustrating.
You might want to look at TomasMikula/Flowless#33 to get a better understanding of how Flowless works.

JordanMartinez on 29 Mar 2017

👍1

I've created a branch in my repo of RichTextFX called jitpack-master that includes the fix I've submitted (TomasMikula/Flowless#32). If you change your build to use that via Jitpack, then this bug will no longer appear. Follow these instructions to update your build to use it. You can revert your build to the next release whenever Tomas makes the next one including the fix.

Maven

Add the repository

xml <repositories> <repository> <id>jitpack.io</id> <url>https://jitpack.io</url> </repository> </repositories>

Add the dependency

xml <dependency> <groupId>com.github.JordanMartinez</groupId> <artifactId>RichTextFX</artifactId> <version>jitpack-master-SNAPSHOT</version> </dependency>