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

Phil Race philip.race at oracle.com
Fri Aug 2 21:09:43 UTC 2019


I just finished testing it and it is +1 and I've added the jdk13-fix-yes 
label.

-phil.

On 8/2/19 11:47 AM, Philip Race wrote:
> I have looked at it and I think it OK now.
> But I have not been able to test it as I am not near any Linux systems 
> right now.
>
> Please follow the jdk13 fix request process I sent you (off-line) two 
> days ago.
>
> -phil.
>
> On 8/2/19, 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/
>>     <http://cr.openjdk.java.net/%7Epbansal/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/f4fdbc13/attachment-0001.html>


More information about the swing-dev mailing list