On Wed, 22 Jul 2026 11:42:48 GMT, John Hendrikx <[email protected]> wrote:
>> This provides and uses a new implementation of `ExpressionHelper`, called >> `ListenerManager` with improved semantics. >> >> See also #837 for a previous attempt which instead of triggering nested >> emissions immediately (like this PR and `ExpressionHelper`) would wait until >> the current emission finishes and then start a new (non-nested) emission. >> >> # Behavior >> >> |Listener...|ExpressionHelper|ListenerManager| >> |---|---|---| >> |Invocation Order|In order they were registered, invalidation listeners >> always before change listeners|(unchanged)| >> |Removal during Notification|All listeners present when notification started >> are notified, but excluded for any nested changes|Listeners are removed >> immediately regardless of nesting| >> |Addition during Notification|Only listeners present when notification >> started are notified, but included for any nested changes|New listeners are >> never called during the current notification regardless of nesting| >> >> ## Nested notifications: >> >> | |ExpressionHelper|ListenerManager| >> |---|---|---| >> |Type|Depth first (call stack increases for each nested level)|(same)| >> |# of Calls|Listeners * Depth (using incorrect old values)|Collapses nested >> changes, skipping non-changes| >> |Vetoing Possible?|No|Yes| >> |Old Value correctness|Only for listeners called before listeners making >> nested changes|Always| >> >> # Performance >> >> |Listener|ExpressionHelper|ListenerManager| >> |---|---|---| >> |Addition|Array based, append in empty slot, resize as needed|(same)| >> |Removal|Array based, shift array, resize as needed|(same)| >> |Addition during notification|Array is copied, removing collected >> WeakListeners in the process|Appended when notification finishes| >> |Removal during notification|As above|Entry is `null`ed (to avoid moving >> elements in array that is being iterated)| >> |Notification completion with changes|-|Null entries (and collected >> WeakListeners) are removed| >> |Notifying Invalidation Listeners|1 ns each|(same)| >> |Notifying Change Listeners|1 ns each (*)|2-3 ns each| >> >> (*) a simple for loop is close to optimal, but unfortunately does not >> provide correct old values >> >> # Memory Use >> >> Does not include alignment, and assumes a 32-bit VM or one that is using >> compressed oops. >> >> |Listener|ExpressionHelper|ListenerManager|OldValueCaching ListenerManager| >> |---|---|---|---| >> |No Listeners|none|none|none| >> |Single InvalidationListener|16 bytes overhead|none|none| >> |Single ChangeListener|20 bytes overhead|none|16 bytes overhe... > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix logic error, luckily 5000 tests caught it > > The following behaviors are now specified: > > > > ... > > * `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. Agreed. > Whether the old value reported is the oldest `==` variant or the newest `==` > variant should not matter I think you mean the oldest `equals` variant vs the newest `equals` variant (references that are `==` are indistinguishable, by definition). I agree that it doesn't really matter. The only reason I brought this up is to keep in mind when reviewing the final docs. As long as the docs give some wiggle room on this, we are fine. > > * 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) Yes, good point. So we could either leave it implied, or specify that it doesn't apply to observable collections. > > 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. That seems best. > > * 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. Yes, I like the idea of specifying both of the above, since, as you say, many applications would break if either of these changed. > > * 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). OK. > > * 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. I am fine with not specifying the 2nd. > > * 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. Good. > > * The non-convergence warning > > We could mention it, but keep it unspecified "...may log a warning...", best > effort like concurrent modification exception. I also think this is fine not to specify it in the docs. You can mention it in the CSR. > I think all these spec changes can be done as part of this PR. Great. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-5663695214
