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

[Florian Kirmaier]

On Mon, 23 May 2022 20:44:47 GMT, Johan Vos <jvos at openjdk.org> wrote:

>> When the size of a ListCell is changed and a scrollTo method is invoked without having a layout calculation in between, the old (wrong) size is used to calculcate the total estimate. This happens e.g. when the size is changed in the `updateItem` method.
>> This PR will immediately resize the cell and sets the new value in the cache containing the cellsizes.
>
> Johan Vos has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Try to keep visible cells at their original position when improving the estimate.
>   This reduces annoying effects when re-layouting cells without relevant changes.
>   
>   This should not be done when we don't have a valid estimate yet/anymore,
>   or when the absoluteOffset is set to infinity (which may happen if the
>   position is set to infinity, which is the case in some calls in Skin classes).

Thank you for the update!

We've tested it, and we made good progress.
In our Application, we often resize the Cells, after the content is loaded.
We've polished the unit test for the following 3 cases. 
(the changes are in this comment)

**1.) Not jumping to the last cell.**
Sometimes, when the last cell is very big, the ListView jumps to one cell above it.
This was supposed to be caught by the previous tests, but due to a bug in the tests, it was missed.
With this new definition of "shouldScrollToBottom" the bug is caught by the unit test:

boolean isLastIndex = scrollToIndex == heights.length - 1;
boolean shouldScrollToBottom = (sizeBelow < viewportLength) || (isLastIndex && lastCellSize >= viewportLength);



**2.) Jumps on height changes**
When the heights of the cells changes, "everything jumps around".
Would it be possible to have the invariant, that the topmost visible element shouldn't move? 
Or alternatively: The selected element should be stable if it is visible.
If this would be implementable, this would make the VirtualFlow behave much more "calm".

**3.) Jumps on click after height changes**
When the heights have changed, and an element is selected, then the position jumps around again.
This is clearly a bug, because selecting a visible element shouldn't have the effect that cells "fly around".
If they should move due to the height changes, this should happen when the height is changed, not when a cell is selected.

For 2 and 3, I have some test-code, which can be added to the end of the method "verifyListViewScrollTo":


    // Additional Tests:
    double previousLayoutY = scrollToCell.getLayoutY();
    System.out.println("previousLayoutY: " + previousLayoutY);
    if(previousLayoutY == 0) {
        System.out.println("ADDITIONAL CHECKS");
        // Upper cell shouldn't move after heights are changed
        List<Integer> alternateHeights = Arrays.stream(heights).map(x -> x + 250).collect(Collectors.toList());
        listView.getItems().setAll(alternateHeights);
        listView.requestLayout();
        Toolkit.getToolkit().firePulse();
        assertEquals("Upper cell shouldn't move after changing heights", previousLayoutY, scrollToCell.getLayoutY(), 1.);

        // 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.);
    }

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

PR: https://git.openjdk.java.net/jfx/pull/712