Re: Aw: Re: Proposal: Add Skin.install() method
Kevin Rushforth
kevin.rushforth at oracle.com
Tue Jul 26 14:55:18 UTC 2022
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 <john.hendrikx at gmail.com>:
>
> 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 <openjfx-dev-retn at openjdk.org> on behalf
> of John Hendrikx <john.hendrikx at gmail.com>
> *Date: *Thursday, 2022/07/21 at 15:49
> *To: *openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
> *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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20220726/e65f67e6/attachment-0001.htm>
More information about the openjfx-dev
mailing list