Thanks Alexander.
All items addressed except last one, please see comments inline.
New Webrev: http://cr.openjdk.java.net/~van/8033699/webrev.02/
Regards,
Vivi
On 9/11/2014 3:21 AM, Alexander Scherbatiy wrote:
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.
eventSrc is Object so still need to be casted, in 02version, the
functionality encapsulated in ButtonGroupInfo class, so now using
activeBtn, no need to cast in this case.
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?
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.
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
