Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 54989 - [CellEditors] [regression] ComboBoxCellEditor does not fire value applied
Summary: [CellEditors] [regression] ComboBoxCellEditor does not fire value applied
Status: RESOLVED WONTFIX
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.0   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Tod Creasey CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 54377 (view as bug list)
Depends on:
Blocks: 55485
  Show dependency tree
 
Reported: 2004-03-16 13:47 EST by Randy Hudson CLA
Modified: 2013-01-16 09:48 EST (History)
8 users (show)

See Also:


Attachments
Excel spreadsheet showing Combo vs. CCombo events (15.00 KB, application/octet-stream)
2004-04-07 17:19 EDT, Carolyn MacLeod CLA
no flags Details
Display Properties gets it wrong (52.13 KB, image/gif)
2004-04-15 14:21 EDT, Veronika Irvine CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Randy Hudson CLA 2004-03-16 13:47:55 EST
1) Drop down a list of choices
2) Select a choice that is different than the currently selected value.

valueApplied should be fired by the cell editor.
Comment 1 Kim Horne CLA 2004-03-16 17:35:01 EST
I wonder if this is a consequence of the changes SWT made to the CCombo control
to fix the extra events that were being fired...
Comment 2 Randy Hudson CLA 2004-03-17 10:19:44 EST
It is not related.  This bug pre-dates the fix for the other bug.  I checked.
Comment 3 Tod Creasey CLA 2004-03-17 10:27:32 EST
This was a new feature from Stefan
Comment 4 Stefan Xenos CLA 2004-03-17 20:25:37 EST
I submitted ComboViewer (not in head yet), not ComboBoxCellEditor. This is
unrelated.
Comment 5 Tod Creasey CLA 2004-03-18 13:54:15 EST
When you say valueApplied you mean valueChanged(boolean oldValidState, boolean 
newValidState)correct?

Right now we cache the selection and thats it. Application happens on default 
selection or loss of focus.
Comment 6 Randy Hudson CLA 2004-03-18 15:06:07 EST
"Application happens on default selection"

Then perhaps SWT is no longer firing default selection? Either way, you 
probably have no way of visually verifying this bug without a real client of 
the propertysheet/celleditors.  If you are sure that the cell editor's code has 
not changed since 2.1, then write an SWT snippet that shows that default 
selection is no longer fired.
Comment 7 Tod Creasey CLA 2004-03-19 09:10:06 EST
I can test using the Tesk list that default selection is working.

STEPS
1) Add a task 
2) Bring up the cell editor on the priority
3) Select an entry
4) Hit return 

And it is applied

If you think there is some behaviour that is not happening please give clear 
steps that I can follow - I have no idea what the problem is you are referring 
to.
Comment 8 Randy Hudson CLA 2004-03-19 10:07:59 EST
Sorry for not being clear.  Steps 1&2 must both be performed using the Mouse, 
not the keyboard.
Comment 9 Randy Hudson CLA 2004-03-19 10:14:11 EST
Actually, in your own list of steps, the value should have been applied at step 
3.

For example, if the task is currently set to LOW priority, and the CellEditor 
appears beside the icon, then you should see the icon change immediately when 
the new value is selected, not upon ENTER.

BTW, I tried sorting task list by priority, which works once.  But if you 
change a task's priority, it does not get resorted.  Is this a bug?
Comment 10 Tod Creasey CLA 2004-03-19 11:22:15 EST
*** Bug 54377 has been marked as a duplicate of this bug. ***
Comment 11 Tod Creasey CLA 2004-03-19 12:48:08 EST
Can you give me an example of different behaviour in 2.1? I am checking with 
the cell editor there but it is covering the icon so I can't see if this has 
been going on since 2.1 or not.
Comment 12 Randy Hudson CLA 2004-03-19 15:40:06 EST
The different behavior is what I said in comment 1,
"valueApplied should be fired by the cell editor"

I have unwrapped ComboBoxCellEditor for you and isolated the SWT change:

public static void main(String[] args) {
	Display display = new Display();
	Shell shell = new Shell();
	shell.setLayout(new GridLayout());
	final CCombo combo = new CCombo(shell, 0);
	combo.setItems(new String[] {"item 1","item 2","item 3"});
	
	combo.addSelectionListener(new SelectionAdapter() {
		public void widgetDefaultSelected(SelectionEvent e) {
			System.out.println("widget default selected");
		}
	});
	
	shell.setSize(400,300);
	shell.open();
	
	while (!shell.isDisposed())
		if (!display.readAndDispatch())
			display.sleep();
}

Steve, "widget default selected" is no longer fired when you select an item 
with the mouse.  This behavior differs from the 2.1 behavior.  My guess is you 
are trying to make it act more like Combo. Is it possible to make Combo like 
CCombo was instead?  It is useful to detect the difference between using ARROW 
keys to scroll through the choices, and mouse clicking directly on the choice 
you want.  Given the current behavior, these two actions cannot be 
distinguished.
Comment 13 Tod Creasey CLA 2004-03-19 15:43:39 EST
So the workaround for us would be to make widgetSelected and 
widgetDefaultSelected do the same thing.

The problem with doing that is that you could not use the mouse to navigate 
then as the editor would keep closing on you.
Comment 14 Randy Hudson CLA 2004-03-19 17:03:28 EST
I didn't understant the last comment.
The workaround is problematic only when using the keyboard to navigate through 
the various choices. In other works:

ALT+DOWN..
DOWN..
DOWN..
DOWN..
ENTER

should produce 1 default selected event.

This is why I prefer the old behavior, and proposed that Combo be change to 
emulate CCombo.
Comment 15 Steve Northover CLA 2004-03-22 11:05:56 EST
Randy, widgetDefaultSelected() should never have been sent from the mouse.  It 
is sent when you hit Return in a Combo.  Can you confirm that this has changed 
between 2.x and 3.x?

One thing, Return is also used for the default button.  If you have a default 
button in a window, it gets the Return first and eats it as part of 
SWT.TRAVERSE_RETURN.  Is that what you are seeing?
Comment 16 Tod Creasey CLA 2004-03-29 11:03:21 EST
Need to decide on the answer for 3.0
Comment 17 Randy Hudson CLA 2004-03-29 11:18:21 EST
Steve, the test snippet is listed in comment 12.  I verified the change between 
2.1 and 3.0.

I don't care exactly what SWT does, but there needs to be a way to distinguish 
between navigating via keyboard, and selecting with the mouse. Users expect to 
see an immediate update (fire value applied) becaues this is how Combos behave 
when they are in toolbars (for example, Zoom, Font Family, Font Size, etc.)
Comment 18 Steve Northover CLA 2004-03-29 12:40:21 EST
The "widgetDefaultSelected" bug has been fixed in CCombo for 3.0.  It was 
bogusly sent in 2.1.x.  The "widgetSelected" method is called when an item is 
selected from the list.  Nothing more for SWT to do here.
Comment 19 Randy Hudson CLA 2004-04-06 14:28:46 EDT
Steve, from what you are saying, it is impossible to restore the 2.1 behavior. 
If widgetSelected is used to fire valueApplied, then keyboard users will have 
valueApplied fired repeatedly as they scroll up and down the list of choices 
using the arrow keys.

Bring up Display Properties in windows XP.  Press ALT+DOWN to drop down the 
choice of themes.  If you use the arrow keys to select a theme, the preview 
area does not reflect the selection changing.  But, if you select a choice from 
the drop down with the mouse, the preview area *does* update.  How can we make 
this distinction in SWT? Is there an event when the drop-down is rolled-up? I 
don't think a mouseDown event is dispatched when clicking on the dropdown.
Comment 20 Ines Khelifi CLA 2004-04-06 15:57:08 EDT
I have verified the widgetDefaultSelected event with Eclipse 2.1, 2.1.3 and 
M8, using the code snippet from <A HREF="#c12">comment 12</A>.

For eclipse 2.1 and 2.1.3:
Event is fired when:
- An item is selected with the mouse.
- The drop down is shown by clicking on the arrow, then an item is selected 
from the drop down with the up and down arrow keys, and the enter key is 
pressed on the selected item.

Event is NOT fired when:
- Traversing the items in the pull down using only the up and down arrow keys.

For M8:
Event is fired when:
- The drop down is shown by clicking on the arrow, then an item is selected 
from the drop down with the up and down arrows, and the enter key is pressed 
on the selected item.

Event is NOT fired when:
- An item is selected with the mouse.
- Traversing the items in the pull down using only the up and down arrow keys.

Comment 21 Randy Hudson CLA 2004-04-06 17:23:30 EDT
ALT+UP_ARROW can also cause the drop-down to rollup. It would be nice if this 
fired some event as well.
Comment 22 Tod Creasey CLA 2004-04-07 10:12:25 EDT
Closing as this change is a result of an SWT fix.
Comment 23 Steve Northover CLA 2004-04-07 11:49:36 EDT
CAR, can you decode all this and make sure that CCombo and Combo are doing the 
right thing?  Specifically we need to confirm that we are ok with comment 
#20.  Thanks.
Comment 24 Randy Hudson CLA 2004-04-07 13:18:31 EDT
This change in behavior is unacceptable for 3.0.
Comment 25 Randy Hudson CLA 2004-04-07 13:20:15 EDT
Specifically, the CellEditor behavior is unacceptable because of its widespread 
use in the property sheet. I do not have an opinion on the CCombo change.
Comment 26 Nick Edgar CLA 2004-04-07 13:43:27 EDT
Randy, I'm not sure why you removed Steve from the cc list when you added me.  I
assume it was an accident.

I'm reassigning this to the SWT component owner since the CCombo change is under
review by SWT.

If there will be a change to the notifications from what we had in 2.1, please
move this back to UI so we can fix up ComboBoxCellEditor accordingly.
Comment 27 Carolyn MacLeod CLA 2004-04-07 17:19:40 EDT
Created attachment 9307 [details]
Excel spreadsheet showing Combo vs. CCombo events

Here's another snippet (pasted below) that compares Combo with CCombo (Editable
and Read-Only). I generated the attached Excel Spreadsheet ;) to compare
selection and defaultSelection events generated for various user mouse/keyboard
actions (if I missed any important ones, let me know).

There are a lot of differences in the 'selection' event generation. The
'defaultSelection' events are somewhat more consistent: if the user hits Enter
with the list visible or not, they get defaultSelection. (If they hit TAB in a
CCombo, they get defaultSelection too - so I can see why everybody got used to
hooking default selection in their property sheets).

I do think that the eclipse 2.1 behavior was bogus, and that it is correct not
to fire defaultSelection when an item is selected. Selecting an item should,
and does, fire the 'selection' event. (Of course, it is probably also bogus to
have Tab generate defaultSelection... Steve?).

Having said that, however, I can see that apps would like to get a consistent
event for "the user is really done selecting now". (or... "Is that your final
answer?" :)  And it seems that users cannot be trusted to hit 'Enter' - they
tend to TAB out of things, or 'select-with-mouse-and-then-go-click-elsewhere',
without ever dreaming that the UI was expecting an 'Enter'.  ;)

Steve - what is CCombo's "raison d'etre"? I think we had to create it because
Combo had a fixed size on some platforms and it looked too big and ugly in a
Table - right? Has anybody checked lately if this is still true? CCombo looks
pretty dated on XP... (but I guess that's another issue...)

Maybe we need to be thinking that CCombo is a whole different beast, and not
trying to make it behave like Combo (because we are failing at that anyhow).
Maybe CCombo is just for being in Tables. Maybe it's ok to change the semantics
of defaultSelection to mean "the list is gone and the user has really selected
something".

Other than a new event for Show/Hide of the list, I can't think of anything
else to help these people. The advantage of adding the new event is that it
allows us to think that we might one day be able to make CCombo look & feel
exactly like Combo (with an extra event). Another advantage is that we don't
have mixed-up semantics for defaultSelection in SWT.

But adding a new event is a bit more of a pain for the apps - they are less
likely to think of checking for every possible case. The old bogus behavior
lets the app just hook defaultSelection, and they will catch the user hitting
Enter, TAB, or select-with-mouse. (Hmmm... just noticed that you will *not*
catch the user using arrow keys to select *without* dropping down the list.
i.e. TAB to give the CCombo focus, use UP or DOWN, then TAB out... how do you
guys handle that case today? You should only be getting 'selection'... are you
bogus in that case?)


Here's the new snippet:

import org.eclipse.swt.*;
import org.eclipse.swt.widgets.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.events.*;
import org.eclipse.swt.custom.*;

public class CComboAccessibilityTest {
	static Display display;
	static Shell shell;
	static Combo combo, readOnlyCombo;
	static CCombo ccombo, readOnlyCcombo;
	
	public static void main(String[] args) {
		display = new Display();
		shell = new Shell(display);
		shell.setLayout(new GridLayout(4, true));
		shell.setText("CCombo Accessibility Test");
		
		Label comboLabel = new Label(shell, SWT.None);
		comboLabel.setText("Platform (Edit, ComboBox, ComboLBox)");
		comboLabel.setLayoutData(new
GridData(GridData.HORIZONTAL_ALIGN_CENTER));
		Label editableComboLabel = new Label(shell, SWT.None);
		editableComboLabel.setText("Platform-ReadOnly");
		editableComboLabel.setLayoutData(new
GridData(GridData.HORIZONTAL_ALIGN_CENTER));
		Label ccomboLabel = new Label(shell, SWT.None);
		ccomboLabel.setText("CCombo (Edit, Button, ListBox)");
		ccomboLabel.setLayoutData(new
GridData(GridData.HORIZONTAL_ALIGN_CENTER));
		Label editableCcomboLabel = new Label(shell, SWT.None);
		editableCcomboLabel.setText("CCombo-ReadOnly");
		editableCcomboLabel.setLayoutData(new
GridData(GridData.HORIZONTAL_ALIGN_CENTER));

		SelectionListener listener = new SelectionListener() {
			public void widgetSelected(SelectionEvent e) {
				System.out.println("selection");
			}
			public void widgetDefaultSelected(SelectionEvent e) {
				System.out.println("default selection");
			}
		};
		
		combo = new Combo(shell, SWT.NONE);
		for (int i = 0; i < 5; i++) {
			combo.add("item" + i);
		}
		combo.setText("item0");
		combo.setLayoutData(new
GridData(GridData.HORIZONTAL_ALIGN_CENTER));
		combo.addSelectionListener(listener);

		readOnlyCombo = new Combo(shell, SWT.READ_ONLY);
		for (int i = 0; i < 5; i++) {
			readOnlyCombo.add("item" + i);
		}
		readOnlyCombo.setText("item0");
		readOnlyCombo.setLayoutData(new
GridData(GridData.HORIZONTAL_ALIGN_CENTER));
		readOnlyCombo.addSelectionListener(listener);

		ccombo = new CCombo(shell, SWT.BORDER);
		for (int i = 0; i < 5; i++) {
			ccombo.add("item" + i);
		}
		ccombo.setText("item0");
		ccombo.setLayoutData(new
GridData(GridData.HORIZONTAL_ALIGN_CENTER));
		ccombo.addSelectionListener(listener);

		readOnlyCcombo = new CCombo(shell, SWT.BORDER | SWT.READ_ONLY);

		for (int i = 0; i < 5; i++) {
			readOnlyCcombo.add("item" + i);
		}
		readOnlyCcombo.setText("item0");
		readOnlyCcombo.setLayoutData(new
GridData(GridData.HORIZONTAL_ALIGN_CENTER));
		readOnlyCcombo.addSelectionListener(listener);

		shell.pack();
		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch()) display.sleep();
		}
	}
}
Comment 28 Steve Northover CLA 2004-04-08 13:16:08 EDT
My god, we may have to put back the bogus SWT.DefaultSelection for the single 
selection case.  I will check with Veronika when she gets back.  It seems that 
we shouldn't be sending it when the user tabs out of a CCombo.

The right answer should be to remove it and fix ComboBoxCellEditor to use the 
SWT.Traverse callback to do it.  ComboBoxCellEditor must already be doing 
something when the user clicks off with the mouse.  Thoughts?
Comment 29 Randy Hudson CLA 2004-04-08 14:38:17 EDT
Yes, on focus lost, all types of cell editors fire value applied already.
Comment 30 Steve Northover CLA 2004-04-12 11:31:11 EDT
VI is back!  She now owns the problem.  Here is my initial position: I believe 
we need to either completely remove the bogus SWT.DefaultSelection 
notification or put it all back.
Comment 31 Veronika Irvine CLA 2004-04-15 11:30:51 EDT
1) https://bugs.eclipse.org/bugs/show_bug.cgi?id=41347
This bug report requested that CCombo NOT fire the default selection on mouse 
selection - thus the change in 3.0.  Since this is consistent with Combo, I 
think this is the correct behaviour.

I am still thinking about the case where the application wants the value 
applied immediately when selected by the mouse versus delayed when selected by 
arrows in the list versus ? when selected by arrows in the text field.

2) I can remove the default selection on tabbing from the CCombo.  I agree 
that this is behaviour teh application should provide and indeed, as Randy 
said in comment #29, Eclipse already handles the focus lost event which would 
copver the tab case as well.
Comment 32 Steve Northover CLA 2004-04-15 11:51:56 EDT
Please fix the tab thing for 3.0.

If the application wants the value to apply right away, then it should hook 
the SWT.Selection callback as well.
Comment 33 Randy Hudson CLA 2004-04-15 14:08:50 EDT
Is it impossible to send a "rolled up" event from CCombo and Combo?  This is 
the *only* way to simulate the behavior seen on Internet Explorer's Address bar 
combo.  Both ALT+UP and MOUSE click cause the combo to "go", but changing 
selection via DOWN, PAGE_DOWN, etc, does not cause it to go.  What is the SWT 
way to do this??
Comment 34 Veronika Irvine CLA 2004-04-15 14:21:11 EDT
Created attachment 9548 [details]
Display Properties gets it wrong

"tab in popup list causing DefaultSelection" removed in head R3.0.

I am not going to add the default selection on mouse selection for the
following reason - it causes inconsistencies between the value displayed in the
text field and the value applied.  

For example, see the Display Properties dialog attached here.  I opened the
list for the Theme combo using Alt+Down.  I then used the arrow keys to change
from Windows XP to Windows Classic.  Then I hit Escape.  Notice that the Sample
shows the Windows XP style but the Theme shows the Windows Classic style. 
Which one is right?  Even Windows does not handle this correctly.

Notice also that in the same dialog, under the Appearance tab, the "Windows and
buttons" Combo always applies the change on Selection (not Default Selection). 
So they are not even consistent within one application.
Comment 35 Veronika Irvine CLA 2004-04-15 14:23:34 EDT
The "default selection on tab" problem has been fixed - there is no longer a 
default selection on tab.

The "no default selection on mouse selection in list" problem will not be 
fixed - this is intentional.
Comment 36 Randy Hudson CLA 2004-04-15 14:51:51 EDT
VI, I am not suggesting that you fire default selected, I am just suggesting 
that you make it possible to create an IE address-bar-like behavior.  There are 
times when doing something on selection change is cheap and fast and it is OK 
to do so, and then there are times (such as loading a large WinXP theme, or 
navigating to a website, or executing an undoable command from the property 
sheet) when you should wait until the user is really finished selecting what 
they want to select.

In the case of the propertysheet, if the user is selecting the layout manager 
for container, then it is unacceptable for the choices between the current and 
the desired to be applied.  If the intermediate values are applied (e.g. 
keyboard user pressing DOWN), the result is *not* the same.  Therefore, 
ComboboxCellEditor cannot apply value on selectionChanged event.
Comment 37 Veronika Irvine CLA 2004-04-15 16:14:58 EDT
I understand the requirement.  I will have a look at what events are provided 
by the native Combo widget to see what is possible.
Comment 38 Steve Northover CLA 2004-04-15 16:42:41 EDT
When it is expensive, use the SWT.DefaultSelection event and make the guy 
press CR for now?
Comment 39 Randy Hudson CLA 2004-05-07 10:05:12 EDT
I'm reopening this bug. It's fine that the focus-lost part has been fixed, but 
the original problem is not addressed yet.
Comment 40 Steve Northover CLA 2004-05-07 10:32:48 EDT
Should this be moved to UI?  Veronika?
Comment 41 Veronika Irvine CLA 2004-05-07 12:05:30 EDT
Randy, you are looking for "IE address-bar-like behavior" but if you look at 
that case as an example you will see that it behaves quite unlike a combo 
box.  For example, you can not use the up and down arrows in the text area to 
change the selection.

If you do not want to have a mismatch between the selection shown in the the 
text area and the selection you have applied to your UI you will need more 
than just the ability to know when the list has been closed.
Comment 42 Steve Northover CLA 2004-05-07 12:46:50 EDT
Can we close this stupid PR yet?
Comment 43 Randy Hudson CLA 2004-05-07 13:23:08 EDT
From eclipse-dev mailing list:

<quote>
I am using a combobox cell editor with table. also, i am using a cell modifier 
subclass. In windows when i run the code, the selection event of the combo box 
is correctly fired in the ComboBoxCellEditor before the modify() method is
called in the CellModifier. However, in linux (red hat version 9), the order is
reversed resulting in that always the modify method is called before the 
widgetSelected() method and thus, the modify event always gets -1 in the value
parameter.
</quote>

Using call hierarchy on:
+ ICellModifier.modify(...)
  + saveEditorValue (TableViewerImpl)
    + applyEditorValue (TableViewerImpl)
      + initCellEditorListener (""")

the last method there is creating an anonymous inner class to listen 
for "applyEditorValue()".  So this is probably the same problem.
Comment 44 Steve Northover CLA 2004-05-07 14:32:38 EDT
Sounds like completely different problem to me (a platform difference in the 
ordering of the events).  Is there a PR and a test case for it?

Randy, I'm going to close this PR.  You can open separate PR's that describes 
the combo box behavior you want or you can try and implement what you want and 
see which API is missing and enter problem reports for that.  In any 
case, "[CellEditors] [regression] ComboBoxCellEditor does not fire value 
applied" no longer captures this discussion and we will not be changing either 
SWT.Selection or SWT.DefaultSelection here to fix that (which is what the 
problem report was about).
Comment 45 Randy Hudson CLA 2004-05-07 15:47:32 EDT
It is a related problem. Although the event ordering is probably an unrelated 
bug, that bug was conveniently hidden by the widgetDefaultSelected event being 
fired in the past.
Comment 46 Randy Hudson CLA 2004-05-11 10:28:31 EDT
moving back to UI for workaround
Comment 47 Tod Creasey CLA 2004-05-11 10:35:12 EDT
We don't intend to diverge from the SWT behaviour if it all possible. If you 
have a suggested patch please attach here.
Comment 48 Tod Creasey CLA 2004-05-11 10:35:42 EDT
Moving to later as we will not have time to address this for 3.0.
Comment 49 Randy Hudson CLA 2004-05-11 10:40:37 EDT
The workaround for CCombo would be to add a Shell listener, and check shell 
activation. I tried this and it works. If normal selection event is received 
when the Shell is not active, defer sending fireValueApplied until the shell 
becomes active again.  Otherwise, the user is using UP/DOWN arrow to change 
selection without dropping down the list.  So in these cases fire value changed 
immediately, or after a slight delay.

Carolyn, if you add a KeyListener to the CCombo in your snippet, it is no 
longer traversible.  I'm going to open a separate bug.
Comment 50 Randy Hudson CLA 2004-05-11 11:01:09 EDT
Tod, this has nothing to do with SWT behavior, but a limitation.  
The "behavior" just happened to be correct in 2.1 but is no longer.
Comment 51 Michael Van Meekeren CLA 2004-05-11 11:37:09 EDT
re-opening
Comment 52 Michael Van Meekeren CLA 2004-05-11 11:38:48 EDT
marking as WONTFIX, it is unfortunate that the bug was in SWT in 2.1 but 
working around it might just as well introduce other problems
Comment 53 Randy Hudson CLA 2004-09-08 14:02:33 EDT
Tod, I've opened bug 73478.
Comment 54 Henno Vermeulen CLA 2013-01-16 09:48:22 EST
A similar issue is present in ComboBoxViewerCellEditor. A possible workaround is described in https://bugs.eclipse.org/bugs/show_bug.cgi?id=230398