RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs
John Hendrikx
jhendrikx at openjdk.org
Thu Jun 27 17:32:05 UTC 2024
On Mon, 25 Mar 2024 13:32:11 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:
> `ListenerManager` is an obvious improvement, as it fixes incorrect behavior and allows listeners to veto changes. However, the behavior of `ListenerManager` is also an implementation detail and not documented anywhere. This leads me to the following questions:
>
> 1. How will users know that they can now do all of the things that were previously broken? Do we need a specification for what is allowed and what's not allowed?
Currently the specification is vague enough that there's a lot of wiggle room. For example, we don't specify whether invalidation listeners are called before change listeners, yet a lot of code will be relying on that unknowingly. We also don't specify whether successive change listener calls should always be a change (ie. never get `A -> A`), or that it should match with what the previous change reported (ie. if called with `? -> B`, then the next call must be `B -> ?`).
IMHO we should though. I would specify for example that:
- Invalidation listeners are called before Change listeners (reason: invalidation listeners are a lower level concept defined in a higher level interface). They definitely should not be mixed (they're defined by two different interfaces).
- Change listeners should (obviously as this MR fixes this) guarantee the old value makes sense:
- Old value will be equal to previous new value (essential for patterns that use the old value to unregister a related listener)
- Never called when old value equals new value (it's not a change then) -- this allows vetoing, and generally saves unnecessary calls
We should probably also specify the order of calls (as code will again unknowingly be relying on this already):
- A listener registered after a listener of the same type will always be called after earlier registered listeners (code relies on this in various ways, even in FX itself)
- Listeners of different types follow a fixed order: invalidation first, changes second (code relies on this already)
- The behavior of `ObservableValue`s that contain mutable values (ie. lists/sets/maps/atomics) will be undefined if those values are mutated while held by an observable (same as when you mutate keys that are currently part of a `Set`).
We can also specify some behavior with regards to when an event can be received when adding and removing listeners, although I think that's less of an issue.
> 2. Should this behavior be required for all valid `ObservableValue` implementations? (This would render many existing implementations defective.)
It's hard to require anything in an interface, but I think the interface should specify this regardless. Just look at an interface like `Set` that requires a specific way of implementing `hashCode`. You can violate it easily, but you will suffer the consequences when comparing sets of different types. Same with custom implementations of `ObservableValue`. You take a risk when using some unvetted 3rd party implementation.
At a minimum all implementations in JavaFX should follow the specification. This will likely cover most implementations of `ObservableValue`, leaving only a few custom implementations that are not 100% spec compliant (note: a lot of the problems only occur with nested changes, which occur only in complicated code that triggers a cascade of changes, usually layout/skin/css related).
A problem there are the Set/List/Map `ObservableValue` implementations. They are not observable values, they are observable collections that deserve their own interface. Invalidation listeners are fine, but value listeners make no sense. I've looked into these before, and all I can say is that they take great liberties with what is considered a "change" (ignoring even the most basic specifications). I'd recommend deprecating the observable value parts of these, and moving users towards either invalidation or the collection specific change listeners.
> 3. If `ObservableValue` implementations are not required to replicate the `ListenerManager` behavior, we should probably make it easily discoverable whether any particular implementation (most of them are properties) supports nested changes/vetoing. In most of the public API, there's no obvious way to see (without looking at the source code) whether a property implementation extends one of the `*PropertyBase` classes.
I think if the implementation is in `javafx.*` it should be correct. Anyone can violate any interface (just look at custom collection implementations which often fail to follow the spec). We could provide a more lenient abstract base class or helper to make it easier to conform to the spec.
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line 58:
>
>> 56: * Constructs a new instance.
>> 57: *
>> 58: * @param accessor an {@link Accessor}, cannot be {@code null}
>
> There is no `accessor` parameter.
Thanks for all the Javadoc checks; I've turned on some IDE warnings for these as it turns out they're harder to get right than I thought :) I fixed a couple more as well.
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line 374:
>
>> 372: while (needed > max) {
>> 373: min = mid;
>> 374: mid = max;
>
> These two lines don't seem to be useful, as neither `min` nor `mid` are ever accessed after this point.
Well spotted, they indeed are not needed in the 2nd loop.
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java line 143:
>
>> 141: */
>> 142: public void fireValueChanged(I instance, T oldValue) {
>> 143: Object data = getData(instance);
>
> The `data` value could be passed into this method, which would save a (potentially not devirtualized) method call.
Thanks, I'll look into that, it might speed up the 1 listener cases a bit. The same applies to OldValueCachingListenerManager#getValue I think. I know it isn't possible for the add/remove calls, as the data may change if they're nested, but for `fireValueChanged` I never really checked after going to this strategy.
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java line 145:
>
>> 143: Object data = getData(instance);
>> 144:
>> 145: if (data instanceof ListenerList) {
>
> Why is `ListenerList` checked first, when most observables only have a single `InvalidationListener`?
For some (unclear to me) reason this order performs better in my benchmark, even for the cases that only have a single invalidation listener. I've tweaked this method extensively, with different orders, and this was about the best I could get it. That said, the differences are small, and we can go with a more logical order.
> modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerList.java line 101:
>
>> 99: * notification otherwise {@code false}
>> 100: */
>> 101: public boolean notifyListeners(ObservableValue<T> observableValue) {
>
> The code in this method is _almost_ identical to `ListenerList.notifyListeners(ObservableValue<T>, T)`.
> Given that this method is somewhat complex, I think it would be good to use a common implementation.
> This will help with code review, and decrease the chance that both methods diverge further with future modifications.
I agree with you there, and I've been looking what would be a good way to achieve this. I will take another look soon. My primary concern is that this is a somewhat critical path, and I would want to ensure that it doesn't cause too much performance regressions (I've already been optimizing all of this code with the help of a JMH test)
> modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerList.java line 164:
>
>> 162: }
>> 163:
>> 164: private void callInvalidationListener(ObservableValue<?> instance, InvalidationListener listener) {
>
> This method is identical to `ListenerList.callInvalidationListener`.
Yes, it would be good to just put these static somewhere. They don't really fit well in the manager class (as the List class which doesn't depend on manager would need to call them there), and the other way around is also odd, given that I only need to call them when not in "list" mode. Still, putting them package private in the ListenerListBase could work....
> modules/javafx.base/src/main/java/javafx/beans/property/ObjectPropertyBase.java line 91:
>
>> 89: @Override
>> 90: public void addListener(InvalidationListener listener) {
>> 91: LISTENER_MANAGER.addListener((ObjectPropertyBase<Object>) this, listener);
>
> I think the unchecked casts here can be removed if `ListenerManagerBase` is declared as `ListenerManagerBase<T, I extends ObservableValue<? extends T>>`, and `OldValueCachingListenerManager` accordingly. Then the `LISTENER_MANAGER` instance can be parameterized as `OldValueCachingListenerManager<Object, ObjectPropertyBase<?>>`.
Thanks, I gave up on that one a bit, I was looking for a better solution, but never got the generics quite the way I wanted them there, but your change works. I was primarily aiming to keep the casts as much out of the inner loops as possible.
I still need to do one cast at this line, but it is a huge improvement:
@Override
public void addListener(ChangeListener<? super T> listener) {
LISTENER_MANAGER.addListener(this, (ChangeListener<Object>) listener);
}
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-2018972993
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167744831
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167744922
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272805838
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272801049
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167754712
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167755526
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167753159
More information about the openjfx-dev
mailing list