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:
"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()."
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().
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.
-- Kevin
On 7/22/2022 9:13 AM, Andy Goryachev wrote:
You do bring a good point! I don't know the rationale behind passing
control pointer to the Skin constructor.
I think Swing got it right, clearly separating
* instantiation (using either a no-arg constructor, or any other
constructor that does not require component pointer)
* configuration (optional step, possibly widely separated in time
and space)
* uninstallation of the old skin
* installation of the new skin
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.
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().
Thank you
-andy
*From: *Marius Hanl <mariush...@web.de>
*Date: *Friday, 2022/07/22 at 05:06
*To: *openjfx-dev@openjdk.org <openjfx-dev@openjdk.org>, Andy
Goryachev <andy.goryac...@oracle.com>
*Subject: *[External] : Aw: Proposal: Add Skin.install() method
I had a similar idea in the past and like the idea.
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.
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.
-- Marius
Am 20.07.22, 23:40 schrieb Andy Goryachev <andy.goryac...@oracle.com>:
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.
