On Wed, 6 May 2020 11:46:27 GMT, Jeanette Winzenburg <faste...@openjdk.org>
wrote:
>> some skins have not been guarding themselves against multiple calls to
>> dispose (see issue for details)
>>
>> Fixed by backing out off dispose if skinnable is null. Added test
>> (parameterized in control class) for all controls in
>> the controls package. Those that failed for the misbehaving skins before are
>> passing after the fix.
>
> Jeanette Winzenburg has updated the pull request incrementally with one
> additional commit since the last revision:
>
> cleanup
>
> - corrected incorrect bug id in commented
> - removed unrelated test change from TextAreaTest
> - updated copyright year
Eventually we might need to add this check to all the skin's dispose() which
will be fixed for cleanup.
Looks good to me, suggested some minor changes.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java
line 222:
> 221: // FIXME : JDK-8244112 - backout if we are already disposed
> 222: // should check for getSkinnable to be null and return
> 223: if (getSkinnable() == null) return;
This comment can be removed. FIXME is getting fixed. :)
modules/javafx.controls/src/shims/java/javafx/scene/control/ControlShim.java
line 38:
> 37: *
> 38: * @param control the control the set the default skin on
> 39: */
typo:- the control the set => the control **to** set
modules/javafx.controls/src/shims/java/javafx/scene/control/ControlShim.java
line 34:
> 33: *
> 34: * Note that this has no noticeable effect if the the control's
> 35: * skin already is set to the default skin (see skinProperty for
typo:- if the the control's => if the control's
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinDisposeContractTest.java
line 159:
> 158: {TreeTableView.class, },
> 159: {TreeView.class, },
> 160: };
Should these controls be included too: `Tooltip`, `HTMLEditor`, `ContextMenu` ?
-------------
Changes requested by arapte (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/209
