RFR: JDK-8304439: Subscription based listeners [v7]

[Jose Pereda]

On Sun, 9 Jul 2023 20:56:06 GMT, John Hendrikx <jhendrikx at 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