On Wed, 22 Feb 2023 19:24:16 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Each time a menu would change scenes, a new set of ListChangeListeners would >> be added to the items in the menu. The bigger problem however is that these >> list change listeners have a strong reference to the scene which is >> potentially a much bigger leak. >> >> The first commit was more straightforward, but there are 2 things that might >> be of concern: >> >> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but >> it'll remove all the ListChangeListeners attached to it, regardless of which >> scene was passed in. Something similar happens with changeListenerMap >> already, so I think it's fine. >> 2. I made the remove method public so that external calls from skins to >> remove the accelerators would remove the ListChangeListener and also because >> all the remove methods are public. >> >> After I wrote more tests I realised using the ObservableLists as keys in the >> WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would >> be simple. The fix is in the second commit. >> >> There are still more issues that stem from the fact that for each anchor >> there could be multiple menus and the current code doesn't account for that. >> For example, swapping context menus on a tab doesn't register change >> listeners on the new context menu because the TabPane itself had a scene >> change listener already. These other issues could relate to JDK-8268374 >> https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 >> https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to >> the fact that there's no logic to remove listeners when Tab/TableColumn's >> are removed from their associated control (TabPane, TableView, >> TreeTableView). >> >> I'm looking at these issues, but I think they're dependent on this fix. >> Either I can add to this PR or I can wait to see what comes out of this and >> fix them subsequently. > > modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java > line 285: > >> 283: } >> 284: >> 285: private static void removeAcceleratorsFromScene(List<? extends >> MenuItem> items, Scene scene) { > > May I suggest a change name: removeAcceleratorsFromScenePrivate() to avoid > confusion? I'm not in favor of using `Private` in a method name. That is clear from the method signature and overloading methods is a valid choice In my opinion, this is fine as is. But we could also think about naming it: `removeAcceleratorsFromSceneImpl()` ------------- PR: https://git.openjdk.org/jfx/pull/1037
