On Mon, 27 Mar 2023 14:36:45 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:
>
> Move Subscription method for invalidations to Observable
>
> - moved Subscription class to the Observable package
I reviewed the proposal in the JBS enhancement and the proposed API in this PR.
I think this would be a useful addition to JavaFX. I left a few questions and
comments.
modules/javafx.base/src/main/java/javafx/beans/Subscription.java line 76:
> 74: * @return a combined {@code Subscription} which will cancel both when
> 75: * cancelled, never {@code null}
> 76: * @throws NullPointerException when {@code other} is {@code null}
Maybe add a sentence that says that this is equivalent to
`Subscription.combine(this, other)`?
modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line
339:
> 337: * @since 21
> 338: */
> 339: default Subscription values(Consumer<? super T> subscriber) {
Same question about the names: we might want to consider a name that has "add"
or "subscribe" in the name.
modules/javafx.base/src/test/java/test/javafx/beans/SubscriptionTest.java line
1:
> 1: package test.javafx.beans;
Needs a standard copyright header.
modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueSubscriptionsTest.java
line 1:
> 1: package test.javafx.beans.value;
Needs a standard copyright header.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1069#pullrequestreview-1480154322
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1230076228
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1230078584
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1230079004
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1230079093
