<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Configuration can be part of the factory or not?</p>
<p>Simple case:</p>
<p> control.installSkin(MySkin::new)</p>
<p>More complicated case:</p>
<p> control.installSkin(c -> {<br>
MySkin s = new MySkin(c); <br>
// configure stuff<br>
return s;<br>
});<br>
<br>
--John<br>
</p>
<div class="moz-cite-prefix">On 22/07/2022 17:58, Andy Goryachev
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:BL0PR10MB2948809E71F677C8BA726A71E5909@BL0PR10MB2948.namprd10.prod.outlook.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style>@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;}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;}div.WordSection1
{page:WordSection1;}</style>
<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
<a class="moz-txt-link-rfc2396E" href="mailto:openjfx-dev-retn@openjdk.org"><openjfx-dev-retn@openjdk.org></a> on behalf of John
Hendrikx <a class="moz-txt-link-rfc2396E" href="mailto:john.hendrikx@gmail.com"><john.hendrikx@gmail.com></a><br>
<b>Date: </b>Thursday, 2022/07/21 at 15:49<br>
<b>To: </b><a class="moz-txt-link-abbreviated" href="mailto:openjfx-dev@openjdk.org">openjfx-dev@openjdk.org</a>
<a class="moz-txt-link-rfc2396E" href="mailto:openjfx-dev@openjdk.org"><openjfx-dev@openjdk.org></a><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"
moz-do-not-send="true">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>
</blockquote>
</body>
</html>