RFR: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin

[Marius Hanl]

On Thu, 11 Jan 2024 20:35:08 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> For some reason the `skinProperty` did not allow to set a new skin when it 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 listener but is then rejected when setting it due to the `skinProperty` class constraint
>> -> `PopupControl` is in a weird state as the current skin was not disposed since it is still set, although we already created 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 MyCustomSkin<C extends PopupControl> extends Skin<C> {
>>     public MyCustomSkin(C skinnable, boolean someFlag) { ... }
>> }
>> 
>> Now if we would change the skin of the `PopupControl` two times like this:
>> 
>> popup.setSkin(new MyCustomSkin(popup, true));
>> popup.setSkin(new MyCustomSkin(popup, 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.
>> 
>> This is the same issue and fix as done for `Control` in [JDK-8276056](https://bugs.openjdk.org/browse/JDK-8276056) (PR: https://github.com/openjdk/jfx/pull/806)
>
> modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java line 216:
> 
>> 214:         private Skin<?> oldValue;
>> 215: 
>> 216:         @Override
> 
> This logic was introduce to deal with skins set from the css [JDK-8096194](https://bugs.openjdk.org/browse/JDK-8096194)
> 
> Are you sure it will still work?  Do we need to test this scenario explicitly?

No, the `skin`/`skinProperty` logic was there before, just moved.
It was added in https://bugs.openjdk.org/browse/JDK-8127070, 
with the comment from `David Grieve`: `The code for handling the skin property in PopupControl needs to look like the code in Control.`

So they basically synchronized the whole skin/skin name API to `PopupControl`.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1331#discussion_r1449380322