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

[Vivi An]

You are right. Fixed now.

http://cr.openjdk.java.net/~van/8033699/webrev.04/

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