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

[Kevin Rushforth]

On Mon, 12 Apr 2021 09:41:03 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.
>
> Johan Vos has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Process reviewer comments

Thanks for the explanations about the algorithm. I have no more questions on the logic.

I think Ajit will review this later this week.

Btw, I did another pass over the code and noted a few more formatting inconsistencies (missing spaces).

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

> 1043:             setPosition(0.);
> 1044:         } else {
> 1045:             setPosition(absoluteOffset/(estimatedSize - viewportLength));

Minor: spacing

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

> 1589:             adjustPositionToIndex(index);
> 1590: //            double offset = - computeOffsetForCell(index);
> 1591: //            adjustByPixelAmount(offset);

I forgot to ask about this last time. Is there a reason to leave the former, now commented-out, code here, as opposed to just removing it?

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

> 1901:     void setViewportLength(double value) {
> 1902:         this.viewportLength = value;
> 1903:         this.absoluteOffset = getPosition() * (estimatedSize -viewportLength);

Minor: spacing

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

> 2562:                 lengthBar.setVisibleAmount(flowLength / sumCellLength);
> 2563:             } else {
> 2564:                 lengthBar.setVisibleAmount(viewportLength/estimatedSize);

Minor: spacing

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

> 2832:         double p = com.sun.javafx.util.Utils.clamp(0, position, 1);
> 2833:         double bound = 0d;
> 2834:         double estSize = estimatedSize/getCellCount();

Minor: spacing

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

> 2837:             double h = getCellSize(i);
> 2838:             if (h < 0) h = estSize;
> 2839:             if (bound +h > absoluteOffset) {

Minor: spacing

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

> 2838:             if (h < 0) h = estSize;
> 2839:             if (bound +h > absoluteOffset) {
> 2840:                 return absoluteOffset-bound;

Minor: spacing

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

> 2852:             double targetOffset = 0;
> 2853:             double estSize = estimatedSize/cellCount;
> 2854:             for (int i = 0; i < index;i++) {

Minor: spacing

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

> 2879:         double origAbsoluteOffset = this.absoluteOffset;
> 2880:         this.absoluteOffset = Math.max(0.d, this.absoluteOffset + numPixels);
> 2881:         double newPosition = Math.min(1.0d, absoluteOffset/(estimatedSize - viewportLength));

Minor: spacing

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

> 3033:             }
> 3034:         }
> 3035:         this.estimatedSize = cnt == 0 ? 1d: tot * itemCount/cnt;

Minor: spacing

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

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