Proposal: Add Skin.install() method

[Andy Goryachev]

Dear Jeanette:

Thank you for responding!

You are right - some of the issues in JDK-8241364<https://bugs.openjdk.org/browse/JDK-8241364> ☂ Cleanup skin implementations to allow switching might be caused by other factors.  Still, from a pure design perspective, the fact that we are altering control while the new skin is being constructed is not right.

I suspect there is a number of other Skins where we might have memory leaks and other issues.  For example, MenuButtonSkinBase:96 tries to add an event handler based on the value of onMousePressed property.  Please correct me if I am wrong - should this event handler also be removed in dispose()?  Same in MenuButtonSkinBase:105 (line numbers provided are for the latest master branch).  Same in MenuButtonSkin:108

We may need to go through all the skins looking for places like this as part of JDK-8241364<https://bugs.openjdk.org/browse/JDK-8241364> ☂ Cleanup skin implementations.

Thank you so much for your comments!

-andy



From: openjfx-dev <openjfx-dev-retn at openjdk.org> on behalf of Jeanette Winzenburg <fastegal at swingempire.de>
Date: Thursday, 2022/07/21 at 09:04
To: openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
Subject: Re: Proposal: Add Skin.install() method

Zitat von Andy Goryachev <andy.goryachev at oracle.com>:

Hi Andy,

good idea to move our discussion from the issues to the mailling -
it's both easier and reaches a broader audience :)

Will answer in about a week or so, for now just stumbled across

> 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:

Hmm .. might be me, but looks like you are implying that with the
proposal in place (and used throughout our skins) those issues
wouldn't have existed? If that's not the case, could you please
clarify what you mean?. On the other hand, if yes, I disagree: the
majority of the issues (in particular those already fixed) are about
an incorrect/incomplete implementation of dispose - that's unrelated
to enhancing/cleaning up the install process. A subset (of the
unfixed) is indeed due to the parallel existence of two skins pointing
to the same control (and modifying its properties)

Cheers, Jeanette


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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20220721/703ce037/attachment-0001.htm>