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
