> 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.
Dean Wookey has updated the pull request incrementally with one additional commit since the last revision: Added more comments and fixed IdentityWrapper hashcode. ------------- Changes: - all: https://git.openjdk.org/jfx/pull/1037/files - new: https://git.openjdk.org/jfx/pull/1037/files/585534e5..856edac6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1037&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1037&range=00-01 Stats: 107 lines in 2 files changed: 87 ins; 4 del; 16 mod Patch: https://git.openjdk.org/jfx/pull/1037.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1037/head:pull/1037 PR: https://git.openjdk.org/jfx/pull/1037
