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>
*Date: *Friday, 2022/07/22 at 13:42
*To: *Andy Goryachev <andy.goryac...@oracle.com>,
openjfx-dev@openjdk.org <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.
