On Mon, 3 Oct 2022 23:07:08 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>>> You are right, @kleopatra - this executeOnceWhenPropertyIsNonNull() is >>> installing a listener. perhaps we should add a similar functionality to >>> LambdaMultiplePropertyChangeListenerHandler and actually install a >>> WeakListener instead. >> >> I'd advise against doing this in an ad-hoc manner. Skins leak memory by >> adding (but not removing) all kinds of listeners or children to their >> controls. Some skins try to address this with weak listeners, or by manually >> removing listeners in the dispose() method. SkinBase attempts to make it >> easier for skins to not leak listeners with the >> `registerInvalidationListener` and `registerChangeListener` APIs, which >> remove an added listener when the skin is disposed (however, both of these >> methods only accept Consumers, not actual >> `InvalidationListener`/`ChangeListener` instances, making the API less >> useful than it could be). >> >> But there are more places where memory is leaked: >> 1. adding, but not removing children to the control >> 2. adding EventHandlers/EventFilters to the control or to other objects (for >> example, MenuBarSkin adds EventHandlers to the control's scene) >> 3. hidden listeners like the one you discovered: >> Utils.executeOnceWhenPropertyIsNonNull >> 4. Label.setLabeLFor(control) >> 5. ListChangeListener, etc. >> >> Most skins exhibit one or several of these memory leaks, which causes them >> to remain strongly referenced by the control even after being disposed. The >> problem here is that JavaFX doesn't offer good tools and APIs to address >> these challenges, so it's incredible easy to create a skin that leaks >> memory. In fact, it's so easy that most built-in skins do it (and these were >> created by the developers of JavaFX itself, so obviously it's a major >> problem if even those people get it wrong so often). >> >> For me, this is a clear indication that we're dealing with an API problem >> first and foremost. I think there should be a better API that makes it easy >> for skins to interact with their associated controls, instead of hand-wiring >> hundreds of little skin-control dependencies (and sure enough, getting it >> wrong every so often). > > You are absolutely right! There are so many places where memory leaks are > being created. My plan is to go through all the skins and take care of them > all. > > LambdaMultiplePropertyChangeListenerHandler is, I believe, a step in the > right direction. I wish there were a more uniform and public way to add > various listeners, implemented as a public class - something that can be used > to yank all listeners with a single method. > > I've done something like that on the application level, see > https://github.com/andy-goryachev/FxTextEditor/blob/master/src/goryachev/fx/FxDisconnector.java > > What we could do (later) is to keep enhancing > LambdaMultiplePropertyChangeListenerHandler by adding more ways of adding > listeners, including full-blown change/invalidation listeners, and then > rename it, move to a public util package and make it a part of the general > purpose API. Sure, but I think this should be done with a clear vision of where we're going, and not just as a collection of ad-hoc helper methods. There are even more problems that a new API should at least consider: Some skins change their behavior depending on the configuration of their associated control at the time when the skin is installed. For example, `MenuButtonSkinBase` adds a `MOUSE_PRESSED` handler only if the control didn't specify one for its onMousePressed property: if (control.getOnMousePressed() == null) { control.addEventHandler(MouseEvent.MOUSE_PRESSED, ... But what happens if the `onMousePressed` property is set after the skin is installed? Rather than a one-time check, cases like these should probably be a dynamic expression: when control.onMousePressedProperty().isNotNull() and skin.isInstalled() control.addEventHandler otherwise control.removeEventHandler ------------- PR: https://git.openjdk.org/jfx/pull/906
