<Swing Dev> Fwd: Add keyboard accessibility to JColorChooser swatch
Sean Chou
zhouyx at linux.vnet.ibm.com
Mon Sep 10 09:42:08 UTC 2012
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
<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>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 <
>> 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/ .
>>>
>>> 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 <
>>> 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/
>>>>
>>>> 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>
>>>> 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
>>>>
>>>>
>>>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120910/61612666/attachment.html>
More information about the swing-dev
mailing list