RFR: 8089589: [ListView] ScrollBar content moves toward-backward during scrolling.

[Kevin Rushforth]

On Tue, 9 Feb 2021 12:21:28 GMT, Johan Vos <jvos at openjdk.org> wrote:

> This PR introduces a refactory for VirtualFlow, fixing a number of issues reported about inconsistent scrolling speed (see https://bugs.openjdk.java.net/browse/JDK-8089589)
> The problem mentioned in the JBS issue (and in related issues) is that the VirtualFlow implementation depends on cell index and cell count, instead of on pixel count. The latter is unknown when the VirtualFlow is created, and pre-calculating the size of a large set of items would be very expensive.
> Therefore, this PR uses a combination of a gradual calculation of the total size in pixels (estimatedSize) and a smoothing part that prevents the scrollback to scroll in the reverse direction as the requested change.
> This PR currently breaks a number of tests that hard-coded depend on a number of evaluations. This is inherit to the approach of this PR: if we want to estimate the total size, we need to do some additional calculations. In this PR, I try to balance between consistent behavior and performance.

I reviewed the code, and tested this, both using automated tests and various manual tests, and it looks good to me. In addition to the simple test attached to the bug report, I can reproduce the bug with `HelloTableView` and the `HorizontalListView` sample of `Ensemble8`.

`HelloTableView`

1. Run `HelloTableview` program
2. Sort by first name
3. Slowly scroll down by dragging the thumb
BUG: the scrolling jitters slightly when you reach the taller row (Jacob Smith Smith Smith)

`Ensemble8`

1. Run `Ensemble8` and select `HorizontalListView` sample
2. Scroll all the way to the right
3. Slowly scroll to the left by dragging the thumb
BUG: the scrolling jitters slightly when you reach the wider row (Long Row 3)

All three test cases run fine with the fix.

I left a few inline questions and comments (mostly about code style w.r.t., inline whitespace, and a couple typos).

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 254:

> 252:      * The following relation should always be true:
> 253:      * 0 <= absoluteOffset <= (estimatedSize - viewportLength)
> 254:      * Based on this relation, he position p is defined as

Typo: `the` position p...

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 1034:

> 1032:      */
> 1033:     void adjustAbsoluteOffset() {
> 1034:         absoluteOffset = (estimatedSize - viewportLength)* getPosition();

Minor: missing space before the `*`

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2883:

> 2881:         double newPosition = Math.min(1.0d, absoluteOffset/(estimatedSize - viewportLength));
> 2882:         // estimatedSize changes may result in opposite effect on position
> 2883:         // in that case, modify current position 1% in the requested direction

Have you done testing to show that a 1% adjustment is a good heuristic?

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2892:

> 2890: 
> 2891:         // once at 95% of the total estimated size, we want a correct size, not
> 2892:         // an estimated size anymore.

I presume that this is a balance between performance and accuracy? It seems to work well. Have you tried other values?

Do you also need to check for `newPosition < 0.05`  if you are scrolling backwards?

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2901:

> 2899:         }
> 2900: 
> 2901:         // if we are at or beyond the edge, correct the absolteOffset

Typo: should be `absoluteOffset`

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2902:

> 2900: 
> 2901:         // if we are at or beyond the edge, correct the absolteOffset
> 2902:         if (newPosition >= 1.d) {

Do you need to also check for `newPosition < 0`  if you are scrolling backwards?

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2914:

> 2912:         double total = 0;
> 2913:         int currentCellCount = getCellCount();
> 2914:         double estSize = estimatedSize/currentCellCount;

MInor: missing spaces before and after `/`

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2972:

> 2970:         // Make sure we have enough space in the cache to store this index
> 2971:         while (idx >= itemSizeCache.size()) {
> 2972:             itemSizeCache.add(itemSizeCache.size(), null);

Can this be simplified to `itemSizeCache.add(null);`?

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java line 1229:

> 1227:     }
> 1228: 
> 1229:     private ArrayLinkedListShim<GraphicalCellStub> circlelist= new ArrayLinkedListShim<GraphicalCellStub>();

Minor: missing space before `=`

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java line 1233:

> 1231:     private VirtualFlowShim createCircleFlow() {
> 1232:             // The second VirtualFlow we are going to test, with 7 cells. Each cell
> 1233:     // contains a Circle whith a radius that varies between cells.

Minor: indentation

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java line 1305:

> 1303:         double s1 = s0;
> 1304:         double position = vf.getPosition();
> 1305:         double newPosition =0d;

Minor: missing space after `=`

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java line 1336:

> 1334:     static ArrayList<Circle> circleList = new ArrayList<>();
> 1335:     static {
> 1336:         circleList.add(new Circle(10));

Minor: Could use `List.of`? (just a suggestion)

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java line 1350:

> 1348:     public GraphicalCellStub() { init(); }
> 1349: 
> 1350:     private void init( ) {

Minor: can remove extra space between `(` and `)`

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java line 1381:

> 1379:         double answer = super.computePrefHeight(width);
> 1380:         if ((idx > -1) && (idx < circleList.size())){
> 1381:             answer = 2* circleList.get(idx).getRadius()+ 6;

Minor: spacing is a little inconsistent for these two lines.

-------------

PR: https://git.openjdk.java.net/jfx/pull/398