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

[Vivi An]

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