8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction

8276326: Empty Cells in TableView supposedly after using setFixedCellSize()
8252566: TreeTableView: broken row layout for fixedCellSize
8346824: Collapsing tree view causes rendering issues

Reviewed-by: angorya, mstrauss

Author: Marius Hanl

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

@@ -32,7 +32,6 @@
 import com.sun.javafx.scene.control.behavior.BehaviorBase;
 import com.sun.javafx.scene.control.behavior.TableRowBehavior;
 
-import javafx.beans.property.DoubleProperty;
 import javafx.collections.FXCollections;
 import javafx.collections.ObservableList;
 import javafx.scene.AccessibleAttribute;
@@ -93,34 +92,41 @@ public TableRowSkin(TableRow<T> control) {
             }
         });
 
-        setupTreeTableViewListeners();
+        setupTableViewListeners();
     }
 
     // FIXME: replace listener to fixedCellSize with direct lookup - JDK-8277000
-    private void setupTreeTableViewListeners() {
+    private void setupTableViewListeners() {
         TableView<T> tableView = getSkinnable().getTableView();
         if (tableView == null) {
             registerInvalidationListener(getSkinnable().tableViewProperty(), e -> {
                 unregisterInvalidationListeners(getSkinnable().tableViewProperty());
-                setupTreeTableViewListeners();
+                setupTableViewListeners();
             });
         } else {
-            DoubleProperty fixedCellSizeProperty = tableView.fixedCellSizeProperty();
-            if (fixedCellSizeProperty != null) {
-                registerChangeListener(fixedCellSizeProperty, e -> {
-                    fixedCellSize = fixedCellSizeProperty.get();
-                    fixedCellSizeEnabled = fixedCellSize > 0;
-                });
-                fixedCellSize = fixedCellSizeProperty.get();
-                fixedCellSizeEnabled = fixedCellSize > 0;
-
-                // JDK-8144500:
-                // When in fixed cell size mode, we must listen to the width of the virtual flow, so
-                // that when it changes, we can appropriately add / remove cells that may or may not
-                // be required (because we remove all cells that are not visible).
+            registerChangeListener(tableView.fixedCellSizeProperty(), e -> {
                 VirtualFlow<TableRow<T>> virtualFlow = getVirtualFlow();
                 if (virtualFlow != null) {
-                    registerChangeListener(virtualFlow.widthProperty(), e -> tableView.requestLayout());
+                    unregisterChangeListeners(virtualFlow.widthProperty());
+                }
+
+                updateCachedFixedSize();
+            });
+
+            updateCachedFixedSize();
+        }
+    }
+
+    private void updateCachedFixedSize() {
+        TableView<T> tableView = getSkinnable().getTableView();
+        if (tableView != null) {
+            fixedCellSize = tableView.getFixedCellSize();
+            fixedCellSizeEnabled = fixedCellSize > 0;
+
+            if (fixedCellSizeEnabled) {
+                VirtualFlow<TableRow<T>> virtualFlow = getVirtualFlow();
+                if (virtualFlow != null) {
+                    registerChangeListener(virtualFlow.widthProperty(), e -> getSkinnable().requestLayout());
                 }
             }
         }

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

@@ -36,7 +36,6 @@
 import javafx.collections.ObservableList;
 import javafx.css.StyleOrigin;
 import javafx.css.StyleableObjectProperty;
-import javafx.geometry.Insets;
 import javafx.geometry.Pos;
 import javafx.scene.Node;
 import javafx.scene.Parent;
@@ -95,11 +94,6 @@ public abstract class TableRowSkinBase<T,
      */
     static final Map<TableColumnBase<?,?>, Double> maxDisclosureWidthMap = new WeakHashMap<>();
 
-    // Specifies the number of times we will call 'recreateCells()' before we blow
-    // out the cellsMap structure and rebuild all cells. This helps to prevent
-    // against memory leaks in certain extreme circumstances.
-    private static final int DEFAULT_FULL_REFRESH_COUNTER = 100;
-
 
     /* *************************************************************************
      *                                                                         *
@@ -113,21 +107,13 @@ public abstract class TableRowSkinBase<T,
      * efficiency we create cells for all columns, even if they aren't visible,
      * and we only create new cells if we don't already have it cached in this
      * map.
-     *
-     * Note that this means that it is possible for this map to therefore be
-     * a memory leak if an application uses TableView and is creating and removing
-     * a large number of tableColumns. This is mitigated in the recreateCells()
-     * function below - refer to that to learn more.
      */
     WeakHashMap<TableColumnBase, Reference<R>> cellsMap;
 
     // This observableArrayList contains the currently visible table cells for this row.
     final List<R> cells = new ArrayList<>();
 
-    private int fullRefreshCounter = DEFAULT_FULL_REFRESH_COUNTER;
-
     boolean isDirty = false;
-    boolean updateCells = false;
 
     // FIXME: replace cached values with direct lookup - JDK-8277000
     double fixedCellSize;
@@ -152,7 +138,7 @@ public TableRowSkinBase(C control) {
         getSkinnable().setPickOnBounds(false);
 
         recreateCells();
-        updateCells(true);
+        updateCells();
 
         // init bindings
         // watches for any change in the leaf columns observableArrayList - this will indicate
@@ -176,7 +162,7 @@ public TableRowSkinBase(C control) {
      *                                                                         *
      **************************************************************************/
 
-    private void updateLeafColumns() {
+    void updateLeafColumns() {
         isDirty = true;
         getSkinnable().requestLayout();
     }
@@ -289,7 +275,6 @@ protected ObjectProperty<Node> graphicProperty() {
                         // earlier rows to update themselves to take into account
                         // this increased indentation.
                         final VirtualFlow<C> flow = getVirtualFlow();
-                        final int thisIndex = getSkinnable().getIndex();
                         for (int i = 0; i < flow.cells.size(); i++) {
                             C cell = flow.cells.get(i);
                             if (cell == null || cell.isEmpty()) continue;
@@ -318,10 +303,13 @@ protected ObjectProperty<Node> graphicProperty() {
         int index = control.getIndex();
         if (index < 0/* || row >= itemsProperty().get().size()*/) return;
 
+        VirtualFlow<C> virtualFlow = getVirtualFlow();
         for (int column = 0, max = cells.size(); column < max; column++) {
             R tableCell = cells.get(column);
             TableColumnBase<T, ?> tableColumn = getTableColumn(tableCell);
 
+            width = snapSizeX(tableColumn.getWidth());
+
             boolean isVisible = true;
             if (fixedCellSizeEnabled) {
                 // we determine if the cell is visible, and if not we have the
@@ -333,7 +321,7 @@ protected ObjectProperty<Node> graphicProperty() {
                 // provided by the developer, and this means that we do not have
                 // to concern ourselves with the possibility that the height
                 // may be variable and / or dynamic.
-                isVisible = isColumnPartiallyOrFullyVisible(tableColumn);
+                isVisible = isColumnPartiallyOrFullyVisible(x, width, virtualFlow);
 
                 y = 0;
                 height = fixedCellSize;
@@ -342,12 +330,9 @@ protected ObjectProperty<Node> graphicProperty() {
             }
 
             if (isVisible) {
-                if (fixedCellSizeEnabled && tableCell.getParent() == null) {
+                if (tableCell.getParent() == null) {
                     getChildren().add(tableCell);
                 }
-                // Note: prefWidth() has to be called only after the tableCell is added to the tableRow, if it wasn't
-                // already. Otherwise, it might not have its skin yet, and its pref width is therefore 0.
-                width = tableCell.prefWidth(height);
 
                 // Added for JDK-8115536, and then updated for JDK-8122708.
                 // We change the alignment from CENTER_LEFT to TOP_LEFT if the
@@ -419,11 +404,7 @@ protected ObjectProperty<Node> graphicProperty() {
                 // This does not appear to impact performance...
                 tableCell.requestLayout();
             } else {
-                width = tableCell.prefWidth(height);
-                if (fixedCellSizeEnabled) {
-                    // we only add/remove to the scenegraph if the fixed cell
-                    // length support is enabled - otherwise we keep all
-                    // TableCells in the scenegraph
+                if (tableCell.getParent() != null) {
                     getChildren().remove(tableCell);
                 }
             }
@@ -473,21 +454,9 @@ boolean isShowRoot() {
         return true;
     }
 
-    void updateCells(boolean resetChildren) {
-        // To avoid a potential memory leak (when the TableColumns in the
-        // TableView are created/inserted/removed/deleted, we have a 'refresh
-        // counter' that when we reach 0 will delete all cells in this row
-        // and recreate all of them.
-        if (resetChildren) {
-            if (fullRefreshCounter == 0) {
-                recreateCells();
-            }
-            fullRefreshCounter--;
-        }
-
+    void updateCells() {
         // if clear isn't called first, we can run into situations where the
         // cells aren't updated properly.
-        final boolean cellsEmpty = cells.isEmpty();
         cells.clear();
 
         final C skinnable = getSkinnable();
@@ -518,23 +487,7 @@ void updateCells(boolean resetChildren) {
             cells.add(cell);
         }
 
-        // update children of each row
-        if (fixedCellSizeEnabled) {
-            // we leave the adding / removing up to the layoutChildren method mostly, but here we remove any children
-            // cells that refer to columns that are removed or not visible.
-            List<Node> toRemove = new ArrayList<>();
-            for (Node cell : getChildren()) {
-                if (!(cell instanceof IndexedCell)) continue;
-                TableColumnBase<T, ?> tableColumn = getTableColumn((R) cell);
-                if (!getVisibleLeafColumns().contains(tableColumn)) {
-                    toRemove.add(cell);
-                }
-            }
-            getChildren().removeAll(toRemove);
-        }
-        if (resetChildren || cellsEmpty) {
-            getChildren().setAll(cells);
-        }
+        getChildren().setAll(cells);
     }
 
     VirtualFlow<C> getVirtualFlow() {
@@ -630,12 +583,8 @@ VirtualFlow<C> getVirtualFlow() {
 
     final void checkState() {
         if (isDirty) {
-            updateCells(true);
+            updateCells();
             isDirty = false;
-            updateCells = false;
-        } else if (updateCells) {
-            updateCells(false);
-            updateCells = false;
         }
     }
 
@@ -655,33 +604,18 @@ void setDirty(boolean dirty) {
      *                                                                         *
      **************************************************************************/
 
-    private boolean isColumnPartiallyOrFullyVisible(TableColumnBase col) {
-        if (col == null || !col.isVisible()) return false;
+    private boolean isColumnPartiallyOrFullyVisible(double start, double width, VirtualFlow<C> virtualFlow) {
+        double end = start + width;
 
-        final VirtualFlow<?> virtualFlow = getVirtualFlow();
         double scrollX = virtualFlow == null ? 0.0 : virtualFlow.getHbar().getValue();
+        double headerWidth = virtualFlow == null ? 0.0 : virtualFlow.getViewportBreadth();
+        double virtualFlowWidth = headerWidth + scrollX;
 
-        // work out where this column header is, and it's width (start -> end)
-        double start = 0;
-        final ObservableList<? extends TableColumnBase> visibleLeafColumns = getVisibleLeafColumns();
-        for (int i = 0, max = visibleLeafColumns.size(); i < max; i++) {
-            TableColumnBase<?,?> c = visibleLeafColumns.get(i);
-            if (c.equals(col)) break;
-            start += c.getWidth();
-        }
-        double end = start + col.getWidth();
-
-        // determine the width of the table
-        final Insets padding = getSkinnable().getPadding();
-        double headerWidth = getSkinnable().getWidth() - padding.getLeft() + padding.getRight();
-
-        return (start >= scrollX || end > scrollX) && (start < (headerWidth + scrollX) || end <= (headerWidth + scrollX));
+        return (start >= scrollX || end > scrollX) && (start < virtualFlowWidth || end <= virtualFlowWidth);
     }
 
     private void requestCellUpdate() {
-        updateCells = true;
         getSkinnable().requestLayout();
-
         // update the index of all children cells (JDK-8119094).
         // Note that we do this after the TableRow item has been updated,
         // rather than when the TableRow index has changed (as this will be
@@ -713,7 +647,6 @@ private void recreateCells() {
         ObservableList<? extends TableColumnBase/*<T,?>*/> columns = getVisibleLeafColumns();
 
         cellsMap = new WeakHashMap<>(columns.size());
-        fullRefreshCounter = DEFAULT_FULL_REFRESH_COUNTER;
         getChildren().clear();
 
         for (TableColumnBase col : columns) {