On Tue, 9 Jan 2024 16:21:39 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Johan Vos has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Cleanup test
>> - Add shim class so that we can access the references to
>> com.sun.glass.ui.Menu instances.
>> Add a test to make sure those references are gone.
>
> 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?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1447895647
