On Thu, 7 Jul 2022 11:12:48 GMT, Johan Vos <[email protected]> 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
