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

[Alexander Scherbatiy]

On 9/11/2014 2:56 AM, Vivi An wrote:
> Hello Alexander,
>
> Thanks for nice review, new patch is available as below:
> webrev: http://cr.openjdk.java.net/~van/8033699/webrev.01/

  163         if ( keyListener != null ) {
  164             button.removeKeyListener( keyListener );

Could you formate the code in the brackets.

  355     private boolean isValidRadioButtonObj(Object obj){
  356         if ((obj != null) && (obj instanceof JRadioButton) &&

It is possible to use "return (obj != null) && ...".


  375         if (isValidRadioButtonObj(eventSrc) == false)

It is possible to use "if (!isValidRadioButtonObj(eventSrc))". The same 
is for line 466.


  379         btnGroupInfo.getButtonGroupInfo();
  380         if (btnGroupInfo.srcFound){
  ....

You can move this code to the ButtonGroupInfo class 
getNewSelectedButton(boolean next) method.
It allows to use variable names (previousBtn) instead of 
btnGroupInfo.previousBtn.

  391             if (newSelectedBtn != null &&
  392                 (newSelectedBtn != (JRadioButton) eventSrc)){

It is not necessary to cast the eventSrc to JRadioButton here.

  401     private class SelectPreviousBtn extends AbstractAction
  402     {

Could you move the bracket to the class declaration line?

  505                 if (isValidRadioButtonObj(eventSrc))
  506                     e.consume();
  507                 jumpToNextComponent((JRadioButton)eventSrc, true);

Line 507 is not under the condition. It means that eventSrc can have 
different type from JRadioButton.


  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?

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