<Swing Dev> [13] RFR JDK-8226964 [Yaru] - GTK L&F: There is no difference between menu selected and de-selected

[Philip Race]

I am not sure about the logic in the if/else blocks.
The way it is structured and the way the conditions are expressed
aren't making it easy for me to follow, or perhaps I am just failing
to understand some subtle requirements.

And why do you do this ?

+                        highlightLuminance += minLuminanceDifference;


Suppose that the highLight luminance difference was already "99",
why do you need to add another "100" to it ?

It seems to me that once we are in the block where we've decided that
the difference is < 100, the goal of every assignment has to be to
make sure the highlight is exactly either 100 brighter or 100 darker 
than the
background, isn't it ?

So why can't we simplify it to this ?

                     if (highlightLuminance >= backgroundLuminance) {
                         if (highlightLuminance + minLuminanceDifference 
<= maxLuminance) {
                            highlightLuminance = backgroundLuminance + 
minLuminanceDifference;
                         } else {
                             highlightLuminance = backgroundLuminance - 
minLuminanceDifference;
                         }
                     } else {
                         if (backgroundLuminance - 
minLuminanceDifference >= minLuminance) {
                             highlightLuminance = backgroundLuminance - 
minLuminanceDifference;
                          } else {
                             highlightLuminance = backgroundLuminance + 
minLuminanceDifference;
                         }
                     }

Two nits
1)  can we avoid "if(" and include a space as in "if (" >
2) There are some lines much > 80 chars.

-phil

On 8/1/19, 1:58 AM, Pankaj Bansal wrote:
>
> Hi Phil,
>
> << nit pick : you spelled luminance wrong here :
>
> <<int actualLuminaceDifference
>
> Corrected
>
> << Some of the variable naming is confusing me.
>
> << So until I get clarity on the intent here I can't verify the 
> correctness of
> the logic that follows although I understand your intent as being to make
> the highlight lighter so long as it is already lighter than the 
> background and
> you don't have to exceed max luminance to do it, and if that won't 
> work make
> the highlight darker instead. And so forth for the other cases.
>
> Yes, this is exactly what I am doing. I understand your point about 
> naming. Once I have defined what is highlight and what is background, 
> I should just use these for naming instead of menubar and menuitem. I 
> have corrected the naming of variables used.
>
> webrev: http://cr.openjdk.java.net/~pbansal/8226964/webrev01/ 
> <http://cr.openjdk.java.net/%7Epbansal/8226964/webrev01/>
>
> Regards,
>
> Pankaj
>
> *From:*Philip Race
> *Sent:* Thursday, August 1, 2019 6:16 AM
> *To:* Pankaj Bansal
> *Cc:* swing-dev at openjdk.java.net
> *Subject:* Re: <Swing Dev> [13] RFR JDK-8226964 [Yaru] - GTK L&F: 
> There is no difference between menu selected and de-selected
>
> nit pick : you spelled luminance wrong here :
>
> int actualLuminaceDifference
>
> Some of the variable naming is confusing me.
>
> Color highlightColor = style.getGTKColor(
> +                        GTKEngine.WidgetType.MENU_ITEM.ordinal(),
>   
> ok so Highlight comes from MENU_ITEM
>   
> and background comes from MENU_BAR :
> +                Color backgroundColor = style.getGTKColor(
> +                        GTKEngine.WidgetType.MENU_BAR.ordinal(),
>
> but then we assign menubar luminance from highlight which was got from 
> menu item
> and menu item luminance from background which was got from menu bar
>
>    double menubarLuminance = getY(highlightColor.getRed(),
> +                        highlightColor.getGreen(), highlightColor.getBlue());
> +                double menuitemLuminance = getY(backgroundColor.getRed(),
> +                        backgroundColor.getGreen(), backgroundColor.getBlue());
>
> Can you explain this ? Or is it wrong ?
>
> I get further confused when I read ..
>
> +                    // when the highlight has more luminance and it's luminance
> +                    // can be increased further by minLuminanceDifference
> +                    if ((menubarLuminance<  menuitemLuminance)&&
>
> .. so menuItemLuminance is greater implying it is the highlight but we 
> derived
> it from the background.
>
> So until I get clarity on the intent here I can't verify the 
> correctness of
> the logic that follows although I understand your intent as being to make
> the highlight lighter so long as it is already lighter than the 
> background and
> you don't have to exceed max luminance to do it, and if that won't 
> work make
> the highlight darker instead. And so forth for the other cases.
>
> -phil.
>
>
>
> On 7/31/19, 3:16 PM, Pankaj Bansal wrote:
>
>     Hi All,
>
>     Please review the following fix for jdk13.
>
>
>     Bug:
>
>     https://bugs.openjdk.java.net/browse/JDK-8226964
>
>     webrev
>
>     http://cr.openjdk.java.net/~pbansal/8226964/webrev00/
>     <http://cr.openjdk.java.net/%7Epbansal/8226964/webrev00/>
>
>     Issue:
>
>     The menu is not getting highlighted on being selected.
>
>     The issue is only reproducible on Yaru theme as the menu highlight
>     color used by java is very similar to the background color of
>     menubar. So the highlight is not properly visible due to poor
>     contrast.
>
>     Fix:
>
>     The fix checks that if the background menubar color and highlight
>     color are very close, make the highlight more darker or lighter,
>     so that it is clearly visible.
>
>     Testing:
>
>     The fix can be verified by running SwingSet2. I have verified this
>     on Ubuntu 16.04, 18.04, 19.04 and OEL 7.5.
>
>
>     Regards,
>     Pankaj Bansal
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/swing-dev/attachments/20190801/2b978a50/attachment-0001.html>