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

[Alexander Scherbatiy]

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
>