<Swing Dev> [13] RFR JDK-8226964 [Yaru] - GTK L&F: There is no difference between menu selected and de-selected
Philip Race
philip.race at oracle.com
Thu Aug 1 16:53:41 UTC 2019
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>
More information about the swing-dev
mailing list