On Mon, 23 May 2022 20:44:47 GMT, Johan Vos <j...@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
