<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 18:52:24 UTC 2019


Thanks for confirming.

+1 from me.

-- Kevin


On 8/2/2019 11:31 AM, Pankaj Bansal wrote:
>
> Hi Kevin,
>
> I have tested this with various color combinations and results were fine.
>
> Regards,
>
> Pankaj
>
> *From:*Kevin Rushforth
> *Sent:* Friday, August 2, 2019 9:32 PM
> *To:* Pankaj Bansal; Philip Race; 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
>
> 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 <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
>
>     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/b2c5461d/attachment-0001.html>


More information about the swing-dev mailing list