<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