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
