<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
These additional change are more disruptive, and I don't see enough
value to go down this path. I prefer to keep the existing
constructor, and add the newly-proposed Skin::install method. Unless
we find something very compelling that can't be done using that
pattern, the cost of making a more intrusive change doesn't seem
justified.<br>
<br>
-- Kevin<br>
<br>
<br>
<div class="moz-cite-prefix">On 7/26/2022 7:39 AM, Marius Hanl
wrote:<br>
</div>
<blockquote type="cite" cite="mid:trinity-28ecbfb1-7c6c-4a64-96bb-e9aa7c4e2c94-1658846371992@msvc-mesg-web004">
<meta name="viewport" content="width=device-width">
<div class="mail_android_message" style="line-height: 1; padding:
0.5em">I don't see how this fixes the underlying problem, since
you could still do stuff like:<br>
<br>
<a href="http://control.installSkin" moz-do-not-send="true">control.installSkin</a>(c
-> {<br>
MySkin s = new MySkin(myOtherControl); <br>
// configure stuff<br>
return s;<br>
});<br>
<br>
So I think the super clean way would still be to have a default
constructor for every skin, while keeping the control
constructor for backwards compatibilty.<br>
<br>
But a check is also already a good step. <br>
It might be also worth to add a 'uninstall' method and move the
'dispose' method content into it and deprecate it.<br>
Then it is much more clear from the wording, but is not 100%
necessary.<br>
<br>
-- Marius</div>
<div class="mail_android_quote" style="line-height: 1; padding:
0.3em">Am 23.07.22, 14:49 schrieb John Hendrikx
<a class="moz-txt-link-rfc2396E" href="mailto:john.hendrikx@gmail.com"><john.hendrikx@gmail.com></a>:
<blockquote class="gmail_quote" style="margin: 0.8ex 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<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>
<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);</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif">This
is an interesting idea.
Control.installSkin(Function<Control,Skin>).</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"> </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.</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"> </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</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif">instantiation
-> configuration -> uninstall old skin ->
install new skin.</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif">with
installSkin(MySkin::new) such a model will be
impossible.</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif">Thank
you</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif">-andy</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New",serif"> </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" moz-do-not-send="true"><openjfx-dev-retn@openjdk.org></a>
on behalf of John Hendrikx <a class="moz-txt-link-rfc2396E" href="mailto:john.hendrikx@gmail.com" moz-do-not-send="true"><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
moz-txt-link-freetext" href="mailto:openjfx-dev@openjdk.org" moz-do-not-send="true">openjfx-dev@openjdk.org</a>
<a class="moz-txt-link-rfc2396E" href="mailto:openjfx-dev@openjdk.org" moz-do-not-send="true"><openjfx-dev@openjdk.org></a><br>
<b>Subject: </b>Re: Proposal: Add Skin.install()
method</span></p>
</div>
<p>Hi Andy,</p>
<p>Was a single step install process considered, something
like:</p>
<p> control.installSkin(MySkin::new);</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.</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.</p>
<p>--John</p>
<div>
<p class="MsoNormal">On 20/07/2022 23:39, Andy Goryachev
wrote:</p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Hi,</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></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></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Thank
you,</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">-andy</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Summary</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">-------</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></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></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Problem</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">-------</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></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></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">This
creates two problems:</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></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></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></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></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></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></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8245145
Spinner: throws IllegalArgumentException when
replacing skin</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8245303
InputMap: memory leak due to incomplete cleanup on
remove mapping</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8268877
TextInputControlSkin: incorrect inputMethod event
handler after switching skin</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8236840
Memory leak when switching ButtonSkin</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8240506
TextFieldSkin/Behavior: misbehavior on switching
skin</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8242621
TabPane: Memory leak when switching skin</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8244657
ChoiceBox/ToolBarSkin: misbehavior on switching skin
</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8245282
Button/Combo Behavior: memory leak on dispose</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8246195
ListViewSkin/Behavior: misbehavior on switching skin</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8246202
ChoiceBoxSkin: misbehavior on switching skin, part 2</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8246745
ListCell/Skin: misbehavior on switching skin</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8247576
Labeled/SkinBase: misbehavior on switching skin</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8253634
TreeCell/Skin: misbehavior on switching skin</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8256821
TreeViewSkin/Behavior: misbehavior on switching skin</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8269081
Tree/ListViewSkin: must remove flow on dispose</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8273071
SeparatorSkin: must remove child on dispose</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8274061
Tree-/TableRowSkin: misbehavior on switching skin</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">JDK-8244419
TextAreaSkin: throws UnsupportedOperation on dispose</span></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></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Solution</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">--------</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></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></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Specifically,
I'd like to propose the following changes:</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> 1.
Add Skin.install() with a default no-op
implementation.</span></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></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></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></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">Impact
Analysis</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">-------------</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></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></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></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></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif"">The
change will require CSR as it modifies a public API.</span></p>
<p class="MsoNormal"><span style="font-family:"Courier New \,serif""> </span></p>
</blockquote>
</div>
</blockquote>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>