<Swing Dev> Request review for 7089914: Focus on image icons are not visible in javaws cache with high contrast mode

[Pavel Porvatov]

Hi Sean,
> Hi Pavel,
>
>      Thanks for your suggestion, I didn't know DesktopProperty could 
> be used this way.
> I modified it a little, it's 
> http://cr.openjdk.java.net/~zhouyx/7089914/webrev.05/ 
> <http://cr.openjdk.java.net/%7Ezhouyx/7089914/webrev.05/> .
>
> The reason for checking high contrast is windows allow 
> "win.3d.backgroundColor" to
> be set black in non-high contrast mode.
>
> The testcase is also cleared. Please review again.
I modified your fix a little bit and rewrote the test (because it 
contained concurrency problems, e.g. the passed field must be volatile 
etc). Here is the commit:
http://hg.openjdk.java.net/jdk8/awt/jdk/rev/362867d5caa4

Thanks, Pavel
>
> On Fri, Feb 10, 2012 at 10:36 PM, Pavel Porvatov 
> <pavel.porvatov at oracle.com <mailto:pavel.porvatov at oracle.com>> wrote:
>
>     Hi Sean,
>>     Hi Pavel,
>>
>>        I modified the patch again. It's now at
>>     http://cr.openjdk.java.net/~zhouyx/7089914/webrev.03/
>>     <http://cr.openjdk.java.net/%7Ezhouyx/7089914/webrev.03/> .
>>     3/4 comments are covered. And there are a little comment bellow,
>>     please have a look. Thanks.
>>
>>
>>     On Mon, Jan 16, 2012 at 6:46 PM, Pavel Porvatov
>>     <pavel.porvatov at oracle.com <mailto:pavel.porvatov at oracle.com>> wrote:
>>
>>         Hi Sean,
>>
>>         There are several comments for your patch:
>>
>>         1. I found one bug. In Win 7 there are several differnet High
>>         Contrast themes. Under one of them ("High Contrast White")
>>         focus is not visible...
>>
>>           This is fixed.
>     Is there any reason to check high contrast mode in the
>     WDesktopProperties class? I think in non-high contrast mode
>     backgroundColor checking will be useful as well
>
>>
>>         2. I also don't like synthetic property. Any desktop property
>>         has listener (see
>>         com.sun.java.swing.plaf.windows.DesktopProperty#pcl) and
>>         updated when correspondent value is changed. But your
>>         win.button.focusColor property is updated only while full
>>         property reloading.
>>
>>           I tried to not use DesktopProperty, but it is found that
>>     the modification becomes much more complex and not working well.
>>     On the other hand, I think
>>           use DesktopProperty is reasonable: it is so much like a
>>     desktop property that the only difference is it can not be
>>     changed separately.
>     What do you think about the following solution:
>
>         static class FocusColorProperty extends DesktopProperty {
>             FocusColorProperty(Object fallback) {
>                 super("win.3d.backgroundColor", fallback);
>             }
>
>             @Override
>             protected Object configureValue(Object value) {
>                 return Color.BLACK.equals(value) ? Color.WHITE :
>     Color.BLACK;
>             }
>         }
>
>     This class solves update problems. Actually I didn't check that
>     but it seems it should work...
>
>
>>
>>         3. Please don't add references to bugs in the code. Everybody
>>         can trace history of the code by VCS.
>>
>>          Reference removed.
>>
>>
>>         4. The changes in the class WindowsRadioButtonUI looks good.
>>         Is it possible to make your TestButton test an automatic one
>>         and add it to the fix?
>>
>>          Test case added.
>     Could you please clean the test from TODO and dead code (like
>     commented code)
>
>     Regards, Pavel
>
>>
>>         Regards, Pavel
>>
>>
>>>         Hi all,
>>>
>>>            This is for bug 7089914,
>>>         http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7089914 .
>>>         OpenJDK uses black as focus color in windows LAF. However,
>>>         in high contrast mode, windows
>>>         uses white as focus color.
>>>            In additional, the patch also
>>>         modified WindowsRadioButtonUI.java so it will reload its style
>>>         when system setting is changed.
>>>
>>>            The webrev link is :
>>>         http://cr.openjdk.java.net/~zhouyx/7089914/webrev.02/
>>>         <http://cr.openjdk.java.net/%7Ezhouyx/7089914/webrev.02/>
>>>
>>>         The previous discussions are:
>>>         http://mail.openjdk.java.net/pipermail/swing-dev/2011-September/001703.html
>>>         http://mail.openjdk.java.net/pipermail/swing-dev/2011-October/001794.html
>>>         http://mail.openjdk.java.net/pipermail/swing-dev/2011-December/001871.html
>>>
>>>         Any comments on the this version? thanks.
>>
>>
>>
>>
>>     -- 
>>     Best Regards,
>>     Sean Chou
>>
>
>
>
>
> -- 
> Best Regards,
> Sean Chou
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120216/28780046/attachment.html>