<Swing Dev> Fwd: Add keyboard accessibility to JColorChooser swatch
Pavel Porvatov
pavel.porvatov at oracle.com
Wed Aug 22 13:16:08 UTC 2012
Hi Sean,
> Hi Pavel,
>
> I updated the patch according to your comments except No.1 and
> No.4, it is now at:
> http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.03/
> <http://cr.openjdk.java.net/%7Ezhouyx/OJDK-61/webrev.03/> .
>
> About comment No.1 ( When right-to-left orientation the Recent
> swatches inverts right and left button ), I tried to set the locale to
> ar_AE, but didn't get a visual result about what I should do, please
> look at http://cr.openjdk.java.net/~zhouyx/OJDK-61/screenshot_3.png
> <http://cr.openjdk.java.net/%7Ezhouyx/OJDK-61/screenshot_3.png> .
> Can anyone give a little help on how to produce a right-to-left
> orientation in demo SwingSet2 or others?
I attached the test. It will be useful to add some tests to your change
(e.g. show JColorChooser with right-to-left orientation, pressing some
keys by awt Robot and checking the result)
>
> About comment No.4(Why new listeners are Serializable ), the
> original MouseListeners in this class are Serializable and I see
> javadoc for Component class says "Developers will need, as always, to
> consider the implications of making an object serializable" .
You are absolutely right about serialization. But if you take a look at
components serialization/deserialization you will see that BEFORE
serialization javax.swing.plaf.ComponentUI#uninstallUI is invoked and
AFTER deserialization javax.swing.plaf.ComponentUI#installUI is invoked.
So serialization is not broken when new listeners added and removed
correctly with instalUI/uninstallUI methods.
>
> Please take a look again, thanks.
Some additional comments below:
1. You should assign null to mainSwatchKeyListener and
recentSwatchKeyListener in the uninstallChooserPanel method
2. Do we have bug in bugs database for your fix? If no, could you please
file a bug with correspondent description. Use that bug number as a
folder name for the webrev, please
3. The code can be a little bit optimized. There is no need to call
repaint every time in code like this:
+ if (selRow > 0) {
+ selRow--;
+ }
+ repaint();
I'd prefer
+ if (selRow > 0) {
+ selRow--;
+ repaint();
+ }
In case KeyEvent.VK_HOME and KeyEvent.VK_END such optimization doesn't
look reasonable for me...
4. Could you please follow our code guide lines (spaces etc).
http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html
5. I have some concerns about selection mark in paintComponent:
a. On light swatches (e.g. on white) the mark looks like it shifted on 1
pixel
b. The color selection looks tricky. Does the following code from
DiagramComponent#paintComponent works
g.setXORMode(Color.WHITE);
g.setColor(Color.BLACK);
?
Regards, Pavel
> On Thu, Aug 16, 2012 at 10:00 PM, Pavel Porvatov
> <pavel.porvatov at oracle.com <mailto:pavel.porvatov at oracle.com>> wrote:
>
> Hi Sean,
>>
>> Updated the repository used in webrev from jdk8-tl
>> to http://hg.openjdk.java.net/jdk8/awt/jdk .
>>
>> new webrev: http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.01/
>> <http://cr.openjdk.java.net/%7Ezhouyx/OJDK-61/webrev.01/>
>>
> I have the following comments about the fix:
>
> 1. When right-to-left orientation the Recent swatches inverts
> right and left button.
> 2. Could you please don't use package visibility when
> fileds/methods/inner classes can be private (e.g. field
> mainSwatchKeyListener)
> 3. I think you should uninstall the introduced listeners in the
> DefaultSwatchChooserPanel#uninstallChooserPanel method
> 4. Why new listeners are Serializable?
> 5. I recommend to use if condition instead of switch/case blocks
> with one branch
> 6. Could you please rename selrow (and similar variables) into selRow
> 7. Can we use Component#isFocusOwner instead of supporting new
> variable showcursor?
> 8. Could you please follow our code guide lines (spaces etc)
>
> Regards, Pavel
>
>
>>
>> ---------- Forwarded message ----------
>> From: *Sean Chou* <zhouyx at linux.vnet.ibm.com
>> <mailto:zhouyx at linux.vnet.ibm.com>>
>> Date: Thu, Aug 9, 2012 at 3:29 PM
>> Subject: <Swing Dev> Add keyboard accessibility to JColorChooser
>> swatch
>> To: swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
>>
>>
>> Hi all,
>>
>> This is a patch to add keyboard accessibility to
>> JColorChooser swatch, so when using TAB, the focus can move
>> to SwatchPanel, choose color with arrow keys and select color
>> with space bar.
>>
>> In current implementation, it is not able to move the focus
>> to SwatchPanel with TAB, this can be seen in SwingSet2 demo.
>> Steps:
>> 1. run SwingSet2 demo
>> 2. click on JColorChooser demo
>> 3. click Background button and Swatches panel appears.
>> 4. Press Tab key => focus moves to OK button as shown in
>> this
>> image http://cr.openjdk.java.net/~zhouyx/OJDK-61/screenshot_1.png
>> <http://cr.openjdk.java.net/%7Ezhouyx/OJDK-61/screenshot_1.png>
>>
>> With this patch, in step4, focus moves to SwatchPanel, as shown
>> here http://cr.openjdk.java.net/~zhouyx/OJDK-61/screenshot_2.png
>> <http://cr.openjdk.java.net/%7Ezhouyx/OJDK-61/screenshot_2.png>
>> Then, arrow keys can be used to choose color and select color by
>> space bar.
>>
>> The webrev
>> is http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.00/
>> <http://cr.openjdk.java.net/%7Ezhouyx/OJDK-61/webrev.00/> .
>>
>> Please take a look.
>>
>>
>> --
>> Best Regards,
>> Sean Chou
>>
>>
>>
>>
>> --
>> 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/20120822/6a3b0f5f/attachment.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: JColorChooserTest.java
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120822/6a3b0f5f/JColorChooserTest.java>
More information about the swing-dev
mailing list