On Tue, 22 Nov 2022 20:22:06 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 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
