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

[Alexander Scherbatiy]

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