On Mon, 12 Apr 2021 08:40:56 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> The method `ControlAcceleratorSupport.doAcceleratorInstall(final List<? >> extends MenuItem> items, final Scene scene)` adds a `ChangeListener` on >> `MenuItem.acceleratorProperty()`. This listener is not removed when the >> MenuItem is removed from scenegraph. >> Adding and removing a MenuItem results in multiple copies of the listener >> added to MenuItem.acceleratorProperty(). >> >> Fix is to remove the listener when MenuItem is removed. >> Fix can be verified by checking the number of instances of >> `ControlAcceleratorSupport.lambda` using _jvisualvm_. >> Without this fix, the number of `ControlAcceleratorSupport.lambda` increase >> in multiple of number of MenuItems being removed and added back. >> With fix, the count is always same as number of MenuItems in scenegraph. >> >> Also there is another ListChangeListener added to a >> `ObservableList<MenuItem> items` in the method >> `ControlAcceleratorSupport.doAcceleratorInstall(final >> ObservableList<MenuItem> items, final Scene scene)`. There was a TODO note >> to remove this listener. >> This listener is added on `MenuBarButton.getItems()` and not on >> `Menu.getItems()`. This `MenuBarButton` is created by `MenuBarSkin` to show >> a `Menu`. This `MenuBarButton` gets disposed when the related `Menu` is >> removed from scenegraph, and so the added `ListChangeListener` gets GCed. >> Hence it is not required to explicitly remove the listener. >> Added a comment explaining this behavior in place of the TODO. > > Ambarish Rapte has updated the pull request incrementally with one additional > commit since the last revision: > > remove indirect test Marked as reviewed by kcr (Lead). ------------- PR: https://git.openjdk.java.net/jfx/pull/429
