On Thu, 16 Feb 2023 15:15:11 GMT, Dean Wookey <dwoo...@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 149: > 147: while (c.next()) { > 148: if (c.wasRemoved()) { > 149: // remove accelerators from the scene this comment seems unnecessary modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java line 154: > 152: > 153: if (c.wasAdded()) { > 154: > ControlAcceleratorSupport.doAcceleratorInstall(c.getAddedSubList(), scene); ControlAcceleratorSupport. qualifier is not necessary here. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java line 160: > 158: }; > 159: > 160: menuListChangeListenerMap.put(listChangeListener, new > WeakReference<>(listChangeListener)); is it possible this method will be called twice with the same (items, scene) arguments? i.e. is it possible to clobber an already registered listChangeListener? 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? modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java line 336: > 334: } > 335: > 336: // We need to store all the listeners added to each ObservableList > so that we can remove them. For this we need could you convert this comment to a javadoc one please? Eclipse shows the javadoc when one clicks on the type anywhere. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java line 355: > 353: > 354: @Override > 355: public void onChanged(Change<? extends MenuItem> c) { the use of this class as both key and value (the latter overrides this method) was a bit confusing to me. clever, but it took some time to understand. Would it be possible to explain in the comments here and on lines 146 and 160 what is going on? modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java line 366: > 364: public boolean equals(Object o) { > 365: if (this == o) return true; > 366: if (o == null || !(o instanceof > IdentityWrapperListChangeListener)) return false; (o instanceof IdentityWrapperListChangeListener) should take care of the null case. if(o instanceof IdentityWrapperListChangeListener that) { return innerList == that.innerList; } return false; And also, could we have curly braces in all the if statements please? ------------- PR: https://git.openjdk.org/jfx/pull/1037
