On Thu, 13 Jul 2023 00:01:23 GMT, John Hendrikx <jhendr...@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
