Re: RFR: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin
On Thu, 1 Feb 2024 12:09:36 GMT, Ambarish Rapte wrote: > @Maran23 This is ready for integration. If you were waiting for me. Thanks, > It looks good, I don't have any comments. Thank you for checking as well! - PR Comment: https://git.openjdk.org/jfx/pull/1331#issuecomment-1921417204
Re: RFR: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin
On Thu, 11 Jan 2024 20:13:09 GMT, Marius Hanl 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 extends Skin {
> 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)
@Maran23
This is ready for integration. If you were waiting for me. Thanks, It looks
good, I don't have any comments.
-
PR Comment: https://git.openjdk.org/jfx/pull/1331#issuecomment-1921183306
Re: RFR: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin
On Thu, 11 Jan 2024 20:13:09 GMT, Marius Hanl 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 extends Skin {
> 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)
Looks good, and works as I expect it.
-
Marked as reviewed by mstrauss (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1331#pullrequestreview-1850261757
Re: RFR: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin
On Thu, 11 Jan 2024 20:13:09 GMT, Marius Hanl 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 extends Skin {
> 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)
@arapte Can you be the second reviewer?
-
PR Comment: https://git.openjdk.org/jfx/pull/1331#issuecomment-1901365098
Re: RFR: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin
On Thu, 11 Jan 2024 20:13:09 GMT, Marius Hanl 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 extends Skin {
> 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)
Marked as reviewed by angorya (Reviewer).
I think the proposed fix is correct. I've tried to re-load skin via CSS (see
https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/bugs/PopupControl_SetSkin_8323615.java
) and it does seem to reload correctly, as far as I can tell.
Loading fails without the fix, as expected.
Still, i would like a second pair of eyes to look at this, if possible.
modules/javafx.controls/src/test/java/test/javafx/scene/control/PopupControlTest.java
line 680:
> 678: assertNotEquals("New skin was not set", oldSkin, newSkin);
> 679: }
> 680:
minor: please remove extra newline, I'll reapprove if you choose to make the
change
-
PR Review: https://git.openjdk.org/jfx/pull/1331#pullrequestreview-1818950331
PR Comment: https://git.openjdk.org/jfx/pull/1331#issuecomment-1889873614
PR Review Comment: https://git.openjdk.org/jfx/pull/1331#discussion_r1450870863
Re: RFR: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin
On Thu, 11 Jan 2024 20:35:08 GMT, Andy Goryachev 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 extends Skin {
>> 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
Re: RFR: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin
On Thu, 11 Jan 2024 20:13:09 GMT, Marius Hanl 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 extends Skin {
> 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?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1331#discussion_r1449367019
