On Wed, 10 Jan 2024 23:20:23 GMT, John Hendrikx <[email protected]> wrote:
>> 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 l...
> 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.
Yeah, the replacing of the property used in `when` is not a normal scenario.
Normally, the property used in the `when` goes out of scope at the same time as
the rest of the references, in cases like:
label.textProperty().when(userInterfaceIsShowing).addListener( ... );
The `userInterfaceIsShowing` is then part of a `Node` based container, or is
not referenced anywhere at all (it's just derived from say
`node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::isShowingProperty)`.
It is important to realize that `when` uses two (normal) listeners:
- One on its condition property (`active`), which is not weak and is **never**
removed (iirc, it can't be weak as otherwise the observable created by `when`
could get GC'd when the condition is `false`, but shouldn't have been as the
condition may yet become `true` again).
- One on its parent property (x in `x.when( ... )`) -- this listener (and thus
its hard reference) is only present when the condition evaluates to `true`
In this specific case there is nothing that tells us that the menu disappeared,
so the best we can do is that when a **new** menu is created that at that time
we discard the old one. To be able to discard it, we unfortunately need to
keep a hard reference to the `active` property we used (so it can be set to
`false`). However, the `when` has a listener on that `active` property (as it
needs to know when the condition changes), and a listener points back to the
observable value that `when` created (it is not, and can't be, a weak
listener). So in order to also cut that hard reference, we need to replace the
`active` property as well after setting it to `false` (if you don't set it to
`false`, there will be a listener on the parent of `when` that keeps a hard
reference still... setting it to `false` removes that listener first).
So definitely not your run-of-the-mill `when` example, but it will at least
always behave deterministically as there are no weak listeners used anywhere.
If there is a bug, it will be easy to reproduce on someone else's machine.
The illustration I added earlier above should hopefully make the above a bit
more clear, but of course some documentation in the code to explain this may be
good too.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1448115136
