On Mon, 4 Jul 2022 13:52:40 GMT, Johan Vos <j...@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.
The fix looks good to me, although I did leave a question about one of the
boundary checks. This will need additional testing. Several of the
`assertEquals` calls have the expected and actual args backwards. I also left a
few minor formatting comments (I didn't note all of them).
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 2321:
> 2319: */
> 2320: void getCellSizesInExpectedViewport(int index) {
> 2321: double estlength = getOrCreateCellSize(index);
Minor: indentation.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 2323:
> 2321: double estlength = getOrCreateCellSize(index);
> 2322: int i = index;
> 2323: while ((estlength < viewportLength) && (++i < getCellCount())){
Minor: need space before `{`
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 2328:
> 2326: if (estlength < viewportLength) {
> 2327: int j = index;
> 2328: while ((estlength < viewportLength) && (--j > 0)) {
Is the `> 0` intentional or should it be `>= 0`?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 3098:
> 3096: estSize = estimatedSize / itemCount;
> 3097:
> 3098: if (keepRatio ) {
Minor: there is an extra space before the `)`
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewKeyInputTest.java
line 1837:
> 1835: }
> 1836:
> 1837: @Ignore // there is no guarantee that there will be 8 selected
> items (can be 7 as well)
Ideally we would have an open bug ID for an `@Ignore`d test. In this case, it
might or might not be worth filing one to fix the test.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2189:
> 2187:
> 2188: @Test public void testUnfixedCellScrollResize() {
> 2189: final ObservableList<Integer> items =
> FXCollections.observableArrayList(300,300,70,20);
Minor: add spaces after the commas.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2197:
> 2195: public void updateItem(Integer item, boolean empty) {
> 2196: super.updateItem(item, empty);
> 2197: if (!empty && (item!=null)) {
Minor: space around the `!=`
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2212:
> 2210: IndexedCell<Integer> cell =
> VirtualFlowTestUtils.getCell(listView, i);
> 2211: if ((cell != null) && (cell.getItem() == 20)) {
> 2212: assertEquals("Last cell doesn't end at listview end",
> cell.getLayoutY(), viewportLength - 20,1.);
Expected and actual args are backwards.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2216:
> 2214: }
> 2215: if ((cell != null) && (cell.getItem() == 70)) {
> 2216: assertEquals("Secondlast cell doesn't end properly",
> cell.getLayoutY(), viewportLength - 20 - 70,1.);
Expected and actual args are backwards.
Minor: space after the last comma.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2224:
> 2222: // resize cells and make sure they align after scrolling
> 2223: ObservableList<Integer> list =
> FXCollections.observableArrayList();
> 2224: list.addAll(300,300,20,21);
Expected and actual args are backwards.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2233:
> 2231: IndexedCell<Integer> cell =
> VirtualFlowTestUtils.getCell(listView, i);
> 2232: if ((cell != null) && (cell.getItem() == 21)) {
> 2233: 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 2237:
> 2235: }
> 2236: if ((cell != null) && (cell.getItem() == 20)) {
> 2237: assertEquals("Secondlast cell doesn't end properly",
> cell.getLayoutY(), viewportLength - 21 - 20,1.);
Expected and actual args are backwards.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2265:
> 2263: Toolkit.getToolkit().firePulse();
> 2264: int cc = VirtualFlowTestUtils.getCellCount(listView);
> 2265: assertEquals(cc, 15);
Expected and actual args are backwards.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2374:
> 2372: double viewportLength = listViewHeight - 2; // it would be
> better to calculate this from listView but there is no API for this
> 2373:
> 2374: for(int height: heights) {
Minor: space after `for`
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2400:
> 2398: assertTrue("Our cell must be visible!",
> scrollToCell.isVisible());
> 2399:
> 2400: if(lastCell.isVisible() && sumOfHeights >= viewportLength) {
Minor: space after `if`
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2406:
> 2404: if(sumOfHeights < viewportLength) {
> 2405: // If we have less cells then space, then all cells are
> shown, and the position of the last cell, is the sum of the height of the
> previous cells.
> 2406: assertEquals("Last cell should be at the bottom, if we
> scroll to it", lastCell.getLayoutY(), sumOfHeights - lastCellSize,1.);
Expected and actual args are backwards.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2410:
> 2408: if(shouldScrollToBottom && sumOfHeights > viewportLength) {
> 2409: // If we scroll to the bottom, then the last cell should be
> exactly at the bottom
> 2410: // assertEquals("", lastCell.getLayoutY(), viewportLength -
> lastCellSize,1.);
Expected and actual args are backwards.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2414:
> 2412: if(!shouldScrollToBottom && sumOfHeights > viewportLength) {
> 2413: // If we don't scroll to the bottom, and the cells are
> bigger than the available space, then our cell should be at the top.
> 2414: assertEquals("Our cell mut be at the top",
> scrollToCell.getLayoutY(), 0,1.);
Expected and actual args are backwards.
-------------
PR: https://git.openjdk.org/jfx/pull/808
