Hi Andy,
Was a single step install process considered, something like:
control.installSkin(MySkin::new);
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.
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.
--John
On 20/07/2022 23:39, Andy Goryachev wrote:
Hi,
I'd like to propose an API change in Skin interface (details below).
Your feedback will be greatly appreciated!
Thank you,
-andy
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.
We can see the damage caused by looking at JDK-8241364
<https://bugs.openjdk.org/browse/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-8236840 Memory leak when switching ButtonSkin
JDK-8240506 TextFieldSkin/Behavior: misbehavior on switching skin
JDK-8242621 TabPane: Memory leak when switching skin
JDK-8244657 ChoiceBox/ToolBarSkin: misbehavior on switching skin
JDK-8245282 Button/Combo Behavior: memory leak on dispose
JDK-8246195 ListViewSkin/Behavior: misbehavior on switching skin
JDK-8246202 ChoiceBoxSkin: misbehavior on switching skin, part 2
JDK-8246745 ListCell/Skin: misbehavior on switching skin
JDK-8247576 Labeled/SkinBase: misbehavior on switching skin
JDK-8253634 TreeCell/Skin: misbehavior on switching skin
JDK-8256821 TreeViewSkin/Behavior: misbehavior on switching skin
JDK-8269081 Tree/ListViewSkin: must remove flow on dispose
JDK-8273071 SeparatorSkin: must remove child on dispose
JDK-8274061 Tree-/TableRowSkin: misbehavior on switching skin
JDK-8244419 TextAreaSkin: throws UnsupportedOperation on dispose
JDK-8244531 Tests: add support to identify recurring issues with
controls et al
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 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. 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.
