On Sun, 9 Jul 2023 20:56:06 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> 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.
That looks good, but it is also a point for the pattern I described earlier:
`node.sceneProperty()` (and also `node.skinProperty()`, since you mentioned it
too), are two very good examples of properties that often return null values,
so your pattern will need to include at list a filter to make sure scene is not
null, but it would be great if it could use something like a `when` there:
node.sceneProperty().when(node.getScene() != null).flatMap...
(pretty much like this hidden gem in
com.sun.javafx.scene.control.skin.Utils::executeOnceWhenPropertyIsNonNull)
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257563777
