<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <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.<br>
    </p>
    <p>--John</p>
    <div class="moz-cite-prefix">On 20/07/2022 23:39, Andy Goryachev
      wrote:<br>
    </div>
    <blockquote type="cite"
cite="mid:BL0PR10MB2948EF69EF37F6800BD1004AE58E9@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;}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.EmailStyle17
        {mso-style-type:personal-compose;
        font-family:"Courier New",serif;
        color:windowtext;}.MsoChpDefault
        {mso-style-type:export-only;}div.WordSection1
        {page:WordSection1;}</style>
      <div class="WordSection1">
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">Hi,<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">I'd like to propose an API change in Skin
            interface (details below).  Your feedback will be greatly
            appreciated!<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>
        <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>
        <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">Summary<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>
        <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().<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>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">Problem<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>
        <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.  <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 creates two problems:<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"> 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.<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"> 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.<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">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:<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">JDK-8245145 Spinner: throws
            IllegalArgumentException when replacing skin<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8245303 InputMap: memory leak due to
            incomplete cleanup on remove mapping<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8268877 TextInputControlSkin: incorrect
            inputMethod event handler after switching skin<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8236840 Memory leak when switching
            ButtonSkin<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8240506 TextFieldSkin/Behavior:
            misbehavior on switching skin<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8242621 TabPane: Memory leak when
            switching skin<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8244657 ChoiceBox/ToolBarSkin:
            misbehavior on switching skin
            <o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8245282 Button/Combo Behavior: memory
            leak on dispose<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8246195 ListViewSkin/Behavior:
            misbehavior on switching skin<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8246202 ChoiceBoxSkin: misbehavior on
            switching skin, part 2<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8246745 ListCell/Skin: misbehavior on
            switching skin<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8247576 Labeled/SkinBase: misbehavior
            on switching skin<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8253634 TreeCell/Skin: misbehavior on
            switching skin<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8256821 TreeViewSkin/Behavior:
            misbehavior on switching skin<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8269081 Tree/ListViewSkin: must remove
            flow on dispose<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8273071 SeparatorSkin: must remove
            child on dispose<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8274061 Tree-/TableRowSkin: misbehavior
            on switching skin<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">JDK-8244419 TextAreaSkin: throws
            UnsupportedOperation on dispose<o:p></o:p></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<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>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">Solution<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>
        <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.<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">Specifically, I'd like to propose the
            following changes:<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"> 1. Add Skin.install() with a default no-op
            implementation.<o:p></o:p></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<o:p></o:p></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()<o:p></o:p></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().<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>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif">Impact Analysis<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>
        <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.  <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">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.<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">The change will require CSR as it modifies
            a public API.<o:p></o:p></span></p>
        <p class="MsoNormal"><span style="font-family:"Courier
            New",serif"><o:p> </o:p></span></p>
      </div>
    </blockquote>
  </body>
</html>