/**
* 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 <[email protected]>
Date: Friday, 2022/07/22 at 14:54
To: Andy Goryachev <[email protected]>, [email protected]
<[email protected]>, Philip Race <[email protected]>
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
<[email protected]><mailto:[email protected]>
Date: Friday, 2022/07/22 at 14:20
To: Andy Goryachev
<[email protected]><mailto:[email protected]>,
[email protected]<mailto:[email protected]>
<[email protected]><mailto:[email protected]>
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
<[email protected]><mailto:[email protected]>
Date: Friday, 2022/07/22 at 13:42
To: Andy Goryachev
<[email protected]><mailto:[email protected]>,
[email protected]<mailto:[email protected]>
<[email protected]><mailto:[email protected]>
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
<[email protected]><mailto:[email protected]> on behalf
of Kevin Rushforth
<[email protected]><mailto:[email protected]>
Date: Friday, 2022/07/22 at 12:33
To: [email protected]<mailto:[email protected]>
<[email protected]><mailto:[email protected]>
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 <[email protected]><mailto:[email protected]>
Date: Friday, 2022/07/22 at 05:06
To: [email protected]<mailto:[email protected]>
<[email protected]><mailto:[email protected]>, Andy Goryachev
<[email protected]><mailto:[email protected]>
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
<[email protected]><mailto:[email protected]>:
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.
