diff --git a/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java b/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java index 25cf09fcc9a..1f5a6ef5304 100644 --- a/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java +++ b/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java @@ -27,6 +27,7 @@ import javafx.beans.InvalidationListener; import javafx.beans.Observable; import javafx.beans.property.ReadOnlyObjectProperty; +import javafx.beans.value.ChangeListener; import javafx.collections.ListChangeListener; import javafx.collections.ObservableList; import javafx.event.Event; @@ -46,6 +47,7 @@ import java.util.List; import java.util.Map; +import java.util.WeakHashMap; public class ControlAcceleratorSupport { @@ -170,22 +172,32 @@ else if (menuitem instanceof CheckMenuItem) { // We also listen to the accelerator property for changes, such // that we can update the scene when a menu item accelerator changes. - menuitem.acceleratorProperty().addListener((observable, oldValue, newValue) -> { - final Map accelerators = scene.getAccelerators(); - - // remove the old KeyCombination from the accelerators map - Runnable _acceleratorRunnable = accelerators.remove(oldValue); - - // and put in the new accelerator KeyCombination, if it is not null - if (newValue != null) { - accelerators.put(newValue, _acceleratorRunnable); - } - }); + menuitem.acceleratorProperty().addListener(getListener(scene, menuitem)); } } } + private static Map> changeListenerMap = new WeakHashMap<>(); + + private static ChangeListener getListener(final Scene scene, MenuItem menuItem) { + + ChangeListener listener = changeListenerMap.get(menuItem); + if (listener == null) { + listener = (observable, oldValue, newValue) -> { + final Map accelerators = scene.getAccelerators(); + // remove the old KeyCombination from the accelerators map + Runnable _acceleratorRunnable = accelerators.remove(oldValue); + + // and put in the new accelerator KeyCombination, if it is not null + if (newValue != null) { + accelerators.put(newValue, _acceleratorRunnable); + } + }; + changeListenerMap.put(menuItem, listener); + } + return listener; + } // --- Remove @@ -220,7 +232,13 @@ public static void removeAcceleratorsFromScene(List items, S for (final MenuItem menuitem : items) { if (menuitem instanceof Menu) { - // TODO remove the menu listener from the menu.items list + // MenuBarSkin uses MenuBarButton to display a Menu. + // The listener that is added on the 'items' in the method + // doAcceleratorInstall(final ObservableList items, final Scene scene) + // is added to the MenuBarButton.getItems() and not to Menu.getItems(). + // If a Menu is removed from scenegraph then it's skin gets disposed(), which disposes the + // related MenuBarButton. So it is not required to remove the listener that was added + // to MenuBarButton.getItems(). // remove the accelerators of items contained within the menu removeAcceleratorsFromScene(((Menu)menuitem).getItems(), scene); @@ -229,6 +247,12 @@ public static void removeAcceleratorsFromScene(List items, S // the scene accelerators map final Map accelerators = scene.getAccelerators(); accelerators.remove(menuitem.getAccelerator()); + + ChangeListener listener = changeListenerMap.get(menuitem); + if (listener != null) { + menuitem.acceleratorProperty().removeListener(listener); + changeListenerMap.remove(menuitem); + } } } } diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlAcceleratorSupportTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlAcceleratorSupportTest.java new file mode 100644 index 00000000000..b8bb23a7252 --- /dev/null +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlAcceleratorSupportTest.java @@ -0,0 +1,101 @@ +/* + * Copyright (c) 2021, 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 + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Oracle designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +package test.javafx.scene.control; + +import javafx.scene.control.MenuBar; +import javafx.scene.control.Menu; +import javafx.scene.control.MenuItem; +import javafx.scene.layout.BorderPane; + +import test.com.sun.javafx.scene.control.infrastructure.StageLoader; +import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.*; + +import org.junit.Test; +import org.junit.BeforeClass; +import static org.junit.Assert.assertEquals; + +public class ControlAcceleratorSupportTest { + + @Test + public void testNumberOfListenersByRemovingAndAddingMenuItems() { + + Menu menu1 = new Menu("1"); + MenuItem item11 = new MenuItem("Item 1"); + MenuItem item12 = new MenuItem("Item 2"); + menu1.getItems().addAll(item11, item12); + + Menu menu2 = new Menu("2"); + MenuItem item21 = new MenuItem("Item 1"); + MenuItem item22 = new MenuItem("Item 2"); + menu2.getItems().addAll(item21, item22); + + MenuBar menuBar = new MenuBar(); + menuBar.getMenus().addAll(menu1, menu2); + BorderPane pane = new BorderPane(); + pane.setTop(menuBar); + + StageLoader sl = new StageLoader(pane); + + assertEquals(1, getListenerCount(item11.acceleratorProperty())); + assertEquals(1, getListenerCount(item12.acceleratorProperty())); + assertEquals(1, getListenerCount(item21.acceleratorProperty())); + assertEquals(1, getListenerCount(item22.acceleratorProperty())); + + menu1.getItems().clear(); + assertEquals(0, getListenerCount(item11.acceleratorProperty())); + assertEquals(0, getListenerCount(item12.acceleratorProperty())); + assertEquals(1, getListenerCount(item21.acceleratorProperty())); + assertEquals(1, getListenerCount(item22.acceleratorProperty())); + + menu2.getItems().clear(); + assertEquals(0, getListenerCount(item11.acceleratorProperty())); + assertEquals(0, getListenerCount(item12.acceleratorProperty())); + assertEquals(0, getListenerCount(item21.acceleratorProperty())); + assertEquals(0, getListenerCount(item22.acceleratorProperty())); + + menu1.getItems().addAll(item11, item12); + assertEquals(1, getListenerCount(item11.acceleratorProperty())); + assertEquals(1, getListenerCount(item12.acceleratorProperty())); + assertEquals(0, getListenerCount(item21.acceleratorProperty())); + assertEquals(0, getListenerCount(item22.acceleratorProperty())); + + menu2.getItems().addAll(item21, item22); + assertEquals(1, getListenerCount(item11.acceleratorProperty())); + assertEquals(1, getListenerCount(item12.acceleratorProperty())); + assertEquals(1, getListenerCount(item21.acceleratorProperty())); + assertEquals(1, getListenerCount(item22.acceleratorProperty())); + + menu2.getItems().clear(); + menu1.getItems().clear(); + + assertEquals(0, getListenerCount(item11.acceleratorProperty())); + assertEquals(0, getListenerCount(item12.acceleratorProperty())); + assertEquals(0, getListenerCount(item21.acceleratorProperty())); + assertEquals(0, getListenerCount(item22.acceleratorProperty())); + + sl.dispose(); + } +}