On Fri, 11 Sep 2026 23:48:56 GMT, Kevin Rushforth <[email protected]> wrote:
> This will be a great improvement to the listener notification behavior of > properties and bindings, so I'm looking forward to getting it integrated soon. Thank you for the review, you did uncover a nasty edge case that I do have a solution for but would like to gather some feedback on first (see other comment). > I reviewed the spec changes and have a few high level comments. > > The following behaviors are now specified: > > * `newValue` is the current value at invocation > * `oldValue` is documented as the preceding observed value, although this > doesn't allow for it being the initial value when the listener is first > notified; additionally, setting a value that is "equals" but not "==" to the > previous will suppress that notification, but cause a subsequent notification > to use the newer of the two equal-but-not-same objects (this is a preexisting > inconsistency that affects the new spec). I think since we've always defined change listeners in terms of `equals` (we don't fire if things are `equals`) this is not really inconsistent. Whether the old value reported is the oldest `==` variant or the newest `==` variant should not matter and I think the system can lean either way here. Tracking the oldest `==` variant runs the risk of holding on to some old stale reference. > * Later change listeners are documented as seeing changes made by earlier > change listeners (including the ability to veto, which prevents later > listeners from seeing intermediate values) > * The behavior of adding and removing change listeners during notification > > Implied, but not specified: > > * A ChangeListener will never be called with `oldValue` equal to `newValue` > (should this be made explicit?) We could do that, but that does break for the observable list/map/sets which don't truly provide old values (arguably, they should never have allowed change listeners, only their specific variants and invalidation listeners) > Most of the above behaviors of a ChangeListener only apply to > ObservableValueBase and other JavaFX properties and bindings that were > migrated to use the new listener manager implementation (not, for example, > JavaBeanObjectProperty, collection property classes, such as ListProperty, > and collection bindings, such as ListBinding). One solution would be to move > the guarantees to ObservableValue and list the classes that apply those > guarantees, perhaps in an @implNote. Agreed. > The following were discussed in the PR, but not currently specified. The > questions I have are: which of these should be specified? For the ones that > we want to specify, should they be done as part of this PR or as a separate > doc task? > > * invalidation listeners are called before change listeners We could specify this, as changing this now or in the future would likely cause subtle breakage in all non-trivial current FX applications. It is very easy to rely on this ordering accidentally. > * within each group (invalidation, change), the listeners are called in the > order they were registered Same as above; we could specify this as changing it now or later is likely to subtly break existing applications. In this case it also aligns with the veto behavior which would be hard to do if these lists were unordered. > * depth-first nested notification This is I think the only choice (as breadth first would require delaying notifications somehow which may surprise the user as `getValue` may be ahead of what was notified so far). So I guess we can specify it (`ExpressionHelper` was the same, and I tried a breadth first approach and it didn't seem viable as an alternative). > * For nested notifications > > * only those change listeners and invalidation listeners that have already > been notified will be notified again > * later change listeners receive a collapsed notification that omits > intermediate values I think this is part of the change listener docs, but we'd need to add it to the invalidation listener docs as well. The 2nd one is a consequence of the guarantees we make for change listeners (old value = previous new value AND getValue showing what was provided as new value). Not sure if we need to call that out explicitly. > * Addition / removal behavior of invalidation listeners This is specified for add/remove change listeners, but should also be specified for invalidation listeners. I'll fix it. > * The non-convergence warning We could mention it, but keep it unspecified "...may log a warning...", best effort like concurrent modification exception. I think all these spec changes can be done as part of this PR. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-5657973430
