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

[John Hendrikx]

On Sun, 9 Apr 2023 21:27:36 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:
> 
>   Address review comments
>   
>   - Added extra tests cases that count invalidations
>   - Added explanatory comment

> I like the test coverage now.
> 
> I tried to test the condition
> 
> ```java
>         else if (isValid() && source.getValue() != getValue()) {
>             invalidate();
>         }
> ```
> 
> with this sequence:
> 
> ```java
>                 property.set("e");
>                 condition.set(false); 
>                 property.set("y");
>                 property.set("e");
>                 condition.set(true);
> ```
> 
> I wanted to test the change to the behavior of the condition changing while the binding is valid, and the value is the same. However, the check for the value is done with reference equality, so it will always invalidate (unless I create my own objects and reuse them). Is this to align with the behavior of a change, or will `equals` make more sense here?

Well, I am just following the specification here, but JavaFX itself isn't...

We may need to make a decision here.  JavaFX is not consistent itself where it applies `equals` and where it uses reference equality:

|Class|Equality Type|
|---|---|
|*PropertyBase except String in setValue|==|
|StringPropertyBase in setValue|equals|
|ExpressionHelper before calling ChangeListeners|equals|

The specification on `ObservableValue` says:

> All implementing classes in the JavaFX library check for a change using reference equality (and not object equality, {@code Object#equals(Object)}) of the value.

But I'm afraid that's not true.  While indeed all the PropertyBase classes "setValue" methods (aside from String), use reference equality, this is not the case for the helper they use for notifying listeners (`ExpressionHelper` uses `equals`).

For immutable classes, this is absolutely fine, and this is why it works pretty okay for `StringPropertyBase`.  However, using equals on **mutable** classes is going to land you into trouble, as you may miss reference changes that turn out to be significant later (ie. two instances may be equals now, but they may not be equals in the future, and if you have the wrong reference, you will not have the correct instance).  There is a test that shows how this can fail with an `ObjectProperty<ObservableList>` (although it has nothing to do with the `ObservableList`... `ObjectProperty<List>` would have similar problems): `LambdaMultipleListHandlerTest#testInvalidationOfListValuedObservable`, but it is easy enough to reproduce yourself:

     SomethingMutable instance1 = ...;
     SomethingMutable instance2 = instance1.clone();
     ObjectProperty<SomethingMutable> prop = new SimpleObjectProperty<>(instance1);

     prop.addListener((obj, o, n) -> { store n somewhere });
     prop.setValue(instance2);  // invalidates, but won't inform change listeners as they're equals

     // the value stored by the change listener is now the WRONG instance!!
     
Also see my post of 21/3/2023 "Changes detected by reference equality for all ObservableValues... except String" on the mailinglist.

# Solution Proposal

As said before, `equals` and immutable types are not the problem.  You get in trouble when types are mutable.  So I propose the following: use reference equality everywhere but give String a special status. Why String?  It is immutable and has a special status in Java, similar to primitive types (ie, it has its own operator `+`, and can be declared without `new String`).  Arrays also are special in Java, but these are mutable so reference equality is what we'd want here.

I further propose to put this new form of JavaFX equality in a common class, so it can be used easily whenever you need to do check equality (like in a binding or `ExpressionHelper` for example).  

Testing by reference equality is really the best solution... you may get a change you didn't want, but at least you are kept up to date with the correct instance.  For Strings we can make an exception, but using `equals` for all other object types, which includes mutable types, is going to cause headaches.

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

PR Comment: https://git.openjdk.org/jfx/pull/1056#issuecomment-1501249711