On Tue, 5 Mar 2024 08:55:41 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> 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)
>
> @hjohn There is an important issue using `when()`, because the listener is
> added to a new property, not to the original property.
>
> Therefore, using an invalidation listener doesn't trigger any more
> notifications if you are changing values more than once to the original
> property.
>
> This can happen in this SystemMenuBar case with the disabled property of any
> given `Menu` or `MenuItem`, if you simply change it twice or more times,
> while the application is active. You will notice that it will remain always
> with the same first state.
>
> It is hard to add a system test in this case because the public Menu/MenuItem
> and the private peers MenuBase/MenuBaseItem are in sync, and only the
> communication to the native part is missing. But it can be easily viewed if
> you run it manually.
>
> Let me clarify it with a very simple test, that reproduces the case of the
> disabledProperty:
>
> mb.disableProperty().when(active).addListener(valueModel ->
> glassMenu.setEnabled(!mb.isDisable()));
>
>
> Simply run this manually:
>
> 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);
>
>
> while you probably would be expecting:
>
>
> b: true
> when: true
> b: false
> when: false
>
>
> the test prints:
>
>
> b: true
> when: true
> b: false
>
>
> so using `when()` we miss the changes after the first invalidation.
>
> I could go into details of why this happens, but just notice that we have a
> _new property_ after all, and if you are not calling `when.getValue()`, it
> doesn't get validated anymore after the first change.
>
> I see three possible fixes to this issue.
>
> 1. If you change the sample to:
>
> BooleanProperty b = new SimpleBooleanProperty();
> BooleanProperty active = new SimpleBooleanProperty(true);
>
> b.addListener(o -> System.out.println("b: " + b.get()));
> ObservableValue<Boolean> c = b.when(active);
> c.addListener(o -> System.out.println("when: " + c.getValue()));
> b.set(true);
> b.set(false);
>
> then you get the expected result:
>
>
> b: true
> when: true
> b: false
> when: false
>
>
> 2. Since this change implies some more refactoring, that is why I suggested
> using the change listener instead:
>
>
> BooleanProperty b = new SimpleBooleanProperty();
> BooleanProperty active = new SimpleBooleanProperty(true);
>
> b.addListener(...
Thanks, you are right. The property generated by `when` is not getting
revalidated, because its specific `getValue` is never called when using an
invalidation listener on it. This seems to be a bit of pitfall that applies to
all such mapped properties (not exclusive to `when`).
I'm fine with any of the options. Option 3 certainly is the most convenient.
I'm more wondering if there should be some action taken in for the `map`,
`flatMap`, `when`, `orElse` constructs to make them work better with
invalidation listeners, or if that's not possible, a note explaining that
adding an invalidation listener to such a construct probably is not doing what
you think it is doing.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1514914613
