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
