<Swing Dev> [8] Review request for 4199622 RFE: JComboBox shouldn't sending ActionEvents for keyboard navigation

[Vladislav Karnaukhov]

Hello Alex, all,

though I definitely agree with your approach on default JComboBox 
behavior, I don't believe your test really shows a problem.

The method BasicComboBoxUI.getNextIndex is private and is used only to 
determine a delta we should add to or subtract from current list 
position to determine an index of an item we're going to set selected in 
a combo box. Moreover, current implementation always sets list box index 
equal to combo box index when list' popup method is called, see
BasicComboPopup.show() method. This method is public and theoretically 
could be overridden, but in this case users should now what they're 
doing, right?

When an user navigates with a keyboard, combo box item is always set 
equal to list item (by JComboBox design). With mouse navigation it's not 
true, which is shown by your test.

My property, however, somewhat emulates mouse navigation with a 
keyboard. With this property ON list and combo box indexes may not be 
equal, indeed; but this should not, to my understanding, affect user 
experience. An user may still operate these indexes either separately or 
in conjunction.

Please advice. I'm definitely open to this discussion and, of course, am 
trying to implement this feature as safe and reliable as possible.

Regards,
- Vlad

On 2/7/2013 10:30 PM, Alexander Potochkin wrote:
> Hello Alexander
>>
>>  - key == PAGE_UP / (key == PAGE_DOWN)
>>   Are ui.listBox.getSelectedIndex() and comboBox.getSelectedIndex()
>> the same when the "ComboBox.noActionOnKeyNavigation"
>>   property is unset? If so, only the ui.listBox.getSelectedIndex() can
>> be used for both cases.
>>
>>  - key == HIDE
>>    I would suggest to set the ui.listBox.selected index back to the
>> comboBox.selected index to prevent some issues where these two indices
>> are expected to be the same.
>
> I would rather not change this code.
> We have experienced a lot of unexpected issues when we made even more
> trivial changes.
> The number of scenarios with custom editors, popups and so on is enormous
> and sometimes the users can expect those values to be different.
>
> Run the test given below.
> Open the combobox and highlight an item in the middle of the popup.
> Press backspace.
> Notice that those values are not the same.
>
> I strongly believe that old code should behave *exactly* the same when the
> "ComboBox.noActionOnKeyNavigation" is off.
>
> Would you agree?
>
> (Yep, this means that I don't approve this fix at this moment.)
>
> Thanks
> alexp
>
> import javax.accessibility.AccessibleContext;
> import javax.swing.*;
> import javax.swing.plaf.basic.ComboPopup;
> import javax.swing.plaf.metal.MetalLookAndFeel;
> import java.awt.*;
> import java.awt.event.KeyAdapter;
> import java.awt.event.KeyEvent;
>
> public class ComboTest extends JFrame {
>
>      public ComboTest() {
>          setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
>          setLayout(new FlowLayout());
>
>          final JComboBox<String> cb = new JComboBox<>(
>                  new String[]{"one", "two", "three","one2", "two2",
> "three2","one3", "two3", "three3"});
>          cb.setEditable(true);
>
>          cb.getEditor().getEditorComponent().addKeyListener(new
> KeyAdapter() {
>              @Override
>              public void keyPressed(KeyEvent e) {
>                  System.out.println("cb.getSelectedIndex() = " +
> cb.getSelectedIndex());
> System.out.println("getComboPopup(cb).getList().getSelectedIndex() = "
>                          + getComboPopup(cb).getList().getSelectedIndex());
>              }
>          });
>
>          add(cb);
>          setSize(200, 300);
>      }
>
>      private ComboPopup getComboPopup(JComboBox cb) {
>          AccessibleContext context = cb.getAccessibleContext();
>          for (int i = 0; i < context.getAccessibleChildrenCount(); i++) {
>              Object c = context.getAccessibleChild(i);
>              if (c instanceof ComboPopup) {
>                  return (ComboPopup) c;
>              }
>          }
>          return null;
>      }
>
>      public static void main(String... args) throws Exception {
>          UIManager.setLookAndFeel(new MetalLookAndFeel());
>          SwingUtilities.invokeLater(new Runnable() {
>              public void run() {
>                  new ComboTest().setVisible(true);
>              }
>          });
>      }
> }
>
>>
>>  Thanks,
>>  Alexandr.
>>
>> On 1/30/2013 2:36 PM, Vladislav Karnaukhov wrote:
>>> Hello Alexandr, all,
>>>
>>> please find a new version here:
>>> http://cr.openjdk.java.net/~vkarnauk/4199622/webrev.05/
>>>
>>> On 1/29/2013 07:26 PM, Alexander Scherbatiy wrote:
>>>>
>>>> - WindowsLookAndFeel and ComboBox.noActionOnKeyNavigation property
>>>> is set
>>>>     Press down. Next item is selected and the action listener is
>>>> invoked. Is it expected behavior (the drop down list is not shown in
>>>> this case)?
>>> Yes, it's expected. Customer would like to turn off event firing only
>>> when popup is open. To keep consistency over different LAFs, I only
>>> handle the new flag when drop-down is showing. Under Windows, if drop
>>> list wasn't shown, JComboBox will work as usual even if the flag was
>>> set.
>>>
>>>>
>>>> - It seems that the code below can have a shorter form:
>>>>                  if
>>>> (UIManager.getBoolean("ComboBox.noActionOnKeyNavigation")) {
>>>>                      if (!comboBox.isPopupVisible()) {
>>>>                          comboBox.setSelectedIndex(si+1);
>>>>                      }
>>>>                  } else {
>>>>                      comboBox.setSelectedIndex(si+1);
>>>>                 }
>>>>    ->
>>>>      if ( !(UIManager.getBoolean("ComboBox.noActionOnKeyNavigation") &&
>>>> comboBox.isPopupVisible()) ) {
>>>>            comboBox.setSelectedIndex(si+1);
>>>>       }
>>> Agree; fixed.
>>>
>>>>
>>>> - doTest() method in the test throws some exception. It can throw robot
>>>> and toolkit exceptions as well.
>>> Agree; fixed.
>>>
>>>>
>>>> Thanks,
>>>>   Alexandr.
>>>>
>>