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

[John Hendrikx]

On Sat, 18 Feb 2023 23:35:40 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> 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
> 
>      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.

@nlisker @mstr2  I'm going to update this soon with an (IMHO) **much** better solution, that handles all the edge cases.

- Recursive algorithm, listeners with a higher index get a summarized change if earlier listeners are changing values
- No copying of listener lists ever occurs; instead, removing listeners is deferred (they're `null`-ed), and only removed when no notifications are running
- This also means that a removed listener is immediately disabled as it is not part of some copy, so they will **never** receive another notification, nested or otherwise
- An added listener is immediately added and will receive notifications immediately from the top level notification loop (nested loops never get that far as nested loops are stopped when they reach the same index as a higher level loop)
- If after a top level notification completes it was discovered that one or more listeners were removed, the listener list is compacted (removing null elements) -- order is maintained (too much depends on it.  This isn't a copy, but could be optimized if there were many removals (could use `removeIf` for this purpose which may be optimized for `ArrayList`).

The implementation has the following characteristics:

1. For change listeners, the old value received is always the previous new value.
2. The new value received always matches property.getValue()
3. Listeners with a higher index receive less change events when earlier listeners make changes, but the events are all consistent
4. A removed listener will not get another notification
5. An added listener will receive its first notification immediately, although will not see all the nested changes going on (it is the last listener after all, so item (3) applies)
6. As said, no copying or locking of lists

Here's a sample output with 5 listeners, where 0, 2 and 4 just register changes, and 1 uppercases a String and 3 will ensure the String contains 2 characters:

    Notifying 0 of change from A to b
    Notifying 1 of change from A to b
      (listener 1 uppercases "b")
      Notifying 0 of change from b to B
      Notifying 1 of change from b to B
    Notifying 2 of change from A to B
    Notifying 3 of change from A to B
      (listener 3 adds a character "B" -> "Bb")
      Notifying 0 of change from B to Bb
      Notifying 1 of change from B to Bb
      (listener 1 uppercases "Bb")
        Notifying 0 of change from Bb to BB
        Notifying 1 of change from Bb to BB
      Notifying 2 of change from B to BB
      Notifying 3 of change from B to BB
    Notifying 4 of change from A to BB

As you can see, all changes make sense and the final listener only receives the full change.

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

PR Comment: https://git.openjdk.org/jfx/pull/837#issuecomment-1492986002