<Swing Dev> [9] Review request for 8033699: Incorrect radio button behavior
Vivi An
vivi.an at oracle.com
Tue Sep 30 21:15:19 UTC 2014
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