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

[John Hendrikx]

On Tue, 14 Feb 2023 16:03:52 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 two additional commits since the last revision:
> 
>  - Fix javadoc link
>  - Update doc

@nlisker Thanks, I had to take a good look what was happening there, but I think you discovered a minor problem, let's talk about that first.  I only patched the "generic" version of `ExpressionHelper` as I figured that only when you have more than 1 listener the problem with incorrect old values can occur.  When you have only 1 listener, it doesn't use the version I patched, so you're seeing the standard behavior there.  It's not strictly incorrect, but it differs from when you have multiple listeners:

- Single listener version will do the nested change call right away, so it stops the current `changed` method, and starts a nested one
- Multi listener version will "queue" up another change, but finishes all `changed` method calls first before doing doing the notifications for the nested change

Now, this is not strictly incorrect from an old value perspective, and is still within specs as well I think, but it does look a bit inconsistent. I could look into making this more consistent (and adding a test to ensure it is).

### Depth vs Breadth first

> With your patch, each event finishes its run and only then the next event happens. This is the "breadth-first" approach.
However, there is another one:

> This approach starts events before the previous ones finished, and goes back to the original event later. This is the "depth-first" approach. I don't think that either is wrong. This one makes sense and it's a behavior I can reason about: the listener is loyal to the event at the time it happened (and the "real" value is accessible with get).

Okay, from a specification standpoint, I don't much have a preference for breadth or depth first.  My main goal was fixing the problem of incorrect old values as it can cause problems with listener (de)registration.  But I do see your point that we may want to choose or the other.  I went for breadth first here as I think it makes a bit more sense (finishing up one notification before starting another) and also is a lot easier to keep track of.  

Depth first may be a bit harder to do; when a nested change occurs you probably should completely stop the current notification and start with the new one.  The problem here is getting the old value correct; if you stopped somewhere in the middle, then the first half needs the original new value as its old value and the modified value as its new value, while the second half needs the original old value as its old value and the modified value as its new value.  The second half also gets only a change old->modified while in reality it was two changes (old->new and new->modified).

I couldn't quite see which you prefer here; you said "This one makes sense" but not quite sure which version it refers to (I suppose the depth first version?)

Let me know which direction you prefer (breadth or depth), and whether or not I should fix the case for the single listeners.  My preference would be: breadth first and make single listeners consistent with how multi listeners handle their events.

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

PR: https://git.openjdk.org/jfx/pull/837