diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java index 789c457887a..02f4a3b723a 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java @@ -918,15 +918,13 @@ public final void setCellCount(int value) { @Override protected void invalidated() { super.invalidated(); + adjustAbsoluteOffset(); requestLayout(); } }; public final double getPosition() { return position.get(); } public final void setPosition(double value) { position.set(value); - // When the position is changed explicitly, we need to make sure - // the absolute offset is changed accordingly. - adjustAbsoluteOffset(); } public final DoubleProperty positionProperty() { return position; } @@ -1052,27 +1050,18 @@ void adjustPosition() { /** {@inheritDoc} */ @Override protected void layoutChildren() { - double origAbsoluteOffset = absoluteOffset; - recalculateEstimatedSize(); - // if the last modification to the position was done via scrollPixels, - // the absoluteOffset and position are already in sync. - // However, the position can be modified via different ways (e.g. by - // moving the scrollbar thumb), so we need to recalculate absoluteOffset - // There is an exception to this: if cells are added/removed, we want - // to keep the absoluteOffset constant, hence we need to adjust the position. - - if (lastCellCount != getCellCount()) { - absoluteOffset = origAbsoluteOffset; - adjustPosition(); - } else { - adjustAbsoluteOffset(); - } + // when we enter this method, the absoluteOffset and position are + // already determined. In case this method invokes other methods + // that may change either absoluteOffset or position, it is the + // responsability of those methods to make sure both parameters are + // changed in a consistent way. + // For example, the recalculateEstimatedSize method also recalculates + // the absoluteOffset and position. + if (needsRecreateCells) { lastWidth = -1; lastHeight = -1; releaseCell(accumCell); -// accumCell = null; -// accumCellParent.getChildren().clear(); sheet.getChildren().clear(); for (int i = 0, max = cells.size(); i < max; i++) { cells.get(i).updateIndex(-1); @@ -1334,6 +1323,7 @@ void adjustPosition() { } computeBarVisiblity(); + recalculateAndImproveEstimatedSize(0); recreatedOrRebuilt = recreatedOrRebuilt || rebuild; updateScrollBarsAndCells(recreatedOrRebuilt); @@ -1579,6 +1569,8 @@ private boolean tryScrollOneCell(int targetIndex, boolean downOrRight) { * @param index the index */ public void scrollToTop(int index) { + // make sure we have the real size of cells that are likely to be rendered + getCellSizesInExpectedViewport(index); boolean posSet = false; if (index > getCellCount() - 1) { @@ -1764,6 +1756,7 @@ public T getCell(int index) { // to a severe performance decrease. This seems to be OK, as // getCell() is only used for cell measurement purposes. // pile.remove(i); + resizeCell(cell); return cell; } } @@ -1901,8 +1894,12 @@ private final double getViewportBreadth() { */ private double viewportLength; void setViewportLength(double value) { + if (value == this.viewportLength) { + return; + } this.viewportLength = value; this.absoluteOffset = getPosition() * (estimatedSize - viewportLength); + recalculateEstimatedSize(); } double getViewportLength() { return viewportLength; @@ -1996,6 +1993,8 @@ protected void resizeCell(T cell) { double height = Math.max(getMaxPrefBreadth(), getViewportBreadth()); cell.resize(fixedCellSizeEnabled ? getFixedCellSize() : Utils.boundedSize(cell.prefWidth(height), cell.minWidth(height), cell.maxWidth(height)), height); } + // when a cell is resized, our estimate needs to be updated. + recalculateAndImproveEstimatedSize(0); } /** @@ -2311,6 +2310,28 @@ void setCellDirty(int index) { requestLayout(); } + /** + * 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. + * @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); + int i = index; + while ((estlength < viewportLength) && (++i < getCellCount())) { + estlength = estlength + getOrCreateCellSize(i); + } + if (estlength < viewportLength) { + int j = index; + while ((estlength < viewportLength) && (j-- > 0)) { + estlength = estlength + getOrCreateCellSize(j); + } + } + recalculateEstimatedSize(); + } + private void startSBReleasedAnimation() { if (sbTouchTimeline == null) { /* @@ -2442,6 +2463,26 @@ private void initViewport() { lengthBar.setVirtual(true); } + /** + * In case we are not rendering the first cell + * AND + * there is empty room after the last cell, + * the cells need to be shifted down to fill the empty area. + */ + private void shiftDown() { + T lastNonEmptyCell = getLastVisibleCell(); + T firstCell = cells.getFirst(); + int index = getCellIndex(firstCell); + double end = getCellPosition(lastNonEmptyCell) + getCellLength(lastNonEmptyCell); + double delta = viewportLength - end; + if ((index > 0) && (delta > 0)) { + for (int i = 0; i < cells.size(); i++) { + T cell = cells.get(i); + positionCell(cell, getCellPosition(cell) + delta); + } + } + } + private void updateScrollBarsAndCells(boolean recreate) { // Assign the hbar and vbar to the breadthBar and lengthBar so as // to make some subsequent calculations easier. @@ -2486,6 +2527,7 @@ private void updateScrollBarsAndCells(boolean recreate) { offset += getCellLength(cell); } + shiftDown(); } // Toggle visibility on the corner @@ -2848,6 +2890,9 @@ private double computeViewportOffset(double position) { } private void adjustPositionToIndex(int index) { + if (index > 0) getOrCreateCellSize(index-1); + getOrCreateCellSize(index); + recalculateEstimatedSize(); int cellCount = getCellCount(); if (cellCount <= 0) { setPosition(0.0f); @@ -2859,7 +2904,7 @@ private void adjustPositionToIndex(int index) { if (cz < 0) cz = estSize; targetOffset = targetOffset+ cz; } - this.absoluteOffset = targetOffset; + this.absoluteOffset = (estimatedSize < viewportLength) ? 0 : targetOffset; adjustPosition(); } @@ -2954,6 +2999,7 @@ private double computeOffsetForCell(int itemIndex) { } private double getOrCreateCellSize (int idx, boolean create) { + if (idx < 0) return -1; // is the current cache long enough to contain idx? if (itemSizeCache.size() > idx) { // is there a non-null value stored in the cache? @@ -2964,28 +3010,34 @@ private double getOrCreateCellSize (int idx, boolean create) { if (!create) return -1; boolean doRelease = false; - // Do we have a visible cell for this index? - T cell = getVisibleCell(idx); - if (cell == null) { // we might get the accumcell here - cell = getCell(idx); - doRelease = true; - } // Make sure we have enough space in the cache to store this index while (idx >= itemSizeCache.size()) { itemSizeCache.add(itemSizeCache.size(), null); } - // if we have a valid cell, we can populate the cache double answer = 1d; - if (isVertical()) { - answer = cell.getLayoutBounds().getHeight(); + if (getFixedCellSize() > 0) { + answer = getFixedCellSize(); + itemSizeCache.set(idx, answer); } else { - answer = cell.getLayoutBounds().getWidth(); - } - itemSizeCache.set(idx, answer); + // Do we have a visible cell for this index? + T cell = getVisibleCell(idx); + if (cell == null) { // we might get the accumcell here + cell = getCell(idx); + doRelease = true; + } - if (doRelease) { // we need to release the accumcell - releaseCell(cell); + // if we have a valid cell, we can populate the cache + if (isVertical()) { + answer = cell.getLayoutBounds().getHeight(); + } else { + answer = cell.getLayoutBounds().getWidth(); + } + itemSizeCache.set(idx, answer); + + if (doRelease) { // we need to release the accumcell + releaseCell(cell); + } } return answer; } @@ -3020,6 +3072,11 @@ private void recalculateEstimatedSize() { private void recalculateAndImproveEstimatedSize(int improve) { int itemCount = getCellCount(); int cacheCount = itemSizeCache.size(); + boolean keepRatio = ((cacheCount > 0) && !Double.isInfinite(this.absoluteOffset)); + double estSize = estimatedSize / itemCount; + + int oldIndex = computeCurrentIndex(); + double oldOffset = computeViewportOffset(getPosition()); int added = 0; while ((itemCount > itemSizeCache.size()) && (added < improve)) { getOrCreateCellSize(itemSizeCache.size()); @@ -3035,7 +3092,22 @@ private void recalculateAndImproveEstimatedSize(int improve) { cnt++; } } - this.estimatedSize = cnt == 0 ? 1d: tot * itemCount / cnt; + this.estimatedSize = cnt == 0 ? 1d : tot * itemCount / cnt; + estSize = estimatedSize / itemCount; + + if (keepRatio) { + double newOffset = 0; + for (int i = 0; i < oldIndex; i++) { + double h = getCellSize(i); + if (h < 0) { + h = estSize; + } + newOffset += h; + } + this.absoluteOffset = newOffset + oldOffset; + adjustPosition(); + } + } private void resetSizeEstimates() { diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewKeyInputTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewKeyInputTest.java index a8961998756..e7e1323d70c 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewKeyInputTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewKeyInputTest.java @@ -1834,6 +1834,7 @@ private boolean isAnchor(int index) { sl.dispose(); } + @Ignore("JDK-8289909") // there is no guarantee that there will be 8 selected items (can be 7 as well) @Test public void test_rt34407_up_up_down() { final int items = 100; listView.getItems().clear(); diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java index 7eabd95b965..4f5198927f9 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java @@ -40,6 +40,7 @@ import java.util.List; import java.util.ListIterator; import java.util.NoSuchElementException; +import java.util.stream.Collectors; import javafx.application.Platform; import javafx.beans.binding.Bindings; import javafx.beans.property.ObjectProperty; @@ -1135,12 +1136,12 @@ public void updateItem(String color, boolean empty) { listView.scrollTo(5); Platform.runLater(() -> { Toolkit.getToolkit().firePulse(); - assertEquals(5, rt_35395_counter); + assertTrue(rt_35395_counter < 30); rt_35395_counter = 0; listView.scrollTo(55); Platform.runLater(() -> { Toolkit.getToolkit().firePulse(); - assertEquals(useFixedCellSize ? 21 : 23, rt_35395_counter); + assertEquals(useFixedCellSize ? 17 : 71, rt_35395_counter); sl.dispose(); }); }); @@ -2183,4 +2184,243 @@ private void attemptGC(WeakReference weakRef, int n) { } } } + + @Test + public void testUnfixedCellScrollResize() { + final ObservableList items = FXCollections.observableArrayList(300, 300, 70, 20); + final ListView listView = new ListView(items); + listView.setPrefHeight(400); + double viewportLength = 398; // it would be better to calculate this from listView but there is no API for this + listView.setCellFactory(lv -> new ListCell() { + @Override + public void updateItem(Integer item, boolean empty) { + super.updateItem(item, empty); + if (!empty && (item != null)) { + this.setPrefHeight(item); + } + } + }); + StageLoader sl = new StageLoader(listView); + Toolkit.getToolkit().firePulse(); + listView.scrollTo(2); + Toolkit.getToolkit().firePulse(); + int cc = VirtualFlowTestUtils.getCellCount(listView); + boolean got70 = false; + boolean got20 = false; + for (int i = 0; i < cc; i++) { + IndexedCell cell = VirtualFlowTestUtils.getCell(listView, i); + if ((cell != null) && (cell.getItem() == 20)) { + assertEquals("Last cell doesn't end at listview end", viewportLength - 20, cell.getLayoutY(), 1.); + got20 = true; + } + if ((cell != null) && (cell.getItem() == 70)) { + assertEquals("Secondlast cell doesn't end properly", viewportLength - 20 - 70, cell.getLayoutY(), 1.); + got70 = true; + } + } + assertTrue(got20); + assertTrue(got70); + // resize cells and make sure they align after scrolling + ObservableList list = FXCollections.observableArrayList(); + list.addAll(300, 300, 20, 21); + listView.setItems(list); + listView.scrollTo(4); + Toolkit.getToolkit().firePulse(); + got20 = false; + boolean got21 = false; + for (int i = 0; i < cc; i++) { + IndexedCell cell = VirtualFlowTestUtils.getCell(listView, i); + if ((cell != null) && (cell.getItem() == 21)) { + assertEquals("Last cell doesn't end at listview end", viewportLength - 21, cell.getLayoutY(), 1.); + got21 = true; + } + if ((cell != null) && (cell.getItem() == 20)) { + assertEquals("Secondlast cell doesn't end properly", viewportLength - 21 - 20, cell.getLayoutY(), 1.); + got20 = true; + } + } + assertTrue(got20); + assertTrue(got21); + } + + @Test + public void testNoEmptyEnd() { + final ObservableList items = FXCollections.observableArrayList(200, 200, 200, 200, 200, 200, 200, 200, 20, 20, 20, 20, 20, 20, 20); + final ListView listView = new ListView(items); + listView.setPrefHeight(400); + double viewportLength = 398; + listView.setCellFactory(lv -> new ListCell() { + @Override + public void updateItem(Integer item, boolean empty) { + super.updateItem(item, empty); + if (!empty && (item != null)) { + this.setPrefHeight(item); + } + } + }); + StageLoader sl = new StageLoader(listView); + Toolkit.getToolkit().firePulse(); + listView.scrollTo(14); + Toolkit.getToolkit().firePulse(); + int cc = VirtualFlowTestUtils.getCellCount(listView); + assertEquals(15, cc); + boolean got70 = false; + for (int i = 0; i < cc; i++) { + IndexedCell cell = VirtualFlowTestUtils.getCell(listView, i); + int tens = Math.min(15 - i, 7); + int hundreds = Math.max(8 - i, 0); + double exp = 398 - 20 * tens - 200 * hundreds; + double real = cell.getLayoutY(); + if (cell.isVisible()) { + assertEquals(exp, real, 0.1); + } + } + } + + @Test + public void testMoreUnfixedCellScrollResize() { + + // Sanity Check - it has to work with cases, where all cells have the same sizes + testScrollTo(360, 3, new Integer[]{20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20}); + testScrollTo(360, 3, new Integer[]{20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20}); + testScrollTo(360, 1, new Integer[]{20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20}); + testScrollTo(360, -1, new Integer[]{20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20}); + + // With 100 it's wrong, when addIncremental is set. + testScrollTo(360, 3, new Integer[]{100, 100, 100, 100, 100, 100, 100, 100, 100}); + testScrollTo(360, -1, new Integer[]{100, 100, 100, 100, 100, 100, 100, 100, 100}); + + // More complicated tests + testScrollTo(360, 2, new Integer[]{300, 300, 70, 20}); + testScrollTo(400, 2, new Integer[]{200, 200, 200, 200, 200, 200, 200, 200, 20, 20, 20, 20, 20, 20, 20}); + testScrollTo(400, -1, new Integer[]{200, 200, 200, 200, 200, 200, 200, 200, 20, 20, 20, 20, 20, 20, 20}); + testScrollTo(400, 3, new Integer[]{200, 200, 200, 200, 200, 200, 200, 200, 20, 20, 20, 20, 20, 20, 20}); + testScrollTo(400, -1, new Integer[]{200, 200, 200, 200, 200, 200, 200, 200, 20, 20, 20, 20, 20, 20, 500, 20, 500, 20, 500}); + testScrollTo(400, -1, new Integer[]{200, 200, 200, 200, 200, 200, 200, 200, 20, 20, 20, 20, 20, 20, 400, 20, 400, 20, 400}); + testScrollTo(400, 2, new Integer[]{500, 500, 20, 20, 100, 100, 100, 100, 100, 100}); + testScrollTo(400, 8, new Integer[]{500, 500, 20, 20, 100, 100, 100, 100, 100, 100, 300, 300, 300, 300}); + + testScrollTo(400, 2, new Integer[]{300, 300, 20, 20}); + testScrollTo(400, 2, new Integer[]{300, 300, 20, 20, 200, 200}); + testScrollTo(400, 2, new Integer[]{20, 20, 20, 500, 500}); + + testScrollTo(400, 2, new Integer[]{200, 200, 200, 200, 200, 200, 200, 200, 20, 20, 20, 20, 20, 20, 20}); + testScrollTo(400, -1, new Integer[]{200, 200, 200, 200, 200, 200, 200, 200, 20, 20, 20, 20, 20, 20, 20}); + testScrollTo(400, 3, new Integer[]{200, 200, 200, 200, 200, 200, 200, 200, 20, 20, 20, 20, 20, 20, 20}); + testScrollTo(400, -1, new Integer[]{200, 200, 200, 200, 200, 200, 200, 200, 20, 20, 20, 20, 20, 20, 500, 20, 500, 20, 500}); + testScrollTo(400, -1, new Integer[]{200, 200, 200, 200, 200, 200, 200, 200, 20, 20, 20, 20, 20, 20, 400, 20, 400, 20, 400}); + testScrollTo(400, 2, new Integer[]{500, 500, 20, 20, 100, 100, 100, 100, 100, 100}); + testScrollTo(400, 2, new Integer[]{500, 500, 500, 500, 500}); + + } + + public void testScrollTo(int listViewHeight, int scrollToIndex, Integer[] heights) { + if (scrollToIndex == -1) { + scrollToIndex = heights.length - 1; + } + testScrollTo(false, false, false, listViewHeight, scrollToIndex, heights); + testScrollTo(true, false, false, listViewHeight, scrollToIndex, heights); + testScrollTo(true, true, false, listViewHeight, scrollToIndex, heights); + testScrollTo(true, true, true, listViewHeight, scrollToIndex, heights); + + } + + public void testScrollTo(boolean addIncremental, boolean layoutTwice, boolean selectIndex, int listViewHeight, int scrollToIndex, Integer[] heights) { + final ListView listView = new ListView(); + listView.setPrefHeight(listViewHeight); + listView.setCellFactory(lv -> new ListCell() { + @Override + public void updateItem(Integer item, boolean empty) { + super.updateItem(item, empty); + if (!empty && (item != null)) { + this.setPrefHeight(item); + } + } + }); + StageLoader sl = new StageLoader(new VBox(listView)); + if (addIncremental) { + // Adding the elments incrementally seems to make a difference! + for (var height : heights) { + // Adding them incrementally seems to be relevant + listView.getItems().add(height); + Toolkit.getToolkit().firePulse(); + listView.requestLayout(); + Toolkit.getToolkit().firePulse(); + listView.requestLayout(); + + } + } else { + listView.getItems().addAll(heights); + Toolkit.getToolkit().firePulse(); + + } + listView.scrollTo(scrollToIndex); + Toolkit.getToolkit().firePulse(); + if (layoutTwice) { + listView.requestLayout(); + Toolkit.getToolkit().firePulse(); + } + if (selectIndex) { + listView.getSelectionModel().select(scrollToIndex); + Toolkit.getToolkit().firePulse(); + } + + verifyListViewScrollTo(listView, listViewHeight, scrollToIndex, heights); + } + + public static void verifyListViewScrollTo(ListView listView, int listViewHeight, int scrollToIndex, Integer[] heights) { + double sumOfHeights = 0; + double viewportLength = listViewHeight - 2; // it would be better to calculate this from listView but there is no API for this + + for (int height : heights) { + sumOfHeights += height; + } + double sizeBelow = 0; + for (int i = scrollToIndex; i < heights.length; i += 1) { + sizeBelow += heights[i]; + } + + IndexedCell firstCell = VirtualFlowTestUtils.getCell(listView, 0); + IndexedCell scrollToCell = VirtualFlowTestUtils.getCell(listView, scrollToIndex); + IndexedCell lastCell = VirtualFlowTestUtils.getCell(listView, heights.length - 1); + + double lastCellSize = heights[heights.length - 1]; + + boolean scrolledToTop = scrollToIndex == 0; + boolean scrolledToBottomElement = scrollToIndex == heights.length - 1; + boolean scrolledToBottom = false; + boolean isLastIndex = scrollToIndex == heights.length - 1; + boolean shouldScrollToBottom = (sizeBelow < viewportLength) || (isLastIndex && lastCellSize >= viewportLength); + + if (Math.abs(lastCell.getLayoutY() - (viewportLength - lastCellSize)) <= 1.0) { + scrolledToBottom = true; + } + + assertTrue("Our cell must be visible!", scrollToCell.isVisible()); + + if (lastCell.isVisible() && sumOfHeights >= viewportLength) { + // There shouldn't be any space between the last cell and the bottom + assertTrue("Last cell shouldn't leave space between itself and the bottom", lastCell.getLayoutY() + 1 > (viewportLength - lastCellSize)); + } + if (sumOfHeights < viewportLength) { + // 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. + assertEquals("Last cell should be at the bottom, if we scroll to it", sumOfHeights - lastCellSize, lastCell.getLayoutY(), 1.); + } + if (!shouldScrollToBottom && sumOfHeights > viewportLength) { + // 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. + assertEquals("Our cell mut be at the top", 0, scrollToCell.getLayoutY(), 1.); + } + // Additional Tests: + double previousLayoutY = scrollToCell.getLayoutY(); + if (previousLayoutY == 0) { + // Upper cell shouldn't move after heights are changed + List 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.); + } + + } + } diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java index 8956cb590ba..869bb65b702 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java @@ -4251,14 +4251,14 @@ private void test_rt_35395(boolean testCell, boolean useFixedCellSize) { Platform.runLater(() -> { Toolkit.getToolkit().firePulse(); assertTrue(rt_35395_counter > 0); - assertTrue(rt_35395_counter < 18); + assertTrue(rt_35395_counter < 39); rt_35395_counter = 0; treeTableView.scrollTo(55); Platform.runLater(() -> { Toolkit.getToolkit().firePulse(); assertTrue(rt_35395_counter > 0); - assertTrue(rt_35395_counter < 30); + assertTrue(rt_35395_counter < 90); sl.dispose(); }); });