On Wed, 10 Jan 2024 20:05:16 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> No, the explicit goal of this construction is to set active to false (in
>> case it exists) so that existing listeners can be released; followed by
>> creating a new active property that is by default set to true until
>> `setMenus` is called again (e.g. after unfocus/focus events).
>> I noticed however that it is not as simple as that, and the current PR
>> introduces regression, as the `SystemMenuBarTest.testMemoryLeak` test is now
>> failing. Listeners are not only registered when `setMenus` is called, but
>> also when menu(Item)s are added to menus. I'm not sure if a single `active`
>> property can address this, as we don't want to remove all listeners for all
>> menuitems in case a particular menuitem is added to a particular menu.
>> @hjohn does that make sense?
>
> thank you for clarification!
>
> perhaps we ought to add a comment explaining why: it's one thing to create a
> single chain of conditions linked to a property, but here we keep creating
> new property instance, so the side effects are not that obvious.
> I noticed however that it is not as simple as that, and the current PR
> introduces regression, as the SystemMenuBarTest.testMemoryLeak test is now
> failing. Listeners are not only registered when setMenus is called, but also
> when menu(Item)s are added to menus. I'm not sure if a single active property
> can address this, as we don't want to remove all listeners for all menuitems
> in case a particular menuitem is added to a particular menu.
@hjohn does that make sense?
So, if I understood correctly, that test is dealing not with the entire menu
bar disappearing (for which the `active` property ensures clean up), but with a
submenu getting cleared, and then a new item added. Any old items that were
added should be garbage collectable after clearing.
This is a bit tricky. Basically I think it means that the listeners on a
certain `MenuBase` wrapper should be removed in two cases:
1. When the entire menu is no longer needed (what we have now)
2. When the `MenuBase` in question is removed from its parent
I think this may be resolved in one of two ways:
1. Create a specific active condition for `when` that includes a check on
parent and on the top level `active` property
2. Gather all listeners added into a single `Subscription` (currently the
`Subscription` that is returned is discarded, track this somewhere (probably
easiest in a the user property map) taking care not to create a new hard
reference (not sure if that will be an issue). The `Subscription` can then be
obtained and unsubscribed early.
Now I think option 1 can be achieved by doing something like the code below.
**Note:** completely untested, and did not check yet if we aren't creating any
new problematic hard references; I can test this out over the weekend if you
like.
// given the global `active` property...
// and a MenuBase mb, for which we can discover if it is a root menu or
not...
// and which exposes a new property hasParentProperty (you can derive this
property
// if you like by doing `parentMenuProperty.map(Objects::nonNull)`)
BooleanProperty forWhen = active;
if (mb "is not a root menu") { // important we don't do this for root
// include "and" condition that it also must have a parent to be
considered "active"
forWen = active.flatMap(b -> b ? mb.hasParentProperty() : active);
}
// then use `forWhen` in your `when` conditions instead of `active` as
before.
What the above does is that using `flatMap` it creates a listener on
`hasParentProperty`. If it changes, the entire expression result may change.
In normal Java code it would simply be:
boolean forWhen = active && mb.hasParent();
... except of course that the above code would not react to changes.
The logical and (`&&`) is achieved with the ternary expression. If `b` is
`true` then we need to check the second part of the `&&` which is
`hasParentProperty`. If `b` was already `false`, then the there is no need to
check further. In any case, a new **property** must be returned from
`flatMap`, so that will either be `hasParentProperty` in case `active` is
`true` or the `active` property (which then must be `false`).
Such scenario's are rare, but I've been considering adding specific support on
`ObservableValue` for this.
There is also the `and` function on `ObservableBooleanValue` -- you can test
this one as well, but I fear that it may be using a weak listener in there and
that it may get collected early if you don't reference the intermediate
expressions. There is no such risk with `flatMap`.
I haven't looked further into option 2.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1448099725
