On Mon, 23 Mar 2020 23:40:39 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> ButtonSkin adds a `ChangeListener` to `Control.sceneProperty()` which >> results in leaking the `ButtonSkin` itself when >> the `Button`'s skin is changed to a new `ButtonSkin`. Using a >> `WeakChangeListener` instead of `ChangeListener` solves >> the issue. >> Please take a look. > > In general, there are two approaches to avoiding listener-related memory > leaks. One is to use a WeakListener; the other > is to explicitly remove the listener when the object is removed or otherwise > no longer needed. > Using a WeakListener is certainly easier, but runs the risk of the listener > being removed too early and not cleaning up > after itself. I'm not suggesting that's the case here, but it is worth > looking at. The one thing I would ask you to > take a look at is whether it would matter if the old skin didn't call > `setDefaultButton(oldScene, false)` when removed > (and similarly `setCancelButton`). @aghaisas can you also review? ------------- PR: https://git.openjdk.java.net/jfx/pull/147
