<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
        {font-family:"Cambria Math";
        panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
        {font-family:Calibri;
        panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
        {font-family:"Apple Color Emoji";
        panose-1:0 0 0 0 0 0 0 0 0 0;}
@font-face
        {font-family:"Times New Roman \(Body CS\)";
        panose-1:2 11 6 4 2 2 2 2 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
        {margin:0in;
        font-size:10.0pt;
        font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
        {mso-style-priority:99;
        color:blue;
        text-decoration:underline;}
span.EmailStyle19
        {mso-style-type:personal-reply;
        font-family:"Courier New",serif;
        color:windowtext;}
.MsoChpDefault
        {mso-style-type:export-only;
        font-size:10.0pt;}
@page WordSection1
        {size:8.5in 11.0in;
        margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
        {page:WordSection1;}
--></style>
</head>
<body lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif">Dear Jeanette:<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif">Thank you for responding!<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif">You are right - some of the issues in JDK-8241364<<a href="https://bugs.openjdk.org/browse/JDK-8241364">https://bugs.openjdk.org/browse/JDK-8241364</a>>
</span><span style="font-size:11.0pt;font-family:"Apple Color Emoji"">☂</span><span style="font-size:11.0pt;font-family:"Courier New",serif"> Cleanup skin implementations to allow switching might be caused by other factors.  Still, from a pure design perspective,
 the fact that we are altering control while the new skin is being constructed is not right.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif">I suspect there is a number of other Skins where we might have memory leaks and other issues.  For example, MenuButtonSkinBase:96 tries to add an event handler based on the
 value of onMousePressed property.  Please correct me if I am wrong - should this event handler also be removed in dispose()?  Same in MenuButtonSkinBase:105 (line numbers provided are for the latest master branch).  Same in MenuButtonSkin:108<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif">We may need to go through all the skins looking for places like this as part of JDK-8241364<<a href="https://bugs.openjdk.org/browse/JDK-8241364">https://bugs.openjdk.org/browse/JDK-8241364</a>>
</span><span style="font-size:11.0pt;font-family:"Apple Color Emoji"">☂</span><span style="font-size:11.0pt;font-family:"Courier New",serif"> Cleanup skin implementations.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif">Thank you so much for your comments!<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif">-andy<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New",serif"><o:p> </o:p></span></p>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal" style="margin-bottom:12.0pt"><b><span style="font-size:12.0pt;color:black">From:
</span></b><span style="font-size:12.0pt;color:black">openjfx-dev <openjfx-dev-retn@openjdk.org> on behalf of Jeanette Winzenburg <fastegal@swingempire.de><br>
<b>Date: </b>Thursday, 2022/07/21 at 09:04<br>
<b>To: </b>openjfx-dev@openjdk.org <openjfx-dev@openjdk.org><br>
<b>Subject: </b>Re: Proposal: Add Skin.install() method<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:11.0pt"><br>
Zitat von Andy Goryachev <andy.goryachev@oracle.com>:<br>
<br>
Hi Andy,<br>
<br>
good idea to move our discussion from the issues to the mailling -  <br>
it's both easier and reaches a broader audience :)<br>
<br>
Will answer in about a week or so, for now just stumbled across<br>
<br>
> We can see the damage caused by looking at  <br>
> JDK-8241364<<a href="https://bugs.openjdk.org/browse/JDK-8241364">https://bugs.openjdk.org/browse/JDK-8241364</a>>
</span><span style="font-size:11.0pt;font-family:"Apple Color Emoji"">☂</span><span style="font-size:11.0pt"> Cleanup 
<br>
> skin implementations to allow switching, which refers a number of  <br>
> bugs:<br>
<br>
Hmm .. might be me, but looks like you are implying that with the  <br>
proposal in place (and used throughout our skins) those issues  <br>
wouldn't have existed? If that's not the case, could you please  <br>
clarify what you mean?. On the other hand, if yes, I disagree: the  <br>
majority of the issues (in particular those already fixed) are about  <br>
an incorrect/incomplete implementation of dispose - that's unrelated  <br>
to enhancing/cleaning up the install process. A subset (of the  <br>
unfixed) is indeed due to the parallel existence of two skins pointing  <br>
to the same control (and modifying its properties)<br>
<br>
Cheers, Jeanette<br>
<br>
<br>
> Hi,<br>
><br>
> I'd like to propose an API change in Skin interface (details below).  <br>
>  Your feedback will be greatly appreciated!<br>
><br>
> Thank you,<br>
> -andy<br>
><br>
><br>
><br>
><br>
><br>
> Summary<br>
> -------<br>
><br>
> Introduce a new Skin.install() method with an empty default  <br>
> implementation.  Modify Control.setSkin(Skin) implementation to  <br>
> invoke install() on the new skin after the old skin has been removed  <br>
> with dispose().<br>
><br>
><br>
> Problem<br>
> -------<br>
><br>
> Presently, switching skins is a two-step process: first, a new skin  <br>
> is constructed against the target Control instance, and is attached  <br>
> (i.s. listeners added, child nodes added) to that instance in the  <br>
> constructor.  Then, Control.setSkin() is invoked with a new skin -  <br>
> and inside, the old skin is detached via its dispose() method.<br>
><br>
> This creates two problems:<br>
><br>
>  1. if the new skin instance is discarded before setSkin(), it  <br>
> remains attached, leaving the control in a weird state with two  <br>
> skins attached, causing memory leaks and performance degradation.<br>
><br>
>  2. if, in addition to adding listeners and child nodes, the skin  <br>
> sets a property, such as an event listener, or a handler, it  <br>
> overwrites the current value irreversibly.  As a result, either the  <br>
> old skin would not be able to cleanly remove itself, or the new skin  <br>
> would not be able to set the new values, as it does not know whether  <br>
> it should overwrite or keep a handler installed earlier (possibly by  <br>
> design).  Unsurprisingly, this also might cause memory leaks.<br>
><br>
> We can see the damage caused by looking at  <br>
> JDK-8241364<<a href="https://bugs.openjdk.org/browse/JDK-8241364">https://bugs.openjdk.org/browse/JDK-8241364</a>>
</span><span style="font-size:11.0pt;font-family:"Apple Color Emoji"">☂</span><span style="font-size:11.0pt"> Cleanup 
<br>
> skin implementations to allow switching, which refers a number of  <br>
> bugs:<br>
><br>
> JDK-8245145 Spinner: throws IllegalArgumentException when replacing skin<br>
> JDK-8245303 InputMap: memory leak due to incomplete cleanup on remove mapping<br>
> JDK-8268877 TextInputControlSkin: incorrect inputMethod event  <br>
> handler after switching skin<br>
> JDK-8236840 Memory leak when switching ButtonSkin<br>
> JDK-8240506 TextFieldSkin/Behavior: misbehavior on switching skin<br>
> JDK-8242621 TabPane: Memory leak when switching skin<br>
> JDK-8244657 ChoiceBox/ToolBarSkin: misbehavior on switching skin<br>
> JDK-8245282 Button/Combo Behavior: memory leak on dispose<br>
> JDK-8246195 ListViewSkin/Behavior: misbehavior on switching skin<br>
> JDK-8246202 ChoiceBoxSkin: misbehavior on switching skin, part 2<br>
> JDK-8246745 ListCell/Skin: misbehavior on switching skin<br>
> JDK-8247576 Labeled/SkinBase: misbehavior on switching skin<br>
> JDK-8253634 TreeCell/Skin: misbehavior on switching skin<br>
> JDK-8256821 TreeViewSkin/Behavior: misbehavior on switching skin<br>
> JDK-8269081 Tree/ListViewSkin: must remove flow on dispose<br>
> JDK-8273071 SeparatorSkin: must remove child on dispose<br>
> JDK-8274061 Tree-/TableRowSkin: misbehavior on switching skin<br>
> JDK-8244419 TextAreaSkin: throws UnsupportedOperation on dispose<br>
> JDK-8244531 Tests: add support to identify recurring issues with  <br>
> controls et al<br>
><br>
><br>
> Solution<br>
> --------<br>
><br>
> This problem does not exist in e.g. Swing because the steps of  <br>
> instantiation, uninstalling the old ComponentUI ("skin"), and  <br>
> installing a new skin are cleanly separated.  ComponentUI  <br>
> constructor does not alter the component itself,  <br>
> ComponentUI.uninstallUI(JComponent) cleanly removes the old skin,  <br>
> ComponentUI.installUI(JComponent) installs the new skin.  We should  <br>
> follow the same model in javafx.<br>
><br>
> Specifically, I'd like to propose the following changes:<br>
><br>
>  1. Add Skin.install() with a default no-op implementation.<br>
>  2. Clarify skin creation-attachment-detachment sequence in Skin and  <br>
> Skin.install() javadoc<br>
>  3. Modify Control.setSkin(Skin) method (== invalidate listener in  <br>
> skin property) to call oldSkin.dispose() followed by newSkin.install()<br>
>  4. Many existing skins that do not set properties in the  <br>
> corresponding control may remain unchanged.  The skins that do, such  <br>
> as TextInputControlSkin (JDK-8268877), must be refactored to utilize  <br>
> the new install() method.  I think the refactoring would simply move  <br>
> all the code that accesses its control instance away from the  <br>
> constructor to install().<br>
><br>
><br>
> Impact Analysis<br>
> -------------<br>
><br>
> The changes should be fairly trivial.  Only a subset of skins needs  <br>
> to be refactored, and the refactoring itself is trivial.<br>
><br>
> The new API is backwards compatible with the existing code, the  <br>
> customer-developed skins can remain unchanged (thanks to default  <br>
> implementation).  In case where customers could benefit from the new  <br>
> API, the change is trivial.<br>
><br>
> The change will require CSR as it modifies a public API.<br>
<br>
<br>
<o:p></o:p></span></p>
</div>
</div>
</body>
</html>