On Mon, 11 Jul 2022 09:11:58 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:
>
> Add link to issue about ignored test
> Reverse expected and actual args in some tests.
I've retested this version of the PR, and I can confirm, that it fixes the bug
in the real-world application! So no regression happened when moving to the new
PR! So great work!
Not sure whether the first two tests are still necessary.
The tests I wrote, basically used these test as a template and tried to
generalize/parametrize it. But I guess more tests don't do any harm.
There are 2 issues, we shouldn't forget about **after** this PR:
1.) In my original tests I had a snippet which is removed in this version. I
still think the test is correct but it fails. So we should investigate it.
// After clicking, the cells shouldn't move
listView.getSelectionModel().select(scrollToIndex);
listView.requestLayout();
Toolkit.getToolkit().firePulse();
assertEquals("Upper cell shouldn't move after changing heights and clicking",
previousLayoutY, scrollToCell.getLayoutY(), 1.);
2.)
In the previous [PR](https://github.com/openjdk/jfx/pull/712) we reported
issues when scrolling after jumping to an index - including a video. As
discussed with Johan we would like to investigate this further after this PR.
-------------
PR: https://git.openjdk.org/jfx/pull/808
