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

Kevin Rushforth kevin.rushforth at oracle.com
Fri Aug 2 16:01:50 UTC 2019


This version looks good to me. I presume you've tested various 
combinations of colors for highlight and background colors?

-- Kevin


On 8/2/2019 8:34 AM, Pankaj Bansal wrote:
>
> Hi Phil/Kevin,
>
> I have updated the code for the suggestions given offline. Now, we are 
> not using YCrCb color model and just using the color difference in 
> individual components of background and highlight color to change the 
> highlight color.
>
> webrev: http://cr.openjdk.java.net/~pbansal/8226964/webrev03/
>
> Regards,
>
> Pankaj
>
> *From:*Kevin Rushforth
> *Sent:* Friday, August 2, 2019 3:47 AM
> *To:* Philip Race; 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
>
> The conversion to / from RGB and YCbCr look OK, but what you do to 
> derive a new color is not. It will work for gray-scale colors, but if 
> you deviate from gray it will drastically change the color, even 
> generating out of range values if one component is small and another 
> is large.
>
> Here are some examples in YCbCr space :
>
> orig color: 100, 50, 0
> derived color: 112, 194, 99   (too green)
>
> orig color: 55, 0, 225
> derived color: 286, 32, 324   (out of range)
>
> I think it will be better to convert to something like HSV (aka HSB) 
> and do the lightening or darkening in that space. I tried a quick test 
> program using JavaFX, which has built-in RGB <--> HSB conversion and 
> that works as I would expect.
>
> Here are the same examples in HSB space:
>
> orig color: 100, 50, 0
> derived color: 200, 100, 0
>
> orig color: 55, 0, 225
> derived color: 31, 0, 125
>
> -- Kevin
>
> On 8/1/2019 2:50 PM, Philip Race wrote:
>
>     The if/else looks OK now.
>
>     But can you point to the exact the source you used for the
>     formulas for RGB->YCbCr
>     conversion and the conversion back from YCbCr to RGB ?
>
>     Kevin & I were trying to figure it out and then Kevin wrote some test
>     programs to see how round tripping works and found some
>     significant problems.
>
>     I'll let Kevin reply with his findings.
>
>     -phil.
>
>
>     On 8/1/19, 11:58 AM, Pankaj Bansal wrote:
>
>         Hello Phil,
>
>         << So why can't we simplify it to this ?
>
>         I was trying something similar, but your method looks much
>         simpler to understand. I have used it.
>
>         I think in line2,  instead of
>
>         if (highlightLuminance + minLuminanceDifference <=
>         maxLuminance) {
>
>         you meant this. Rest I have used as such.
>
>         if (backgroundLuminance + minLuminanceDifference <=
>         maxLuminance) {
>
>         <<1)  can we avoid "if(" and include a space as in "if (" >
>         <<2) There are some lines much > 80 chars.
>
>         Fixed
>
>         webrev: http://cr.openjdk.java.net/~pbansal/8226964/webrev02/
>         <http://cr.openjdk.java.net/%7Epbansal/8226964/webrev02/>
>
>         Regards,
>
>         Pankaj
>
>         *From:*Philip Race
>         *Sent:* Thursday, August 1, 2019 10:24 PM
>         *To:* Pankaj Bansal
>         *Cc:* swing-dev at openjdk.java.net
>         <mailto: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
>
>         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
>             <mailto: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/20190802/5bf63007/attachment-0001.html>


More information about the swing-dev mailing list