Hi Sean,
The fix and test look good for me.
I think that there are areas for improvements like initialization the
selRow and selCol variables to the color coordinates defined in the
constructor (new JColorChooser(Color.GREEN)). However it is not
necessary for this fix.
Thanks,
Alexandr.
On 9/17/2012 10:28 AM, Sean Chou wrote:
Hi Pavel,
The new webrev is at: http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.09/ .
The missed sync is added.
On Sat, Sep 15, 2012 at 12:41 AM, Pavel Porvatov
<[email protected]> wrote:
Hi Sean,
Hi Pavel,
Thank you very much for testing on MacOS, I have windows and linux only for now.
The new patch is at: http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.08/ .
Your version of the test is wrong. Why did you remove realSync before hitting
keys? It's possible that invokeLater with
recentSwatchPanel.requestFocusInWindow will be executed AFTER all keys are
pressed and the test will fail... Moreover requestFocusInWindow invocation
doesn't guarantee that recentSwatchPanel has focus because of javadoc:
* Developers must never
* assume that this Component is the focus owner until this
* Component receives a FOCUS_GAINED event.
Regards, Pavel.
Please have a look.
On Wed, Sep 12, 2012 at 4:30 PM, Pavel Porvatov<[email protected]>
wrote:
Hi Sean,
Unfortunately the test fails on MacOS. That's because JTabbedPane doesn't have
focus after window is visible under AquaLAF. You can modify your test like this:
SwingUtilities.invokeLater(new Runnable() {
@Override
public void run() {
selectedColor = colorChooser.getColor();
Component recentSwatchPanel = Util.findSubComponent(colorChooser,
"RecentSwatchPanel");
if (recentSwatchPanel == null) {
throw new RuntimeException("RecentSwatchPanel not found");
}
recentSwatchPanel.requestFocusInWindow();
}
});
toolkit.realSync();
// Tab to move the focus to MainSwatch
Util.hitKeys(robot, KeyEvent.VK_SHIFT, KeyEvent.VK_TAB);
// Select the color on right
Util.hitKeys(robot, KeyEvent.VK_RIGHT);
Util.hitKeys(robot, KeyEvent.VK_RIGHT);
Util.hitKeys(robot, KeyEvent.VK_SPACE);
I checked, it works on Mac.
After the changes
1. There is no need to declare selectedColor as a volatile field
2. Robot and Toolkit are used in one method and can be converted into local
variables
3. The click method can be removed
Regards, Pavel
Hi Pavel,
Thanks for the tip, it is modified.
The new webrev is at: http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.07/ .
Please take a look.
On Tue, Sep 11, 2012 at 12:57 AM, Pavel Porvatov<[email protected]>
wrote:
Hi Sean,
The fix looks good but you are still using Swing components from different
threads. There is a useful method: regtesthelpres.Util.invokeOnEDT
Hi Pavel,
I made a new patch, it is at
http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.06/ .
Please take a look.
About orientation, I also set mainSwatch's top-right corner as (0, 0), so the
top-right color is white in right-to-left orientation.
That ok.
Regards, Pavel
On Fri, Sep 7, 2012 at 10:15 PM, Pavel Porvatov<[email protected]>
wrote:
HI Sean,
Hi Pavel,
Thanks for the comments. I have a question about right-to-left orientation.
As the effect of "this instanceof RecentSwatchPanel", with first version of my patch is
applied, if left key is pressed when choosing color on recentSwatch, it move the focus to the color
on right side; but when choose color on mainSwatch, left is still left. And when the focus is on
the tab, left key also move the focus to the tab on left side, as well as controls on other tabs. I
only found this left-right reverse on RecentSwatchPanel, I removed the code related. Can you give
some information about what is the right behavior? Is it "default on top-right, grows from
right-to-left, but left key still moves left" ?
In right-to-left orientation all components are placed from right to left (as
you can observe on JColorChooser for example), but the keys LEFT and RIGHT work
as always.
Regards, Pavel
On Wed, Sep 5, 2012 at 9:32 PM, Pavel Porvatov<[email protected]>
wrote:
Hi Sean,
Still have some comments
1. Could you please replace requestFocus by requestFocusInWindow? (read javadoc
for the details)
2. What's the reason to change
- setSelectedColor(color);
+ getColorSelectionModel().setSelectedColor(color);
?
You removed null checking...
In new code (e.g. in MainSwatchKeyListener) you could use
setSelectedColor(color) as well.
3. This code looks strange:
public boolean isFocusTraversable() {
- return false;
+ return true;
}
I believe we can remove this "hack" at all and just use setFocusable(true) in
constructor...
4. You are still using incorrect code formatting, e.g. in
"getColorForCell(column,row);"
5. You removed right-to-left orientation from the RecentSwatchPanel. Could you
please explain that changes?
6. About right-to-left orientation: I believe the default corner of the marker
should be upper-right, but not upper-left. HOME and END keys should work
according to orientation as well...
The test should be fixed as well:
a. What does line 75 mean?
75 if (true) return ;
b. Why Swing fields are volatile? Swing components are not thread safe and
should be accessed only from the EDT
c. What's the reason to have click(int...keys) instead of click(int key)?
d. Constants should be in UPPER_CASE
Regards, Pavel
Hi Pavel,
I uploaded http://cr.openjdk.java.net/~zhouyx/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 .
Please take a look.
On Wed, Aug 22, 2012 at 9:16 PM, Pavel Porvatov<[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/ .
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<[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/
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]>
Date: Thu, Aug 9, 2012 at 3:29 PM
Subject:<Swing Dev> Add keyboard accessibility to JColorChooser swatch
To: [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
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);
}
});
}
}
--
Best Regards,
Sean Chou
--
Best Regards,
Sean Chou
--
Best Regards,
Sean Chou
--
Best Regards,
Sean Chou
--
Best Regards,
Sean Chou
--
Best Regards,
Sean Chou
