On Thu, 11 Jan 2024 20:35:08 GMT, Andy Goryachev <ango...@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