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