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

[Nir Lisker]

On Sun, 5 Feb 2023 20:11:35 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

> I don't think we should care about depth-first, breadth-first. The only thing that I think is important here is that the contract of ChangeListener is respected. I think that that contract should be:
...

I'll be more concrete. Here is my test program:


public class ListenersTest {

    private static int inv = 0;

    public static void main(String[] args) {
        with1Change();
    }

    static void with1Change() {
        inv = 0;
        var property = new SimpleIntegerProperty(0);

        ChangeListener<? super Number> listenerA = (obs, ov, nv) -> {
            inv++;
            String spaces = IntStream.range(1, inv).mapToObj(i -> "  ").reduce(String::concat).orElse("") + inv;
            System.out.println(spaces + " bA " + ov + "->" + nv + " (" + property.get() + ")");
            property.set(5);
            System.out.println(spaces + " aA " + ov + "->" + nv + " (" + property.get() + ")");
        };
        property.addListener(listenerA);

        ChangeListener<? super Number> listenerB = (obs, ov, nv) -> {
            inv++;
            String spaces = IntStream.range(1, inv).mapToObj(i -> "  ").reduce(String::concat).orElse("") + inv;
            System.out.println(spaces + " bB "  + ov + "->" + nv + " (" + property.get() + ")");
            System.out.println(spaces + " aB " + ov + "->" + nv + " (" + property.get() + ")");
        };
        property.addListener(listenerB);

        property.set(1);
        System.out.println("---------\n");
    }
}


With the patch:

1 bA 0->1 (1)
1 aA 0->1 (5)
  2 bB 0->1 (5)
  2 aB 0->1 (5)
    3 bA 1->5 (5)
    3 aA 1->5 (5)
      4 bB 1->5 (5)
      4 aB 1->5 (5)

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:

1 bA 0->1 (1)
  2 bA 1->5 (5)
  2 aA 1->5 (5)
    3 bB 1->5 (5)
    3 aB 1->5 (5)
1 aA 0->1 (5)
      4 bB 0->1 (5)
      4 aB 0->1 (5)

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

Without the patch:

1 bA 0->1 (1)
  2 bA 1->5 (5)
  2 aA 1->5 (5)
    3 bB 1->5 (5)
    3 aB 1->5 (5)
1 aA 0->1 (5)
      4 bB 0->5 (5)
      4 aB 0->5 (5)

I agree that at step 4 the 0->5 event is wrong because the events are only 0->1 and 1->5.

If you comment out the line `property.addListener(listenerB);` (only register A), then both with and without the patch I get

1 bA 0->1 (1)
  2 bA 1->5 (5)
  2 aA 1->5 (5)
1 aA 0->1 (5)

while with delaying nested events I would expect:

1 bA 0->1 (1)
1 aA 0->1 (5)
  2 bA 1->5 (5)
  2 aA 1->5 (5)

So this looks inconsistent to me.

The fix for the lock being released is good regardless, it's the behavioral change that I'm not sold on.

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

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