<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