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

Thu Jul 13 02:29:20 UTC 2023

On Thu, 13 Jul 2023 00:01:23 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:
> 
>   Fix ObservableSubscriptionsTest

Reviewed the non-tests code. The implementation looks good, added comments on the docs.

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

> 99:      * {@code invalidationSubscriber} whenever it becomes invalid. If the same
> 100:      * subscriber is subscribed more than once, then it will be notified more
> 101:      * than once. That is, no check is made to ensure uniqueness.

Similar to the comments on `ObservableValue`:

Creates a {@code Subscription} on this {@code Observable} that calls the given
{@code invalidationSubscriber}  whenever it becomes invalid. This {@code Subscription}
is akin to an {@code InvalidationListener}.

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

> 107:      * lifecycle than the subscriber, the subscriber must be unsubscribed
> 108:      * when no longer needed as the subscription will otherwise keep the subscriber
> 109:      * from being garbage collected.

I think that the explanations starting from "If the same" are no longer needed if you add something like what I suggested for `Subscription`. It covers the lifecycle and multiple subscriptions explanations already.

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

> 31: /**
> 32:  * A subscription encapsulates how to cancel it without having
> 33:  * to keep track of how it was created.

I would like to see some more info here, like what it is and how it's used, and a note on comparison with listeners. Taking a page out of `ChangeListener`, I took a stab at it:

Executes code on change or invalidation events. A {@code Subscription} can be created by subscribing to invalidation
events on an {@code Observable} using {@link Observable#subscribe(Runnable)}
and to change events on an {@code ObsefvableValue} using {@link Observable#subscribe(Consumer)}
and {@link Observable#subscribe(BiConsumer)}. It can be unsubscribed using {@link #unsuscribe}.
<p>
Subscriptions don't block the the {@code Observable} from being garbage collected, and will be collected with
it (if they are not subscribed on other {@code Observable}s, see below). However, since {@code Observable}s
blocks their subscriptions from being GC'd, {@code unsuscribe()} must be called if they are to be eligible for
GC before the {@code Observable} is. Considering creating a derived {@code ObservableValue}
using {@link #when(ObservableValue)} and subscribing on this derived observable value
to automatically decouple the lifecycle of the subscriber from the original {@code ObservableValue}
when some condition holds:
{@code
obs.when(cond).subscribe(...)
}
<p>
Subscriptions can also be combined using {@link #combine} and {@link #and}, which allows for
multiple subscriptions to be unsubscribed together. This is useful when they share the same lifecycle,
which is usually the case when they are subscribed on the same {@code Observable}.
<p>
The same subscriber can be subscribed to multiple {@code Observable}s, including more than once on the
same {@code Observable}, in which case it will be executed more than once. That is, no uniqueness check is made.
<p>
{@code Subscription}s behave similarly to {@link InvalidationListener}s and {@code ChangeListener}s. An
advantage of {@code Subscription} is that it encapsulates how to cancel it without having to keep track of
how it was created.

This will also alleviate some repeated verbosity in the subscribe methods.

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

> 60:      * Cancels this subscription, or does nothing if already cancelled.<p>
> 61:      *
> 62:      * Implementors must ensure the implementation is idempotent.

Maybe add a short explanation on idempotent:

"...is idempotent (a no-op if called more than once)".

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

> 319:      * using {@link #when(ObservableValue)} and subscribing on this derived observable value
> 320:      * to automatically decouple the lifecycle of the subscriber from this
> 321:      * {@code ObservableValue} when some condition holds.

I think that these explanations are no longer needed if you add something like what I suggested for `Subscription`. It covers the lifecycle and multiple subscriptions explanations already.

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

> 341:      * Creates a {@link Subscription} on this value which immediately provides
> 342:      * the current value to the given {@code valueSubscriber}, followed by any
> 343:      * subsequent values whenever its value changes.

Similar comments to the other method, but I would also talk about why the value is also sent immediately. Consider this phrasing:

Creates a {@code Subscription} on this {@code ObservableValue} that calls the given {@code 
changeSubscriber} with the new value immediately and whenever its value changes. This {@code Subscription}
is akin to a {@code ChangeListener} with only the {@code T newValue} parameter.
The {@code valueSubscriber} is called immediately for convenience since usually the user will
want to initialize a value and then update on changes.

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

> 352:      * using {@link #when(ObservableValue)} and subscribing on this derived observable value
> 353:      * to automatically decouple the lifecycle of the subscriber from this
> 354:      * {@code ObservableValue} when some condition holds.

I think that these explanations are no longer needed if you add something like what I suggested for `Subscription`. It covers the lifecycle and multiple subscriptions explanations already.

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

PR Review: https://git.openjdk.org/jfx/pull/1069#pullrequestreview-1527377314
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1261842659
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1261895740
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1261873147
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1261901036
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1261896234
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1261839343
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1261896328