On Tue, 15 Nov 2022 18:30:33 GMT, Andy Goryachev <ango...@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
