<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>