On Mon, 25 Mar 2024 13:32:11 GMT, Michael Strauß <mstra...@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
