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

[Johan Vos]

On Fri, 9 Apr 2021 22:12:54 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Johan Vos has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Process reviewer comments
>
> 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...

changed

> 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 `*`

changed

> 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`

changed

> 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 `/`

changed

> 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);`?

That won't add enough elements. For example, in case idx = 10, and the itemSizeCache has 5 elements, we need to add 5 empty elements to the cache, as they might get queried/pushed, e.g. we might do
`itemSizeCache.set(7, 123.45);`

> 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 `=`

changed

> 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

changed

> 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 `=`

changed

> 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)

changed

> 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 `)`

changed

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

changed

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

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