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

[Kevin Rushforth]

On Mon, 4 Jul 2022 13:52:40 GMT, Johan Vos <jvos at openjdk.org> wrote:

> In order to fix the issues reported in JDK-8089589, the VirtualFlow now uses the real sizes of the elements that are displayed. This allows for a more natural way of scrolling through lists that contain items of very different sizes.
> The algorithm that is used, does not calculate the size of each individual cell upfront, as that would be a major performance overhead (especially for large lists). Instead, we estimate the total size and the size of the individual cells based on the real measured size of a (growing) number of cells. 
> 
> There are a number of key variables that depend on this estimated size:
> * position
> * aboluteOffset
> * currentIndex
> 
> As a consequence, when the estimated size is changing, the absoluteOffset may change which may lead to a new currentIndex. If this happens during a layout phase, or a "complex" operation (e.g. scroll to an item and select it), this may lead to visually unexpected artifacts. While the rendering becomes more "correct" when the estimated size is improving, it is more important that we do not create visual confusion. 
> 
> The difficulty is that there are a large number of ways to manipulate the VirtualFlow, and different entry points may have different expectations about which variables should be kept constant (or only changed gradually).
> 
> In this PR, we focus on consistency in the scrollTo method, where we want to keep the top-cell constant. In case the estimated size is improved during the scrollTo execution, or the next layoutChildren invocation, this implies keeping the result of computeCurrentIndex() constant so that after scrolling to a cell and selecting it, the selected cell is indeed the one that is scrolled to.
> 
> This PR contains a number of tests for this scrollTo behaviour, that should also be considered as regression test in case we want to add more invariants later.

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

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.

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

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

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

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.

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.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 2197:

> 2195:             public void updateItem(Integer item, boolean empty) {
> 2196:                 super.updateItem(item, empty);
> 2197:                 if (!empty && (item!=null)) {

Minor: space around the `!=`

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 2212:

> 2210:             IndexedCell<Integer> cell = VirtualFlowTestUtils.getCell(listView, i);
> 2211:             if ((cell != null) && (cell.getItem() == 20)) {
> 2212:                 assertEquals("Last cell doesn't end at listview end", cell.getLayoutY(), viewportLength - 20,1.);

Expected and actual args are backwards.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 2216:

> 2214:             }
> 2215:             if ((cell != null) && (cell.getItem() == 70)) {
> 2216:                 assertEquals("Secondlast cell doesn't end properly", cell.getLayoutY(), viewportLength - 20 - 70,1.);

Expected and actual args are backwards.
Minor: space after the last comma.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 2224:

> 2222:         // resize cells and make sure they align after scrolling
> 2223:         ObservableList<Integer> list = FXCollections.observableArrayList();
> 2224:         list.addAll(300,300,20,21);

Expected and actual args are backwards.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 2233:

> 2231:             IndexedCell<Integer> cell = VirtualFlowTestUtils.getCell(listView, i);
> 2232:             if ((cell != null) && (cell.getItem() == 21)) {
> 2233:                 assertEquals("Last cell doesn't end at listview end", cell.getLayoutY(), viewportLength - 21,1.);

Expected and actual args are backwards.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 2237:

> 2235:             }
> 2236:             if ((cell != null) && (cell.getItem() == 20)) {
> 2237:                 assertEquals("Secondlast cell doesn't end properly", cell.getLayoutY(), viewportLength - 21 - 20,1.);

Expected and actual args are backwards.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 2265:

> 2263:         Toolkit.getToolkit().firePulse();
> 2264:         int cc = VirtualFlowTestUtils.getCellCount(listView);
> 2265:         assertEquals(cc, 15);

Expected and actual args are backwards.

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`

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`

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 2406:

> 2404:         if(sumOfHeights < viewportLength) {
> 2405:             // If we have less cells then space, then all cells are shown, and the position of the last cell, is the sum of the height of the previous cells.
> 2406:             assertEquals("Last cell should be at the bottom, if we scroll to it", lastCell.getLayoutY(), sumOfHeights - lastCellSize,1.);

Expected and actual args are backwards.

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.

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.

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

PR: https://git.openjdk.org/jfx/pull/808