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

[Vivi An]

Thanks Alexander.

All items addressed except last one, please see comments inline.
New Webrev: http://cr.openjdk.java.net/~van/8033699/webrev.02/

Regards,

Vivi

On 9/11/2014 3:21 AM, Alexander Scherbatiy wrote:
> 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.
eventSrc is Object so still need to be casted, in 02version, the 
functionality encapsulated in ButtonGroupInfo class, so now using 
activeBtn, no need to cast in this case.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20140912/d688da7e/attachment.html>