RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v21]

[Michael Strauß]

On Thu, 26 Feb 2026 01:39:14 GMT, John Hendrikx <jhendrikx at openjdk.org> 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:
> 
>   Remove unreliable unnecessary extra check

We should probably update the specification in some places. Here are some suggestions:


public interface ChangeListener<T> {

    /**
     * Called when the value of an {@link ObservableValue} changes.
     * <p>
-    * In general, it is considered bad practice to modify the observed value in
-    * this method.
+    * When this method is invoked, {@code newValue} represents the current value of the {@code observable};
+    * that is, it is equal to {@code observable.getValue()} at the time of invocation. The {@code oldValue}
+    * is equivalent to the {@code newValue} of the previous notification delivered to that listener.
+    * <p>
+    * If a change listener modifies the observable value in its callback, other registered change listeners
+    * that have not been notified at that point will receive the newly-modified value. This ensures that a
+    * change listener will always observe the effective change from its last-observed {@code oldValue} to
+    * the current value of the {@code observable} at the time the listener is invoked.
+    * <p>
+    * However, it is usually considered bad practice to modify the observable value from a listener callback
+    * because the order of listener registrations is often not strictly enforceable. This gives earlier
+    * listeners an order-dependent veto over the values that later listeners will observe.
+    *
-    * @param observable
-    *            The {@code ObservableValue} which value changed
-    * @param oldValue
-    *            The old value
-    * @param newValue
-    *            The new value
+    * @param observable the changed {@code ObservableValue}
+    * @param oldValue the last value that was observed by this listener
+    * @param newValue the current value of the {@code observable}
     */
    void changed(ObservableValue<? extends T> observable, T oldValue, T newValue);
}



public interface ObservableValue<T> extends Observable {

    /**
     * ...
+    * <p>
+    * If this method is called from a listener callback while a change notification
+    * is in progress, the added listener will not be notified as part of the ongoing
+    * notification.
     */
    void addListener(ChangeListener<? super T> listener);

    /**
     * ...
+    * <p>
+    * If this method is called from a listener callback while a change notification
+    * is in progress, the removal takes effect immediately and the removed listener
+    * will not be notified as part of the ongoing notification if it has not been
+    * notified so far.
     */
    void removeListener(ChangeListener<? super T> listener);
    ...
}


The following spec seems to contradict the semantics of this PR:

public abstract class ObservableValueBase<T> implements ObservableValue<T> {

    /**
     * Notify the currently registered observers of a value change.
-    *
-    * This implementation will ignore all adds and removes of observers that
-    * are done while a notification is processed. The changes take effect in
-    * the following call to fireValueChangedEvent.
     */
    protected void fireValueChangedEvent();
    ...
}

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-3968231835