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
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.
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).
* 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?)
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.
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
* within each group (invalidation, change), the listeners are called in the
order they were registered
* depth-first nested notification
* 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
* Addition / removal behavior of invalidation listeners
* The non-convergence warning
modules/javafx.base/src/main/java/javafx/beans/value/ChangeListener.java line
53:
> 51: * that is, it is equal to {@code observable.getValue()} at the time
> of invocation. The {@code oldValue}
> 52: * is the value that was reported as {@code newValue} in the previous
> notification delivered to the same
> 53: * listener.
This is only true for the classes that now use the listener manager
implementation. Also, for the first notification, the `oldValue` was not
reported as `newValue` in a previous notification because there was none.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1081#pullrequestreview-5184276530
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3994243339
