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

[Marius Hanl]

On Tue, 4 Apr 2023 15:22:48 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

> This provides and uses a new implementation of `ExpressionHelper`, called `ListenerManager` with improved semantics.
> 
> # 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 overhead|
> |Multiple listeners|57 + 4 per listener (excluding unused slots)|57 + 4 per listener (excluding unused slots)|61 + 4 per listener (excluding unused slots)|
> 
> # About nested changes
> 
> Nested changes are simply changes that are made to a property that is currently in the process of notifying its listeners. This...

I completely forgot about this PR, but it looks very interesting, especially about the nested events.
If helpful, I can test this soon in a bigger application, especially for any regressions. And of course also review the code, but need more time for that.

Tested with a big application, did not find any regression. All listeners still work as expected, tested especially a lot of 'if something is selected, this button should be enabled/disabled' and the like.
I did not checked the code yet, just a little bit.
One question: Should I test something special/do you see a case which could cause a problem here?

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

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