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

[John Hendrikx]

On Tue, 21 Feb 2023 16:31:10 GMT, John Hendrikx <jhendrikx at 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