Hi Pavel,
I
uploaded http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.05/
<http://cr.openjdk.java.net/%7Ezhouyx/OJDK-61/webrev.05/> . And
reported http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7194184 .
Some details about update:
1. A testcase is added in this webrev, but it checks keyboard
accessibility only. I tried to check right-to-left
orientation with the testcase attached, but it doesn't work well
on windows.
2. " g.setXORMode(Color.WHITE); g.setColor(Color.BLACK); " is
not used because the mark is not obvious for light blue-pink area
and dark green area, as this
image: http://cr.openjdk.java.net/~zhouyx/OJDK-61/screenshot_4.png
<http://cr.openjdk.java.net/%7Ezhouyx/OJDK-61/screenshot_4.png> .
Please take a look.
On Wed, Aug 22, 2012 at 9:16 PM, Pavel Porvatov
<[email protected] <mailto:[email protected]>> wrote:
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
<[email protected]
<mailto:[email protected]>> 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* <[email protected]
<mailto:[email protected]>>
Date: Thu, Aug 9, 2012 at 3:29 PM
Subject: <Swing Dev> Add keyboard accessibility to
JColorChooser swatch
To: [email protected]
<mailto:[email protected]>
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
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);
}
});
}
}
--
Best Regards,
Sean Chou
