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

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

Closed
RodrigoSantiago opened this issue Nov 10, 2016 · 18 comments

Comments

@RodrigoSantiago
Copy link

RodrigoSantiago commented Nov 10, 2016

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

@RodrigoSantiago RodrigoSantiago changed the title Clean style makes the code area scroll Clean style does the code area scroll Nov 21, 2016
@synth3
Copy link

synth3 commented Dec 24, 2016

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
Copy link

synth3 commented Jan 10, 2017

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
Copy link

synth3 commented Jan 10, 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);
}

@JordanMartinez
Copy link
Contributor

@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 JordanMartinez changed the title Clean style does the code area scroll Restyling large portions of the document causes area to scroll when it shouldn't. Mar 17, 2017
@richATavail
Copy link

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!

@JordanMartinez
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

@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.

@richATavail
Copy link

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

@JordanMartinez
Copy link
Contributor

I've cleaned up the demo to reproduce the code automatically once run:

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

Replacing text with sample code
parChanges is now pushing a mod
Finished subscribing to par change
Height Estimate changed to: 3216.0
Scroll count: 0 | Changed to: 3200.0
Width changed to: 583.0
Layout call count: 1
Scroll count: 1 | Changed to: 3184.0
Scroll count: 2 | Changed to: 3200.0
Scroll count: 3 | Changed to: 3184.0
Scroll count: 4 | Changed to: 3168.0
Scroll count: 5 | Changed to: 3200.0
Scroll count: 6 | Changed to: 3184.0
Scroll count: 7 | Changed to: 3152.0
Scroll count: 8 | Changed to: 3200.0
Scroll count: 9 | Changed to: 3184.0
Scroll count: 10 | Changed to: 3136.0
Scroll count: 11 | Changed to: 3200.0
Scroll count: 12 | Changed to: 3184.0
Scroll count: 13 | Changed to: 3120.0
Scroll count: 14 | Changed to: 3200.0
Scroll count: 15 | Changed to: 3184.0
Scroll count: 16 | Changed to: 3104.0
Scroll count: 17 | Changed to: 3200.0
Scroll count: 18 | Changed to: 3184.0
Scroll count: 19 | Changed to: 3088.0
Scroll count: 20 | Changed to: 3200.0
Scroll count: 21 | Changed to: 3184.0
Scroll count: 22 | Changed to: 3072.0
Scroll count: 23 | Changed to: 3200.0
Scroll count: 24 | Changed to: 3184.0
Scroll count: 25 | Changed to: 3056.0
Scroll count: 26 | Changed to: 3200.0
Scroll count: 27 | Changed to: 3184.0
Scroll count: 28 | Changed to: 3040.0
Scroll count: 29 | Changed to: 3200.0
Scroll count: 30 | Changed to: 3184.0
Scroll count: 31 | Changed to: 3024.0
Scroll count: 32 | Changed to: 3200.0
Scroll count: 33 | Changed to: 3184.0
Scroll count: 34 | Changed to: 3008.0
Scroll count: 35 | Changed to: 3200.0
Scroll count: 36 | Changed to: 3184.0
Scroll count: 37 | Changed to: 2992.0
Scroll count: 38 | Changed to: 3200.0
Scroll count: 39 | Changed to: 3184.0
Scroll count: 40 | Changed to: 2976.0
Scroll count: 41 | Changed to: 3200.0
Scroll count: 42 | Changed to: 3184.0
Scroll count: 43 | Changed to: 2960.0
Scroll count: 44 | Changed to: 3200.0
Scroll count: 45 | Changed to: 3184.0
Scroll count: 46 | Changed to: 2944.0
Scroll count: 47 | Changed to: 3200.0
Scroll count: 48 | Changed to: 3184.0
Scroll count: 49 | Changed to: 2928.0
Scroll count: 50 | Changed to: 3200.0
Scroll count: 51 | Changed to: 3184.0
Scroll count: 52 | Changed to: 2912.0
Scroll count: 53 | Changed to: 3200.0
Scroll count: 54 | Changed to: 3184.0
Scroll count: 55 | Changed to: 2896.0
Scroll count: 56 | Changed to: 3200.0
Scroll count: 57 | Changed to: 3184.0
Scroll count: 58 | Changed to: 2880.0
Scroll count: 59 | Changed to: 3200.0
Scroll count: 60 | Changed to: 3184.0
Scroll count: 61 | Changed to: 2864.0
Scroll count: 62 | Changed to: 3200.0
Scroll count: 63 | Changed to: 3184.0
Scroll count: 64 | Changed to: 2848.0
Scroll count: 65 | Changed to: 3200.0
Scroll count: 66 | Changed to: 3184.0
Scroll count: 67 | Changed to: 2832.0
Scroll count: 68 | Changed to: 3200.0
Scroll count: 69 | Changed to: 3184.0
Scroll count: 70 | Changed to: 2816.0
Scroll count: 71 | Changed to: 3200.0
Scroll count: 72 | Changed to: 2816.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
Copy link
Contributor

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:

@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
Copy link
Contributor

@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.

@synth3
Copy link

synth3 commented Mar 28, 2017

@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.

@JordanMartinez
Copy link
Contributor

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

@JordanMartinez
Copy link
Contributor

I've created a branch in my repo of RichTextFX called jitpack-master that includes the fix I've submitted (FXMisc/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

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

Add the dependency

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

Gradle

repositories {
    maven {
        url 'https://jitpack.io'
    }
}

dependencies {
    compile "com.github.JordanMartinez:RichTextFX:jitpack-master-SNAPSHOT"
}

Sbt

resolvers += "jitpack" at "https://jitpack.io"

libraryDependencies += "com.github.JordanMartinez" % "RichTextFX" % "jitpack-master-SNAPSHOT"

@JordanMartinez
Copy link
Contributor

@RodrigoSantiago Also noted the following in his comment here:

If the class has an equals overload, the error does not occur
If the equals return false when two have the same style classes, the error happens

@JordanMartinez JordanMartinez added this to the 0.7-M7 milestone Oct 3, 2017
@JordanMartinez
Copy link
Contributor

The latest 0.6-SNAPSHOT release of Flowless now resolves this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants