(original) (raw)

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/ �. �

� � �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� . �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@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/�

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@linux.vnet.ibm.com>
Date: Thu, Aug 9, 2012 at 3:29 PM
Subject: Add keyboard accessibility to JColorChooser swatch
To: swing-dev@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

With this patch, in step4, focus moves to�SwatchPanel, as shown here�http://cr.openjdk.java.net/\~zhouyx/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/ �.�

Please take a look.

\--
Best Regards,
Sean Chou

\--
Best Regards,
Sean Chou

\--
Best Regards,
Sean Chou

import javax.swing.*;

import java.awt.*;



public class JColorChooserTest {

� � public static void main(String[] args) {

� � � � SwingUtilities.invokeLater(new Runnable() {

� � � � � � @Override

� � � � � � public void run() {

� � � � � � � � JFrame frame = new JFrame();



� � � � � � � � JColorChooser chooser = new JColorChooser();

� � � � � � � � chooser.setComponentOrientation(ComponentOrientation.RIGHT_TO_LEFT);

� � � � � � � � frame.add(chooser);



� � � � � � � � frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

� � � � � � � � frame.setSize(600, 400);

� � � � � � � � frame.setVisible(true);

� � � � � � }

� � � � });

� � }

}