diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java index 494199e60cc..2140da2e0de 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java @@ -267,10 +267,22 @@ public TabPaneSkin(TabPane control) { /** {@inheritDoc} */ @Override public void dispose() { + if (getSkinnable() == null) return; + if (selectionModel != null) { selectionModel.selectedItemProperty().removeListener(weakSelectionChangeListener); selectionModel = null; } + getSkinnable().getTabs().removeListener(weakTabsListener); + tabHeaderArea.dispose(); + + // Control and Skin share the list of children, so children that are + // added by Skin are actually added to control's list of children, + // so a skin should remove the children that it adds. + getChildren().remove(tabHeaderArea); + for (Tab tab : getSkinnable().getTabs()) { + removeTabContent(tab); + } super.dispose(); @@ -499,7 +511,7 @@ private void removeTabs(List removedList) { if (tabRegion != null) { tabRegion.isClosing = true; - tabRegion.removeListeners(tab); + tabRegion.dispose(); removeTabContent(tab); EventHandler cleanup = ae -> { @@ -579,8 +591,10 @@ private void addTabs(List addedList, int from) { } } + ListChangeListener tabsListener; + WeakListChangeListener weakTabsListener; private void initializeTabListener() { - getSkinnable().getTabs().addListener((ListChangeListener) c -> { + tabsListener = c -> { List tabsToRemove = new ArrayList<>(); List tabsToAdd = new ArrayList<>(); @@ -659,7 +673,9 @@ private void initializeTabListener() { // Fix for RT-34692 getSkinnable().requestLayout(); - }); + }; + weakTabsListener = new WeakListChangeListener<>(tabsListener); + getSkinnable().getTabs().addListener(weakTabsListener); } private void addTabContent(Tab tab) { @@ -673,14 +689,18 @@ private void addTabContent(Tab tab) { private void removeTabContent(Tab tab) { for (TabContentRegion contentRegion : tabContentRegions) { if (contentRegion.getTab().equals(tab)) { - contentRegion.removeListeners(tab); - getChildren().remove(contentRegion); - tabContentRegions.remove(contentRegion); + removeTabContent(contentRegion); break; } } } + private void removeTabContent(TabContentRegion contentRegion) { + contentRegion.dispose(); + tabContentRegions.remove(contentRegion); + getChildren().remove(contentRegion); + } + private void updateTabPosition() { tabHeaderArea.invalidateScrollOffset(); getSkinnable().applyCss(); @@ -1208,6 +1228,14 @@ private double firstTabIndent() { positionInArea(controlButtons, controlStartX, controlStartY, btnWidth, btnHeight, /*baseline ignored*/0, HPos.CENTER, VPos.CENTER); } + + void dispose() { + for (Node child : headersRegion.getChildren()) { + TabHeaderSkin header = (TabHeaderSkin) child; + header.dispose(); + } + controlButtons.dispose(); + } } /* End TabHeaderArea */ @@ -1495,7 +1523,7 @@ public void handle(MouseEvent me) { if (me.getButton().equals(MouseButton.MIDDLE)) { if (showCloseButton()) { if (behavior.canCloseTab(tab)) { - removeListeners(tab); + dispose(); behavior.closeTab(tab); } } @@ -1541,10 +1569,9 @@ private boolean showCloseButton() { } }; - private void removeListeners(Tab tab) { + private void dispose() { + tab.getStyleClass().removeListener(weakStyleClassListener); listener.dispose(); - inner.getChildren().clear(); - getChildren().clear(); setOnContextMenuRequested(null); setOnMousePressed(null); } @@ -1693,7 +1720,7 @@ private void updateContent() { } } - private void removeListeners(Tab tab) { + public void dispose() { tab.selectedProperty().removeListener(weakTabSelectedListener); tab.contentProperty().removeListener(weakTabContentListener); } @@ -1785,11 +1812,8 @@ private void positionArrow(Pane btn, StackPane arrow, double x, double y, double getChildren().add(inner); - tabPane.sideProperty().addListener(valueModel -> { - Side tabPosition = getSkinnable().getSide(); - downArrow.setRotate(tabPosition.equals(Side.BOTTOM)? 180.0F : 0.0F); - }); - tabPane.getTabs().addListener((ListChangeListener) c -> setupPopupMenu()); + tabPane.sideProperty().addListener(weakSidePropListener); + tabPane.getTabs().addListener(weakTabsListenerForPopup); showControlButtons = false; if (isShowTabsMenu()) { showControlButtons = true; @@ -1798,6 +1822,21 @@ private void positionArrow(Pane btn, StackPane arrow, double x, double y, double getProperties().put(ContextMenu.class, popup); } + InvalidationListener sidePropListener = e -> { + Side tabPosition = getSkinnable().getSide(); + downArrow.setRotate(tabPosition.equals(Side.BOTTOM)? 180.0F : 0.0F); + }; + ListChangeListener tabsListenerForPopup = e -> setupPopupMenu(); + WeakInvalidationListener weakSidePropListener = + new WeakInvalidationListener(sidePropListener); + WeakListChangeListener weakTabsListenerForPopup = + new WeakListChangeListener<>(tabsListenerForPopup); + + void dispose() { + getSkinnable().sideProperty().removeListener(weakSidePropListener); + getSkinnable().getTabs().removeListener(weakTabsListenerForPopup); + } + private boolean showTabsMenu = false; private void showTabsMenu(boolean value) { @@ -2058,11 +2097,7 @@ private void setupReordering(StackPane headersRegion) { dragState = DragState.NONE; this.headersRegion = headersRegion; updateListeners(); - getSkinnable().tabDragPolicyProperty().addListener((observable, oldValue, newValue) -> { - if (oldValue != newValue) { - updateListeners(); - } - }); + registerChangeListener(getSkinnable().tabDragPolicyProperty(), inv -> updateListeners()); } private void handleHeaderMousePressed(MouseEvent event) { diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java index d996538b65e..727210759c6 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java @@ -1213,10 +1213,8 @@ class TabPaneSkin1 extends TabPaneSkin { } } - @Ignore("JDK-8242621") @Test public void testNPEOnSwitchSkinAndChangeSelection() { - // Because of JDK-8242621, this test fails with NPE. tabPane.getTabs().addAll(tab1, tab2); root.getChildren().add(tabPane); stage.show(); 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 da5df50b5d6..0ee424eb8fa 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 @@ -37,11 +37,14 @@ import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.*; import javafx.scene.Scene; +import javafx.scene.control.skin.TabPaneSkin; import javafx.scene.control.Button; import javafx.scene.control.ChoiceBox; import javafx.scene.control.Control; import javafx.scene.control.ListCell; import javafx.scene.control.ListView; +import javafx.scene.control.Tab; +import javafx.scene.control.TabPane; import javafx.scene.control.ToolBar; import javafx.scene.control.TreeCell; import javafx.scene.control.TreeItem; @@ -257,6 +260,63 @@ public void testToolBarFocus() { assertEquals("first item in toolbar must be focused", bar.getItems().get(0), scene.getFocusOwner()); } +//-------- TabPane + @Test + public void testChildrenCountAfterSkinIsReplaced() { + TabPane tabPane = new TabPane(); + tabPane.getTabs().addAll(new Tab("0"), new Tab("1")); + installDefaultSkin(tabPane); + int childrenCount = tabPane.getChildrenUnmodifiable().size(); + replaceSkin(tabPane); + assertEquals(childrenCount, tabPane.getChildrenUnmodifiable().size()); + } + + @Test + public void testChildrenCountAfterSkinIsRemoved() { + TabPane tabPane = new TabPane(); + assertEquals(0, tabPane.getChildrenUnmodifiable().size()); + tabPane.getTabs().addAll(new Tab("0"), new Tab("1")); + installDefaultSkin(tabPane); + assertEquals(3, tabPane.getChildrenUnmodifiable().size()); + tabPane.setSkin(null); + assertNull(tabPane.getSkin()); + assertEquals(0, tabPane.getChildrenUnmodifiable().size()); + } + + @Test + public void testNPEWhenTabsAddedAfterSkinIsReplaced() { + TabPane tabPane = new TabPane(); + tabPane.getTabs().addAll(new Tab("0"), new Tab("1")); + installDefaultSkin(tabPane); + replaceSkin(tabPane); + tabPane.getTabs().addAll(new Tab("2"), new Tab("3")); + } + + @Test + public void testNPEWhenTabRemovedAfterSkinIsReplaced() { + TabPane tabPane = new TabPane(); + tabPane.getTabs().addAll(new Tab("0"), new Tab("1")); + installDefaultSkin(tabPane); + replaceSkin(tabPane); + tabPane.getTabs().remove(0); + } + + @Test + public void testAddRemoveTabsAfterSkinIsReplaced() { + TabPane tabPane = new TabPane(); + tabPane.getTabs().addAll(new Tab("0"), new Tab("1")); + installDefaultSkin(tabPane); + assertEquals(2, tabPane.getTabs().size()); + assertEquals(3, tabPane.getChildrenUnmodifiable().size()); + replaceSkin(tabPane); + tabPane.getTabs().addAll(new Tab("2"), new Tab("3")); + assertEquals(4, tabPane.getTabs().size()); + assertEquals(5, tabPane.getChildrenUnmodifiable().size()); + tabPane.getTabs().clear(); + assertEquals(0, tabPane.getTabs().size()); + assertEquals(1, tabPane.getChildrenUnmodifiable().size()); + } + //---------------- setup and initial /** diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java index cb505aa5a60..ff08bb3d982 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java @@ -115,8 +115,6 @@ public static Collection data() { SplitPane.class, TableRow.class, TableView.class, - // @Ignore("8242621") - TabPane.class, // @Ignore("8244419") TextArea.class, // @Ignore("8240506")