On Tue, 19 Dec 2023 12:24:14 GMT, Johan Vos <j...@openjdk.org> wrote:
> As for the memory leak issue: there were several. The first commit in this PR
> fixed a clear memory leak, but the one that is still left is not described in
> the issue. It occurs because whenever the SystemMenuBar is shown after it was
> not shown, all MenuItems will be readded using `GlassSystemMenu.setMenu`.
> This will invoke `GlassSystemMenu.insertMenuItem(... MenuItemBase
> menuItem...)` Inside this method, platform-specific menuItems will be
> created, that will adapt when the `MenuItemBase` properties are changing. The
> annoying thing, as stated in the first comment of that method, is that we
> don't know if the passed `MenuItemBase` instance was previously used in this
> method (in which case the listeners are regisered to this instance) or not.
> That is why for example the `visibilityListener` is unconditionally removed
> (even if we don't know if it was ever added) and then added. We can do the
> same for the other properties (preventing the use of weak listeners), but it
> would be nice if we
could address this case using the api introduced in the deterministic
listeners management (I would very much prefer that over weak listeners). I'm
not too familiar with the new API though, but it seems difficult to do this
_inside_ this method, as we would still add a listener each time the method is
invoked with an item that already has a listener registered?
It's still a bit unclear for me. So what I write below may be off the mark.
>From what I can see, `setMenus` will first remove all items, then re-add them.
> The `clearMenu` function seems like a logical place to remove listeners that
>may be present. The problem is where to keep track off the listeners. I see
>a few options:
1. Track the listeners in a few `Map`s in `GlassSystemMenu`, something like
`Map<MenuBase, InvalidationListener>`, with a different `Map` for each listener
type (text, disabled, mnemonic, etc). Or something more elaborate with a
listener holder, nested maps, etc.
These maps can then iterated when `setMenus` is called to clear out ALL the
listeners (or as part of `clearMenu`.
2. Similar to above but store the listener information in the properties of
`Menu`
3. Using the new resource management system. Attach all listeners with a `when`
condition based on a `SimpleBooleanProperty` that you **replace** each time
`setMenus` is called (replacement is needed in this case as we don't know if
the menus were used before or not, but it is still relatively clean).
The last one works like this:
Create a new non-final field: `BooleanProperty active`
In `setMenus` do:
if (active != null) {
active.set(false); // this releases all listeners attached with
`when(active)`
}
active = new SimpleBooleanProperty(true); // replace active with a new
property, as we can't reuse it
Then attach all listeners conditionally like so:
mb.textProprty().when(active).addListener( ... );
The idea here is that because `active` is a different property each time
`setMenus` is called, all old listeners will be removed when active goes to
`false`, and will be eligible for GC because the `active` property was also
replaced (and the old one, which we just set to `false`, is not referenced
anywhere anymore).
Illustration:
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1283#issuecomment-1862755206
