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

Vivi An vivi.an at oracle.com
Wed Sep 17 23:09:09 UTC 2014


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

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


More information about the swing-dev mailing list