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
Test7194184.java
Description: Binary data
