<Swing Dev> [9] Review request for 8033699: Incorrect radio button behavior
Alexander Zvegintsev
alexander.zvegintsev at oracle.com
Wed Oct 1 12:47:08 UTC 2014
Hello Vivi,
the fix looks good to me in general, but we lost some functionality,
here is some modified code from the previous version of the test:
ButtonGroup btnGrp = new ButtonGroup();
btnGrp.add(radioBtn1);
btnGrp.add(radioBtn2);
btnGrp.add(radioBtn3);
radioBtn1.setSelected(true);
mainFrame.getContentPane().add(btnStart);
mainFrame.getContentPane().add(radioBtn1);
mainFrame.getContentPane().add(radioBtn2);
mainFrame.getContentPane().add(new JButton("MIDDLE"));
mainFrame.getContentPane().add(radioBtn3);
mainFrame.getContentPane().add(btnEnd);
After this fix we are not able to select "MIDDLE" button with a keyboard
anymore.
If we change ButtonGroup order to
btnGrp.add(radioBtn3);
btnGrp.add(radioBtn2);
We get into a loop and we are unable to select btnEnd and all following
components(Shift+Tab can help in that case).
I think that these cases deserves some attention.
--
Thanks,
Alexander.
On 10/01/2014 01:15 AM, Vivi An wrote:
> Hello Alexander,
>
> Thanks for the review.
>
> New patch is available under
> http://cr.openjdk.java.net/~van/8033699/webrev.06/
>
> Regards,
>
> Vivi
>
> On 9/19/2014 3:22 AM, Alexander Zvegintsev wrote:
>> Hello Vivi,
>>
>> Since you are touching this code, could you please add missing
>> @Override annotations?
>>
> Added
>> void jumpToNextComponent(boolean next) {
>> if (!getButtonGroupInfo())
>> return;
>>
>> This leads to an issue: we can't switch to a next component with a
>> TAB key if JRadioButton
>> is not in a ButtonGroup. (It may be a reason to write another test.)
> Fixed
>>
>> private boolean isValidRadioButtonObj(Object obj) {
>> return ((obj != null) && (obj instanceof JRadioButton) &&
>> ((JRadioButton) obj).isVisible() &&
>> ((JRadioButton) obj).isEnabled());
>> }
> Fixed
>>
>> There is no need to do a null check before instanceof [1]
>>
>> * @param evt, the event object.
>> * @param next, indicate if it's next one
>> */
>> private void selectRadioButton(ActionEvent event, boolean next) {
>>
>> typo: evt should be event
>>
> Fixed
>>
>> I run the test and it fails at least on Linux. It happens due to a
>> big auto delay:
>> Robot holds key for too long so system sends TAB key multiple time.
>> So I think auto delay should be about 100 ms.
>>
>> robot.setAutoDelay(300);
>>
>> It is enough to call it once after Robot creation, this will
>> automatically add a 300 ms delay
>> between Robot generated events (such as keyPress, mouseMove, etc).
>> If you really want a delay between test runs please use Thread.sleep().
>>
>> I see many calls to hitKey() and then to realSync(), so we can move
>> realSync() call to the end of hitKey().
>> checkResult() may be combined with runTest(), hence we call one
>> function instead of two.
>>
>> int sum = 0;
>> for (int i = 0; i < testResults.length; i++) {
>> if (testResults[i] == false)
>> sum++;
>> }
>>
>> if (sum != 0){
>> System.out.println("Total 5 tests, failed test(s): " + sum);
>> throw new RuntimeException("Test failed.");
>> }
>>
>>
>> I don't think that this logic is applicable here, each test depends
>> on previous: Test2 failure means that
>> focus is on a wrong component, so all following tests will fail.
>> BTW, this code is unreachable in case of test failure, since
>> checkResult() already throws an exception.
>>
>> http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.20.2
> Test is updated, non-grouped radio button test is added, test passed
> on Oracle Linux 5 u6 32 bits and Windows 7.
>>
>> --
>> Thanks,
>> Alexander.
>>
>> On 09/18/2014 09:23 PM, Vivi An wrote:
>>> 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