Hello Vlad,
Thanks for the review.
<<I don't understand why it was created as the method. Since you are changing
this part, maybe it will make sense and replace it with a constant string? I
hope Java compile can optimize method calls in situations like this one, if you
use constant strings:
I think it is done this way so if we need to change the logic for setting
prefix, it can be done inside the function, though I am not very sure of this.
Anyway, this is done this way for the UI components like BasicMenuUI,
BasicButtonUI etc. So it can't be changed here alone for BasicMenuItemUI with
this fix. If this needs to be done, please file a bug in JBS and I will look
into it and will fix this for all the UI components together with separate
review.
<< What do you think, would it be more efficient, if your "first" line will be
executed in the "else" branch of the "if" statement?
The icon which is set before the if statement is being used in nested if
statement. So I think this needs to stay like this. Please let me know if this
looks good.
&& iconFactory.isCompatible(checkIcon, prefix))
Regards,
Pankaj Bansal
-----Original Message-----
From: Volodin, Vladislav [mailto:vladislav.volo...@sap.com]
Sent: Wednesday, February 5, 2020 6:56 PM
To: Pankaj Bansal <pankaj.b.ban...@oracle.com>
Cc: swing-dev@openjdk.java.net
Subject: RE: <Swing Dev> [15] RFR JDK-8216329: Cannot resize CheckBoxItemMenu
in Synth L&F with setHorizontalTextPosition
Hello Pankaj,
I apologize, if I interfere your job, I decided to study your code to learn JDK
better, and I found out that the class BasicMenuItemUI has the method that
looks quite simple:
/*Returns a property prefix
* @return a property prefix */
protected String getPropertyPrefix() {
return "MenuItem";
}
I don't understand why it was created as the method. Since you are changing
this part, maybe it will make sense and replace it with a constant string? I
hope Java compile can optimize method calls in situations like this one, if you
use constant strings:
String prefix = getPropertyPrefix();
...
(...) iconFactory = (...) UIManager.get(prefix + ".checkIconFactory");
Another thing:
I see that in your code, you extract checkIcon, that might be overwritten by
"if" statement:
checkIcon = UIManager.getIcon(prefix + ".checkIcon"); // here we assign, and
(look down) boolean isColumnLayout = ...; if (isColumnLayout) { ...
if (...) {
checkIcon = iconFactory.getIcon(menuItem); // <-- here we might
overwrite checkIcon
}
}
What do you think, would it be more efficient, if your "first" line will be
executed in the "else" branch of the "if" statement?
Feel free to ignore my suggestion, if you think that it misleading.
Thanks,
Vlad
-----Original Message-----
From: swing-dev <swing-dev-boun...@openjdk.java.net> On Behalf Of Pankaj Bansal
Sent: Mittwoch, 5. Februar 2020 14:00
To: Sergey Bylokhov <sergey.bylok...@oracle.com>; swing-dev@openjdk.java.net
Subject: Re: <Swing Dev> [15] RFR JDK-8216329: Cannot resize CheckBoxItemMenu
in Synth L&F with setHorizontalTextPosition
Hello Sergey,
<< ok.
<<Do we need to remove the listener added to the menuItem?
<<I guess it will be added every time we change L&F to the windows and will
never be removed.
Yes, it should be done.
Webrev: http://cr.openjdk.java.net/~pbansal/8216329/webrev02/
Regards,
Pankaj
-----Original Message-----
From: Sergey Bylokhov
Sent: Thursday, January 30, 2020 5:59 AM
To: Pankaj Bansal <pankaj.b.ban...@oracle.com>; swing-dev@openjdk.java.net
Subject: Re: <Swing Dev> [15] RFR JDK-8216329: Cannot resize CheckBoxItemMenu
in Synth L&F with setHorizontalTextPosition
On 1/29/20 2:25 am, Pankaj Bansal wrote:
> One more point, I am able to reproduce the current issue with Synth
> LookAndFeel in all platforms without fix and it works fine with the fix.
ok.
Do we need to remove the listener added to the menuItem?
I guess it will be added every time we change L&F to the windows and will never
be removed.
>
> Regards,
> Pankaj
>
> -----Original Message-----
> From: Pankaj Bansal
> Sent: Wednesday, January 29, 2020 3:19 PM
> To: Sergey Bylokhov; swing-dev@openjdk.java.net
> Subject: Re: <Swing Dev> [15] RFR JDK-8216329: Cannot resize
> CheckBoxItemMenu in Synth L&F with setHorizontalTextPosition
>
> Hello Sergey,
>
> << Can you please double check that it is not possible to reproduce
> JDK-8152981 even if the test is modified in some way?
> <<For example if some other "basic" L&F will be used(Motif, Aqua)?
> I changed the test in JDK-8152981 to run on all installed LookAndFeels on
> windows, linux and Mac after removing the windows only condition. The tests
> passes on all platforms with all LookAndFeels with the current fix.
> I can check in this change in JDK-8152981 test along with the current fix if
> needed, though I feel it is not required as the issue was originally only in
> WindowsLookAndFeel.
>
> Regards,
> Pankaj Bansal
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Wednesday, January 29, 2020 1:17 PM
> To: Pankaj Bansal; swing-dev@openjdk.java.net
> Subject: Re: <Swing Dev> [15] RFR JDK-8216329: Cannot resize
> CheckBoxItemMenu in Synth L&F with setHorizontalTextPosition
>
> On 1/28/20 4:33 pm, Sergey Bylokhov wrote:
>> On 1/27/20 7:15 am, Pankaj Bansal wrote:
>>> << It is not a big issue, but for such a fix we will need a proper
>>> specification and CSR, it is like adding a new method to the public class.
>>> It is preferable to try to fix it in some other way first.
>>> I did not realize earlier that this can be done by making changes in
>>> WindowsMenuItemUI without calling the updateCheckIcon by moving the code in
>>> updateCheckIcon method in WindowsMenuItemUI class. I have made the changes
>>> for the same and all works fine. Also, I have removed the updateCheckIcon
>>> method from BasicMenuItemUI class as it is not needed.
>>
>> Can you please double check that it is not possible to reproduce JDK-8152981
>> even if the test is modified in some way?
>
> For example if some other "basic" L&F will be used(Motif, Aqua)?
>
>
--
Best regards, Sergey.
