On Sat, 18 Feb 2023 23:35:40 GMT, Nir Lisker <nlis...@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
