<Swing Dev> [9] Review request for 8033699: Incorrect radio button behavior
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Thu Sep 18 07:55:23 UTC 2014
The fix looks good to me.
Thanks,
Alexandr.
On 9/18/2014 3:09 AM, Vivi An wrote:
> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the swing-dev
mailing list