From 8206f82107c931ab6f59d32c84833437d197e947 Mon Sep 17 00:00:00 2001 From: Marius Hanl Date: Fri, 22 Nov 2024 20:54:12 +0100 Subject: [PATCH 1/2] 8277000: Tree-/TableRowSkin: replace listener to fixedCellSize by live lookup --- .../scene/control/skin/TableRowSkin.java | 26 ++++---- .../scene/control/skin/TableRowSkinBase.java | 28 +++++---- .../scene/control/skin/TreeTableRowSkin.java | 36 +++-------- .../scene/control/skin/TableSkinShim.java | 8 +-- .../scene/control/skin/SkinCleanupTest.java | 60 ++++++++++++++++++- 5 files changed, 96 insertions(+), 62 deletions(-) diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java index 03dcce9e46e..36bc327081d 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java +++ b/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,25 @@ public TableRowSkin(TableRow control) { } }); - setupTreeTableViewListeners(); + setupTableViewListeners(); } - // FIXME: replace listener to fixedCellSize with direct lookup - JDK-8277000 - private void setupTreeTableViewListeners() { + private void setupTableViewListeners() { TableView 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; - + if (getFixedCellSize() > 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). VirtualFlow> virtualFlow = getVirtualFlow(); if (virtualFlow != null) { - registerChangeListener(virtualFlow.widthProperty(), e -> tableView.requestLayout()); + registerChangeListener(virtualFlow.widthProperty(), e -> getTableView().requestLayout()); } } } @@ -231,6 +221,12 @@ private TableView getTableView() { return getSkinnable().getTableView(); } + @Override + double getFixedCellSize() { + TableView tableView = getTableView(); + return tableView != null ? tableView.getFixedCellSize() : super.getFixedCellSize(); + } + // test-only TableViewSkin getTableViewSkin() { TableView tableView = getSkinnable().getTableView(); diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java index 9fa531a8368..692ba750d70 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java @@ -41,6 +41,7 @@ import javafx.scene.Node; import javafx.scene.Parent; import javafx.scene.control.*; +import javafx.scene.layout.Region; import javafx.util.Duration; import com.sun.javafx.tk.Toolkit; @@ -129,10 +130,6 @@ public abstract class TableRowSkinBase graphicProperty() { TableColumnBase tableColumn = getTableColumn(tableCell); boolean isVisible = true; - if (fixedCellSizeEnabled) { + if (getFixedCellSize() > 0) { // we determine if the cell is visible, and if not we have the // ability to take it out of the scenegraph to help improve // performance. However, we only do this when there is a @@ -336,13 +333,13 @@ protected ObjectProperty graphicProperty() { isVisible = isColumnPartiallyOrFullyVisible(tableColumn); y = 0; - height = fixedCellSize; + height = getFixedCellSize(); } else { height = h; } if (isVisible) { - if (fixedCellSizeEnabled && tableCell.getParent() == null) { + if (getFixedCellSize() > 0 && 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 @@ -420,7 +417,7 @@ protected ObjectProperty graphicProperty() { tableCell.requestLayout(); } else { width = tableCell.prefWidth(height); - if (fixedCellSizeEnabled) { + if (getFixedCellSize() > 0) { // we only add/remove to the scenegraph if the fixed cell // length support is enabled - otherwise we keep all // TableCells in the scenegraph @@ -432,6 +429,10 @@ protected ObjectProperty graphicProperty() { } } + double getFixedCellSize() { + return Region.USE_COMPUTED_SIZE; + } + int getIndentationLevel(C control) { return 0; } @@ -519,7 +520,7 @@ void updateCells(boolean resetChildren) { } // update children of each row - if (fixedCellSizeEnabled) { + if (getFixedCellSize() > 0) { // 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 toRemove = new ArrayList<>(); @@ -559,7 +560,8 @@ VirtualFlow getVirtualFlow() { /** {@inheritDoc} */ @Override protected double computePrefHeight(double width, double topInset, double rightInset, double bottomInset, double leftInset) { - if (fixedCellSizeEnabled) { + double fixedCellSize = getFixedCellSize(); + if (fixedCellSize > 0) { return fixedCellSize; } @@ -592,7 +594,8 @@ VirtualFlow getVirtualFlow() { /** {@inheritDoc} */ @Override protected double computeMinHeight(double width, double topInset, double rightInset, double bottomInset, double leftInset) { - if (fixedCellSizeEnabled) { + double fixedCellSize = getFixedCellSize(); + if (fixedCellSize > 0) { return fixedCellSize; } @@ -622,7 +625,8 @@ VirtualFlow getVirtualFlow() { /** {@inheritDoc} */ @Override protected double computeMaxHeight(double width, double topInset, double rightInset, double bottomInset, double leftInset) { - if (fixedCellSizeEnabled) { + double fixedCellSize = getFixedCellSize(); + if (fixedCellSize > 0) { return fixedCellSize; } return super.computeMaxHeight(width, topInset, rightInset, bottomInset, leftInset); diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java index 3ec810579c4..e417a92694e 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java @@ -97,10 +97,6 @@ public TreeTableRowSkin(TreeTableRow control) { ListenerHelper lh = ListenerHelper.get(this); - lh.addChangeListener(control.indexProperty(), (ev) -> { - updateCells = true; - }); - lh.addChangeListener(control.treeItemProperty(), (ev) -> { updateTreeItem(); // There used to be an isDirty = true statement here, but this was @@ -111,7 +107,6 @@ public TreeTableRowSkin(TreeTableRow control) { setupTreeTableViewListeners(); } - // FIXME: replace listener to fixedCellSize with direct lookup - JDK-8277000 private void setupTreeTableViewListeners() { TreeTableView treeTableView = getSkinnable().getTreeTableView(); if (treeTableView == null) { @@ -130,40 +125,19 @@ private void setupTreeTableViewListeners() { } }); - DoubleProperty fixedCellSizeProperty = getTreeTableView().fixedCellSizeProperty(); - if (fixedCellSizeProperty != null) { - registerChangeListener(fixedCellSizeProperty, (x) -> { - updateCachedFixedSize(); - }); - updateCachedFixedSize(); - + if (getFixedCellSize() > 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). VirtualFlow> virtualFlow = getVirtualFlow(); if (virtualFlow != null) { - registerChangeListener(getVirtualFlow().widthProperty(), (x) -> { - if (getSkinnable() != null) { - TreeTableView t = getSkinnable().getTreeTableView(); - t.requestLayout(); - } - }); + registerChangeListener(getVirtualFlow().widthProperty(), e -> getTreeTableView().requestLayout()); } } } } - private void updateCachedFixedSize() { - if (getSkinnable() != null) { - TreeTableView t = getSkinnable().getTreeTableView(); - if (t != null) { - fixedCellSize = t.getFixedCellSize(); - fixedCellSizeEnabled = fixedCellSize > 0.0; - } - } - } - /* ************************************************************************* * * * Listeners * @@ -351,6 +325,12 @@ private TreeTableView getTreeTableView() { return getSkinnable().getTreeTableView(); } + @Override + double getFixedCellSize() { + TreeTableView treeTableView = getTreeTableView(); + return treeTableView != null ? treeTableView.getFixedCellSize() : super.getFixedCellSize(); + } + private void updateDisclosureNodeAndGraphic() { if (getSkinnable().isEmpty()) { getChildren().remove(graphic); diff --git a/modules/javafx.controls/src/shims/java/javafx/scene/control/skin/TableSkinShim.java b/modules/javafx.controls/src/shims/java/javafx/scene/control/skin/TableSkinShim.java index 7f32970abec..930c6006d6c 100644 --- a/modules/javafx.controls/src/shims/java/javafx/scene/control/skin/TableSkinShim.java +++ b/modules/javafx.controls/src/shims/java/javafx/scene/control/skin/TableSkinShim.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2021, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -117,13 +117,11 @@ public static VirtualFlow getVirtualFlow(TreeTableView table) { //----------------- Tree/TableRowSkin state public static boolean isFixedCellSizeEnabled(TableRow tableRow) { - TableRowSkin skin = (TableRowSkin) tableRow.getSkin(); - return skin.fixedCellSizeEnabled; + return tableRow.getTableView().getFixedCellSize() > 0; } public static boolean isFixedCellSizeEnabled(TreeTableRow tableRow) { - TreeTableRowSkin skin = (TreeTableRowSkin) tableRow.getSkin(); - return skin.fixedCellSizeEnabled; + return tableRow.getTreeTableView().getFixedCellSize() > 0; } public static boolean isDirty(TableRow tableRow) { diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java index 2ecd0df38a5..65fda2bdb4b 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java @@ -121,7 +121,6 @@ public class SkinCleanupTest { /** * Test access to fixedCellSize via lookup (not listener) */ - @Disabled("JDK-8277000") @Test public void testTreeTableRowFixedCellSizeListener() { TreeTableView tableView = createPersonTreeTable(false); @@ -271,6 +270,37 @@ public void testTreeTableRowFixedCellSizeEnabled() { assertTrue(isFixedCellSizeEnabled(tableRow), "fixed cell size enabled"); } + @Test + public void testTreeTableRowVirtualFlowWidthListenerReplaceSkin() { + TreeTableView tableView = createPersonTreeTable(false); + tableView.setFixedCellSize(24); + showControl(tableView, true); + VirtualFlow flow = getVirtualFlow(tableView); + TreeTableRow tableRow = (TreeTableRow) getCell(tableView, 1); + replaceSkin(tableRow); + Toolkit.getToolkit().firePulse(); + TreeTableRowSkin rowSkin = (TreeTableRowSkin) tableRow.getSkin(); + assertNotNull( + unregisterChangeListeners(rowSkin, flow.widthProperty()), + "row skin must have listener to virtualFlow width"); + } + + /** + * Sanity test: listener to flow's width is registered. + */ + @Test + public void testTreeTableRowVirtualFlowWidthListener() { + TreeTableView tableView = createPersonTreeTable(false); + tableView.setFixedCellSize(24); + showControl(tableView, true); + VirtualFlow flow = getVirtualFlow(tableView); + TreeTableRow tableRow = (TreeTableRow) getCell(tableView, 1); + TreeTableRowSkin rowSkin = (TreeTableRowSkin) tableRow.getSkin(); + assertNotNull( + unregisterChangeListeners(rowSkin, flow.widthProperty()), + "row skin must have listener to virtualFlow width"); + } + @Test public void testTreeTableRowTracksVirtualFlowReplaceSkin() { TreeTableView tableView = createPersonTreeTable(false); @@ -550,7 +580,6 @@ private TreeTableView createPersonTreeTable(boolean installSkin) { /** * Test access to fixedCellSize via lookup (not listener) */ - @Disabled("JDK-8277000") @Test public void testTableRowFixedCellSizeListener() { TableView tableView = createPersonTable(false); @@ -644,6 +673,7 @@ public void testTableRowFixedCellSizeEnabled() { @Test public void testTableRowVirtualFlowWidthListenerReplaceSkin() { TableView tableView = createPersonTable(false); + tableView.setFixedCellSize(24); showControl(tableView, true); VirtualFlow flow = getVirtualFlow(tableView); TableRow tableRow = (TableRow) getCell(tableView, 1); @@ -661,6 +691,7 @@ public void testTableRowVirtualFlowWidthListenerReplaceSkin() { @Test public void testTableRowVirtualFlowWidthListener() { TableView tableView = createPersonTable(false); + tableView.setFixedCellSize(24); showControl(tableView, true); VirtualFlow flow = getVirtualFlow(tableView); TableRow tableRow = (TableRow) getCell(tableView, 1); @@ -670,6 +701,31 @@ public void testTableRowVirtualFlowWidthListener() { "row skin must have listener to virtualFlow width"); } + @Test + public void testTableRowTracksVirtualFlowReplaceSkin() { + TableView tableView = createPersonTable(false); + showControl(tableView, true); + VirtualFlow flow = getVirtualFlow(tableView); + TableRow tableRow = (TableRow) getCell(tableView, 1); + replaceSkin(tableRow); + Toolkit.getToolkit().firePulse(); + TableRowSkin rowSkin = (TableRowSkin) tableRow.getSkin(); + checkFollowsWidth(flow, (Region) rowSkin.getNode()); + } + + /** + * Sanity test checks that tree table row skin tracks the virtual flow width. + */ + @Test + public void testTableRowTracksVirtualFlowWidth() { + TableView tableView = createPersonTable(false); + showControl(tableView, true); + VirtualFlow flow = getVirtualFlow(tableView); + TableRow tableRow = (TableRow) getCell(tableView, 1); + TableRowSkin rowSkin = (TableRowSkin) tableRow.getSkin(); + checkFollowsWidth(flow, (Region) rowSkin.getNode()); + } + /** * Sanity: children don't pile up with fixed cell size. */ From 63119d957449e6f272f019515c9c5aeb19b02fec Mon Sep 17 00:00:00 2001 From: Marius Hanl Date: Tue, 10 Dec 2024 00:51:18 +0100 Subject: [PATCH 2/2] Call getFixedCellSize() once --- .../scene/control/skin/TableRowSkinBase.java | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java index 692ba750d70..f3e7b50f14a 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java @@ -315,12 +315,13 @@ protected ObjectProperty graphicProperty() { int index = control.getIndex(); if (index < 0/* || row >= itemsProperty().get().size()*/) return; + double fixedCellSize = getFixedCellSize(); for (int column = 0, max = cells.size(); column < max; column++) { R tableCell = cells.get(column); TableColumnBase tableColumn = getTableColumn(tableCell); boolean isVisible = true; - if (getFixedCellSize() > 0) { + if (fixedCellSize > 0) { // we determine if the cell is visible, and if not we have the // ability to take it out of the scenegraph to help improve // performance. However, we only do this when there is a @@ -333,13 +334,13 @@ protected ObjectProperty graphicProperty() { isVisible = isColumnPartiallyOrFullyVisible(tableColumn); y = 0; - height = getFixedCellSize(); + height = fixedCellSize; } else { height = h; } if (isVisible) { - if (getFixedCellSize() > 0 && tableCell.getParent() == null) { + if (fixedCellSize > 0 && 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 @@ -417,12 +418,10 @@ protected ObjectProperty graphicProperty() { tableCell.requestLayout(); } else { width = tableCell.prefWidth(height); - if (getFixedCellSize() > 0) { - // we only add/remove to the scenegraph if the fixed cell - // length support is enabled - otherwise we keep all - // TableCells in the scenegraph - getChildren().remove(tableCell); - } + // we only add/remove to the scenegraph if the fixed cell + // length support is enabled - otherwise we keep all + // TableCells in the scenegraph + getChildren().remove(tableCell); } x += width;