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; [email protected]
*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:* [email protected] <mailto:[email protected]>
*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:* [email protected]
<mailto:[email protected]>
*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:* [email protected]
<mailto:[email protected]>
*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
