On Sat, 18 Feb 2023 19:09:59 GMT, Nir Lisker <nlis...@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
