RFR: 8294809: ListenerHelper for managing and disconnecting listeners
Nir Lisker
nlisker at gmail.com
Thu Oct 13 08:47:10 UTC 2022
Hi,
I agree with John. The work on managing listeners has been in the works for
a long while now and it provides a lot more flexibility for less technical
debt. I have already reviewed his 'when' proposal (sans its implementation
on Node) and I think that it's the way to go.
Fluent bindings already existed for years in ReactFX, on which they are
based. As such, the community has a lot of experience with its paradigms (I
have used its Val/Var since 2016 continuously). 'when', which is called
'conditionOn' in ReactFX, was proposed with the initial set of fluent
bindings, but I suggested at the time to separate it to make the process
more smooth and manageable (it's one of those special PRs that requires 3
reviewers...).
In addition, phase 2 of the plan is to expose Subscription as a public API.
This should be sufficient to address these concerns.
Listeners are deeply ingrained in JavaFX. This work is not done in a
vacuum. For example, John recently found another issue with regards to GC,
where ExpressionHelper still holds the value of the observable. These
should be taken into account when making strides in this area,
The history can be traced back through this (broken) thread:
https://mail.openjdk.org/pipermail/openjfx-dev/2021-March/029405.html
https://mail.openjdk.org/pipermail/openjfx-dev/2021-April/029574.html
https://mail.openjdk.org/pipermail/openjfx-dev/2021-September/031952.html
etc.
On Thu, Oct 13, 2022 at 9:40 AM John Hendrikx <john.hendrikx at gmail.com>
wrote:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20221013/f10f4ffb/attachment-0001.htm>
More information about the openjfx-dev
mailing list