<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;}
@font-face
{font-family:"Courier New \,serif";
panose-1:2 7 3 9 2 2 5 2 4 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}
span.EmailStyle20
{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="#0563C1" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-family:"Courier New",serif">> control.installSkin(MySkin::new);<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif">This is an interesting idea. Control.installSkin(Function<Control,Skin>).<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif">One of the requirements we ought to consider is maximizing the backward compatibility. If we were to add a new installSkin method it would not solve the problem with the old method, and replacing
setSkin(Skin) with installSkin() would break compatibility with the existing code.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif">There is one more reason to allow for creation of a skin outside of setSkin() - configuration. Imagine customer skins require configuration, in which case the sequence of events looks like
this<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif">instantiation -> configuration -> uninstall old skin -> install new skin.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif">with installSkin(MySkin::new) such a model will be impossible.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif">Thank you<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif">-andy<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="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 John Hendrikx <john.hendrikx@gmail.com><br>
<b>Date: </b>Thursday, 2022/07/21 at 15:49<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>
<p>Hi Andy,<o:p></o:p></p>
<p>Was a single step install process considered, something like:<o:p></o:p></p>
<p> control.installSkin(MySkin::new);<o:p></o:p></p>
<p>This would also make it much more clear that Skins are single use only, where the API currently has to bend over backwards to make that clear and enforce it.<o:p></o:p></p>
<p>Other than that, I think your suggestion would be a definite improvement over the current situation. Something never felt quite right about how skins where set up and attached to controls, it felt fragile and cumbersome -- possibly as the result of relying
on a writable property as the means to install a skin.<o:p></o:p></p>
<p>--John<o:p></o:p></p>
<div>
<p class="MsoNormal">On 20/07/2022 23:39, Andy Goryachev wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Hi,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">I'd like to propose an API change in Skin interface (details below). Your feedback will be greatly appreciated!</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Thank you,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">-andy</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Summary</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">-------</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Introduce a new Skin.install() method with an empty default implementation. Modify Control.setSkin(Skin) implementation to invoke install() on the new skin after the old skin has been removed
with dispose().</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Problem</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">-------</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Presently, switching skins is a two-step process: first, a new skin is constructed against the target Control instance, and is attached (i.s. listeners added, child nodes added) to that instance
in the constructor. Then, Control.setSkin() is invoked with a new skin - and inside, the old skin is detached via its dispose() method. </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">This creates two problems:</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> 1. if the new skin instance is discarded before setSkin(), it remains attached, leaving the control in a weird state with two skins attached, causing memory leaks and performance degradation.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> 2. if, in addition to adding listeners and child nodes, the skin sets a property, such as an event listener, or a handler, it overwrites the current value irreversibly. As a result, either
the old skin would not be able to cleanly remove itself, or the new skin would not be able to set the new values, as it does not know whether it should overwrite or keep a handler installed earlier (possibly by design). Unsurprisingly, this also might cause
memory leaks.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">We can see the damage caused by looking at
<a href="https://bugs.openjdk.org/browse/JDK-8241364">JDK-8241364</a> </span><span style="font-family:"Apple Color Emoji"">☂</span><span style="font-family:"Courier New",serif">
<i>Cleanup skin implementations to allow switching</i>, which refers a number of bugs:</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8245145 Spinner: throws IllegalArgumentException when replacing skin</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8245303 InputMap: memory leak due to incomplete cleanup on remove mapping</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8268877 TextInputControlSkin: incorrect inputMethod event handler after switching skin</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8236840 Memory leak when switching ButtonSkin</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8240506 TextFieldSkin/Behavior: misbehavior on switching skin</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8242621 TabPane: Memory leak when switching skin</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8244657 ChoiceBox/ToolBarSkin: misbehavior on switching skin
</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8245282 Button/Combo Behavior: memory leak on dispose</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8246195 ListViewSkin/Behavior: misbehavior on switching skin</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8246202 ChoiceBoxSkin: misbehavior on switching skin, part 2</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8246745 ListCell/Skin: misbehavior on switching skin</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8247576 Labeled/SkinBase: misbehavior on switching skin</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8253634 TreeCell/Skin: misbehavior on switching skin</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8256821 TreeViewSkin/Behavior: misbehavior on switching skin</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8269081 Tree/ListViewSkin: must remove flow on dispose</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8273071 SeparatorSkin: must remove child on dispose</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8274061 Tree-/TableRowSkin: misbehavior on switching skin</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8244419 TextAreaSkin: throws UnsupportedOperation on dispose</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8244531 Tests: add support to identify recurring issues with controls et al</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Solution</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">--------</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">This problem does not exist in e.g. Swing because the steps of instantiation, uninstalling the old ComponentUI ("skin"), and installing a new skin are cleanly separated. ComponentUI constructor
does not alter the component itself, ComponentUI.uninstallUI(JComponent) cleanly removes the old skin, ComponentUI.installUI(JComponent) installs the new skin. We should follow the same model in javafx.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Specifically, I'd like to propose the following changes:</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> 1. Add Skin.install() with a default no-op implementation.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> 2. Clarify skin creation-attachment-detachment sequence in Skin and Skin.install() javadoc</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> 3. Modify Control.setSkin(Skin) method (== invalidate listener in skin property) to call oldSkin.dispose() followed by newSkin.install()</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> 4. Many existing skins that do not set properties in the corresponding control may remain unchanged. The skins that do, such as TextInputControlSkin (JDK-8268877), must be refactored to
utilize the new install() method. I think the refactoring would simply move all the code that accesses its control instance away from the constructor to install().</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Impact Analysis</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">-------------</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">The changes should be fairly trivial. Only a subset of skins needs to be refactored, and the refactoring itself is trivial. </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">The new API is backwards compatible with the existing code, the customer-developed skins can remain unchanged (thanks to default implementation). In case where customers could benefit from
the new API, the change is trivial.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">The change will require CSR as it modifies a public API.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span><o:p></o:p></p>
</blockquote>
</div>
</body>
</html>