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
