On Tue, 5 Mar 2024 07:34:32 GMT, John Hendrikx <jhendr...@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
