On Wed, 6 Jul 2022 11:40:40 GMT, Kevin Rushforth <[email protected]> wrote:
> The fix looks good to me, although I did leave a question about one of the
> boundary checks. This will need additional testing. Several of the
> `assertEquals` calls have the expected and actual args backwards. I also left
> a few minor formatting comments (I didn't note all of them).
I did a reformat on the sections that you mentioned. Doing a reformat with
NetBeans on the whole file would cause too many changes that would hide the
real changes, so that is probably a bit too much?
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
> line 2321:
>
>> 2319: */
>> 2320: void getCellSizesInExpectedViewport(int index) {
>> 2321: double estlength = getOrCreateCellSize(index);
>
> Minor: indentation.
done
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
> line 2323:
>
>> 2321: double estlength = getOrCreateCellSize(index);
>> 2322: int i = index;
>> 2323: while ((estlength < viewportLength) && (++i < getCellCount())){
>
> Minor: need space before `{`
done
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
> line 2328:
>
>> 2326: if (estlength < viewportLength) {
>> 2327: int j = index;
>> 2328: while ((estlength < viewportLength) && (--j > 0)) {
>
> Is the `> 0` intentional or should it be `>= 0`?
it should also allow cases where j == 0 indeed. Fixed by replacing `--j` to
`j--`
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
> line 3098:
>
>> 3096: estSize = estimatedSize / itemCount;
>> 3097:
>> 3098: if (keepRatio ) {
>
> Minor: there is an extra space before the `)`
done
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewKeyInputTest.java
> line 1837:
>
>> 1835: }
>> 1836:
>> 1837: @Ignore // there is no guarantee that there will be 8 selected
>> items (can be 7 as well)
>
> Ideally we would have an open bug ID for an `@Ignore`d test. In this case, it
> might or might not be worth filing one to fix the test.
I filed https://bugs.openjdk.org/browse/JDK-8289909 for this.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
> line 2189:
>
>> 2187:
>> 2188: @Test public void testUnfixedCellScrollResize() {
>> 2189: final ObservableList<Integer> items =
>> FXCollections.observableArrayList(300,300,70,20);
>
> Minor: add spaces after the commas.
done
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
> line 2374:
>
>> 2372: double viewportLength = listViewHeight - 2; // it would be
>> better to calculate this from listView but there is no API for this
>> 2373:
>> 2374: for(int height: heights) {
>
> Minor: space after `for`
done
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
> line 2400:
>
>> 2398: assertTrue("Our cell must be visible!",
>> scrollToCell.isVisible());
>> 2399:
>> 2400: if(lastCell.isVisible() && sumOfHeights >= viewportLength) {
>
> Minor: space after `if`
done
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
> line 2410:
>
>> 2408: if(shouldScrollToBottom && sumOfHeights > viewportLength) {
>> 2409: // If we scroll to the bottom, then the last cell should
>> be exactly at the bottom
>> 2410: // assertEquals("", lastCell.getLayoutY(), viewportLength
>> - lastCellSize,1.);
>
> Expected and actual args are backwards.
this particular assert was commented out, and I removed it. I reverted the args
in the previous one though.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
> line 2414:
>
>> 2412: if(!shouldScrollToBottom && sumOfHeights > viewportLength) {
>> 2413: // If we don't scroll to the bottom, and the cells are
>> bigger than the available space, then our cell should be at the top.
>> 2414: assertEquals("Our cell mut be at the top",
>> scrollToCell.getLayoutY(), 0,1.);
>
> Expected and actual args are backwards.
done
-------------
PR: https://git.openjdk.org/jfx/pull/808
