8298728: Cells in VirtualFlow jump after resizing

Reviewed-by: aghaisas, angorya

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java

@@ -1603,6 +1603,7 @@ public void scrollToTop(int index) {
      * @return the number of pixels actually moved
      */
     public double scrollPixels(final double delta) {
+        int oldIndex = computeCurrentIndex();
         // Short cut this method for cases where nothing should be done
         if (delta == 0) return 0;
 
@@ -1613,6 +1614,7 @@ public double scrollPixels(final double delta) {
         double pos = getPosition();
         if (pos == 0.0f && delta < 0) return 0;
         if (pos == 1.0f && delta > 0) return 0;
+        getCellSizesInExpectedViewport(oldIndex);
         recalculateEstimatedSize();
         double answer = adjustByPixelAmount(delta);
         if (pos == getPosition()) {
@@ -2313,23 +2315,33 @@ void setCellDirty(int index) {
     /**
      * Make sure the sizes of the cells that are likely to be visible are known.
      * When updates to the cell size estimates are occurring, we don't want the current
-     * visible content to be modified.
+     * visible content to be modified. The existing offset and index are respected.
      * @param index the index of the cell that should be positioned at the top of
      * the viewport in the next layout cycle.
      */
     void getCellSizesInExpectedViewport(int index) {
-        double estlength = getOrCreateCellSize(index);
+        double oldOffset = computeViewportOffset(getPosition());
+        int oldIndex = computeCurrentIndex();
+        double cellLength = getOrCreateCellSize(index);
+        if (index > 0) {
+            getOrCreateCellSize(index - 1);
+        }
+        if (index < getCellCount() - 1) {
+            getOrCreateCellSize(index + 1);
+        }
+        double estlength = cellLength;
         int i = index;
         while ((estlength < viewportLength) && (++i < getCellCount())) {
             estlength = estlength + getOrCreateCellSize(i);
         }
+        estlength = cellLength;
         if (estlength < viewportLength) {
             int j = index;
             while ((estlength < viewportLength) && (j-- > 0)) {
                 estlength = estlength + getOrCreateCellSize(j);
             }
         }
-        recalculateEstimatedSize();
+        recalculateAndImproveEstimatedSize(0, oldIndex, oldOffset);
     }
 
     private void startSBReleasedAnimation() {
@@ -2919,6 +2931,7 @@ private void adjustPositionToIndex(int index) {
         }
 
     }
+
     /**
      * Adjust the position based on a delta of pixels. If negative, then the
      * position will be adjusted negatively. If positive, then the position will
@@ -3055,20 +3068,31 @@ private double getOrCreateCellSize (int idx, boolean create) {
     /**
      * Update the size of a specific cell.
      * If this cell was already in the cache, its old value is replaced by the
-     * new size.
-     * @param cell
+     * new size. The total size of the flow will be recalculated, respecting the
+     * current index and offset.
+     * If the specific cell is the "current" cell (which is the first cell that is
+     * at least partially visible), the offset used for the viewport needs to be
+     * recalculated in case the new size is different from the cached size. This way,
+     * we keep the end of the current cell (and start of the cell at current + 1)
+     * constant. An exception to this is when the current cell starts at offset 0,
+     * in which case we keep the (0) offset as is.
+     * @param cell the cell which size has to be calculated
      */
     void updateCellSize(T cell) {
         int cellIndex = cell.getIndex();
+        int currentIndex = computeCurrentIndex();
+        double oldOffset = computeViewportOffset(getPosition());
+
+
         if (itemSizeCache.size() > cellIndex) {
-        if (isVertical()) {
-            double newh = cell.getLayoutBounds().getHeight();
-            itemSizeCache.set(cellIndex, newh);
-          } else {
-            double newh = cell.getLayoutBounds().getWidth();
-            itemSizeCache.set(cellIndex, newh);
-          }
+            Double oldSize = itemSizeCache.get(cellIndex);
+            double newSize = isVertical() ? cell.getLayoutBounds().getHeight() : cell.getLayoutBounds().getWidth();
+            itemSizeCache.set(cellIndex, newSize);
+            if ((cellIndex == currentIndex) && (oldSize != null) && (oldOffset != 0)) {
+                oldOffset = oldOffset + newSize - oldSize;
+            }
         }
+        recalculateAndImproveEstimatedSize(0, currentIndex, oldOffset);
     }
 
     /**
@@ -3082,15 +3106,24 @@ private void recalculateEstimatedSize() {
     private boolean recalculating = false;
 
     private void recalculateAndImproveEstimatedSize(int improve) {
+        recalculateAndImproveEstimatedSize(improve, -1, computeViewportOffset(getPosition()));
+    }
+
+    /**
+     * Recalculate the estimated size. If an oldIndex different from  -1 is supplied, that value will
+     * be respected:
+     * at the end of this calculation, we make sure that if the current index is calculated, it will
+     * be the same as the old index. If the oldIndex is -1, there is no guarantee about the new index.
+     */
+    private void recalculateAndImproveEstimatedSize(int improve, int oldIndex, double oldOffset) {
         if (recalculating) return;
         recalculating = true;
         try {
             int itemCount = getCellCount();
             int cacheCount = itemSizeCache.size();
             boolean keepRatio = ((cacheCount > 0) && !Double.isInfinite(this.absoluteOffset));
 
-            int oldIndex = computeCurrentIndex();
-            double oldOffset = computeViewportOffset(getPosition());
+            if (oldIndex < 0) oldIndex = computeCurrentIndex();
             int added = 0;
             while ((itemCount > itemSizeCache.size()) && (added < improve)) {
                 getOrCreateCellSize(itemSizeCache.size());

modules/javafx.controls/src/shims/java/javafx/scene/control/skin/VirtualFlowShim.java

@@ -58,6 +58,11 @@ public double getCellPosition(T cell) {
         return super.getCellPosition(cell);
     }
 
+    @Override
+    public void setCellDirty(int idx) {
+        super.setCellDirty(idx);
+    }
+
     @Override
     public void recreateCells() {
         super.recreateCells();

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java

@@ -66,6 +66,7 @@
 import javafx.scene.control.cell.CheckBoxListCell;
 import javafx.scene.control.cell.ComboBoxListCell;
 import javafx.scene.control.cell.TextFieldListCell;
+import javafx.scene.control.skin.VirtualFlow;
 import javafx.scene.image.ImageView;
 import javafx.scene.input.KeyCode;
 import javafx.scene.layout.VBox;
@@ -1166,7 +1167,7 @@ public void updateItem(String color, boolean empty) {
                                 listView.scrollTo(55);
                                 Platform.runLater(() -> {
                                     Toolkit.getToolkit().firePulse();
-                                    assertEquals(useFixedCellSize ? 17 : 71, rt_35395_counter);
+                                    assertEquals(useFixedCellSize ? 17 : 101, rt_35395_counter);
                                     sl.dispose();
                                 });
                             });
@@ -2510,6 +2511,9 @@ public static void verifyListViewScrollTo(ListView listView, int listViewHeight,
             listView.requestLayout();
             Toolkit.getToolkit().firePulse();
             assertEquals("Upper cell shouldn't move after changing heights", previousLayoutY, scrollToCell.getLayoutY(), 1.);
+            VirtualFlow vf = VirtualFlowTestUtils.getVirtualFlow(listView);
+            vf.scrollPixels(-1);
+            assertEquals("Upper cell should move 1 pixels, after scrolling 1 pixel", previousLayoutY + 1, scrollToCell.getLayoutY(), 1.);
         }
 
     }

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java

@@ -1424,6 +1424,73 @@ protected double computeMaxHeight(double width) {
         scene.setRoot(flow);
         assertEquals(flow.shim_getHbar().getValue(), flow.get_clipView_getX(), 0);
     }
+
+    @Test public void testChangingCellSize() {
+        int[] heights = {100, 100, 100, 100, 100, 100, 100, 100, 100};
+        VirtualFlowShim<IndexedCell> flow = new VirtualFlowShim();
+        flow.setVertical(true);
+        flow.setCellFactory(p -> new CellStub(flow) {
+            @Override public void updateIndex(int i) {
+                super.updateIndex(i);
+                if ((i > -1) &&(i < heights.length)){
+                    this.setPrefHeight(heights[i]);
+                }
+            }
+           @Override public void updateItem(Object ic, boolean empty) {
+               super.updateItem(ic, empty);
+               if (ic instanceof Integer) {
+                   Integer idx = (Integer)ic;
+                   if (idx > -1) {
+                       this.setMinHeight(heights[idx]);
+                       this.setPrefHeight(heights[idx]);
+                   }
+               }
+            }
+        });
+        flow.setCellCount(heights.length);
+        flow.setViewportLength(400);
+        flow.resize(400, 400);
+        flow.layout();
+IndexedCell firstCell = VirtualFlowShim.cells_getFirst(flow.cells);
+        // Before scrolling, top-cell must have index 0
+assertEquals(0, firstCell.getIndex());
+        // We now scroll to item with index 3
+        flow.scrollToTop(3);
+        flow.layout();
+        firstCell = VirtualFlowShim.cells_getFirst(flow.cells);
+        // After scrolling, top-cell must have index 3
+        // index(pixel);
+        // 3 (0); 4 (100); 5 (200); 6 (300)
+        assertEquals(3, firstCell.getIndex());
+        IndexedCell thirdCell = VirtualFlowShim.cells_get(flow.cells, 3);
+        double l3y = thirdCell.getLayoutY();
+        // the third visible cell must be at 3 x 100 = 300
+        assertEquals(l3y, 300, 0.1);
+        assertEquals(6, thirdCell.getIndex());
+        assertEquals(300, thirdCell.getLayoutY(), 1.);
+
+
+        for (int i = 0 ; i < heights.length; i++) {
+            heights[i] = 220;
+            flow.setCellDirty(i);
+        }
+        flow.setCellCount(heights.length);
+        flow.layout();
+        firstCell = VirtualFlowShim.cells_get(flow.cells, 0);
+        // After resizing, top-cell must still have index 3
+        assertEquals(3, firstCell.getIndex());
+        assertEquals(0, firstCell.getLayoutY(),1);
+        IndexedCell secondCell = VirtualFlowShim.cells_get(flow.cells, 1);
+        assertEquals(4, secondCell.getIndex());
+        assertEquals(220, secondCell.getLayoutY(),1);
+        // And now scroll down 10 pixels
+        flow.scrollPixels(10);
+        flow.layout();
+        firstCell = VirtualFlowShim.cells_get(flow.cells, 0);
+        // After resizing, top-cell must still have index 3
+        assertEquals(3, firstCell.getIndex());
+        assertEquals(-10, firstCell.getLayoutY(),1);
+    }
 }
 
 class GraphicalCellStub extends IndexedCellShim<Node> {
@@ -1486,13 +1553,13 @@ public String toString() {
 
 class CellStub extends IndexedCellShim {
     String s;
-   // VirtualFlowShim flow;
+    VirtualFlowShim flow;
 
     public CellStub(VirtualFlowShim flow) { init(flow); }
     public CellStub(VirtualFlowShim flow, String s) { init(flow); this.s = s; }
 
     private void init(VirtualFlowShim flow) {
-     //   this.flow = flow;
+        this.flow = flow;
         setSkin(new SkinStub<>(this));
         updateItem(this, false);
     }
@@ -1502,6 +1569,6 @@ public void updateIndex(int i) {
         super.updateIndex(i);
 
         s = "Item " + getIndex();
-//        updateItem(getIndex(), getIndex() >= flow.getCellCount());
+        updateItem(getIndex(), getIndex() >= flow.getCellCount());
     }
 }