On Tue, 18 Feb 2025 18:14:55 GMT, Michael Strauß <[email protected]> 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
