<Swing Dev> [9] Review request for 8033699: Incorrect radio button behavior

Alexander Zvegintsev alexander.zvegintsev at oracle.com
Thu Oct 2 07:20:01 UTC 2014


Sorry, I wasn't clear,
provided code with the "MIDDLE" button is just an example what user can 
write (pretty odd one btw)
it is not in your tests.

By lost functionality I mean that after this fix this user can't 
navigate to the "MIDDLE" button via keyboard
(or to the btnEnd after ButtonGroup reordering).

--
Thanks,
Alexander.

02.10.2014 0:40, Vivi An wrote:
> Thanks Alexander.
>
> I have some questions,  please see comments inline.
>
> Regards,
>
> ~ Vivi
>
> On 10/1/2014 5:47 AM, Alexander Zvegintsev wrote:
>> Hello Vivi,
>>
>> the fix looks good to me in general, but we lost some functionality,
> By functionality,  you mean the functionality of the test, am I right?
>
>> here is some modified code from the previous version of the test:
>>
>>         ButtonGroup btnGrp = new ButtonGroup();
>>         btnGrp.add(radioBtn1);
>>         btnGrp.add(radioBtn2);
>>         btnGrp.add(radioBtn3);
>>         radioBtn1.setSelected(true);
>>
>>         mainFrame.getContentPane().add(btnStart);
>>         mainFrame.getContentPane().add(radioBtn1);
>>         mainFrame.getContentPane().add(radioBtn2);
>>         mainFrame.getContentPane().add(new JButton("MIDDLE"));
>>         mainFrame.getContentPane().add(radioBtn3);
>>         mainFrame.getContentPane().add(btnEnd);
> Just double checked the test in previous version and current version, 
> did not find a button "MIDDLE" like previous code snippet,  or I 
> misunderstood, this is pseudo code?
> Compare to the old test code, new changes made are:
> 1) Radio button group( radioBtn1, radioBtn2 and radioBtn1 ) added to a 
> panel ("box" in below code snippet) and then to main frame, border 
> also added too to separate from new added non grouped single radio button
> 2) A single radio button (Not Grouped) is added to cover the bug found 
> in last round
> 3) Tests 1- 6 are adjusted for preceding change to cover areas would 
> like to test
>
> Below is snippet from latest version, the "box" is the panel which 
> contains radio button group, a screenshot added too
>   110         mainFrame.getContentPane().add(btnStart);
>   111         mainFrame.getContentPane().add(box);
>   112         mainFrame.getContentPane().add(radioBtnSingle);
>   113         mainFrame.getContentPane().add(btnEnd);
>
>
>
>>
>> After this fix we are not able to select "MIDDLE" button with a 
>> keyboard anymore.
>> If we change ButtonGroup order to
>>
>>         btnGrp.add(radioBtn3);
>>         btnGrp.add(radioBtn2);
>>
>> We get into a loop and we are unable to select btnEnd and all 
>> following components(Shift+Tab can help in that case).
>> I think that these cases deserves some attention.
>>
>> -- 
>> Thanks,
>> Alexander.
>>
>> On 10/01/2014 01:15 AM, Vivi An wrote:
>>> Hello Alexander,
>>>
>>> Thanks for the review.
>>>
>>> New patch is available under 
>>> http://cr.openjdk.java.net/~van/8033699/webrev.06/
>>>
>>> Regards,
>>>
>>> Vivi
>>>
>>> On 9/19/2014 3:22 AM, Alexander Zvegintsev wrote:
>>>> Hello Vivi,
>>>>
>>>> Since you are touching this code, could you please add missing 
>>>> @Override annotations?
>>>>
>>> Added
>>>> void jumpToNextComponent(boolean next) {
>>>>             if (!getButtonGroupInfo())
>>>>                 return;
>>>>
>>>> This leads to an issue: we can't switch to a next component with a 
>>>> TAB key if JRadioButton
>>>> is not in a ButtonGroup. (It may be a reason to write another test.)
>>> Fixed
>>>>
>>>>     private boolean isValidRadioButtonObj(Object obj) {
>>>>         return ((obj != null) && (obj instanceof JRadioButton) &&
>>>>                     ((JRadioButton) obj).isVisible() &&
>>>>                     ((JRadioButton) obj).isEnabled());
>>>>     }
>>> Fixed
>>>>
>>>> There is no need to do a null check before instanceof [1]
>>>>
>>>>      * @param evt, the event object.
>>>>      * @param next, indicate if it's next one
>>>>      */
>>>>     private void selectRadioButton(ActionEvent event, boolean next) {
>>>>
>>>> typo: evt should be event
>>>>
>>> Fixed
>>>>
>>>> I run the test and it fails at least on Linux. It happens due to a 
>>>> big auto delay:
>>>> Robot holds key for too long so system sends TAB key multiple time.
>>>> So I think auto delay should be about 100 ms.
>>>>
>>>> robot.setAutoDelay(300);
>>>>
>>>> It is enough to call it once after Robot creation, this will 
>>>> automatically add a 300 ms delay
>>>> between Robot generated events (such as keyPress, mouseMove, etc).
>>>> If you really want a delay between test runs please use 
>>>> Thread.sleep().
>>>>
>>>> I see many calls to hitKey() and then to realSync(), so we can move 
>>>> realSync() call to the end of hitKey().
>>>> checkResult() may be combined with runTest(), hence we call one 
>>>> function instead of two.
>>>>
>>>>         int sum = 0;
>>>>         for (int i = 0; i < testResults.length; i++) {
>>>>             if (testResults[i] == false)
>>>>                 sum++;
>>>>         }
>>>>
>>>>         if (sum != 0){
>>>>             System.out.println("Total 5 tests, failed test(s): " + 
>>>> sum);
>>>>             throw new RuntimeException("Test failed.");
>>>>         }
>>>>
>>>>
>>>> I don't think that this logic is applicable here, each test depends 
>>>> on previous: Test2 failure means that
>>>> focus is on a wrong component, so all following tests will fail.
>>>> BTW, this code is unreachable in case of test failure, since 
>>>> checkResult() already throws an exception.
>>>>
>>>> http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.20.2 
>>>>
>>> Test is updated, non-grouped radio button test is added,  test 
>>> passed on Oracle Linux 5 u6 32 bits and Windows 7.
>>>>
>>>> -- 
>>>> Thanks,
>>>> Alexander.
>>>>
>>>> On 09/18/2014 09:23 PM, Vivi An wrote:
>>>>> Thank you Alexandr.
>>>>>
>>>>> Need one more review for this. Alexander, could you please take a 
>>>>> look at below fix?
>>>>> http://cr.openjdk.java.net/~van/8033699/webrev.05/
>>>>>
>>>>> Thanks
>>>>>
>>>>> Vivi
>>>>>
>>>>> On 9/18/2014 12:55 AM, Alexander Scherbatiy wrote:
>>>>>>
>>>>>>   The fix looks good to me.
>>>>>>
>>>>>>   Thanks,
>>>>>>   Alexandr.
>>>>>>
>>>>>> On 9/18/2014 3:09 AM, Vivi An wrote:
>>>>>>> Hello Alex,
>>>>>>>
>>>>>>> On 9/17/2014 12:44 AM, Alexander Scherbatiy wrote:
>>>>>>>> On 9/17/2014 9:17 AM, Vivi An wrote:
>>>>>>>>> You are right. Fixed now.
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~van/8033699/webrev.04/
>>>>>>>>
>>>>>>>>  528             if (e.isShiftDown()) {
>>>>>>>>  529                 if (e.getKeyCode() == KeyEvent.VK_TAB) {
>>>>>>>>  537                         // ... 
>>>>>>>> btnGroupInfo.jumpToNextComponent(false);
>>>>>>>>  539                 }
>>>>>>>>  540             } else if (e.getKeyCode() == KeyEvent.VK_TAB) {
>>>>>>>>  548                     // ... 
>>>>>>>> btnGroupInfo.jumpToNextComponent(true);
>>>>>>>>  550             }
>>>>>>>>
>>>>>>>>   The code in the if/else branches is almost the same except 
>>>>>>>> the true/false argument.
>>>>>>>>   I would suggest to unify them in the following way:
>>>>>>>>
>>>>>>>>      if (e.getKeyCode() == KeyEvent.VK_TAB) {
>>>>>>>>         // ... jumpToNextComponent(!e.isShiftDown())
>>>>>>>>      }
>>>>>>>>
>>>>>>> Fixed.
>>>>>>>>
>>>>>>>>  510             if (next)
>>>>>>>>  511                 KeyboardFocusManager.
>>>>>>>>  512 
>>>>>>>> getCurrentKeyboardFocusManager().focusNextComponent((JComponent)lastBtn);
>>>>>>>>  513             else
>>>>>>>>  514                 KeyboardFocusManager.
>>>>>>>>  515 
>>>>>>>> getCurrentKeyboardFocusManager().focusPreviousComponent((JComponent)firstBtn);
>>>>>>>>
>>>>>>>>  This code can be also a little bit optimized:
>>>>>>>>
>>>>>>>> KeyboardFocusManager.getCurrentKeyboardFocusManager().
>>>>>>>>         focusPreviousComponent((JComponent) (next ? lastBtn : 
>>>>>>>> firstBtn));
>>>>>>>>
>>>>>>> It's different function call,  one for focusNextComponent and 
>>>>>>> the other for focusPreviousComponent, not able to optimize in 
>>>>>>> suggested way.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~van/8033699/webrev.05/
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Vivi
>>>>>>>
>>>>>>>>  Thanks,
>>>>>>>>  Alexandr.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thank you
>>>>>>>>>
>>>>>>>>> ~ Vivi
>>>>>>>>>
>>>>>>>>> On 9/16/2014 12:39 AM, Alexander Scherbatiy wrote:
>>>>>>>>>>
>>>>>>>>>>   I have missed one more case:
>>>>>>>>>>  527         public void keyPressed(KeyEvent e) {
>>>>>>>>>>  528             if (e.getKeyCode() == KeyEvent.VK_TAB) {
>>>>>>>>>>                         // jumpToNextComponent(true)
>>>>>>>>>>  538             }
>>>>>>>>>>  539
>>>>>>>>>>  540             if (e.getKeyCode() == KeyEvent.VK_TAB && 
>>>>>>>>>> e.isShiftDown()) {
>>>>>>>>>> //jumpToNextComponent(false)
>>>>>>>>>>  550             }
>>>>>>>>>>  551         }
>>>>>>>>>>
>>>>>>>>>>   It seems that if e.isShiftDown() is true then both 
>>>>>>>>>> jumpToNextComponent(true) and jumpToNextComponent(false) 
>>>>>>>>>> methods can be called.
>>>>>>>>>>
>>>>>>>>>>   Thanks,
>>>>>>>>>>   Alexandr.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 9/16/2014 7:38 AM, Vivi An wrote:
>>>>>>>>>>> Fixed. New Webrev: 
>>>>>>>>>>> http://cr.openjdk.java.net/~van/8033699/webrev.03/
>>>>>>>>>>>
>>>>>>>>>>> Thanks Alex.
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>>
>>>>>>>>>>> ~ Vivi
>>>>>>>>>>>
>>>>>>>>>>> On 9/15/2014 2:59 AM, Alexander Scherbatiy wrote:
>>>>>>>>>>>> On 9/12/2014 8:56 PM, Vivi An wrote:
>>>>>>>>>>>>> Thanks Alexander.
>>>>>>>>>>>>>
>>>>>>>>>>>>> All items addressed except last one, please see comments 
>>>>>>>>>>>>> inline.
>>>>>>>>>>>>> New Webrev: 
>>>>>>>>>>>>> http://cr.openjdk.java.net/~van/8033699/webrev.02/
>>>>>>>>>>>>
>>>>>>>>>>>>     163         if ( keyListener != null ) {
>>>>>>>>>>>>
>>>>>>>>>>>>     Could you remove spaces in the brackets? After code 
>>>>>>>>>>>> formatting it should be "if (keyListener != null) {"
>>>>>>>>>>>>
>>>>>>>>>>>>>> 543             if (next && btnGroupInfo.lastBtn != null)
>>>>>>>>>>>>>>  544 KeyboardFocusManager.
>>>>>>>>>>>>>>  545 
>>>>>>>>>>>>>> getCurrentKeyboardFocusManager().focusNextComponent((JComponent)btnGroupInfo.lastBtn);
>>>>>>>>>>>>>>  546             else if (btnGroupInfo.firstBtn != null)
>>>>>>>>>>>>>>  547 KeyboardFocusManager.
>>>>>>>>>>>>>>  548 
>>>>>>>>>>>>>> getCurrentKeyboardFocusManager().focusPreviousComponent((JComponent)btnGroupInfo.firstBtn);
>>>>>>>>>>>>>>  549         }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Should the first button be focused If next is true and 
>>>>>>>>>>>>>> last button is null?
>>>>>>>>>>>>> Don't think last button could be null when this function 
>>>>>>>>>>>>> is triggered, last and first will always be set in case 
>>>>>>>>>>>>> there is at least one valid (enabled and visible) radio 
>>>>>>>>>>>>> button in the group.
>>>>>>>>>>>>
>>>>>>>>>>>>   It seems that lastBtn != null and firstBtn != null checks 
>>>>>>>>>>>> are not necessary in this case.
>>>>>>>>>>>>
>>>>>>>>>>>>   Thanks,
>>>>>>>>>>>>   Alexandr.
>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ~ Vivi
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 9/8/2014 5:43 AM, Alexander Scherbatiy wrote:
>>>>>>>>>>>>>>>> On 9/4/2014 10:56 PM, Vivi An wrote:
>>>>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Please review fix for JDK-8033699. Changes made:
>>>>>>>>>>>>>>>>> 1) When tab or shift-tab key pressed, focus moved to 
>>>>>>>>>>>>>>>>> next/previous
>>>>>>>>>>>>>>>>> component outside of the radio button group
>>>>>>>>>>>>>>>>> 2) Using up/down or left/right arrow key to change 
>>>>>>>>>>>>>>>>> selection inside
>>>>>>>>>>>>>>>>> radio button group
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Bug:https://bugs.openjdk.java.net/browse/JDK-8033699
>>>>>>>>>>>>>>>>> Webrev:http://cr.openjdk.java.net/~van/8033699/webrev.00/
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>   56     private KeyListener keyListener = null;
>>>>>>>>>>>>>>>>   57     private KeyHandler handler = null;
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> It seems that these both fields point to the same 
>>>>>>>>>>>>>>>> object. Is it possible to get rid of one of them?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>  152         if ( keyListener != null ) {
>>>>>>>>>>>>>>>>  153 button.removeKeyListener( keyListener );
>>>>>>>>>>>>>>>>  154         }
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Some UIs also set the listener to null in the 
>>>>>>>>>>>>>>>> uninstallUI()/uninstallListeners() methods (like 
>>>>>>>>>>>>>>>> BasicComboBoxUI or BasicColorChooserUI).
>>>>>>>>>>>>>>>> Should the keyListener also be set to null to free the 
>>>>>>>>>>>>>>>> KeyHandler object?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>  369         if (!(eventSrc instanceof JRadioButton &&
>>>>>>>>>>>>>>>>  370             ((JRadioButton) eventSrc).isVisible() 
>>>>>>>>>>>>>>>> && ((JRadioButton) eventSrc).isEnabled()))
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This check is used several times. It can be moved to a 
>>>>>>>>>>>>>>>> separate method.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>  373         // Get the button model from the source.
>>>>>>>>>>>>>>>>  374         ButtonModel model = ((AbstractButton) 
>>>>>>>>>>>>>>>> eventSrc).getModel();
>>>>>>>>>>>>>>>>  375         if (!(model instanceof DefaultButtonModel))
>>>>>>>>>>>>>>>>  376             return;
>>>>>>>>>>>>>>>>  377
>>>>>>>>>>>>>>>>  378         // If the button model is 
>>>>>>>>>>>>>>>> DefaultButtonModel, and use it, otherwise return.
>>>>>>>>>>>>>>>>  379         DefaultButtonModel bm = 
>>>>>>>>>>>>>>>> (DefaultButtonModel) model;
>>>>>>>>>>>>>>>>  380         if (bm == null)
>>>>>>>>>>>>>>>>  381             return;
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The second check is not necessary because (model 
>>>>>>>>>>>>>>>> instanceof DefaultButtonModel) returns false for null 
>>>>>>>>>>>>>>>> model.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>  404         AbstractButton curElement = null;
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>   The curElement variable declaration could be moved 
>>>>>>>>>>>>>>>> into the 'while' block.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>  408             if (curElement instanceof JRadioButton &&
>>>>>>>>>>>>>>>>  409 ((JRadioButton) curElement).isVisible() &&
>>>>>>>>>>>>>>>>  410 ((JRadioButton) curElement).isEnabled()){
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>   It is possible to use 'continue' here to not put the 
>>>>>>>>>>>>>>>> other code inside the 'if' block.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>  418                 else if (!srcFound){
>>>>>>>>>>>>>>>>  422                 } else if (srcFound && nextBtn == 
>>>>>>>>>>>>>>>> null){
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> It is not necessary to check the srcFound in the second 
>>>>>>>>>>>>>>>> 'if' because it should already have true value.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>  444             if (newSelectedBtn != null){
>>>>>>>>>>>>>>>>  445 newSelectedBtn.requestFocusInWindow();
>>>>>>>>>>>>>>>>  446 newSelectedBtn.setSelected(true);
>>>>>>>>>>>>>>>>  447             }
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Is it possible here that newSelectedBtn == eventSrc?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>  522         private void 
>>>>>>>>>>>>>>>> jumpToNextComponent(JRadioButton btn, boolean next){
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The code that retrieves elements from a button group is 
>>>>>>>>>>>>>>>> also used in the selectRadioButton()
>>>>>>>>>>>>>>>> and can be moved to a separate method.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> JPRT build succeeded, JCK tests for JRadiobutton and 
>>>>>>>>>>>>>>>>> JCheckBox all passed.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thank you
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Vivi
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20141002/9b92cc93/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: image/png
Size: 32466 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20141002/9b92cc93/attachment.png>


More information about the swing-dev mailing list