On Sun, 5 Feb 2023 20:11:35 GMT, John Hendrikx <jhendr...@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
