On 9/11/2014 2:56 AM, Vivi An wrote:
Hello Alexander,
Thanks for nice review, new patch is available as below:
webrev: http://cr.openjdk.java.net/~van/8033699/webrev.01/
163 if ( keyListener != null ) {
164 button.removeKeyListener( keyListener );
Could you formate the code in the brackets.
355 private boolean isValidRadioButtonObj(Object obj){
356 if ((obj != null) && (obj instanceof JRadioButton) &&
It is possible to use "return (obj != null) && ...".
375 if (isValidRadioButtonObj(eventSrc) == false)
It is possible to use "if (!isValidRadioButtonObj(eventSrc))". The same
is for line 466.
379 btnGroupInfo.getButtonGroupInfo();
380 if (btnGroupInfo.srcFound){
....
You can move this code to the ButtonGroupInfo class
getNewSelectedButton(boolean next) method.
It allows to use variable names (previousBtn) instead of
btnGroupInfo.previousBtn.
391 if (newSelectedBtn != null &&
392 (newSelectedBtn != (JRadioButton) eventSrc)){
It is not necessary to cast the eventSrc to JRadioButton here.
401 private class SelectPreviousBtn extends AbstractAction
402 {
Could you move the bracket to the class declaration line?
505 if (isValidRadioButtonObj(eventSrc))
506 e.consume();
507 jumpToNextComponent((JRadioButton)eventSrc, true);
Line 507 is not under the condition. It means that eventSrc can have
different type from JRadioButton.
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?
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
