RFR: 8290844: Add Skin.install() method [v2]

Marius Hanl mhanl at openjdk.org
Thu Aug 11 14:08:37 UTC 2022


On Thu, 11 Aug 2022 13:55:10 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>> I like this idea.
>> 
>> However, I am afraid that the impact of the check in PopupControl might be greater than expected.  In addition to your ComboBoxPopupcontrol.createPopup() fix you provided earlier, there is a similar situation with TooltipSkin in PopupcontrolTest:604:
>> 
>> 
>>         Tooltip tooltip = new Tooltip("Hello");
>>         TooltipSkin skin = new TooltipSkin(tooltip);
>>         popup.setSkin(skin);
>> 
>> 
>> it is not entirely clear to me why a TooltipSkin is installed as a skin for the popup instead of the tooltip.
>> 
>> I think we should rather say that this 1:1 rule does not apply to PopupControls.
>
> It is a bit weird as `Popups` are not using the `getSkinnable` method it looks like. And also the `getSkinnable` method javadoc states: `Gets the Skinnable to which this Skin is assigned.` which is not true for the `ComboBox` Popup (as it sets the `ComboBox` as the skinnable) - while it makes sense that the `ComboBox` is set as styleable parent.

So it is even more weird:
`getSkinnable()` is always called in `Control` skins, `getNode()` is only called in `Control.getSkinNode()`, `PaginationSkin` and `TableColumnHeader` (so not very much).
`getNode()` is always called for `Popups` (no `getSkinnable()` is called).

The only exception is the internal class `InputFieldSkin`, where the node is an 'inner TextField' and the skinnable the control where it is installed on.

-------------

PR: https://git.openjdk.org/jfx/pull/845


More information about the openjfx-dev mailing list