On Sat, 28 Mar 2020 10:29:39 GMT, Jeanette Winzenburg <faste...@openjdk.org>
wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Changes for Kevin's review comment
>
> 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);
Corrected in the next commit...
> 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.
Thanks for the test case, I did minor changes to it and included in the next
commit.
The NPE can occur even without `button.setDefaultButton(true);`.
Please take a look
-------------
PR: https://git.openjdk.java.net/jfx/pull/147
