On Fri, 24 Feb 2023 10:04:48 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. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added more comments and fixed IdentityWrapper hashcode. Just bumping this to end it for another 4 weeks. @arapte ------------- PR Comment: https://git.openjdk.org/jfx/pull/1037#issuecomment-1495689005
