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/ . 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
