On Sun, 2 Jul 2023 11:46:19 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:
>
> 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
