RFR: JDK-8304439: Subscription based listeners [v7]

[Jose Pereda]

On Sun, 2 Jul 2023 11:46:19 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Makes `Subscription` public (removing some of its methods that are unnecessary), and adds methods that can provide `Subscription`s in `ObservableValue`.
>
> John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add newline at end of ConditionalBinding file

modules/javafx.base/src/main/java/javafx/beans/Observable.java line 110:

> 108:     default Subscription subscribe(Runnable subscriber) {
> 109:         Objects.requireNonNull(subscriber, "subscriber cannot be null");
> 110:         InvalidationListener listener = obs -> subscriber.run();

InvalidationListeners are widely used in this pattern:


javaFXProperty.addListener(new InvalidationListener() {
            @Override
            public void invalidated(Observable observable) {
                if (value.get() != null) {
                    // do something
                    javaFXProperty.removeListener(this);
                }
            }
        });


Does subscribe/unsubscribe allows something like this?

modules/javafx.base/src/main/java/javafx/beans/Observable.java line 110:

> 108:     default Subscription subscribe(Runnable subscriber) {
> 109:         Objects.requireNonNull(subscriber, "subscriber cannot be null");
> 110:         InvalidationListener listener = obs -> subscriber.run();

In the `subscribe` implementations, Invalidation/ChangeListeners are added as local variables. Have you run any test to check that they don't end up being gc'ed, because no strong references being held?

And also related to this, how would you add a WeakInvalidation/WeakChangeListener? Maybe, that's up to the developer, overriding the default implementation of `subscribe`?

modules/javafx.base/src/main/java/javafx/beans/Subscription.java line 77:

> 75:      */
> 76:     default Subscription and(Subscription other) {
> 77:         Objects.requireNonNull(other, "other cannot be null");

As per javadoc, this equivalent to `Subscription.combine(this, other)`, so then, couldn't you just call it instead of doing a different implementation?

modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 345:

> 343:         ChangeListener<T> listener = (obs, old, current) -> subscriber.accept(current);
> 344: 
> 345:         subscriber.accept(getValue());  // eagerly send current value

What is the reason of this logic being done only for the Consumer and not for the BiConsumer (or the Runnable for that matter)?

modules/javafx.base/src/test/java/test/javafx/beans/ObservableSubscriptionsTest.java line 57:

> 55:         value.set("B");
> 56: 
> 57:         assertEquals(1, calls.get());

I'd add a comment here about: this works as long as `value` doesn't get validated again, i.e with a call to `value.get()`.  (Imagine that someone is running tests and wants to add a printout of `value`...)

modules/javafx.base/src/test/java/test/javafx/beans/ObservableSubscriptionsTest.java line 58:

> 56: 
> 57:         assertEquals(1, calls.get());
> 58:     }

Maybe you could also extend this test to show how `unsubscribe` works

modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueSubscriptionsTest.java line 97:

> 95: 
> 96:         assertNull(lastCall.get());
> 97:     }

Can you add a test where you subscribe using a Runnable (therefore adding an InvalidationListener) and with a Consumer o BiConsumer (adding a ChangeListener)? Also unsubscribing them separately.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257526387
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257526612
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257529022
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257521581
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257521933
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257522153
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257523574