8294809: ListenerHelper for managing and disconnecting listeners

[andy-goryachev-oracle]

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.
• keeps track of the listeners and the corresponding ObservableValues
• provides a single disconnect() method to remove all the listeners in one go.
• optionally, it should be possible to add a lambda (Runnable) to a group of properties
• optionally, there should be a boolean flag to fire the lambda immediately
• strongly suggest implementing an IDisconnectable functional interface with a single disconnect() method

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 "MenuBarSkin: memory leak when changing skin"
and other skins, as a replacement for LambdaMultiplePropertyChangeListenerHandler.

#908

#914

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8294809: ListenerHelper for managing and disconnecting listeners

Reviewers

• Kevin Rushforth (@kevinrushforth - Reviewer)
• Ajit Ghaisas (@aghaisas - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/908/head:pull/908
$ git checkout pull/908

Update a local copy of the PR:
$ git checkout pull/908
$ git pull https://git.openjdk.org/jfx pull/908/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 908

View PR using the GUI difftool:
$ git pr show -t 908

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/908.diff

[bridgekeeper]

👋 Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 12: Full - Incremental (79ae28ce)
• 11: Full - Incremental (00566eda)
• 10: Full - Incremental (8cef69c1)
• 09: Full - Incremental (2a295c24)
• 08: Full - Incremental (470f42c1)
• 07: Full - Incremental (e0e0aabb)
• 06: Full - Incremental (e78ed6db)
• 05: Full - Incremental (7a1fa625)
• 04: Full - Incremental (71ca35ee)
• 03: Full - Incremental (e36d3289)
• 02: Full - Incremental (c518cbf9)
• 01: Full - Incremental (2df4a85d)
• 00: Full (af77693c)

[mlbridge]

Mailing list message from John Hendrikx on openjfx-dev:

Hi Andy,

There is another PR that will help with this:
https://github.com//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.

[mlbridge]

Mailing list message from Nir Lisker on openjfx-dev:

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//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//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...af77693

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20221013/f10f4ffb/attachment-0001.htm>

[andy-goryachev-oracle]

Thank you @nlisker and @hjohn for providing the link and explanations.

You are right, ListenerHelper is somewhat similar to Subscription in that it allows for quick disconnect().

It is, however, designed for a different purpose, that goes beyond what is needed from a simple listener management. Specifically,

• it assumes multiple listeners/handlers/callbacks from the start
• it allows for invoking listeners/callbacks immediately as it is a frequent pattern not only in Skins but also in application code (I do want to make this class a part of public API eventually)
• unlike Subscription, we can addEventHandlers and addEventFilters
• unlike Subscription, we can add Weak listeners/callbacks, though I admit it might be a design decision to use or not to use that

to summarize, I see some minor overlap between Subscription and ListenerHelper, but mostly these are two different beasts designed for two different purposes.

I do agree with the call to cooperate! Would love to.

what do you think?

[andy-goryachev-oracle]

Reading your proposal I see you might be looking to support weak
listeners as well. Personally, I believe these are better replaced

You have a very good point here, considering the purpose of this class. Let me see if support for weak listeners should be removed. Thank you!

[andy-goryachev-oracle]

Support for weak listeners cannot be removed, for example in a situation when Control gets removed from the scene graph and therefore becomes eligible for collection.

[andy-goryachev-oracle]

we would need more descriptive names for ChLi, MaChLi, and so forth (presuming they need to be exposed).

they don't need to be exposed, will convert them to private.

edit: already private.

[kevinrushforth]

This looks ready to go in. I left a few minor comments and questions, but I am approving it as is.

[openjdk]

@andy-goryachev-oracle This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8294809: ListenerHelper for managing and disconnecting listeners

Reviewed-by: kcr, aghaisas

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

• 67088be: 8256397: MultipleSelectionModel throws IndexOutOfBoundException
• 4a697f1: 8297130: ComboBox popup doesn't close after selecting value that was added with 'runLater'

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @aghaisas) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[kevinrushforth]

Looks OK, although I left another comment on the (now less so) redundant test.

[andy-goryachev-oracle]

/integrate

[openjdk]

@andy-goryachev-oracle This pull request has not yet been marked as ready for integration.

[andy-goryachev-oracle]

oh no, all the approvals have been invalidated...

[andy-goryachev-oracle]

/integrate

[openjdk]

@andy-goryachev-oracle
Your change (at version 79ae28c) is now ready to be sponsored by a Committer.

[kevinrushforth]

/sponsor

[openjdk]

Going to push as commit 3376228.
Since your change was applied there have been 2 commits pushed to the master branch:

• 67088be: 8256397: MultipleSelectionModel throws IndexOutOfBoundException
• 4a697f1: 8297130: ComboBox popup doesn't close after selecting value that was added with 'runLater'

Your commit was automatically rebased without conflicts.

[openjdk]

@kevinrushforth @andy-goryachev-oracle Pushed as commit 3376228.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.