On Mon, 27 Mar 2023 14:36:45 GMT, John Hendrikx <[email protected]> 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
Just took a quick look.
Ideally, we would not split the world into subscriptions and listeners. Adding
a return type to `addListener` and `removeListener` is source compatible, but
not binary. Is this something that we strictly can't do? Is the disruption
factor too high?
modules/javafx.base/src/main/java/javafx/beans/Subscription.java line 51:
> 49: * @throws NullPointerException when {@code subscriptions} is {@code
> null} or contains {@code null}
> 50: */
> 51: static Subscription of(Subscription... subscriptions) {
`combine` sounds like a better name to me.
modules/javafx.base/src/main/java/javafx/beans/Subscription.java line 58:
> 56: subscription.unsubscribe();
> 57: }
> 58: };
This can be
`return () -> list.forEach(Subscription::unsubscribe);`
modules/javafx.base/src/main/java/javafx/beans/Subscription.java line 85:
> 83: other.unsubscribe();
> 84: };
> 85: }
This looks like a special case of the `of` method:
default Subscription and(Subscription other) {
return of(this, other);
}
although this implementation creates an array, which might be what you're
trying to avoid.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1069#pullrequestreview-1389078204
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1169356681
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1169356850
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1169356504
