Re: [External] : Aw: Proposal: Add Skin.install() method
Kevin Rushforth
kevin.rushforth at oracle.com
Sat Jul 23 13:11:41 UTC 2022
Yes, something like this. We can discuss the wording of the doc changes
in the PR along with the other proposed API changes.
Thanks.
-- Kevin
On 7/22/2022 2:56 PM, Andy Goryachev wrote:
>
> /**
>
> * Sets the skin that will render this {@link Control}.
>
> * <p>
>
> * To ensure a one-to-one relationship between a {@code Control}
> and its
>
> * {@code Skin}, this method must check (for any non-null value) that
>
> * {@link Skin#getSkinnable()}, throwing an IllegalArgumentException
>
> * in the case of mismatch.
>
> * returns the same value as this Skinnable.
>
> *
>
> * @param value the skin value for this control
>
> *
>
> * @throws IllegalArgumentException if {@link Skin#getSkinnable()}
> returns
>
> * value other than this Skinnable.
>
> */
>
> public void setSkin(Skin<?> value);
>
> -andy
>
> *From: *Kevin Rushforth <kevin.rushforth at oracle.com>
> *Date: *Friday, 2022/07/22 at 14:54
> *To: *Andy Goryachev <andy.goryachev at oracle.com>,
> openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>, Philip Race
> <philip.race at oracle.com>
> *Subject: *Re: [External] : Aw: Proposal: Add Skin.install() method
>
> 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>
> <mailto:kevin.rushforth at oracle.com>
> *Date: *Friday, 2022/07/22 at 14:20
> *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
>
> 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/20220723/b91d9343/attachment-0001.htm>
More information about the openjfx-dev
mailing list