RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v6]

[Andy Goryachev]

On Wed, 10 Jan 2024 19:55:35 GMT, Johan Vos <jvos at 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