Proposal: Add Skin.install() method

Sat Jul 23 12:48:42 UTC 2022

Configuration can be part of the factory or not?

Simple case:

       control.installSkin(MySkin::new)

More complicated case:

       control.installSkin(c -> {
            MySkin s = new MySkin(c);
            // configure stuff
            return s;
       });

--John

On 22/07/2022 17:58, Andy Goryachev wrote:
>
> > control.installSkin(MySkin::new);
>
> This is an interesting idea. Control.installSkin(Function<Control,Skin>).
>
> One of the requirements we ought to consider is maximizing the 
> backward compatibility.  If we were to add a new installSkin method it 
> would not solve the problem with the old method, and replacing 
> setSkin(Skin) with installSkin() would break compatibility with the 
> existing code.
>
> There is one more reason to allow for creation of a skin outside of 
> setSkin() - configuration. Imagine customer skins require 
> configuration, in which case the sequence of events looks like this
>
> instantiation -> configuration -> uninstall old skin -> install new skin.
>
> with installSkin(MySkin::new) such a model will be impossible.
>
> Thank you
>
> -andy
>
> *From: *openjfx-dev <openjfx-dev-retn at openjdk.org> on behalf of John 
> Hendrikx <john.hendrikx at gmail.com>
> *Date: *Thursday, 2022/07/21 at 15:49
> *To: *openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
> *Subject: *Re: Proposal: Add Skin.install() method
>
> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20220723/6f1e5c62/attachment-0001.htm>