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

[Jose Pereda]

On Tue, 5 Mar 2024 07:34:32 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java line 226:
>> 
>>> 224:         mb.textProperty().when(active).addListener(valueModel -> glassMenu.setTitle(parseText(mb)));
>>> 225:         mb.disableProperty().when(active).addListener(valueModel -> glassMenu.setEnabled(!mb.isDisable()));
>>> 226:         mb.mnemonicParsingProperty().when(active).addListener(valueModel -> glassMenu.setTitle(parseText(mb)));
>> 
>> Since you are not calling `get()` from the `when` resulting observable (to allow for invalidation), these need to use a `ChangeListener` instead of an `InvalidationListener`, otherwise, while `active` doesn't change, any change in those menuBase properties will trigger just one notification. 
>> 
>> Alternative, just replace `addListener` with  `subscribe` (as it internally uses a `ChangeListener`).
>> 
>> @hjohn does this make sense to you?
>
> This could be a problem normally, but I think in this case you won't be able to get this to produce incorrect results.
> 
> `parseText` is accessing the property (and it calls `isMnemonicParsing`), but only if the text is not empty.  It's sort of "safe" as an empty text can't contain a mnemonic anyway, and you'd need to change the text at a minimum first to see the effect of turning mnemonic parsing on/off.  Changing the text would trigger the `textProperty` listener, which will call `parseText` and read the mnemonic property.
> 
> A change listener would be more obviously correct, but it is not strictly needed here.
> 
> (not all `subscribe`s use change listener, the one that takes a `Runnable` uses invalidation)

There is an issue using `when()`, because you add the listener to a new property, not to the original property. Therefore, using an invalidation listener doesn't trigger notifications if you changing values more than once to the original property.
Simply run this test:

        BooleanProperty b = new SimpleBooleanProperty();
        BooleanProperty active = new SimpleBooleanProperty(true);

        b.addListener(o -> System.out.println("b: " + b.get()));
        b.when(active).addListener(o -> System.out.println("when: " + b.get()));
        b.set(true);
        b.set(false);


It prints:


b: true
when: true
b: false


so with `when` we miss the changes! (I can go into details of why this happens, but just notice we have a new property after all, and not calling `when.get()` it doesn't get validated anymore).

This can happen in this SystemMenuBar case with the disabled property of any given `Menu` or `MenuItem`, if you simply change it twice, while the application is active. 

My proposal of using `subscribe` implied using the `Consumer` (therefore the "replacement").

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1512390742