Re: [External] : Aw: Proposal: Add Skin.install() method
Philip Race
philip.race at oracle.com
Fri Jul 22 21:17:52 UTC 2022
Kevin is right. If you look (for a good example) in JDK's java.base
module for "new Error"
you'll be hard-pressed to find Error being thrown in response to a
passed in argument.
The cases I see are when something completely unexpected or "should not
happen" went
wrong in some implementation code.
And there are zero [*] cases of the javadoc pattern
"throws Error"
or
"exception Error"
-phil
* I did find a couple in java.desktop which made me grumble since
there's nothing we can do about those now.
On 7/22/22 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>
> *Date: *Friday, 2022/07/22 at 13:42
> *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
>
> 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/32e70955/attachment-0001.htm>
More information about the openjfx-dev
mailing list