On Thu, 20 Feb 2025 03:08:33 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
> I've thought about it and I would like to hear what others think. I have a
> behavior in mind that I find intuitive, but it doesn't mean others will. It
> actually includes throwing SOEs when listeners fight to set a value, I don't
> think it's an incorrect behavior (user's fault). Using your Z, A to D order,
> where A and C change values,
>
> ```
> Z 0->1
> A 0->1; set 2
> Z 1->2;
> A 1->2; no change
> B 0->2
> C 0->2; sets 3
> Z 2->3
> A 2->3; if A sets 2 we will get a SOE because of recursive changes; let's
> say A wants value>=2 and not ==2
> B 2->3
> C 2->3; no change
> D 0->3
> D ignores set->2 event since it got a more updated value (tracked by the
> progress value?)
> B, C, D ignore set->1 event since they got a more updated value
> ```
Apart from the nesting you have, this is exactly what this PR currently does.
The problem with the nesting you have is that we can really only track one old
value per nesting. If we need to track individual old values then we need to
wrap each listener and give the wrapper a field with what value was sent last
to it. For reference, this is what the current PR does:
Z 0->1
A 0->1; set 2
Z 1->2;
A 1->2; no change
(exits nesting, original loop has correct old value to send)
B 0->2
C 0->2; sets 3
Z 2->3
A 2->3; if A sets 2 we will get a SOE because of recursive changes; let's say
A wants value>=2 and not ==2
B 2->3
C 2->3; no change
(exits nesting, original loop has correct old value to send)
D 0->3
(removed the "ignore lines", they're not needed)
> This should still preserve the 3 rules you outlined. Each listener reacts
> immediately (depth first) and always in order. This causes the last listener
> to act to be the one that determines the final value, and not the first one.
The amount of nesting really doesn't change much in when a listener acts (aside
from niche edge cases with listener addition/removal on the **same** property,
which I'm now of the opinion that we should not be looking at too closely).
Also I'm not sure what you mean with your last sentence. If `D` were to change
something, every listener would get called again, and again have the option to
change things.
Also note, this kind of value changes with listeners is never going to work
with arbitrary listeners from different sources, unless they have
non-conflicting rules that only move "forward" (ie. if listener A receives X,
and changes it to Y, then receives Z, but also changes that to Y, then we're
moving backwards and we'll not be able to resolve this kind of conflict without
a more elaborate rule system -- in this system it won't result in a SOE (but
maybe it should), but in `ExpressionHelper` it would).
The only real way to enforce a rule on a property is to override `set` (and if
you want rules from multiple sources, construct a different mechanism for it,
`RuleBasedProperty` or something). Currently you can also enforce a rule by
ensuring you install the first invalidation listener (but that's already
"hoping" that the notification system is implemented in a certain way). We may
however want to keep that intact as FX has Controls relying on this (but
updating the controls is an option).
When it comes to value changes, you can only have it one way; either last wins
and earlier listeners may be unaware of a value being reset, or first wins and
later listeners may be unaware of a value being reset. Neither will be
intuitive; given the way listeners are currently already used for "veto" style
behavior, I think it would be far better to go for the "first wins" option, as
anyone can always install a later listener breaking whatever you intended in
the last-wins case.
So I think the questions we need to answer are:
<h3>Are we going to specify the "vetoing" behavior in such detail that users
may rely on this? </h3>
<h3>Should conflicting modifications in listeners lead to an exception or
warning log instead of silent failure? </h3>
To clarify that last question; if an earlier listener persistently changes a
value to 3, and a later listener persistently changes it to 4, then the earlier
listener currently wins, and when notifying the later listener, we notice that
the old value is the same as the new value so its 2nd notification is elided
(ie, it received `0 -> 3` but changes it to `4`; the first listener sets it
back to `3` again; now we need to notify the later listener with `3 -> 3` which
we don't do) -- this leads to this listener being under the impression the
value is 4 (the value that it set), while it has changed back to 3.
Note: all cases where two listeners are in conflict, and are kept informed of
ALL changes (breaking Rule 3 for example) will lead to SOE. I think our main
problem is that this doesn't happen in the current implementation, but instead
the conflict is "resolved" silently, with later listeners having the wrong
impression. They can check this though if they really want, but doing this:
laterListener = (obs, ov, nv)-> {
property.set(4); // triggers nested notification
if(property.get() != 4) {
// oops, someone changed it back or changed it again...
// it may be possible to resolve that here, but NOT by changing
it to 4 again...
}
}
I think a good rule is that only one listener should be changing values, if any
at all; anything more, and you're going to need a system that is not as
simplistic as a notification system.
> The ignored values end up being the later ones. This, of course, doesn't deal
> with removal/addition of listeners. I think that removal should work
> immediately as it does now, but still allow the immediate notifications
> prior. About addition I'm still not sure.
Remember, this is adding/removing of listeners on the same property by a
listener on that property. It probably never happens in current code as it
does not make a lot of sense; `ExpressionHelper` does not handle this
gracefully either, just differently. Adding/removing listeners on other
properties will work as expected.
> I want to take a boarder view again for a moment. I went back to #837 to make
> sure I didn't drift off course. The fixes for change listeners only pertain
> to nested events, right? Without nested events, even the current
> `ExpressionListener` works correctly, yes?
That's correct. This PR fixes old values for nested cases, as bad old values
(or duplicated new values) can be very harmful for the common case of managing
a chain of property listeners (ie. node -> scene -> window). If there were
even a single nested change there by some user listener, the chain will end up
broken with too few or too many listeners as one of the three rules I outlined
would be broken and code isn't expecting this.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-2671611805
