<Swing Dev> [9] Review request for 8033699: Incorrect radio button behavior
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Wed Sep 17 07:44:06 UTC 2014
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())
}
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));
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