<Swing Dev> [9] Review request for 8033699: Incorrect radio button behavior
Vivi An
vivi.an at oracle.com
Thu Sep 18 17:23:02 UTC 2014
Thank you Alexandr.
Need one more review for this. Alexander, could you please take a look
at below fix?
http://cr.openjdk.java.net/~van/8033699/webrev.05/
Thanks
Vivi
On 9/18/2014 12:55 AM, Alexander Scherbatiy wrote:
>
> 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