/**
* 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.rushfo...@oracle.com>
*Date: *Friday, 2022/07/22 at 14:54
*To: *Andy Goryachev <andy.goryac...@oracle.com>,
openjfx-dev@openjdk.org <openjfx-dev@openjdk.org>, Philip Race
<philip.r...@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.rushfo...@oracle.com>
<mailto:kevin.rushfo...@oracle.com>
*Date: *Friday, 2022/07/22 at 14:20
*To: *Andy Goryachev <andy.goryac...@oracle.com>
<mailto:andy.goryac...@oracle.com>, openjfx-dev@openjdk.org
<openjfx-dev@openjdk.org> <mailto:openjfx-dev@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.rushfo...@oracle.com>
<mailto:kevin.rushfo...@oracle.com>
*Date: *Friday, 2022/07/22 at 13:42
*To: *Andy Goryachev <andy.goryac...@oracle.com>
<mailto:andy.goryac...@oracle.com>, openjfx-dev@openjdk.org
<openjfx-dev@openjdk.org> <mailto:openjfx-dev@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-r...@openjdk.org>
<mailto:openjfx-dev-r...@openjdk.org> on behalf of Kevin
Rushforth <kevin.rushfo...@oracle.com>
<mailto:kevin.rushfo...@oracle.com>
*Date: *Friday, 2022/07/22 at 12:33
*To: *openjfx-dev@openjdk.org <openjfx-dev@openjdk.org>
<mailto:openjfx-dev@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 <mariush...@web.de>
<mailto:mariush...@web.de>
*Date: *Friday, 2022/07/22 at 05:06
*To: *openjfx-dev@openjdk.org
<openjfx-dev@openjdk.org>
<mailto:openjfx-dev@openjdk.org>, Andy Goryachev
<andy.goryac...@oracle.com>
<mailto:andy.goryac...@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.goryac...@oracle.com>
<mailto:andy.goryac...@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.
