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

[John Hendrikx]

On Wed, 15 Feb 2023 09:46:22 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:
> 
>   Fix no change detection
>   
>   Old text was uppercased while new text is always lowercase.

> With this test
> 
> ```java
>      static void with2Changes() {
>          inv = 0;
>          var property = new SimpleIntegerProperty(0);
> 
>          ChangeListener<? super Number> listenerA = (obs, ov, nv) -> {
>              inv++;
>              String spaces = spaces();
>              System.out.println(spaces + " bA " + ov + "->" + nv + " (" + property.get() + ")");
>              property.set(5);
>              System.out.println(spaces + " aA " + ov + "->" + nv + " (" + property.get() + ")");
>          };
> 
>          ChangeListener<? super Number> listenerB = (obs, ov, nv) -> {
>              inv++;
>              String spaces = spaces();
>              System.out.println(spaces + " bB "  + ov + "->" + nv + " (" + property.get() + ")");
>              property.set(6);
>              System.out.println(spaces + " aB " + ov + "->" + nv + " (" + property.get() + ")");
>          };
> 
>          property.addListener(listenerA);
>          property.addListener(listenerB);
> 
>          property.set(1);
>      }
> ```
> 
> I get
> 
> ```
> 1 bA 0->1 (1)
> 1 aA 0->1 (5)
>   2 bB 0->1 (5)
>   2 aB 0->1 (6)
>     3 bA 1->6 (6)
>     3 aA 1->6 (5)
>       4 bB 1->6 (5)
>       4 aB 1->6 (6)
> ```
> 
> I think that we are missing a 1->5 event originating in listener A, and maybe a 5->6 event. I'm honestly not sure what the behavior should be here.

If you add the missing events, this would be an infinite loop, so that's probably not the best way to go.

I think you really need to look at the emissions as a whole, and then this doesn't look too bad.  First, the indents and counts you use are a bit confusing.  The first emission is:

    Before: 0
    Emission 1 {
      1 bA 0->1 (1)
      1 aA 0->1 (5)
      2 bB 0->1 (5)
      2 aB 0->1 (6)
    }
    Nested triggered: yes
    After: 6

As changes occurred during the first emission, another is done afterwards.  The changes are collapsed; the value is only re-evaluated after a full emission. I think that's probably the best thing we could do (or you'd get an infinite loop).

The second emission is:

      Before: 1
      {
        3 bA 1->6 (6)
        3 aA 1->6 (5)
        4 bB 1->6 (5)
        4 aB 1->6 (6)
      }
      Nested triggered: yes (it's always triggered with this listener setup, they can never agree)
      After: 6

The third emission is empty, as there is a `!newValue.equals(oldValue)` check.  Invalidations may still fire.

I think it would be best to note that if there are multiple change listeners modifying the values, that it is up to the user to ensure they're not conflicting. Even though this case doesn't result in an infinite loop, it could easily have been (and under the old code it actually results in `StackOverflowError`).

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

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