On Fri, 9 Apr 2021 22:12:54 GMT, Kevin Rushforth <k...@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
