Re: [9] Review request for 8033699: Incorrect radio button behavior

[Alexander Scherbatiy]

  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