RFR: 8294809: ListenerHelper for managing and disconnecting listeners [v6]

[Kevin Rushforth]

On Fri, 21 Oct 2022 20:24:07 GMT, Andy Goryachev <angorya at openjdk.org> 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.
>> - 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](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
>> 
>> https://github.com/openjdk/jfx/pull/914
>
> Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 20 additional commits since the last revision:
> 
>  - 8294809: map change listener
>  - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper
>  - 8294809: generics
>  - 8294809: is alive
>  - Revert "8294809: removed weak listeners support"
>    
>    This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18.
>  - 8294809: removed weak listeners support
>  - 8294809: use weak reference correctly this time
>  - 8294809: tests
>  - 8294809: remove
>  - 8294809: change listener with callback
>  - ... and 10 more: https://git.openjdk.org/jfx/compare/836729e3...7a1fa625

The stated intention of this PR is to provide a helpful utility to enable Skins to more easily manage their listeners to avoid memory leaks. I am not sold on the idea of this migrating to public API, but we can defer that (longer) discussion. As long as this does not impact the public API surface, we can evaluate it as a internal helper class that enables a few memory leak bug fixes.

To that end, you will need at least one change, since this PR _does_ add to the public API surface (and exposes an internal type in the process).

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line 86:

> 84:      * map (although it doesn't really do much harm).
> 85:      */
> 86:     @Deprecated // replace with listenerHelper

We only use `@Deprecated` annotation on public API, so you should limit this to a comment.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line 222:

> 220:      * @since 20
> 221:      */
> 222:     protected ListenerHelper listenerHelper() {

This would make it part of the public API, so you will need to find some other way to do this without adding anything to the public API surface.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line 241:

> 239:      */
> 240:     // TODO I would like to deprecate and remove these methods, and replace them by listenerHelper().add**()
> 241:     protected final void registerChangeListener(ObservableValue<?> observable, Consumer<ObservableValue<?>> operation) {

I don't think we want to deprecate this. It is part of the public API, so there would need to be a compelling reason to deprecate it.

-------------

Changes requested by kcr (Lead).

PR: https://git.openjdk.org/jfx/pull/908