<Swing Dev> [9] Review request for 8033699: Incorrect radio button behavior
Vivi An
vivi.an at oracle.com
Thu Oct 9 16:13:11 UTC 2014
Thanks Alexander. Will make the fix.
Regards,
Vivi
On 10/9/2014 2:52 AM, Alexander Zvegintsev wrote:
> Hello Vivi
>
> We can move Hashset creation to btnsInGroup's declaration, thus the
> following lines can be removed
> 460 if (btnsInGroup == null)
> 461 btnsInGroup = new HashSet<JRadioButton>();
>
> Otherwise the fix looks good to me.
> Thanks,
>
> Alexander.
> On 10/09/2014 09:53 AM, Vivi An wrote:
>> Thank you Alexander,
>>
>> New patch with fix:
>> http://cr.openjdk.java.net/~van/8033699/webrev.08/
>>
>> Regards,
>>
>> Vivi
>>
>> On 10/8/2014 11:36 AM, Alexander Zvegintsev wrote:
>>> Hello Vivi,
>>>
>>> getFocusTransferBaseComponent():
>>> Window parentWnd = SwingUtilities.getWindowAncestor((Component) activeBtn);
>>> if (parentWnd instanceof Container) {
>>> Container container = (Container)parentWnd;
>>> Window extending Container class, hence there is no need to use
>>> instanceof, null
>>> check is enough here. container variable is redundant here, you may
>>> reuse parentWnd.
>>> There are many redundant class casts in this function.
>>> Also I suggest to use ternary operator to increase readability.
>>>
>>> Something like this:
>>>
>>> Component getFocusTransferBaseComponent(boolean next) {
>>> Component focusBaseComp = activeBtn;
>>> Window container =
>>> SwingUtilities.getWindowAncestor(activeBtn);
>>> if (container != null) {
>>> FocusTraversalPolicy policy =
>>> container.getFocusTraversalPolicy();
>>> Component comp = next
>>> ? policy.getComponentAfter(container, activeBtn)
>>> : policy.getComponentBefore(container,
>>> activeBtn);
>>>
>>> // If next component in the button group, use
>>> last/first button as base focus
>>> // otherwise, use the activeButton as the base focus
>>> if (containsInGroup(comp)) {
>>> focusBaseComp = next ? lastBtn : firstBtn;
>>> }
>>> }
>>>
>>> return focusBaseComp;
>>> }
>>>
>>> Small typo in test: Seperate - > Separate (2 times)
>>>
>>> --
>>> Thanks,
>>> Alexander.
>>> On 10/08/2014 09:45 AM, Vivi An wrote:
>>>> Hello Alexander,
>>>>
>>>> Latest patched is ready for review, test case is updated too:
>>>> http://cr.openjdk.java.net/~van/8033699/webrev.07/
>>>>
>>>> Thanks
>>>>
>>>> Vivi
>>>>
>>>> On 10/2/2014 2:46 PM, Vivi An wrote:
>>>>> Thanks Alexander. Will look into it.
>>>>>
>>>>> Regards,
>>>>>
>>>>> ~ Vivi
>>>>>
>>>>> On 10/2/2014 12:20 AM, Alexander Zvegintsev wrote:
>>>>>> Sorry, I wasn't clear,
>>>>>> provided code with the "MIDDLE" button is just an example what
>>>>>> user can write (pretty odd one btw)
>>>>>> it is not in your tests.
>>>>>>
>>>>>> By lost functionality I mean that after this fix this user can't
>>>>>> navigate to the "MIDDLE" button via keyboard
>>>>>> (or to the btnEnd after ButtonGroup reordering).
>>>>>>
>>>>>> --
>>>>>> Thanks,
>>>>>> Alexander.
>>>>>> 02.10.2014 0:40, Vivi An wrote:
>>>>>>> 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/20141009/d5fe5ddd/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: image/png
Size: 32466 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20141009/d5fe5ddd/attachment.png>
More information about the swing-dev
mailing list