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

[John Hendrikx]

On Sat, 18 Feb 2023 19:09:59 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

> > I couldn't quite see which you prefer here; you said "This one makes sense" but not quite sure which version it refers to (I suppose the depth first version?)
> 
> I should have said "this one makes sense **too**". The point was that while your fix is good, it's not the only good fix, and I wasn't sold on choosing the one in this PR just yet. If "breadth-first" is easier to do and requires less memory to remember the old values, then I have no problem with "depth-first". 

Confusing me again here :-)  Did you mean to say "breadth-first" where you said "depth-first" ?

Breadth first is for sure a lot easier, as the old values are much easier to get correct for it.

I've given depth first some more thought, but every approach I think of requires some kind of stack or tracking of which listeners weren't notified yet of the original set that we wanted to notify (and I think we'll need to remember even more if there is another change before the first and second one finishes). 

> I don't have a strict preference for either approach mostly because I don't do nested modifications myself.

I've been wondering how often it happens -- I may be able to put a breakpoint or println in the nested code and see if this happens in a large FX application (in layout code perhaps).

> I'm not at all sure that we need so specify this behavior, but we need to be consistent with it. @arapte Why do you think we should say what order the nested event is dispatched at?

Perhaps we could limit the explanation to just mentioning that its possible the "new value" may not be the same as `ObservableValue#getValue` if nested changes occured.  Also, I'm curious why it would still be a bad practice to modify the value again (now that the old values are always correct); I think it's pretty much the standard pattern to veto a change (like a numeric field rejecting out of range values or non-numeric characters).

> I'm also investigating the effect of this patch in conjunction with addition/removal of listeners in conjunction with a nested event.

Yes, perhaps this may require an additional unit test.

@nlisker if we're going for breadth-first, shall I fix the cases for single listeners as well (finish the notification before calling the same listener again, instead of recursively calling into it?)

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

PR: https://git.openjdk.org/jfx/pull/837