RFR: 8294809: ListenerHelper for managing and disconnecting listeners

[John Hendrikx]

Hi Andy,

There is another PR that will help with this: 
https://github.com/openjdk/jfx/pull/830

It adds a `when` method on `ObservableValue`:

       default ObservableValue<T> when(ObservableValue<Boolean> condition)

This method can be used for all kinds of things, and one of them is that 
it could be used to manage listeners.

It would work like this:

     ObservableValue<Boolean> active = new SimpleBooleanProperty(true);

     MySkin() {
getSkinnable().textProperty().when(active).addListener(this::updateSkinStuffs);
getSkinnable().fontProperty().when(active).addListener(this::updateSkinStuffs);
getSkinnable().wrapTextProperty().when(active).addListener(this::updateSkinStuffs);
     }

     void dispose() {
          active.set(false);  // kills all listeners/bindings/etc in a 
single line
     }

Also, it's possible to extend (and later expose) the 
`com.sun.javafx.binding.Subscription` class, which can help as well:

It has these methods, but more can be added:

      Subscription subscribe(ObservableValue<T> observableValue, 
Consumer<? super T> subscriber)

      Subscription subscribeInvalidations(ObservableValue<?> 
observableValue, Runnable runnable)

It could be used like this to manage subscriptions:

      Subscription subscription = 
Subscription.subscribe(label.textProperty(), value -> { ... } );

Subscriptions can be chained:

      Subscription subscription = 
Subscription.subscribe(label.textProperty(), value -> { ... } )
             .and(Subscription.subscribe(label.fontProperty(), value -> 
{ ... } )
             .and(Subscription.subscribe(label.wrapTextProperty(), value 
-> { ... } );

They can then be released all at once by doing:

      subscription.unsubscribe();

Reading your proposal I see you might be looking to support weak 
listeners as well. Personally, I believe these are better replaced with 
lazy listeners (via `ObservableValue`'s fluent methods) instead of 
doubling down on these and adding special support.

Perhaps we can collaborate on this.  Nir Lisker also has interests in 
this area I believe :-)

I added some more comments inline.

--John

On 12/10/2022 23:05, Andy Goryachev wrote:
> Introduction
>
> There is a number of places where various listeners (strong as well as weak) are added, to be later disconnected in one go. For example, Skin implementations use dispose() method to clean up the listeners installed in the corresponding Control (sometimes using LambdaMultiplePropertyChangeListenerHandler class).
>
> This pattern is also used by app developers, but there is no public utility class to do that, so every one must invent their own, see for instance
> https://github.com/andy-goryachev/FxTextEditor/blob/master/src/goryachev/fx/FxDisconnector.java
>
> Proposal
>
> It might be beneficial to create a class (ListenerHelper, feel free to suggest a better name) which:
>
> - provides convenient methods like addChangeListener, addInvalidationListener, addWeakChangeListener, etc.
`Subscription` has some of these, and the `when` proposal would not need 
them as the original methods can be used.  For fluent bindings with 
`when` weak listeners are probably not need at all.
> - keeps track of the listeners and the corresponding ObservableValues
Both `Subscription` and the `when` proposal could do this. `Subcription` 
by keeping track of a result value, `when` by creating a boolean value 
that can be toggled.  Note that the boolean value if toggled back to 
`true` would resubscribe automatically -- it acts as a breaker between 
the actual property and the listener that can turned on and off.
> - provides a single disconnect() method to remove all the listeners in one go.
Yes, `Subscription#unsubscribe` or for `when` by toggling the boolean 
value to `false`.
> - optionally, it should be possible to add a lambda (Runnable) to a group of properties

I suppose that could be a signature like:

       Subscription#subscribeInvalidations(Runnable runnable, 
ObservableValue<?> ... values);

> - optionally, there should be a boolean flag to fire the lambda immediately
I'm guessing you mean for invalidation listeners, as for change 
listeners this already happens (this is not entirely clear, and not well 
documented).  I do know that `Subscription#subscribe` with a `Consumer` 
will always eagerly send you the current value.
> - strongly suggest implementing an IDisconnectable functional interface with a single disconnect() method
`Subscription` has this.
> Make it Public Later
>
> Initially, I think we could place the new class in package com.sun.javafx.scene.control; to be made publicly accessible later.
>
> Where It Will Be Useful
>
> [JDK-8294589](https://bugs.openjdk.org/browse/JDK-8294589) "MenuBarSkin: memory leak when changing skin"
> and other skins, as a replacement for LambdaMultiplePropertyChangeListenerHandler.
>
> https://github.com/openjdk/jfx/pull/908#:~:text=19%20hours%20ago-,8295175%3A%20SplitPaneSkinSkin%3A%20memory%20leak%20when%20changing%20skin%20%23911,-Draft
>
> -------------
>
> Commit messages:
>   - 8294809: use weak reference correctly this time
>   - 8294809: tests
>   - 8294809: remove
>   - 8294809: change listener with callback
>   - 8294809: skin base
>   - 8294809: whitespace
>   - 8294809: event handlers and filters
>   - 8294809: cleanup
>   - 8294809: override
>   - 8294809: null checks
>   - ... and 4 more: https://git.openjdk.org/jfx/compare/9768b5e4...af77693c
>
> Changes: https://git.openjdk.org/jfx/pull/908/files
>   Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=908&range=00
>    Issue: https://bugs.openjdk.org/browse/JDK-8294809
>    Stats: 1498 lines in 4 files changed: 1495 ins; 2 del; 1 mod
>    Patch: https://git.openjdk.org/jfx/pull/908.diff
>    Fetch: git fetch https://git.openjdk.org/jfx pull/908/head:pull/908
>
> PR: https://git.openjdk.org/jfx/pull/908