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

[Vivi An]

Hello Alexander,

Thanks for nice review, new patch is available as below:
webrev: http://cr.openjdk.java.net/~van/8033699/webrev.01/

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