<Swing Dev> [9] Review request for 8033699: Incorrect radio button behavior
Vivi An
vivi.an at oracle.com
Wed Oct 1 20:40:17 UTC 2014
Thanks Alexander.
I have some questions, please see comments inline.
Regards,
~ Vivi
On 10/1/2014 5:47 AM, Alexander Zvegintsev wrote:
> Hello Vivi,
>
> the fix looks good to me in general, but we lost some functionality,
By functionality, you mean the functionality of the test, am I right?
> 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);
Just double checked the test in previous version and current version,
did not find a button "MIDDLE" like previous code snippet, or I
misunderstood, this is pseudo code?
Compare to the old test code, new changes made are:
1) Radio button group( radioBtn1, radioBtn2 and radioBtn1 ) added to a
panel ("box" in below code snippet) and then to main frame, border also
added too to separate from new added non grouped single radio button
2) A single radio button (Not Grouped) is added to cover the bug found
in last round
3) Tests 1- 6 are adjusted for preceding change to cover areas would
like to test
Below is snippet from latest version, the "box" is the panel which
contains radio button group, a screenshot added too
110 mainFrame.getContentPane().add(btnStart);
111 mainFrame.getContentPane().add(box);
112 mainFrame.getContentPane().add(radioBtnSingle);
113 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
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20141001/dd65dbd6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fjaghjec.png
Type: image/png
Size: 32466 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20141001/dd65dbd6/fjaghjec.png>
More information about the swing-dev
mailing list