RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed

[Johan Vos]

On Wed, 6 Jul 2022 11:40:40 GMT, Kevin Rushforth <kcr at openjdk.org> 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