Hi Alexander, Thank you !
On Tue, Sep 18, 2012 at 10:34 PM, Alexander Scherbatiy <[email protected]> wrote: > 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 > > -- Best Regards, Sean Chou
