Re: [External] : Aw: Proposal: Add Skin.install() method
Kevin Rushforth
kevin.rushforth at oracle.com
Fri Jul 22 21:54:27 UTC 2022
Yes, but a RuntimeException only appears in the javadoc, and not in the
method signature (only checked exceptions show up in the method signature).
-- Kevin
On 7/22/2022 2:39 PM, Andy Goryachev wrote:
>
> Thank you, Phil and Kevin.
>
> @Override
>
> public final void setSkin(Skin<?> value) throws IllegalArgumentException {
>
> -andy
>
> *From: *Kevin Rushforth <kevin.rushforth at oracle.com>
> *Date: *Friday, 2022/07/22 at 14:20
> *To: *Andy Goryachev <andy.goryachev at oracle.com>,
> openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
> *Subject: *Re: [External] : Aw: Proposal: Add Skin.install() method
>
> No, sorry. Error doesn't communicate the right thing. This is exactly
> is what RuntimeException is intended to be used for. It's no different
> from passing an out of range numerical value or a null to a method
> that doesn't take null. In all similar cases, the method will document
> the exception cases with '@throws' javadoc tags, and the developer can
> then understand that they passed something bad to the method. FWIW,
> I've never seen any Java API throw Error in this manner.
>
> -- Kevin
>
> On 7/22/2022 2:02 PM, Andy Goryachev wrote:
>
> I do mean java.lang.Error.
>
> The goal is to prevent an incorrect code from being shipped to the
> end user. There are no tools at the API level to enforce the 1:1
> relationship, so it cannot be checked at compile time.
>
> The next best thing is to fail during the development, thus an
> Error. It should be an error and not a RuntimeException because
> it communicates a design error, and not a run time, i.e. a
> legitimate run time condition. It is also not an
> IllegalArgumentException because there should be no scenario when
> this could happen.
>
> In other words, the condition should get fixed by a redesign
> rather than by handling/ignoring an exception. As stated in the
> Error javadoc
>
> An Error is a subclass of Throwable that indicates serious
> problems that a reasonable application should not try to catch.
> Most such errors are abnormal conditions. The ThreadDeath error,
> though a "normal" condition, is also a subclass of Error because
> most applications should not try to catch it.
>
> if this idea seems to radical, I am ok with making it an
> IllegalArgumentException.
>
> -andy
>
> *From: *Kevin Rushforth <kevin.rushforth at oracle.com>
> <mailto:kevin.rushforth at oracle.com>
> *Date: *Friday, 2022/07/22 at 13:42
> *To: *Andy Goryachev <andy.goryachev at oracle.com>
> <mailto:andy.goryachev at oracle.com>, openjfx-dev at openjdk.org
> <openjfx-dev at openjdk.org> <mailto:openjfx-dev at openjdk.org>
> *Subject: *Re: [External] : Aw: Proposal: Add Skin.install() method
>
> 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>
> <mailto:openjfx-dev-retn at openjdk.org> on behalf of Kevin
> Rushforth <kevin.rushforth at oracle.com>
> <mailto:kevin.rushforth at oracle.com>
> *Date: *Friday, 2022/07/22 at 12:33
> *To: *openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
> <mailto: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/63a0ba05/attachment-0001.htm>
More information about the openjfx-dev
mailing list