Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 156264

Summary: [DataBinding] Enter key does not cause data-binding to fire on a Text control
Product: [Eclipse Project] Platform Reporter: Andy Maleh <andy>
Component: UIAssignee: Boris Bokowski <bokowski>
Status: RESOLVED DUPLICATE QA Contact:
Severity: enhancement    
Priority: P3 CC: djo, qualidafial
Version: 3.2   
Target Milestone: ---   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
Updated TextObservableValue and SWTObservables
none
Updated TextObservableValue and SWTObservables (a)
none
Updated TextObservableValue, TextObservableValueTests, and SWTObservables none

Description Andy Maleh CLA 2006-09-05 17:00:28 EDT
This is related to Data-Binding Framework.

In an RCP project, I encountered a missing feature that my client wants. It is required for several screens that use data-binding to update calculations off of text control values.

The client wants calculations to get updated when hitting Enter or Carriage Return on a text control. Currently, they only get updated when tabbing off.

I implemented a fix in TextObservableValue by updating the keyListener code:

keyListener = new KeyListener() {
	public void keyPressed(KeyEvent e) {
		if (e.character == SWT.ESC && bufferedValue != null) {
			// Revert the value in the text field to the model value
			text.setText(bufferedValue);
		}
                // Begin new code
		String oldValue = bufferedValue;
		String newValue = text.getText();
		if (e.character == '\r' || e.character == '\n') {
			if (!newValue.equals(oldValue)) {
				fireValueChange(Diffs.createValueDiff(oldValue,
						newValue));
			}
			
		}
                // End new code
	}

	public void keyReleased(KeyEvent e) {
	}
};

I would like to contribute this fix, but first, I would like to get your opinion on how to generalize this solution. 
For example:
-Is it better to allow programmers to specify characters that fire-off data-binding events in a text control, or should we keep Enter and Carriage Return hard-coded?
-What other widgets need this fix/customization? For example, editable combos are another candidate for this fix.
-Is this already done elsewhere?

Thank you,

Andy
Comment 1 Boris Bokowski CLA 2006-09-06 08:52:40 EDT
*** Bug 156251 has been marked as a duplicate of this bug. ***
Comment 2 Boris Bokowski CLA 2006-09-06 21:07:06 EDT
(In reply to comment #0)
> I would like to contribute this fix, but first, I would like to get your
> opinion on how to generalize this solution. 

Thanks!

> -Is it better to allow programmers to specify characters that fire-off
> data-binding events in a text control, or should we keep Enter and Carriage
> Return hard-coded?

Hard to tell. I would keep it hard-coded until someone comes with a good example where you would want to use another key.

> -What other widgets need this fix/customization? For example, editable combos
> are another candidate for this fix.

Yes. Makes sense.

> -Is this already done elsewhere?

No, I don't think.  It should be optional though whether the Enter key is processed - for example, in dialogs, the Enter key goes to the default button.

Have a look at the (new in HEAD) class SWTObservables.  I would like to see getText() generalized to take a text widget, and an "observable style" int for specifying whether Enter should be handled, whether to use FocusOut or Modify as the time of updating, and whether to support Escape to undo edits. Specifying NONE should result in reasonable defaults.
Comment 3 Andy Maleh CLA 2006-09-11 10:34:12 EDT
Thank you Boris.

I will take your suggestions into account.

Andy
Comment 4 Andy Maleh CLA 2006-09-14 16:18:41 EDT
I implemented your suggestions Boris. 

I will submit a patch tonight once I am done testing the new code.
Comment 5 Andy Maleh CLA 2006-09-15 02:46:05 EDT
I spent some time tonight testing the new code. 

I would like to test it more before I submit the patch however. 

I think I will get it done sometime this weekend.
Comment 6 Andy Maleh CLA 2006-09-25 02:02:59 EDT
Created attachment 50775 [details]
Updated TextObservableValue and SWTObservables

TextObservableValue is modified to accept a new optional style bit (SWT.CR) to fire change events when hitting ENTER. It can only be used in conjunction with SWT.FocusOut.
Comment 7 Andy Maleh CLA 2006-09-25 02:12:59 EDT
Created attachment 50779 [details]
Updated TextObservableValue and SWTObservables (a)

Use this one as the last one did not have defaults programmed for SWT.NONE.
SWT.NONE is equivalent to SWT.FocusOut | SWT.CR
Comment 8 Andy Maleh CLA 2006-09-25 02:14:03 EDT
Comment on attachment 50775 [details]
Updated TextObservableValue and SWTObservables

This patch was missing correct implementation for SWT.NONE. Use newer patch instead.
Comment 9 Andy Maleh CLA 2006-09-25 02:31:07 EDT
Implementing the feature turned out to be a bit tricky. Apparently, the style bit SWT.Modify conflicts with SWT.ESC and SWT.CR, so TextObservableValue was unable to correctly recognize the update mode when receiving an observableStyle like "SWT.Modify | SWT.ESC" or "SWT.FocusOut | SWT.ESC" (always thought it was SWT.Modify). 

I decided not to provide the SWT.ESC option anymore, keeping the escape behavior always enabled. I kept the SWT.CR option, but had the program never accept something like "SWT.Modify | SWT.CR", and that solved the conflict between those styles. You can look at the code for more details on the implementation.

SWT.NONE now signifies the following defaults: "SWT.FocusOut | SWT.CR"

If you know of other ways for solving the style-bit-conflict problem, I'd be curious to learn of them. The only other ways I could think of are getting the ESC and CR information from extra parameters in the TextObservableValue constructor or from new styles that we add to the SWT class to avoid the bit conflict problem with SWT.Modify.

Please let me know if you have any other suggestions.

Best regards,

Andy
Comment 10 Dave Orme CLA 2006-09-25 09:46:38 EDT
Thanks for the great work!

(Reopening until the patch is evaluated/verified/applied. :-)

Comment 11 Boris Bokowski CLA 2006-09-25 11:21:26 EDT
(In reply to comment #9)
> "SWT.Modify | SWT.ESC" or "SWT.FocusOut | SWT.ESC"

These SWT constants are not meant to be bitwise OR'ed.  I would rather use our own constants, defined in SWTObservables, for example:

public final int NONE = 0;
public final int UPDATE_ON_MODIFY = 1<<0;
public final int UPDATE_ON_ENTER = 1<<1;
public final int UPDATE_ON_FOCUS_OUT = 1<<2;
public final int REVERT_ON_ESC = 1<<3;

I am not sure what would be a good default.  One solution to this is not to provide a default for now, and to add convenience methods later.
Comment 12 Andy Maleh CLA 2006-09-25 12:51:25 EDT
Awesome! Thanks Boris.

I am not sure what you mean by "add convenience methods later" though.

David likes the concept of convention over configuration, which made me eager to implement a default to make programming easier. That enables us to have an SWTObservables.getText(text) method that does not take a second parameter and passes SWT.NONE to TextObservableValue. However, if we do not know what the most suitable defaults are for framework users, we could delay that till later.

Regards,

Andy
Comment 13 Boris Bokowski CLA 2006-09-25 14:32:16 EDT
(In reply to comment #12)
> David likes the concept of convention over configuration, which made me eager
> to implement a default to make programming easier.

The problem with conventions is that so many are possible.  Let's wait with adding convenience methods until we understand the possible uses better.

> That enables us to have an
> SWTObservables.getText(text) method that does not take a second parameter and
> passes SWT.NONE to TextObservableValue.

I'm sure you meant SWTObservables.NONE ;-)  That aside, I think passing NONE should create an observable value that does not attach any listeners.

Comment 14 Andy Maleh CLA 2006-09-27 01:36:26 EDT
Created attachment 50985 [details]
Updated TextObservableValue, TextObservableValueTests, and SWTObservables

I implemented the suggested constants, but placed them in TextObservableValue because I did not want to introduce a dependency from TextObservableValue to SWTObservables to avoid creating a dependency cycle betwen them.

I am a little tired, and am afraid I made a mistake somewhere (despite that all my functional tests passed). So, I will review the code again in a few days, possibly during the weekend.

Please let me know your thoughts on it.
Comment 15 Matthew Hall CLA 2009-01-21 20:52:58 EST
We are discussing supporting SWTObservables.observeText(control, SWT.DefaultSelection) in bug 256543

*** This bug has been marked as a duplicate of bug 256543 ***