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

[Kevin Rushforth]

On Tue, 22 Nov 2022 20:22:06 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 28 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper
>  - 8294809: review comments
>  - 8294809: review comments
>  - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper
>  - 8294809: review comments
>  - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper
>  - 8294809: whitespace
>  - 8294809: no public api
>  - 8294809: map change listener
>  - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper
>  - ... and 18 more: https://git.openjdk.org/jfx/compare/4f11de10...00566eda

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

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

> 77:     private static Function<SkinBase<?>,ListenerHelper> accessor;
> 78: 
> 79:     public ListenerHelper(Object owner) {

I note that this is unused, other than in unit tests.

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

> 92:     }
> 93: 
> 94:     public IDisconnectable addDisconnectable(Runnable r) {

I note that this is unused, other than in unit tests.

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

> 112:     }
> 113: 
> 114:     protected boolean isAliveOrDisconnect() {

Minor: could be private

modules/javafx.controls/src/test/java/test/javafx/scene/control/TestListenerHelper.java line 278:

> 276: 
> 277:     @Test
> 278:     public void testInvalidationListener_Callback() {

Minor: This test is redundant, and the name of the test method is misleading. Unlike `addChangeListener`, which really does have two similar flavors -- one taking a `ChangeListener` and one taking a `Consumer`, there isn't a flavor of `addInvalidationListener` that takes a Consumer, so this test method is testing the same method (same signature) as `testInvalidationListener_Plain`.

modules/javafx.controls/src/test/java/test/javafx/scene/control/TestListenerHelper.java line 295:

> 293: 
> 294:     @Test
> 295:     public void testInvalidationListener_Callback_FireImmediately() {

Minor: redundant (tests the same method signature as `testInvalidationListener_Plain_FireImmediately`.

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

Marked as reviewed by kcr (Lead).

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