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

Fri Jul 22 20:41:58 UTC 2022

I don't know if you really meant Error, as in java.lang.Error, but it 
would need to be a subclass of RuntimeException. 
IllegalArgumentException seems the natural choice (a case could possibly 
be made for IllegalStateException). Throwing an Error is not the right 
thing for a library to do in response to an application passing in an 
illegal or unexpected argument to a method or constructor. It is for 
truly exceptional things that a programmer cannot anticipate (like 
running out of memory).

-- Kevin

On 7/22/2022 12:37 PM, Andy Goryachev wrote:
>
> I would rather throw an Error in Skinnable.setSkin() when mismatch is 
> detected.  This is a design error that should be caught early in 
> development rather than a run time exception.
>
> -andy
>
> *From: *openjfx-dev <openjfx-dev-retn at openjdk.org> on behalf of Kevin 
> Rushforth <kevin.rushforth at oracle.com>
> *Date: *Friday, 2022/07/22 at 12:33
> *To: *openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
> *Subject: *Re: [External] : Aw: Proposal: Add Skin.install() method
>
> 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
>
>      1. instantiation (using either a no-arg constructor, or any other
>         constructor that does not require component pointer)
>      2. configuration (optional step, possibly widely separated in
>         time and space)
>      3. uninstallation of the old skin
>      4. 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> <mailto:mariushanl at web.de>
>     *Date: *Friday, 2022/07/22 at 05:06
>     *To: *openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
>     <mailto:openjfx-dev at openjdk.org>, Andy Goryachev
>     <andy.goryachev at oracle.com> <mailto: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> <mailto: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/745028f3/attachment-0001.htm>