These additional change are more disruptive, and I don't see enough
value to go down this path. I prefer to keep the existing constructor,
and add the newly-proposed Skin::install method. Unless we find
something very compelling that can't be done using that pattern, the
cost of making a more intrusive change doesn't seem justified.
-- Kevin
On 7/26/2022 7:39 AM, Marius Hanl wrote:
I don't see how this fixes the underlying problem, since you could
still do stuff like:
control.installSkin <http://control.installSkin>(c -> {
MySkin s = new MySkin(myOtherControl);
// configure stuff
return s;
});
So I think the super clean way would still be to have a default
constructor for every skin, while keeping the control constructor for
backwards compatibilty.
But a check is also already a good step.
It might be also worth to add a 'uninstall' method and move the
'dispose' method content into it and deprecate it.
Then it is much more clear from the wording, but is not 100% necessary.
-- Marius
Am 23.07.22, 14:49 schrieb John Hendrikx <[email protected]>:
Configuration can be part of the factory or not?
Simple case:
control.installSkin(MySkin::new)
More complicated case:
control.installSkin(c -> {
MySkin s = new MySkin(c);
// configure stuff
return s;
});
--John
On 22/07/2022 17:58, Andy Goryachev wrote:
> control.installSkin(MySkin::new);
This is an interesting idea.
Control.installSkin(Function<Control,Skin>).
One of the requirements we ought to consider is maximizing the
backward compatibility. If we were to add a new installSkin
method it would not solve the problem with the old method, and
replacing setSkin(Skin) with installSkin() would break
compatibility with the existing code.
There is one more reason to allow for creation of a skin
outside of setSkin() - configuration. Imagine customer skins
require configuration, in which case the sequence of events
looks like this
instantiation -> configuration -> uninstall old skin ->
install new skin.
with installSkin(MySkin::new) such a model will be impossible.
Thank you
-andy
*From: *openjfx-dev <[email protected]> on behalf
of John Hendrikx <[email protected]>
*Date: *Thursday, 2022/07/21 at 15:49
*To: *[email protected] <[email protected]>
*Subject: *Re: Proposal: Add Skin.install() method
Hi Andy,
Was a single step install process considered, something like:
control.installSkin(MySkin::new);
This would also make it much more clear that Skins are single
use only, where the API currently has to bend over backwards
to make that clear and enforce it.
Other than that, I think your suggestion would be a definite
improvement over the current situation. Something never felt
quite right about how skins where set up and attached to
controls, it felt fragile and cumbersome -- possibly as the
result of relying on a writable property as the means to
install a skin.
--John
On 20/07/2022 23:39, Andy Goryachev wrote:
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.
