Changes detected by reference equality for all ObservableValues... except String

[John Hendrikx]

Looks like it is even more fuzzy than I thought...

ExpressionHelper (used by almost all property implementations) checks 
before sending out changed events using `equals`:

      final boolean changed = (currentValue == null)? (oldValue != null) 
: !currentValue.equals(oldValue);

What this means is that when you do:

       T a = new T();
       T b = a.clone();

       ObjectProperty<T> x = new SimpleObjectProperty<>(a);

       x.set(b);

The above triggers an invalidation because a != b, but no changes 
because a.equals(b) even though b here is a totally different instance. 
As change listeners weren't called, code may still be referring to a; if 
a is mutated, these changes would be seen by such downstream code, while 
a `getValue` (which would return b) would not see them.

The behavior is "okay" for immutable T's (of which String is one), but 
could still be surprising.

For StringProperty such code would trigger no invalidation and no changes

--John

On 21/03/2023 12:26, John Hendrikx wrote:
> I had a run in today with a (known) issue where StringProperty seems 
> to violate the implementation spec as described by ObservableValue for 
> which I'd really like to investigate a solution:
>
>  * 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.
>
> Yet, in StringPropertyBase#set, we see this:
>
>         if ((value == null)? newValue != null : 
> !value.equals(newValue)) {
>             value = newValue;
>             markInvalid();
>         }
>
> Note the use of `equals` here.  In ObjectPropertyBase (and all the 
> other PropertyBase classes, even something like ListPropertyBase) it's 
> this:
>
>         if (value != newValue) {
>             value = newValue;
>             markInvalid();
>         }
>
> Now, I can understand why this is done, as String is almost regarded 
> as a primitive class in Java, however, it does make downstream 
> properties which may not know they're dealing with Strings and that 
> need to check for changes a bit harder to implement. A downstream 
> property may want to check for changes to prevent an unnecessary 
> invalidation for example.
>
> This could look like this:
>
>         if (isValid() && source.getValue() != getValue()) {
>             invalidate();
>         }
>
> Or this:
>
>             T value = source.getValue();
>
>             if (cachedValue != value) {
>                 cachedValue = value;
>                 invalidate();
>             }
>
> Now, since StringProperty strictly speaking violates the 
> implementation specification for JavaFX provided properties, but not 
> the general contract of an ObservableValue, there are I think two 
> possible actions:
>
> 1) Make StringProperty no longer violate this implementation 
> specification and make it use reference equality; this can mean 
> invalidations and changes are triggered where, according to equals, 
> the old and new value are the same.
>
> 2) Make an exception for String values, as they're almost like 
> primitives in Java, and update the implementation specification
>
> In the first case, downstream properties can continue to use reference 
> equality to do their checks.  In the second case, we may need to 
> change these to specifically use `equals` when a String value is 
> encountered.  Strictly speaking that would mean changing 
> ObjectPropertyBase as well to change its equality:
>
> Old:
>
>         if (value != newValue) {
>             value = newValue;
>             markInvalid();
>         }
>
> New:
>
>         boolean changed = value instanceof String s ? 
> !s.equals(newValue) : value != newValue;
>
>         if (changed) {
>             value = newValue;
>             markInvalid();
>         }
>
> This way of checking equality would best be encoded somewhere that is 
> easy to reach for other code that needs to do the same, so I'd propose 
> a static method on ObservableValue (or somewhere else):
>
>     static <T> boolean isEquals(T a, T b) {
>       return a instanceof String s ? s.equals(b) : a == b;
>     }
>
> I'd like to hear your thoughts on this as it is can have consequences 
> for how the `when` fluent binding prevents invalidations, but also 
> other (future) implementations that sometimes deal with Strings, and 
> sometimes with any other kind of object.
>
> --John
>