<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
Yes, something like this. We can discuss the wording of the doc
changes in the PR along with the other proposed API changes.<br>
<br>
Thanks.<br>
<br>
-- Kevin<br>
<br>
<br>
<div class="moz-cite-prefix">On 7/22/2022 2:56 PM, Andy Goryachev
wrote:<br>
</div>
<blockquote type="cite" cite="mid:BL0PR10MB29489C554DB3E1845ABE4E84E5909@BL0PR10MB2948.namprd10.prod.outlook.com">
<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;}@font-face
{font-family:"Courier New \,serif ";
panose-1:2 7 3 9 2 2 5 2 4 4;}@font-face
{font-family:"Iosevka Fixed SS16";
panose-1:2 0 5 9 3 0 0 0 0 4;}@font-face
{font-family:"Courier New \,serif\;color\:black ";
panose-1:2 7 3 9 2 2 5 2 4 4;}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:#0563C1;
text-decoration:underline;}p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0in;
margin-right:0in;
margin-bottom:0in;
margin-left:.5in;
font-size:10.0pt;
font-family:"Calibri",sans-serif;}p.p1, li.p1, div.p1
{mso-style-name:p1;
margin:0in;
font-size:9.0pt;
font-family:"Iosevka Fixed SS16";}span.s1
{mso-style-name:s1;
color:#646464;}span.s2
{mso-style-name:s2;
color:#4E8F00;}span.EmailStyle23
{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;}ol
{margin-bottom:0in;}ul
{margin-bottom:0in;}</style>
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16"">
<span style="color:#FF2600">/**</span><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> * Sets the skin that will
render this {@link Control}.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> * <p><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> * To ensure a one-to-one
relationship between a {@code Control} and its<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> * {@code Skin}, this method
must check (for any non-null value) that<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> * {@link
Skin#getSkinnable()}, throwing an IllegalArgumentException<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> * in the case of mismatch.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> * returns the same value as
this Skinnable.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> *<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> * @param value the skin value
for this control<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> * <o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> * @throws
IllegalArgumentException if {@link Skin#getSkinnable()}
returns<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> * value other than this
Skinnable.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16";color:#FF2600"> */<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Iosevka Fixed
SS16"">
<span style="color:#4E8F00">public</span> <span style="color:#4E8F00">void</span> setSkin(Skin<?>
value);<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">-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>
<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">Kevin
Rushforth <a class="moz-txt-link-rfc2396E" href="mailto:kevin.rushforth@oracle.com"><kevin.rushforth@oracle.com></a><br>
<b>Date: </b>Friday, 2022/07/22 at 14:54<br>
<b>To: </b>Andy Goryachev
<a class="moz-txt-link-rfc2396E" href="mailto:andy.goryachev@oracle.com"><andy.goryachev@oracle.com></a>, <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>, Philip Race
<a class="moz-txt-link-rfc2396E" href="mailto:philip.race@oracle.com"><philip.race@oracle.com></a><br>
<b>Subject: </b>Re: [External] : Aw: Proposal: Add
Skin.install() method<o:p></o:p></span></p>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:11.0pt">Yes, but a RuntimeException only
appears in the javadoc, and not in the method signature
(only checked exceptions show up in the method signature).<br>
<br>
-- Kevin<br>
<br>
<o:p></o:p></span></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt">On
7/22/2022 2:39 PM, Andy Goryachev wrote:<o:p></o:p></span></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif"">Thank you, Phil and Kevin.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif""> </span><o:p></o:p></p>
<p class="p1"><span class="s1">@Override</span><o:p></o:p></p>
<p class="p1"><span class="s2">public</span> <span class="s2">final</span>
<span class="s2">
void</span> setSkin(Skin<?> value) throws
IllegalArgumentException {<o:p></o:p></p>
<p class="p1"> <o:p></o:p></p>
<p class="p1"> <o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif"">-andy</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif""> </span><o:p></o:p></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">Kevin
Rushforth <a href="mailto:kevin.rushforth@oracle.com" moz-do-not-send="true">
<kevin.rushforth@oracle.com></a><br>
<b>Date: </b>Friday, 2022/07/22 at 14:20<br>
<b>To: </b>Andy Goryachev <a href="mailto:andy.goryachev@oracle.com" moz-do-not-send="true"><andy.goryachev@oracle.com></a>,
<a href="mailto:openjfx-dev@openjdk.org" moz-do-not-send="true" class="moz-txt-link-freetext">openjfx-dev@openjdk.org</a>
<a href="mailto:openjfx-dev@openjdk.org" moz-do-not-send="true">
<openjfx-dev@openjdk.org></a><br>
<b>Subject: </b>Re: [External] : Aw: Proposal: Add
Skin.install() method</span><o:p></o:p></p>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:11.0pt">No, sorry. Error doesn't
communicate the right thing. This is exactly is what
RuntimeException is intended to be used for. It's no
different from passing an out of range numerical value or
a null to a method that doesn't take null. In all similar
cases, the method will document the exception cases with
'@throws' javadoc tags, and the developer can then
understand that they passed something bad to the method.
FWIW, I've never seen any Java API throw Error in this
manner.<br>
<br>
-- Kevin<br>
<br>
<br>
</span><o:p></o:p></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt">On
7/22/2022 2:02 PM, Andy Goryachev wrote:</span><o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif "">I do mean java.lang.Error.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif "">The goal is to prevent an incorrect code
from being shipped to the end user. There are no tools
at the API level to enforce the 1:1 relationship, so it
cannot be checked at compile time. </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif "">The next best thing is to fail during
the development, thus an Error. It should be an error
and not a RuntimeException because it communicates a
design error, and not a run time, i.e. a legitimate run
time condition. It is also not an
IllegalArgumentException because there should be no
scenario when this could happen.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif "">In other words, the condition should get
fixed by a redesign rather than by handling/ignoring an
exception. As stated in the Error javadoc</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:8.5pt;font-family:"Arial",sans-serif;color:black;background:#ECEBEC">An </span><span style="font-size:8.5pt;font-family:"Courier New
\,serif\;color\:black "">Error</span><span style="font-size:8.5pt;font-family:"Arial",sans-serif;color:black;background:#ECEBEC"> is
a subclass of </span><span style="font-size:8.5pt;font-family:"Courier New
\,serif\;color\:black "">Throwable</span><span style="font-size:8.5pt;font-family:"Arial",sans-serif;color:black;background:#ECEBEC"> that
indicates
<span style="background:yellow;mso-highlight:yellow">serious
problems that a reasonable application should not try
to catch</span>. Most such errors are abnormal
conditions. The </span><span style="font-size:8.5pt;font-family:"Courier New
\,serif\;color\:black "">ThreadDeath</span><span style="font-size:8.5pt;font-family:"Arial",sans-serif;color:black;background:#ECEBEC"> error,
though a "normal" condition, is also a subclass of </span><span style="font-size:8.5pt;font-family:"Courier New
\,serif\;color\:black "">Error</span><span style="font-size:8.5pt;font-family:"Arial",sans-serif;color:black;background:#ECEBEC"> because
most applications should not try to catch it.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif "">if this idea seems to radical, I am ok
with making it an IllegalArgumentException.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif "">-andy</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif ""> </span><o:p></o:p></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">Kevin
Rushforth <a href="mailto:kevin.rushforth@oracle.com" moz-do-not-send="true">
<kevin.rushforth@oracle.com></a><br>
<b>Date: </b>Friday, 2022/07/22 at 13:42<br>
<b>To: </b>Andy Goryachev <a href="mailto:andy.goryachev@oracle.com" moz-do-not-send="true"><andy.goryachev@oracle.com></a>,
<a href="mailto:openjfx-dev@openjdk.org" moz-do-not-send="true" class="moz-txt-link-freetext">openjfx-dev@openjdk.org</a>
<a href="mailto:openjfx-dev@openjdk.org" moz-do-not-send="true">
<openjfx-dev@openjdk.org></a><br>
<b>Subject: </b>Re: [External] : Aw: Proposal: Add
Skin.install() method</span><o:p></o:p></p>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:11.0pt">I don't know if you really
meant Error, as in java.lang.Error, but it would need to
be a subclass of RuntimeException.
IllegalArgumentException seems the natural choice (a
case could possibly be made for IllegalStateException).
Throwing an Error is not the right thing for a library
to do in response to an application passing in an
illegal or unexpected argument to a method or
constructor. It is for truly exceptional things that a
programmer cannot anticipate (like running out of
memory).<br>
<br>
-- Kevin<br>
<br>
<br>
<br>
</span><o:p></o:p></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt">On
7/22/2022 12:37 PM, Andy Goryachev wrote:</span><o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif "">I would rather throw an Error in
Skinnable.setSkin() when mismatch is detected. This
is a design error that should be caught early in
development rather than a run time exception.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif "">-andy</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New
\,serif ""> </span><o:p></o:p></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 href="mailto:openjfx-dev-retn@openjdk.org" moz-do-not-send="true">
<openjfx-dev-retn@openjdk.org></a> on behalf
of Kevin Rushforth <a href="mailto:kevin.rushforth@oracle.com" moz-do-not-send="true">
<kevin.rushforth@oracle.com></a><br>
<b>Date: </b>Friday, 2022/07/22 at 12:33<br>
<b>To: </b><a href="mailto:openjfx-dev@openjdk.org" moz-do-not-send="true" class="moz-txt-link-freetext">openjfx-dev@openjdk.org</a>
<a href="mailto:openjfx-dev@openjdk.org" moz-do-not-send="true">
<openjfx-dev@openjdk.org></a><br>
<b>Subject: </b>Re: [External] : Aw: Proposal: Add
Skin.install() method</span><o:p></o:p></p>
</div>
<p class="MsoNormal"><span style="font-size:11.0pt">I
would not be in favor of adding a no-arg constructor
to SkinBase, for the reasons Andy gave. Additionally,
there would be no way to avoid braking the contract of
Skin::getSkinnable which says:<br>
<br>
"This value will only ever go from a non-null to null
value when the Skin is removed from the Skinnable, and
only as a consequence of a call to dispose()."<br>
<br>
<br>
<br>
<br>
<br>
</span><o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">At the very minimum, we should
explain in Skin javadoc that creating a skin for one
control and setting it in the other is a no-no. Or,
perhaps we should explicitly check for this
condition in setSkin().</span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:11.0pt"><br>
I agree completely. At a minimum this enhancement
should change the docs for setSkin to say that a skin
created for one control should not (must not?) be used
in another control. And unless there is a legitimate
use case I haven't thought of, I think we could
consider an explicit check, and either throw an
Exception (this seems the best choice, unless there
are compatibility concerns), or else log a warning and
treat it as a no-op.<br>
<br>
-- Kevin<br>
<br>
<br>
<br>
<br>
</span><o:p></o:p></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt">On
7/22/2022 9:13 AM, Andy Goryachev wrote:</span><o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">You do bring a good point! I
don't know the rationale behind passing control
pointer to the Skin constructor.
</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">I think Swing got it right,
clearly separating</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<ol style="margin-top:0in" type="1" start="1">
<li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo3"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">instantiation (using either a
no-arg constructor, or any other constructor that
does not require component pointer)</span><o:p></o:p></li>
<li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo3"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">configuration (optional step,
possibly widely separated in time and space)</span><o:p></o:p></li>
<li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo3"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">uninstallation of the old skin</span><o:p></o:p></li>
<li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo3"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">installation of the new skin</span><o:p></o:p></li>
</ol>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">What you are proposing - moving
to a default constructor makes the most sense. It
comes with a high price though - everyone with a
custom skin implementation would need to change
their code. </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">At the very minimum, we should
explain in Skin javadoc that creating a skin for one
control and setting it in the other is a no-no. Or,
perhaps we should explicitly check for this
condition in setSkin().</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">Thank you</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">-andy</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></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">Marius Hanl <a href="mailto:mariushanl@web.de" moz-do-not-send="true">
<mariushanl@web.de></a><br>
<b>Date: </b>Friday, 2022/07/22 at 05:06<br>
<b>To: </b><a href="mailto:openjfx-dev@openjdk.org" moz-do-not-send="true" class="moz-txt-link-freetext">openjfx-dev@openjdk.org</a>
<a href="mailto:openjfx-dev@openjdk.org" moz-do-not-send="true">
<openjfx-dev@openjdk.org></a>, Andy
Goryachev <a href="mailto:andy.goryachev@oracle.com" moz-do-not-send="true">
<andy.goryachev@oracle.com></a><br>
<b>Subject: </b>[External] : Aw: Proposal: Add
Skin.install() method</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt">I
had a similar idea in the past and like the idea.<br>
Ideally, setting/switching a skin is a one step
process. Currently you can construct a skin for a
control and set it after to a different control.<br>
<br>
Your approach sounds good, if you can set a skin
by creating a new skin (with a default
constructor) and then the setSkin() method will
actually trigger the install process on the
control (this), this will work and solve the
problem above. But for backward compatibilty we
still need to keep the skin constructor with the
control as parameter and think about deprecating
it.<br>
<br>
-- Marius</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt">Am
20.07.22, 23:40 schrieb Andy Goryachev
<a href="mailto:andy.goryachev@oracle.com" moz-do-not-send="true"><andy.goryachev@oracle.com></a>:</span><o:p></o:p></p>
<blockquote style="border:none;border-left:solid
#CCCCCC 1.0pt;padding:0in 0in 0in
6.0pt;margin-left:4.8pt;margin-top:4.8pt;margin-right:0in;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">Hi,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">Thank you,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">-andy</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">Summary</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">-------</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">Problem</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">-------</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">This creates two problems:</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Apple
Color Emoji"">☂</span><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif "">JDK-8236840 Memory leak when
switching ButtonSkin</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif "">JDK-8244419 TextAreaSkin:
throws UnsupportedOperation on dispose</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">Solution</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">--------</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">Impact Analysis</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif "">-------------</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;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-size:11.0pt;font-family:"Courier
New \,serif ""> </span><o:p></o:p></p>
</blockquote>
</div>
</blockquote>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span style="font-size:11.0pt"><o:p> </o:p></span></p>
</div>
</blockquote>
<br>
</body>
</html>