<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