On Tue, 21 Feb 2023 16:31:10 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> This contains the following:
>> - Nested changes or invalidations using ExpressionHelper are delayed until
>> the current emission completes
>> - This fixes odd change events being produced (with incorrect oldValue)
>> - Also fixes a bug in ExpressionHelper where a nested change would unlock
>> the listener list early, which could cause a
>> `ConcurrentModificationException` if a nested change was combined with a
>> remove/add listener call
>> - A test for ExpressionHelper to verify the new behavior
>> - A test for all *Property and *Binding classes that verifies correct
>> listener behavior at the API level (this tests gets 85% coverage on
>> ExpressionHelper on its own, the only thing it is not testing is the locking
>> behavior, which is not relevant at the API level).
>> - A fix for `WebColorFieldSkin` which triggered a nested change which used a
>> flag to prevent an event loop (I've changed it now to match how
>> `DoubleFieldSkin` and `IntegerFieldSkin` do it
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Do not collapse nested changes into a single change
I've also come to the conclusion that a depth first approach is not really
feasible unless you either allow for:
- collapsing some changes into a single change (for the listeners called after
listeners that triggered a nested change)
- incorrect old values
Given three listeners, A, B, C where B changes values to even values, it's
really hard to give the last listeners in the chain (C) correct values.
The implementation before this PR does:
A: 0 -> 1
B: 0 -> 1 [modifies value to 2, triggering nested emission]
A: 1 -> 2
B: 1 -> 2
C: 1 -> 2
C: 0 -> 2
A correct depth first approach would have to avoid sending out `C: 1 -> 2`, in
which case it sees only a collapsed change. Sending `C: 0 -> 1` first somehow
is near impossible to keep track of, and then when it returns to the top level,
it would need to send `C: 1 -> 2`.
For comparison, this PR does:
A: 0 -> 1
B: 0 -> 1 [tracks nested change to 2]
C: 0 -> 1
[first emission finished, next starts, there are no recursive calls]
A: 1 -> 2
B: 1 -> 2
C: 1 -> 2
-------------
PR: https://git.openjdk.org/jfx/pull/837
