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
