<Swing Dev> Fwd: Add keyboard accessibility to JColorChooser swatch
Pavel Porvatov
pavel.porvatov at oracle.com
Mon Sep 10 16:57:09 UTC 2012
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/
> <http://cr.openjdk.java.net/%7Ezhouyx/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
> <pavel.porvatov at oracle.com <mailto:pavel.porvatov at oracle.com>> 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
>> <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
>>
>
>
>
>
> --
> Best Regards,
> Sean Chou
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120910/54a8c7c2/attachment.html>
More information about the swing-dev
mailing list