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

[John Hendrikx]

On Tue, 18 Feb 2025 18:14:55 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

> Should we document somewhere that safe vetoing is only possible with the first `InvalidationListener`, and not with a `ChangeListener`? This would also require us to specify that invalidation listeners are always invoked before change listeners.

Specifying that invalidation happens before change callbacks is something we probably should do no matter what; in my earlier experiments I found that messing with this order breaks a lot of code in FX itself (couldn't get it to build), let alone in code that uses FX.

The term "vetoing" is something I probably should not have used.  I just used it to indicate a value was changed within a notification callback for the same property; its not an actual veto system (if listeners are unaware of each other, a veto system can easily get deadlocked with conflicting requirements).  So yes, a listener that is not the first listener can't truly enforce that a property value meets some restrictions.  The correct way to do vetoing is to override `set` on the property (although if you created the property and registered its first invalidation listener, you can get away with it); both this implementation and `ExpressionHelper` will do a final `equals` before notifying change listeners, and if `set` simply refuses to modify the value due to restrictions, no change notification will go out (you may see invalidations though).

The fact that a change of value within a callback can result in later listeners to not be called ("vetoing") is just something that arises from providing correct old values.  The rules I've been using for old and new values are:

Rule 1: Old value received in callback X must match new value received in callback X-1
Rule 2: Old value should never `Object::equals` new value (collection properties break this rule)
Rule 3: The received new value is the same as `property::get`

If you agree with these rules, then we can see what this means for the implementation.

>From these rules it arises that a later listener may not need to be informed if an earlier listener changed a value.  Given two listeners A and B, if the last call to B was "5 -> 6", then if another value change occurs (to 7) and A changes the value back to 6, then we have a few options:

1. Call B with `6 -> 6`
2. Call B with `7 -> 6`
3. Call B with `6 -> 7` followed by `7 -> 6`
4. Don't call B

When looking at the above options, remember that `B` was last called with a new value of `6`.

Option 1 is avoided by `ExpressionHelper`, it will never do this (but in order to do so will provide bad old values).  I think it makes sense for the new implementation to also avoid this as there is little use for telling a listener "Hey, nothing changed"

Option 2 is basically what `ExpressionHelper` does, and breaks Rule 1 -- B never saw a value of `7`, it comes out of nowhere, and it may take actions based on it that would be wrong (trying to remove a listener from property 7, then adding (another) listener on property 6 .. now there are two listeners on 6)

Option 3 could be a sensible option, but does violate Rule 3; the received new value is not the same as the value obtained from reading the property.  This may be very surprising (it looks like a concurrent change, while FX is single threaded). If we choose this path, it may break code that uses change listeners just for the callback, and uses `property::get` to read the value (perhaps in something the listener calls, without passing the actual new value, or even some mix of both)

Option 4 is what this PR implements, and does not break any of the rules that I think make sense for a good change listener callback.

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

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