On Tue, 15 Sep 2026 14:57:00 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:
>
> Implement notifying boolean
modules/javafx.base/src/main/java/javafx/beans/Observable.java line 48:
> 46: *
> 47: * @implNote
> 48: * <p>
The paragraph break is unnecessary here.
modules/javafx.base/src/main/java/javafx/beans/Observable.java line 57:
> 55: * <li>A listener that is removed while a notification is in progress
> is not
> 56: * notified as part of that notification if it has not been
> notified yet, and
> 57: * is not renotified if it has already been notified.
If a listener has already been notified, and then it is removed, what could it
possibly be re-notified of? I understand that you're saying it's _not_ notified
again, but I don't understand why you would even say that.
modules/javafx.base/src/main/java/javafx/beans/Observable.java line 59:
> 57: * is not renotified if it has already been notified.
> 58: * <li>Nested notifications are depth-first, and only renotify the
> listeners
> 59: * that have already been notified.
I would remove this guarantee. Not because it's wrong, but because I feel it is
hard to understand without a thorough explanation and I'd be hard-pressed to
even come up with a legitimate scenario in which application code would depend
on such minutiae.
modules/javafx.base/src/main/java/javafx/beans/Observable.java line 65:
> 63: * and {@link javafx.collections.ObservableSet}, and the collection
> property
> 64: * classes and collection binding classes, do not provide all of the
> guarantees
> 65: * above: for these, a listener that is removed while a notification is in
Instead of listing the guarantees for collections here again, just linking to
the relevant collection classes would centralize the specification for those at
the place where it belongs.
modules/javafx.base/src/main/java/javafx/beans/value/ObservableListValue.java
line 36:
> 34: * {@link ObservableValue}. A {@code ChangeListener} registered on an
> 35: * {@code ObservableListValue} is notified when the contents of the list
> change
> 36: * as well as when the list reference is replaced. When the contents
> change, the
I think that invoking a change listener when only the _contents_ of a list has
changed is a jarring defect. A change listener should only be invoked when the
list instance is changed; for its contents, we have `ListChangeListener`. Now,
I'm not sure if we can ever fix this, but maybe we shouldn't enshrine this
defect into specification with this PR. If we come to the conclusion that it
can't be changed, we can always specify this behavior later.
modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line
85:
> 83: * <ol>
> 84: * <li>All bindings and properties in the JavaFX library support lazy
> evaluation.
> 85: * <li>Properties in the JavaFX library invalidate when the value
> they hold
According to Merriam-Webster, "invalidate" is a transitive verb, but in this
sentence it is used without a direct object. I would recommend "become invalid"
or "are invalidated" here and in other places.
modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line
104:
> 102: * {@code oldValue} is equal to the value that was reported as
> {@code newValue}
> 103: * in the previous notification delivered to that listener.
> 104: * <li>If a change listener modifies the value in its callback, the
> change
It might be clearer to say "ObservableValue" instead of "value".
modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line
105:
> 103: * in the previous notification delivered to that listener.
> 104: * <li>If a change listener modifies the value in its callback, the
> change
> 105: * listeners that have not been notified yet observe the
> modified value; an
"have not been notified yet" -> "have not yet been notified"
modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line
109:
> 107: * A veto may also restore the value to the value that was
> current before the
> 108: * change, in which case the change listeners that have not been
> notified yet
> 109: * are not notified at all.
The last sentence reads a bit repetitive. Suggestion:
"A veto may also restore the value that was current before the change, in which
case the remaining change listeners are not notified at all."
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4067411793
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4067399871
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4067422459
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4067456726
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4067445455
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4067496633
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4067508585
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4067532441
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4067531358
