Fix wrapped text scrollbar flicker

Jugen · Jugen · commit 39a8d3778ff4 · 2022-10-27T21:07:18.000+02:00
Changed averageLengthEstimate and totalLengthEstimate calculation process to avoid intermediary updates during cell refresh.
diff --git a/src/main/java/org/fxmisc/flowless/PausableSuccessionStream.java b/src/main/java/org/fxmisc/flowless/PausableSuccessionStream.java
@@ -3,6 +3,7 @@
 import java.time.Duration;
 import java.util.function.BiFunction;
 import java.util.function.Function;
+import java.util.function.Predicate;
 
 import org.reactfx.AwaitingEventStream;
 import org.reactfx.EventStream;
@@ -13,6 +14,7 @@
 
 import javafx.beans.binding.BooleanBinding;
 import javafx.beans.property.BooleanProperty;
+import javafx.beans.property.SimpleBooleanProperty;
 import javafx.beans.value.ObservableBooleanValue;
 
 class PausableSuccessionStream<O> extends EventStreamBase<O> implements AwaitingEventStream<O> {
@@ -24,6 +26,7 @@ class PausableSuccessionStream<O> extends EventStreamBase<O> implements Awaiting
     private boolean hasEvent = false;
     private BooleanBinding pending = null;
     private BooleanProperty successionOff;
+    private Predicate<O> successionOffCond;
     private O event = null;
 
     /**
@@ -37,26 +40,39 @@ class PausableSuccessionStream<O> extends EventStreamBase<O> implements Awaiting
      * the returned stream are used from the JavaFX application thread.</p>
      *
      * @param timeout the maximum time difference between two subsequent events
-     * in a <em>close</em> succession.
+     * in <em>close</em> succession.
      * @param realTime when true immediately emits the next event and sets
      * realTime back to <em>false</em>.
      */
     public PausableSuccessionStream( EventStream<O> input, Duration timeout, BooleanProperty realTime )
     {
-        this( input, (Function<? super O, ? extends O>) Function.identity(), (a,b) -> b, timeout, realTime );
+        this( input, (Function<? super O, ? extends O>) Function.identity(), (a,b) -> b, timeout, realTime, a -> realTime.get() );
     }
 
-    public PausableSuccessionStream(
+    /**
+    * @param timeout the maximum time difference between two subsequent events
+    * in <em>close</em> succession.
+    * @param condition when true immediately emits the event, otherwise
+    * waits for <em>timeout</em> before emitting the last received event.
+    */
+    public PausableSuccessionStream( EventStream<O> input, Duration timeout, Predicate<O> condition )
+    {
+        this( input, (Function<? super O, ? extends O>) Function.identity(), (a,b) -> b, timeout, new SimpleBooleanProperty(), condition );
+    }
+
+    private PausableSuccessionStream(
             EventStream<O> input,
             Function<? super O, ? extends O> initial,
             BiFunction<? super O, ? super O, ? extends O> reduction,
             Duration timeout,
-            BooleanProperty realTime) {
+            BooleanProperty realTime,
+            Predicate<O> condition) {
 
         this.input = input;
         this.initial = initial;
         this.reduction = reduction;
         this.successionOff = realTime;
+        this.successionOffCond = condition;
 
         this.timer = FxTimer.create(timeout, this::handleTimeout);
     }
@@ -85,9 +101,9 @@ protected final Subscription observeInputs() {
     }
 
     private void handleEvent(O i) {
-        if(successionOff.get())
+    	timer.stop();
+        if(successionOffCond.test(i))
         {
-            timer.stop();
             hasEvent = false;
             event = null;
             emit(i);
diff --git a/src/main/java/org/fxmisc/flowless/SizeTracker.java b/src/main/java/org/fxmisc/flowless/SizeTracker.java
@@ -3,6 +3,7 @@
 import java.time.Duration;
 import java.util.Optional;
 import java.util.function.Function;
+import java.util.function.Supplier;
 
 import javafx.beans.property.SimpleBooleanProperty;
 import javafx.beans.value.ObservableObjectValue;
@@ -63,8 +64,8 @@ public SizeTracker(
 
         this.maxKnownMinBreadth = Val.create(
                 () -> knownBreadths.stream().mapToDouble( Double::doubleValue ).max().orElse(0.0),
-                knownBreadths.changes().filter( c -> knownBreadths.size() > 0 )
-                // skip zero cell events to prevent scrollbar flicker 
+                // skips spurious events resulting from cell replacement (delete then add again)
+                knownBreadths.changes().successionEnds( Duration.ofMillis( 15 ) )
         );
 
         this.breadthForCells = Val.combine(
@@ -77,29 +78,47 @@ public SizeTracker(
                 .map(breadth -> cell -> orientation.prefLength(cell, breadth));
 
         this.lengths = cells.mapDynamic(lengthFn).memoize();
-
         LiveList<Double> knownLengths = this.lengths.memoizedItems();
 
-        this.averageLengthEstimate = Val.create(
-                () -> {
-                    // make sure to use pref lengths of all present cells
-                    for(int i = 0; i < cells.getMemoizedCount(); ++i) {
-                        int j = cells.indexOfMemoizedItem(i);
-                        lengths.force(j, j + 1);
-                    }
-
-                    return knownLengths.stream()
-                        .mapToDouble( Double::doubleValue )
-                        .average().orElse(0.0);
-                },
-                knownLengths.changes().filter( c -> knownLengths.size() > 0 )
-                // skip zero cell events to prevent scrollbar flicker 
+        Supplier<Double> averageKnownLengths = () -> {
+            // make sure to use pref lengths of all present cells
+            for(int i = 0; i < cells.getMemoizedCount(); ++i) {
+                int j = cells.indexOfMemoizedItem(i);
+                lengths.force(j, j + 1);
+            }
+
+            return knownLengths.stream()
+                .mapToDouble( Double::doubleValue )
+                .sorted().average()
+                .orElse( 0.0 );
+        };
+
+        final int AVERAGE_LENGTH = 0, TOTAL_LENGTH = 1;
+        Val<double[/*average,total*/]> lengthStats = Val.wrap(
+                knownLengths.changes().or( cells.sizeProperty().values() )
+                .successionEnds( Duration.ofMillis( 15 ) ) // reduce noise
+                .map( e -> {
+                    double averageLength = averageKnownLengths.get();
+                    int cellCount = e.isRight() ? e.getRight() : cells.size();
+                    return new double[] { averageLength, cellCount * averageLength };
+                } ).toBinding( new double[] { 0.0, 0.0 } )
         );
 
-        this.totalLengthEstimate = Val.combine(
-                averageLengthEstimate,
-                cells.sizeProperty().filter( s -> s > 0 ),
-                (avg, n) -> n * avg);
+        EventStream<double[/*average,total*/]> filteredLengthStats;
+        // briefly hold back changes that may be from spurious events coming from cell refreshes, these
+        // are identified as those where the estimated total length is less than the previous event.
+        filteredLengthStats = new PausableSuccessionStream<>( lengthStats.changes(), Duration.ofMillis(1000), chg -> {
+                double[/*average,total*/] oldStats = chg.getOldValue();
+                double[/*average,total*/] newStats = chg.getNewValue();
+                if ( newStats[TOTAL_LENGTH] < oldStats[TOTAL_LENGTH] ) {
+                    return false; // don't emit yet, first wait & prefer newer values
+                }
+                return true;
+        } )
+        .map( chg -> chg.getNewValue() );
+
+        this.averageLengthEstimate = Val.wrap( filteredLengthStats.map( stats -> stats[AVERAGE_LENGTH] ).toBinding( 0.0 ) );
+        this.totalLengthEstimate = Val.wrap( filteredLengthStats.map( stats -> stats[TOTAL_LENGTH] ).toBinding( 0.0 ) );
 
         Val<Integer> firstVisibleIndex = Val.create(
                 () -> cells.getMemoizedCount() == 0 ? null : cells.indexOfMemoizedItem(0),
diff --git a/src/test/java/org/fxmisc/flowless/VirtualFlowTest.java b/src/test/java/org/fxmisc/flowless/VirtualFlowTest.java
@@ -5,6 +5,7 @@
 import static org.junit.Assert.assertTrue;
 
 import org.junit.Test;
+import org.testfx.util.WaitForAsyncUtils;
 
 import javafx.collections.FXCollections;
 import javafx.collections.ObservableList;
@@ -21,9 +22,9 @@ public void idempotentShowTest() {
         vf.resize(100, 100); // size of VirtualFlow less than that of the cell
         vf.layout();
 
-        vf.show(110.0);
         vf.show(110.0);
         vf.layout();
+
         assertEquals(-10.0, rect.getBoundsInParent().getMinY(), 0.01);
     }
 
@@ -36,9 +37,10 @@ public void idempotentSetLengthOffsetTest() {
         vf.resize(100, 100); // size of VirtualFlow less than that of the cell
         vf.layout();
 
-        vf.setLengthOffset(10.0);
-        vf.setLengthOffset(10.0);
+        WaitForAsyncUtils.waitForFxEvents();
+        vf.setLengthOffset( 10 );
         vf.layout();
+
         assertEquals(-10.0, rect.getBoundsInParent().getMinY(), 0.01);
     }
 
@@ -65,12 +67,14 @@ public void fastVisibleIndexTest() {
         vf.layout();
         assertSame(visibleCells.get(0), vf.getCell(vf.getFirstVisibleIndex()));
         assertSame(visibleCells.get(visibleCells.size() - 1), vf.getCell(vf.getLastVisibleIndex()));
-        assertTrue(vf.getFirstVisibleIndex() <= 50 && 50 <= vf.getLastVisibleIndex());
+        assertTrue(vf.getFirstVisibleIndex() > 40 && vf.getFirstVisibleIndex() <= 50);
+        assertTrue(vf.getLastVisibleIndex() >= 50);
 
         vf.show(99);
         vf.layout();
         assertSame(visibleCells.get(0), vf.getCell(vf.getFirstVisibleIndex()));
         assertSame(visibleCells.get(visibleCells.size() - 1), vf.getCell(vf.getLastVisibleIndex()));
-        assertTrue(vf.getFirstVisibleIndex() <= 99 && 99 <= vf.getLastVisibleIndex());
+        assertTrue(vf.getFirstVisibleIndex() > 50 && vf.getFirstVisibleIndex() <= 99);
+        assertTrue(vf.getLastVisibleIndex() >= 99);
     }
 }
