On Tue, 3 Jan 2023 09:42:05 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> This contains the following: >> - Nested changes or invalidations using ExpressionHelper are delayed until >> the current emission completes >> - This fixes odd change events being produced (with incorrect oldValue) >> - Also fixes a bug in ExpressionHelper where a nested change would unlock >> the listener list early, which could cause a >> `ConcurrentModificationException` if a nested change was combined with a >> remove/add listener call >> - A test for ExpressionHelper to verify the new behavior >> - A test for all *Property and *Binding classes that verifies correct >> listener behavior at the API level (this tests gets 85% coverage on >> ExpressionHelper on its own, the only thing it is not testing is the locking >> behavior, which is not relevant at the API level). >> - A fix for `WebColorFieldSkin` which triggered a nested change which used a >> flag to prevent an event loop (I've changed it now to match how >> `DoubleFieldSkin` and `IntegerFieldSkin` do it > > John Hendrikx has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Merge branch 'master' of https://git.openjdk.org/jfx into > feature/delayed-nested-emission > - Delay nested event emission The fix for the bug with nested events releasing the lock seems fine. I'm still not sure if the behavioral change is what we want the result to be even if it's better than what we have now. I have mentioned this in https://github.com/openjdk/jfx/pull/108#issuecomment-590878643. we need to decide on a spec first, and there are several facets to it as discussed in the mailing list link above (https://github.com/openjdk/jfx/pull/837#pullrequestreview-1233910264). I will re-summarize what I think needs to be defined before implementing a behavior change like the one proposed here: ### Listener order When listeners are registered, they are added to some collection (an array, currently). The add/remove methods hint at an order, but don't guarantee one. Do we specify an order, or say it's unspecified? ### Event dispatch order The order in which listeners are triggered is not necessarily the order of the listeners above. Do we specify a dispatch order, or say it's unspecified? If it's specified, the listener order is probably also specified. We are also dispatching invalidation events before change events arbitrarily. Do we dispatch the event to all listeners of one type and then to the other, or do we want to combine them according to a joint dispatch order? We either say that they are dispatched separately, together in some order (like registration), or that it's unspecified. ### Nested listener modifications If a listener is added/removed during an event dispatch, do we specify it will affect ("be in time for") the nested event chain, the outer event chain, or that it's unspecified? ### Nested value modifications If a listener changes the value of its property during an event dispatch, do we specify that the new event will trigger first, halting the current event dispatch (depth-first approach), or that the new event waits until current one finishes (breadth-first approach), or that it is unspecified? Do we guarantee that when a listener is invoked it sees the new value that was set by the latest nested change, or will it see the value that existed at trigger time, or is it unspecified? If listeners affect each other with nested events, then the dispatch order matters. --- If we answer "unspecified" to the questions above, it allows us more implementation freedom. It will also mean that listeners should be thought of as "lone wolves" - they are not intended to talk to each other or depend on each other; each should do something regardless of what the others are doing and assume its code "territory" is not affected by other listeners. The user takes responsibility for coupling them (nested modifications). On the other hand, no guarantees bring limited usage. The question is, what is the intended usage? ------------- PR: https://git.openjdk.org/jfx/pull/837
