<Swing Dev> Fwd: Add keyboard accessibility to JColorChooser swatch
Pavel Porvatov
pavel.porvatov at oracle.com
Fri Sep 7 14:15:31 UTC 2012
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
> <pavel.porvatov at oracle.com <mailto:pavel.porvatov at oracle.com>> 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/
>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>> <http://cr.openjdk.java.net/%7Ezhouyx/OJDK-61/screenshot_4.png> .
>>
>> Please take a look.
>>
>> On Wed, Aug 22, 2012 at 9:16 PM, Pavel Porvatov
>> <pavel.porvatov at oracle.com <mailto:pavel.porvatov at oracle.com>> 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/
>>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>>> <pavel.porvatov at oracle.com
>>> <mailto:pavel.porvatov at oracle.com>> 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/
>>>> <http://cr.openjdk.java.net/%7Ezhouyx/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* <zhouyx at linux.vnet.ibm.com
>>>> <mailto:zhouyx at linux.vnet.ibm.com>>
>>>> Date: Thu, Aug 9, 2012 at 3:29 PM
>>>> Subject: <Swing Dev> Add keyboard accessibility to
>>>> JColorChooser swatch
>>>> To: swing-dev at openjdk.java.net
>>>> <mailto:swing-dev at openjdk.java.net>
>>>>
>>>>
>>>> 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
>>>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>>>> <http://cr.openjdk.java.net/%7Ezhouyx/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/
>>>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120907/74b1c608/attachment.html>
More information about the swing-dev
mailing list