Proposal: Add Skin.install() method

[Jeanette Winzenburg]

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

> 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

exactly :)

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

Well, we do - the cleanup is under way :) The umbrella issue is for  
keeping track. Have a look at SkinMemoryLeakTest: the 15 skins that  
are excluded from the test are not yet handled (most don't even have  
an issue yet).

-- Jeanette

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