On Mon, 20 Feb 2023 15:56:15 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> John Hendrikx has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Ensure change listeners are not called recursively (single and generic) >> - Remove left over System.out.println's >> - Introduce Emission enum to make logic clearer >> >> The logic only has three states, but using two booleans for this allows >> for four states. > > I see that the same unit test has failed on all three platforms after your > latest change. @kevinrushforth Yes, that's correct (well not correct). The cause is that this new notification variant collapses nested changes into a single change (by not recursively calling into the same listener). After I applied the same treatment to the single listener variant in `ExpressionHelper` one of the tests broke. Apparently, `ScheduledService` is doing some nested changes in an edge case that is tested as part of the `callingCancelFromOnSucceededEventHandlerShouldStopScheduledService` test and this latest change breaks it. The reason it breaks is that it makes multiple changes while within a `ChangeListener#changed` call (it wants to do three state transitions in one go, from `SUCCEEDED` to `READY` to `SCHEDULED` to `CANCELLED`) but the new code collapses these to a single change. So it seems that there is at least some JavaFX code that relies on the behavior that even nested changes should result in separate change notifications. I'm now investigating possible solutions to this; as far as I can see, there are three options: 1. Switch to using a depth-first algorithm for listener notification, which is more similar to the current behavior, but may be hard to implement with correct old values. 2. Keep breadth first, but queue up each nested change and notify listeners for each of these. 3. Adjusting how `ScheduledService` does these state transitions to go from `SUCCEEDED` to `CANCELLED`. I didn't see an easy fix for it after an hour or two of looking, and it may not be worth it as collapsing nested changes may be too great a change. I'm currently looking into the first option to see if it can be done depth-first without having to do extensive tracking of which listeners were notified already and which didn't get any yet, to keep old values correct. ------------- PR: https://git.openjdk.org/jfx/pull/837
