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

[Kevin Rushforth]

On Thu, 7 Jul 2022 11:12:48 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.
>
> Johan Vos has updated the pull request incrementally with one additional commit since the last revision:
> 
>   process reviewer comments

I noted a couple more `assertEquals` calls where it looks like the expected and actual are backwards. The rest looks good. I'll finish my testing tomorrow.

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

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

I think the expected and actual args are backwards here, too.

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

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

expected and actual args are backwards.

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

> 2232:             IndexedCell<Integer> cell = VirtualFlowTestUtils.getCell(listView, i);
> 2233:             if ((cell != null) && (cell.getItem() == 21)) {
> 2234:                 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 2238:

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

expected and actual args are backwards.

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

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