RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs

[John Hendrikx]

On Thu, 13 Apr 2023 15:58:32 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

> John and I discussed this off-list. I will write a short review of this change.

I have some small corrections I think.

> * Nested events are invoked "depth-first", meaning that the parent event propagation is halted until the nested event finishes (see below). **This differs from the current behavior that takes the "breadth-first" approach** - each event finishes before the nested one starts (buggy implementation).

Current behavior in `ExpressionHelper` is also depth first. 

> ### Equality check
> Change events require a comparison method for the old and new value. The 2 candidates are reference equality (`==`) and object equality (`Objects#equals`). There is some inconsistency in JavaFX about how this equality check is made (it is made in a few places on a few different types). It makes sense to do `==` with primitive types, and `equals` with `String` and the primitive wrappers. For other types, it depends on their characteristics. The "safer" option is `==` because a change that is triggered by `!=` can then be tested for `!oldValue.equals(newValue)` in the listener and be vetoed; the opposite is not possible. This might mean that the user will have to give the comparison method that is desired.
> 
> Currently, `==` is used except for `String`. The behavior is preserved in this change, but will be investigated further in order to allows for more sensible change events.

Just to add here, there are actually two checks involved.  When you "set" the value on a property, all properties do a reference equality check (apart from String) to determine whether to fire their listeners.  This means that `InvalidationListener`s always fire in these circumstances.  Change listeners however are protected by a 2nd check that is part of `ExpressionHelper`.  This check uses `equals` for all property types.  This means that the behavior of an `InvalidationListener` + `get` is sometimes subtly different from using a `ChangeListener`.

When looking only at change listeners, this behavior makes sense for any type that is either primitive, immutable or mutable without overriding `equals`.  For types that are mutable **and** override `equals`, the odd situation can occur that no change fires because the two instances are equal, but that the instance reference did change.  When such a type is mutable, any further mutations could be missed.  Simple example:

      List a = new ArrayList<>();
      List b = a.clone();
      ObjectProperty<List> prop = new SimpleObjectProperty<>(a);
      ObjectProperty<List> copy = new SimpleObjectProperty<>(a);

      // keep properties in sync:
      prop.addListener((obs, o, n) -> copy.set(n));
      prop.get().equals(copy.get());  // true :-)

      // change first property:
      prop.set(b);   // no change fired, they're equals!
      
      b.add("Hello");

      prop.get().equals(copy.get());  // false, as prop has reference B, while copy has reference A still...

It doesn't happen too often that properties are used with a type that is mutable with its own `equals` implementation, so this usually works correctly; for cases where you do want to use a mutable type with its own `equals` in an `ObjectProperty` though, I think having a variant of `ObjectProperty` with a reference equality check for its change listeners call may be sufficient.
 
> ### Performance
> Performance both in memory and speed is either equal or slightly worse than the current one. This is because the current behavior is wrong and fixing it entails more complications. In practice, the difference should be small. Registering many listeners on the same observable is not recommended and has caused issues in the past as well. Performance is a WIP and benchmarks will be posted later.

The implementation is able to avoid using a wrapper for single invalidation/change listeners, which improves memory use a bit for the second most common state properties are in (having 1 listener only -- the most common state being having no listeners at all).

As for adding/removing many listeners, I've have changed my stance on this and I don't think we should cater to situations that can have 10.000's of listeners -- even if add/remove performance was much improved, that won't make notifying such high amounts of listeners any better.  Having such high amounts of listeners on a single property is a sign that something is wrong with the design, and even if it were to perform reasonably (which it won't due to the sheer amount of listener calls), it would be better to investigate how to avoid adding so many listeners, perhaps by adding a single listener and distributing the requested information more directly (via a shared model for example).

This implementation will have similar performance when it comes to adding/removing listeners as the current implementation.   It grows and shrinks the listener list on demand, with the major difference being that when the list is locked (due to an ongoing notification) it will avoid modifying the lists in such a way that indices of current listeners change (removals are `null`ed out).  After the list unlocks, the list is compacted if there were any listener additions/removals, and `null`s (and weak listeners) are removed at that time. For this reason it may win out in some cases where listeners are added/removed during notifications, as it does not need to make copies of the listener list, but that is going to be a very rare occurrence.

@nlisker I think I addressed all the issues, do you think it can move forward?

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

PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1507339488
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1625396623