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) 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 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.
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
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). In case where customers could benefit from the new API, the change is trivial.
The change will require CSR as it modifies a public API.
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().
Specification
-------------
javafx.scene.control.Skin interface:
```
/**
* An interface 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.
* <p/>
* A Skin implementation should generally avoid modifying its control outside of
* {@link #install()} method. The recommended life cycle of a Skin implementation
* is as follows:
* <ul>
* <li>instantiation
* <li>configuration, such as passing of dependencies and parameters
* <li>inside of {@link Control#setSkin(Skin)}:
* <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 by {@link Control#setSkin(Skin)} on a pristine control, or after the
* previous skin has been uninstalled via its {@link #dispose()} method.
* This method allows a Skin to register listeners, add child nodes, set
* required properties and/or event handlers.
*/
default public void install() { }
```
javafx.scene.control.Skinnable.setSkin() method:
```
/**
* Sets the skin that will render this {@link Control}.
* <p>
* To ensure a one-to-one relationship between a {@code Control} and its
* {@code Skin}, this method must check (for any non-null value) that
* {@link Skin#getSkinnable()}, throwing an IllegalArgumentException
* in the case of mismatch.
* returns the same value as this Skinnable.
*
* @param value the skin value for this control
*
* @throws IllegalArgumentException if {@link Skin#getSkinnable()} returns
* value other than this Skinnable.
*/
public void setSkin(Skin<?> value);
```
-------
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) 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 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.
A number of related bugs have been collected under JDK-8241364 ☂ Cleanup skin implementations to allow switching, which refers a number of bugs:
JDK-8245303 InputMap: memory leak due to incomplete cleanup on remove mapping
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 (
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). In case where customers could benefit from the new API, the change is trivial.
The change will require CSR as it modifies a public API.
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().
Specification
-------------
javafx.scene.control.Skin interface:
```
/**
* An interface 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.
* <p/>
* A Skin implementation should generally avoid modifying its control outside of
* {@link #install()} method. The recommended life cycle of a Skin implementation
* is as follows:
* <ul>
* <li>instantiation
* <li>configuration, such as passing of dependencies and parameters
* <li>inside of {@link Control#setSkin(Skin)}:
* <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 by {@link Control#setSkin(Skin)} on a pristine control, or after the
* previous skin has been uninstalled via its {@link #dispose()} method.
* This method allows a Skin to register listeners, add child nodes, set
* required properties and/or event handlers.
*/
default public void install() { }
```
javafx.scene.control.Skinnable.setSkin() method:
```
/**
* Sets the skin that will render this {@link Control}.
* <p>
* To ensure a one-to-one relationship between a {@code Control} and its
* {@code Skin}, this method must check (for any non-null value) that
* {@link Skin#getSkinnable()}, throwing an IllegalArgumentException
* in the case of mismatch.
* returns the same value as this Skinnable.
*
* @param value the skin value for this control
*
* @throws IllegalArgumentException if {@link Skin#getSkinnable()} returns
* value other than this Skinnable.
*/
public void setSkin(Skin<?> value);
```