On Sun, 9 Jul 2023 18:50:34 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> 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();
>
> 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`?
There is no need, the returned `Subscription` is implemented using a lambda
that captures the listener (as it needs it to call `removeListener`). As long
as you have the `Subscription` referenced, it can't get GC'd. If you let the
`Subscription` get GC'd, then you'd lose your means to unsubscribe the
listener, just like if you lost the reference to the listener itself, you would
never be able to remove it anymore.
Weak listeners I would not recommend to use in new code if you can avoid it.
They're unpredictable as to when they're removed exactly, and can still receive
notifications when your application may be in a state where you don't expect
them anymore.
The `Subscription` interface is in fact intended to make management of
listeners easier so you can rely less on weak listeners. One pattern I can
really recommend to use is to automatically disable listeners when the UI they
belong with becomes invisible, for example:
private Subscription subscription = Subscription.EMPTY;
node.sceneProperty()
.flatMap(Scene::windowProperty)
.flatMap(Window::showingProperty)
.subscribe(visible -> {
if (visible) {
subscription = Subscription.combine(
// create subscriptions (to external resources) here as
we just became visible!
);
}
else {
// we became invisible, remove listeners (from external
resources) to remove hard references that
// may keep the UI referenced; the other branch will restore
them if the UI becomes visible again
subscription.unsubscribe(); // unsubscribes everything in
one go
}
});
Or you can use `ObservableValue#when` to do something similar, not requiring
subscriptions.
I suspect it may also prove a useful pattern for `Skin`s that have a specific
lifecycle. They can register listeners (and combine in a single subscription)
in their constructor / initializer, and remove them all in their `dispose`
method.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257540910
