On Fri, 4 Nov 2022 20:30:53 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 24 additional > commits since the last revision: > > - 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 > - 8294809: generics > - 8294809: is alive > - Revert "8294809: removed weak listeners support" > > This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. > - 8294809: removed weak listeners support > - ... and 14 more: https://git.openjdk.org/jfx/compare/fa19443d...470f42c1 modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ListenerHelper.java line 70: > 68: * </ul> > 69: */ > 70: public class ListenerHelper implements IDisconnectable { This class is mixing two things- 1. Managing listeners in a generic way - by providing ability to add & removeAll (via disconnect()) 2. Trying to cater to *Skin specific requirement Ideally, (2) above should be done separately. I mean, the class ListenerHelper should not use `SkinBase`. What do you think? modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ListenerHelper.java line 87: > 85: > 86: public static ListenerHelper get(SkinBase<?> skin) { > 87: return accessor.apply(skin); `accessor` can be `null` and needs to be checked. modules/javafx.controls/src/test/java/test/javafx/scene/control/TestListenerHelper.java line 94: > 92: > 93: @Test > 94: public void testChangeListener_MultipleProperties() { Should we address a hypothetical use case where the `ListenerHelper` has added a ChangeListener for 2 properties as done in this test case and wants to remove only ChangeListener for p1? currently `h.disconnet()` removes ChangeListener of both the properties. ------------- PR: https://git.openjdk.org/jfx/pull/908
