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

[Kevin Rushforth]

On Mon, 31 Oct 2022 16:45:51 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 incrementally with one additional commit since the last revision:
> 
>   8294809: whitespace

Before reviewing the code itself, I have a couple global comments and a small number of inline comments.

Since `ListenerHelper` is now an internal implementation detail, we can defer the longer discussion of whether and how to expose this as public API. In its current incarnation, it seems like a helpful utility to fix a number of skin memory leaks, and will likely be an eventual replacement for the (also internal) `LambdaMultiplePropertyChangeListenerHandler` utility class.

If this ever does become public API in the future, we will need a naming discussion as part of the larger discussion of what form the API should take. For example, we wouldn't use `IDisconnectable` as the name of a public interface in the JavaFX API (we don't use the pattern a leading `I` for interfaces anywhere in JavaFX or in the JDK); also, we would need more descriptive names for `ChLi`, `MaChLi`, and so forth (presuming they need to be exposed).

The larger discussion, though, will first need to clearly identify what the purpose of this utility is. If it is specific to control skins, then whatever minimal public API surface that needs to be exposed to do that job so third-party controls could use it is what we would want. If it is to be more general, then it doesn't belong in the `javafx.controls` module at all, but belongs in `javafx.base`. In the either case, but especially for an API in `javafx.base`, we need to ensure that this is generally useful to application developers, and not just for our own internal UI controls. Also, we would need to see how it fits with the existing APIs in `javafx.base`, and the newly proposed APIs in PR #830. Btw, the existing implementation is not suitable to be generic, since it references classes like `Task`, `Node`, `Scene`, `Label`, and `TableColumn`.

As I mentioned above, we don't need to have this discussion unless and until there is a desire to make this public API some future release.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/IDisconnectable.java line 32:

> 30:  * Original code is re-licensed to Oracle by the author.
> 31:  * https://github.com/andy-goryachev/FxTextEditor/blob/master/src/goryachev/common/util/Disconnectable.java
> 32:  * Copyright © 2021-2022 Andy Goryachev <andy at goryachev.com>

Copyright and licensing information doesn't belong in the javadocs. I recommend putting this in a separate ordinary comment block (`/*` rather than `/**`) right above the class docs.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/IDisconnectable.java line 35:

> 33:  */
> 34: @FunctionalInterface
> 35: public interface IDisconnectable {

We don't use the pattern of naming interfaces with a leading `I` in JavaFX (or in the JDK), but since this isn't public API, you can leave it for now.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/IDisconnectable.java line 41:

> 39:      */
> 40:     public void disconnect();
> 41: }

Minor: end the class with a newline

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ListenerHelper.java line 69:

> 67:  * Original code is re-licensed to Oracle by the author.
> 68:  * https://github.com/andy-goryachev/FxTextEditor/blob/master/src/goryachev/fx/FxDisconnector.java
> 69:  * Copyright © 2021-2022 Andy Goryachev <andy at goryachev.com>

Copyright and licensing information doesn't belong in the javadocs. I recommend putting this in a separate ordinary comment block right above the class docs.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ListenerHelper.java line 94:

> 92:         ListenerHelper h = get(skin);
> 93:         h.disconnect();
> 94:     }

Minor: This method might not be needed, since the client code can call:


ListenerHelper.get(this).disconnect();

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ListenerHelper.java line 514:

> 512:         }
> 513:     }
> 514: }

Minor: end the class with a newline

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

> 223:      * Returns the skin's instance of {@link ListenerHelper}, creating it if necessary.
> 224:      *
> 225:      * @since 20

This isn't public API, so you should remove the `@since` tag

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

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