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
In addition to the spec comments, I discovered one implementation bug. If a
property has a single `ChangeListener`, and that listener first adds a second
`ChangeListener` and then changes the value, the second listener will be
notified of the change. If there already were two or more change listeners on
the property, it works as expected.
The following `ObservableValueTest` method, using
`shouldSendCorrectNestedEventsWithOneListener` as a starting point, shows the
bug.
/*
* Tests if the embedded ObservableValue sends sensible change events when
a nested change occurs
* when there is initially one listener that adds a second listener before
making the change.
*/
@ParameterizedTest
@MethodSource("inputs")
<T> void
shouldSendCorrectNestedEventsWithOneListenerThatAddsAnother(Action<T> action, T
value1, T value2, Consumer<T> valueSetter) {
List<Change> changes = new ArrayList<>();
AtomicInteger l2Count = new AtomicInteger(0);
/*
* Create one listener, which adds a second listener and modifies the
value back to value1.
* Verify that the second listener is not called.
*/
action.addListener((_, old, current) -> {
changes.add(new Change("B", old, current));
if (current.equals(value2)) {
action.addListener((_, _, _) -> l2Count.incrementAndGet());
valueSetter.accept(value1);
}
});
// TODO: Remove the following workaround
// WORKAROUND: Uncomment the following to add a dummy listener and the
test will pass
// action.addListener((_, _, _) -> {});
/*
* Start test:
*/
valueSetter.accept(value2);
assertConsistentChangeSequence(changes, value1, value1, Set.of(value1,
value2));
assertEquals(0, l2Count.get());
// TODO: check that a subsequent top-level change will notify l2
}
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-5641978853
