On Fri, 7 Apr 2023 06:36:50 GMT, John Hendrikx <[email protected]> wrote:
>> Description copied from issue:
>>
>> There are up to two additional invalidations performed that really should be
>> avoided, causing downstream fluent bindings to be recomputed with the same
>> values. This is very confusing as these should only be called when there is
>> an actual change, and not called for the same value multiple times in a row.
>>
>> These two extra invalidations have two different causes, each causing an
>> additional invalidation to be triggered:
>>
>> 1) ObjectBinding's `isObserved` is delayed slightly. When you add a
>> listener, the listener is added internally and the binding is made valid;
>> this triggers some downstream activity which checks the `isObserved` status
>> to decide whether to start observation of properties -- unfortunately this
>> still returns `false` at that time. A work-around for this existed by
>> calling `getValue` again in `LazyObjectBinding` with a huge comment
>> explaining why this is needed. Although this works, it still means that a
>> downstream function like `map` is called an additional time while it should
>> really only be called once.
>>
>> The solution is to ensure `isObserved` returns `true` before the
>> `ExpressionHelper` is called. Already verified this solves the problem.
>> This also means the work-around in `LazyObjectBinding` is no longer needed,
>> which seems like a big win.
>>
>> 2) The second additional call originates from a different issue. When
>> `ConditionalBinding` (which implements the `when` construct) sees its
>> condition property changing, it always invalidates itself. This is however
>> only necessary if the current cached value (if it was valid) differs from
>> the current source value. To prevent an unnecessary invalidation, and the
>> resulting revalidation calls that this will trigger, a simple check to see
>> if the value actually changed before invalidating solves this problem.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix review comments
About the second bug. I tested that there are no extra invalidation events
after the patch that were occurring without it. Isn't this something there
should be a test for (a counter for invalidation events)?
modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueWhenTest.java
line 51:
> 49:
> 50: @Test
> 51: void shouldNeverCallDownstreamMapFunction() {
I would add a comment that says why it should never call it. Since this applies
to the `StartsTrue` method too, you can put that comment on their class.
modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueWhenTest.java
line 81:
> 79: condition.set(true);
> 80:
> 81: assertEquals(List.of(), observedMappings);
Is this part necessary? The method that starts with `true` already checks the
transition to `false`. That is, after line 63 `condition.set(true);`, aren't we
at the exact same state that the other method starts at?
Same comment for the observed variant.
modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueWhenTest.java
line 127:
> 125: @Nested
> 126: class WhenObserved {
> 127: @Nested
New line for consistency.
modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueWhenTest.java
line 139:
> 137: property.when(condition)
> 138: .map(x -> { observedMappings.add(x); return x; })
> 139: .addListener((obs, old, current) ->
> observedChanges.add(old + " -> " + current));
What do you think about a variant with an invalidation listener?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1056#pullrequestreview-1376933269
PR Review Comment: https://git.openjdk.org/jfx/pull/1056#discussion_r1161311512
PR Review Comment: https://git.openjdk.org/jfx/pull/1056#discussion_r1161312432
PR Review Comment: https://git.openjdk.org/jfx/pull/1056#discussion_r1161311571
PR Review Comment: https://git.openjdk.org/jfx/pull/1056#discussion_r1161311846
