RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]

[John Hendrikx]

On Mon, 25 Mar 2024 13:32:11 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

> `ListenerManager` is an obvious improvement, as it fixes incorrect behavior and allows listeners to veto changes. However, the behavior of `ListenerManager` is also an implementation detail and not documented anywhere. This leads me to the following questions:
> 
> 1. How will users know that they can now do all of the things that were previously broken? Do we need a specification for what is allowed and what's not allowed?

Currently the specification is vague enough that there's a lot of wiggle room. For example, we don't specify whether invalidation listeners are called before change listeners, yet a lot of code will be relying on that unknowingly.  We also don't specify whether successive change listener calls should always be a change (ie. never get `A -> A`), or that it should match with what the previous change reported (ie. if called with `? -> B`, then the next call must be `B -> ?`).

IMHO we should though.  I would specify for example that:

- Invalidation listeners are called before Change listeners (reason: invalidation listeners are a lower level concept defined in a higher level interface).  They definitely should not be mixed (they're defined by two different interfaces).
- Change listeners should (obviously as this MR fixes this) guarantee the old value makes sense:
  - Old value will be equal to previous new value (essential for patterns that use the old value to unregister a related listener)
  - Never called when old value equals new value (it's not a change then) -- this allows vetoing, and generally saves unnecessary calls

We should probably also specify the order of calls (as code will again unknowingly be relying on this already):

- A listener registered after a listener of the same type will always be called after earlier registered listeners (code relies on this in various ways, even in FX itself)
- Listeners of different types follow a fixed order: invalidation first, changes second (code relies on this already)
- The behavior of `ObservableValue`s that contain mutable values (ie. lists/sets/maps/atomics) will be undefined if those values are mutated while held by an observable (same as when you mutate keys that are currently part of a `Set`).

We can also specify some behavior with regards to when an event can be received when adding and removing listeners, although I think that's less of an issue.

> 2. Should this behavior be required for all valid `ObservableValue` implementations? (This would render many existing implementations defective.)

It's hard to require anything in an interface, but I think the interface should specify this regardless.  Just look at an interface like `Set` that requires a specific way of implementing `hashCode`.  You can violate it easily, but you will suffer the consequences when comparing sets of different types.  Same with custom implementations of `ObservableValue`. You take a risk when using some unvetted 3rd party implementation.

At a minimum all implementations in JavaFX should follow the specification.  This will likely cover most implementations of `ObservableValue`, leaving only a few custom implementations that are not 100% spec compliant (note: a lot of the problems only occur with nested changes, which occur only in complicated code that triggers a cascade of changes, usually layout/skin/css related).

A problem there are the Set/List/Map `ObservableValue` implementations.  They are not observable values, they are observable collections that deserve their own interface.  Invalidation listeners are fine, but value listeners make no sense.  I've looked into these before, and all I can say is that they take great liberties with what is considered a "change" (ignoring even the most basic specifications).  I'd recommend deprecating the observable value parts of these, and moving users towards either invalidation or the collection specific change listeners.

> 3. If `ObservableValue` implementations are not required to replicate the `ListenerManager` behavior, we should probably make it easily discoverable whether any particular implementation (most of them are properties) supports nested changes/vetoing. In most of the public API, there's no obvious way to see (without looking at the source code) whether a property implementation extends one of the `*PropertyBase` classes.

I think if the implementation is in `javafx.*` it should be correct.  Anyone can violate any interface (just look at custom collection implementations which often fail to follow the spec).  We could provide a more lenient abstract base class or helper to make it easier to conform to the spec.

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

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