RFR: JDK-8303897 ObservableValue's when binding should only invalidate when strictly needed [v2]

[Nir Lisker]

On Fri, 7 Apr 2023 06:36:50 GMT, John Hendrikx <jhendrikx at openjdk.org> 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