On Sat, 28 Mar 2020 06:42:06 GMT, Ambarish Rapte <[email protected]> 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.
>
> Ambarish Rapte has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Changes for Kevin's review comment
Changes requested by fastegal (Author).
modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java
line 101:
> 100: };
> 101: WeakChangeListener<Scene> weakChangeListener = new
> WeakChangeListener<>(sceneChangeListener);
> 102:
a loose naming convention for weak listeners seems to be to prepend _weak_ to
the name of the strong listener, which
would be
WeakChangeListener<Scene> weakSceneChangeListener = new
WeakChangeListener<>(sceneChangeListener);
modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java
line 178:
> 177: @Override public void dispose() {
> 178:
> getSkinnable().sceneProperty().removeListener(weakChangeListener);
> 179: super.dispose();
+1! Actually, looks like that manual remove is really needed - otherwise we get
an NPE when removing the default button
from its parent after the skin has been switched:
@Test
public void testDefaultButtonSwitchSkinAndRemove() {
Button button = new Button();
button.setDefaultButton(true);
Group root = new Group(button);
Scene scene = new Scene(root);
Stage stage = new Stage();
stage.setScene(scene);
stage.show();
button.setSkin(new ButtonSkin1(button));
root.getChildren().remove(button);
}
Note: to see this NPE as failing test (vs. its printout on sysout), we need to
re-wire the uncaughtExceptionHandler,
see ComboBoxTest setup/cleanup for an example.
-------------
PR: https://git.openjdk.java.net/jfx/pull/147
