RFR: 8276056: Control.skin.setSkin(Skin) fails to call dispose() on discarded Skin

[Marius Hanl]

On Thu, 30 Jun 2022 16:16:32 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> For some reason the `skinProperty` did not allow to set a new skin which is the same class as the previous one.
>> This leads to multiple issues:
>> 1. When creating a new skin (same class as previous), the skin will likely install some children and listener but is then rejected when setting it due to the `skinProperty` class constraint
>> -> Control is in a weird state as the current skin was not disposed since it is still set, although we already and 'set' a new skin
>> 2. A skin of the same class can behave differently, so it is not really valid to reject a skin just because it is the same class as the previous
>> -> Just imagine we have the following skin class
>> 
>> class MyCustomButtonSkin extends ButtonSkin {
>>     public MyCustomButtonSkin(Button button, boolean someFlag) { super(button); ... }
>> }
>> 
>> Now if we would change the skin of the Button two times like this:
>> 
>> button.setSkin(new MyCustomButtonSkin(button, true));
>> button.setSkin(new MyCustomButtonSkin(button, false));
>> 
>> The second time the skin will be rejected as it is the same class, although I may changed the skin behaviour, in this case demonstrated by a boolean flag for showing purposes.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line 241:
> 
>> 239: 
>> 240:         @Override
>> 241:         //This code is basically a kind of optimization that prevents a Skin that is equal but not instance equal.
> 
> Why was this code here in the first place, considering that switching skins is surely not something that would logically be done repeatedly? Are there performance implications with this change?

I don't know. I think someone guessed that if a new skin is from the same class it must be the same and can therefore be rejected. While this will probably work 95% the times, it is still not a 100% correct assumption.

This has no performance implications.

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

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