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

[Ajit Ghaisas]

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

Code changes are OK. I have noted a few minor comments.

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

> 1303:             // Otherwise, our goal is to leave the index of the cell at the
> 1304:             // top consistent, with the same translation etc.
> 1305:             if (position != 0 && position != 1 && (currentIndex >= cellCount)) {

Comparing a double for equality or inequality is not the best coding practice. Anyway, I see this pattern throughout this file. We can live with it for now.

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

> 656:      * laid out properly.
> 657:      */
> 658:     @Test

Only new line is added after @Test tag. There are a lot of changes of this type which I presume were done by the IDE. I think, we can revert these safely as not to cause unnecessary diffs.

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.

Typo : "whith" -> "with"

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

> 1280:         flow.scrollPixels(-50);
> 1281:         double neg = flow.getPosition();
> 1282:         assertFalse("Moving in negative direction should not decrease position", neg > pos);

"decrease" in log message should be "increase"

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

> 1292:         vf.scrollPixels(-50);
> 1293:         double neg = vf.getPosition();
> 1294:         assertFalse("Moving in negative direction should not decrease position", neg > pos);

"decrease" in log message should be "increase"

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

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