Re: [External] : Aw: Proposal: Add Skin.install() method

Fri Jul 22 19:33:22 UTC 2022

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 <mariushanl at web.de>
> *Date: *Friday, 2022/07/22 at 05:06
> *To: *openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>, Andy 
> Goryachev <andy.goryachev at 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.goryachev at 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20220722/b289a3a3/attachment-0001.htm>