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

[Kevin Rushforth]

On Tue, 15 Nov 2022 18:30:33 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: review comments

As part of reviewing this, I looked at a couple of the fixes that will make use of this, and I have a high-level question on the usage pattern.

Along with the needed changes to fix the leak, PR #925 replaces four calls to `registerChangeListener` -- which is part of the public `SkinBase` API for skins to use to register a listener that is cleaned up when the skin is disposed or when `unregisterChangeListeners` is called. The current implementation of register/unregister methods uses `LambdaMultiplePropertyChangeListenerHandler`, which `ListenerHelper` intends to replace, but I wonder whether it might be better for the individual skins continue to call `registerChangeListener` (along with the related `registerInvalidationListener` and `registerListChangeListener` methods), where those methods are currently used? Eventually, the implementation of `registerChangeListener` and friends can replace `LambdaMultiplePropertyChangeListenerHandler` with `ListenerHelper`. This would minimize the changes needed for the individual memory leak bugs.

So my main question is: is there anything about the addition of ListenerChangeHelper that will make skins that continue to use registerListenerHelper stop working? I suspect not, since I tested PR #925 with that part of the change reverted, and it still works without leaking. Given this, I wondered why you made that change in the `PaginationSkin` (and others for other PRs), and figured I might be missing something.

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

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