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

[Alexander Scherbatiy]

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