Summary
-------

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().


Problem
-------

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, possibly some properties set) 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.

This creates two problems:

 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.

 2. if, in addition to adding listeners and child nodes, the skin sets a property, such as an event listener, or an event handler/filter, 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.

A number of related bugs have been collected under JDK-8241364 ☂ Cleanup skin implementations to allow switching, which refers a number of bugs:

JDK-8245145 Spinner: throws IllegalArgumentException when replacing skin

JDK-8245303 InputMap: memory leak due to incomplete cleanup on remove mapping

JDK-8268877 TextInputControlSkin: incorrect inputMethod event handler after switching skin

JDK-8245145 Spinner: throws IllegalArgumentException when replacing skin


Solution
--------

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.

Specifically, I'd like to propose the following changes:

 1. Add Skin.install() with a default no-op implementation.
 2. Clarify skin life cycle (creation-attachment-detachment sequence) in Skin and Skin.install() javadoc
 3. Modify Control.setSkin(Skin) method (== invalidate listener in skin property) to call oldSkin.dispose() followed by newSkin.install()
 4. Check whether (newSkin.getSkinnable() == this) inside of Control.setSkin(Skin) property handler, and throw an Error if it isn't so (when newSkin is != null of course).
 5. 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().


Impact Analysis
-------------

The changes should be fairly trivial. Only a subset of skins needs to be refactored, and the refactoring itself is trivial.

The new API is backwards compatible with the existing code, the customer-developed skins can remain unchanged (thanks to default implementation), *unless* the customer extends one of the skins that require modification, *and* the code added by the customer depends on functionality moved to install().  Generally speaking, adding child Nodes in the Skin constructor is safe, adding listeners is safe, but setting fields, event handlers/filters is not, and should be moved to install() method (or risk creating the kind of bugs mentioned earlier).

Additional cleanup might be required on all skins that add event handler or another listener to its Skinnable directly, rather than via register*Listener, as those listeners are not removed in dispose().  Preliminary search came up with several skins (MenuButtonSkinBase, MenuButtonSkin, ProgressIndicatorSkin, ScrollBarSkin, TextInputControlSkin, SpinnerSkin), also please see [JDK-8241364](https://bugs.openjdk.org/browse/JDK-8241364)



Specification
-------------

javafx.scene.control.Skin interface:
```
/**
- * Base class for defining the visual representation of user interface controls
- * by defining a scene graph of nodes to represent the skin.
- * A user interface control is abstracted behind the {@link Skinnable} interface.
+ * An interface for defining the visual representation of user interface controls.
+ * <p>
+ * A Skin implementation should generally avoid modifying its control outside of
+ * {@link #install()} method.  The life cycle of a Skin implementation
+ * is as follows:
+ * <ul>
+ * <li>instantiation
+ * <li>configuration, such as passing of dependencies and parameters
+ * <li>when the skin is set on a {@link Skinnable}:
+ * <ul>
+ * <li>uninstalling of the old skin via its {@link #dispose()} method
+ * <li>installing of the new skin via {@link #install()}
+ * </ul>
+ * </ul>
 *
 * @param <C> A subtype of Skinnable that the Skin represents. This allows for
 * Skin implementation to access the {@link Skinnable} implementation,
 * which is usually a {@link Control} implementation.
 * @since JavaFX 2.0
 */
public interface Skin<C extends Skinnable> {


...
/**
+     * Called once when {@link Skin} is set.  This method is called after the previous skin,
+     * if any, has been uninstalled via its {@link #dispose()} method.
+     * The skin can now safely make changes to its associated control, like registering listeners,
+     * adding child nodes, and modifying properties and event handlers.
+     * <p>
+     * Application code must not call this method.
+     * <p>
+     * The default implementation of this method does nothing.
+     *
+     * @implNote
+     * Skins only need to implement {@code install} if they need to make direct changes to the control
+     * like overwriting properties or event handlers.  Such skins should ensure these changes are undone in
+     * their {@link #dispose()} method.
+     *
+     * @since 20
+     */
+    default public void install() { }



     /**
-     * Called by a Skinnable when the Skin is replaced on the Skinnable. This method
-     * allows a Skin to implement any logic necessary to clean up itself after
-     * the Skin is no longer needed. It may be used to release native resources.
-     * The methods {@link #getSkinnable()} and {@link #getNode()}
-     * should return null following a call to dispose. Calling dispose twice
-     * has no effect.
+     * Called when a previously installed skin is about to be removed from its associated control.
+     * This allows the skin to do clean up, like removing listeners and bindings, and undo any changes
+     * to the control's properties.
+     * After this method completes, {@link #getSkinnable()} and {@link #getNode()} should return {@code null}.
+     * <p>
+     * Calling {@link #dispose()} more than once has no effect.
      */
     public void dispose();
```

javafx.scene.control.Skinnable:
```
     /**
-     * Skin is responsible for rendering this {@code Control}. From the
-     * perspective of the {@code Control}, the {@code Skin} is a black box.
-     * It listens and responds to changes in state in a {@code Control}.
+     * The Skin responsible for rendering this {@code Skinnable}. From the
+     * perspective of the {@code Skinnable}, the {@code Skin} is a black box.
+     * It listens and responds to changes in state of its {@code Skinnable}.
      * <p>
-     * There is a one-to-one relationship between a {@code Control} and its
-     * {@code Skin}. Every {@code Skin} maintains a back reference to the
-     * {@code Control}.
+     * Some implementations of {@code Skinnable} define a one-to-one relationship between {@code Skinnable}
+     * and its {@code Skin}. Every {@code Skin} maintains a back reference to the
+     * {@code Skinnable}.  When required, this relationship is enforced when the {@code Skin} is set,
+     * throwing an {@code IllegalArgumentException} if the return value of {@link Skin#getSkinnable()}
+     * is not the same as this {@code Skinnable}.
      * <p>
      * A skin may be null.
      *
-     * @return the skin property for this control
+     * @return the skin property for this Skinnable
      */
     public ObjectProperty<Skin<?>> skinProperty();


–    /**
–     * Sets the skin that will render this {@link Control}
–     * @param value the skin value for this control
–     */
     public void setSkin(Skin<?> value);
 

–    /**
–     * Returns the skin that renders this {@link Control}
–     * @return the skin for this control
–     */
     public Skin<?> getSkin();
```

javafx.scene.control.Control class:

```
      * {@code Skin}. Every {@code Skin} maintains a back reference to the
      * {@code Control} via the {@link Skin#getSkinnable()} method.
      * <p>
+     * To ensure a one-to-one relationship between a {@code Control} and its {@code Skin},
+     * skins which were not created for this control are rejected with an
+     * {@code IllegalArgumentException}.
+     * Then, {@link Skin#dispose()} is called on the old skin, disconnecting
+     * it from the corresponding {@code Control}.  And finally, {@link Skin#install()} is invoked
+     * to complete the process.  Only inside of {@link Skin#install()} should {@code Skin} implementations
+     * set/overwrite properties of their {@code Control} (though some operations like adding/removing a listener
+     * can still be done in the {@code Skin} constructor).
+     * <p>
      * A skin may be null.
+     *
      * @return the skin property for this control
+     * @throws IllegalArgumentException if {@code (skin != null && skin.getSkinnable() != this)}
      */
     @Override public final ObjectProperty<Skin<?>> skinProperty() { return skin; }

```