On Wed, 10 Jan 2024 19:55:35 GMT, Johan Vos <j...@openjdk.org> wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java
>> line 98:
>>
>>> 96: active.set(false);
>>> 97: }
>>> 98: active = new SimpleBooleanProperty(true);
>>
>> should this be
>>
>> if(active == null) {
>> active = new ...
>> } else {
>> active.set(false);
>> }
>>
>> instead?
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1447910710
