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

[Alexander Scherbatiy]

   I am able to reproduce the behaviour from the ComboTest:
    cb.getSelectedIndex() = 0
    getComboPopup(cb).getList().getSelectedIndex() = 4

  Because our initial guess was that these indices are always equal I 
suggest to revert this fix to avoid any possible unpredictable regressions.

  Thanks,
  Alexandr.


On 2/8/2013 2:47 PM, Vladislav Karnaukhov wrote:
> 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.
>>>>>
>>>
>
>