<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
Thu Aug 1 22:16:57 UTC 2019
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
>> *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/20190801/098111bd/attachment-0001.html>
More information about the swing-dev
mailing list