FIXED - bug 171132 - [DataBinding] Incorrect interaction between observeText(field, FocusOut) and Binding.updateModelFromTarget

https://bugs.eclipse.org/bugs/show_bug.cgi?id=171132

3 files changed, 146 insertions, 117 deletions

diff --git a/bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/internal/databinding/internal/swt/TextObservableValue.java b/bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/internal/databinding/internal/swt/TextObservableValue.java
index 737d6a87..5dd1b4d0 100644
--- a/bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/internal/databinding/internal/swt/TextObservableValue.java
+++ b/bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/internal/databinding/internal/swt/TextObservableValue.java
@@ -16,11 +16,6 @@ import org.eclipse.core.databinding.observable.Diffs;
 import org.eclipse.core.databinding.observable.IObservable;
 import org.eclipse.jface.internal.databinding.provisional.swt.AbstractSWTVetoableValue;
 import org.eclipse.swt.SWT;
-import org.eclipse.swt.events.KeyEvent;
-import org.eclipse.swt.events.KeyListener;
-import org.eclipse.swt.events.ShellAdapter;
-import org.eclipse.swt.events.ShellEvent;
-import org.eclipse.swt.events.ShellListener;
 import org.eclipse.swt.events.VerifyEvent;
 import org.eclipse.swt.events.VerifyListener;
 import org.eclipse.swt.widgets.Event;
@@ -63,7 +58,7 @@ public class TextObservableValue extends AbstractSWTVetoableValue {
 	 * SWT event that on firing this observable will fire change events to its
 	 * listeners.
 	 */
-	private int updateEventType;
+	private final int updateEventType;
 
 	/**
 	 * Valid types for the {@link #updateEventType}.
@@ -72,30 +67,18 @@ public class TextObservableValue extends AbstractSWTVetoableValue {
 			SWT.FocusOut, SWT.NONE };
 
 	/**
-	 * Last value set using doSetValue(), or null. This is maintained so that
-	 * when entering text if the consumer were to press [Escape] the value can
-	 * be reverted back to the last known externally-set value.
+	 * Previous value of the Text.
 	 */
-	private String bufferedValue;
+	private String oldValue;
 
 	private Listener updateListener = new Listener() {
 		public void handleEvent(Event event) {
 			if (!updating) {
-				String oldValue = bufferedValue;
 				String newValue = text.getText();
 
-				// If we are updating on focus lost then when we fire the change
-				// event change the buffered value
-				if (updateEventType == SWT.FocusOut) {
-					bufferedValue = text.getText();
-
-					if (!newValue.equals(oldValue)) {
-						fireValueChange(Diffs.createValueDiff(oldValue,
-								newValue));
-					}
-				} else {
-					fireValueChange(Diffs.createValueDiff(oldValue, text
-							.getText()));
+				if (!newValue.equals(oldValue)) {
+					fireValueChange(Diffs.createValueDiff(oldValue, newValue));					
+					oldValue = newValue;
 				}
 			}
 		}
@@ -103,10 +86,6 @@ public class TextObservableValue extends AbstractSWTVetoableValue {
 
 	private VerifyListener verifyListener;
 
-	private KeyListener keyListener;
-
-	private ShellListener shellListener;
-
 	/**
 	 * Constructs a new instance bound to the given <code>text</code> widget
 	 * and configured to fire change events to its listeners at the time of the
@@ -135,21 +114,9 @@ public class TextObservableValue extends AbstractSWTVetoableValue {
 		if (updateEventType != SWT.None) {
 			text.addListener(updateEventType, updateListener);
 		}
-		// If the update policy is SWT.Modify then the model is notified of
-		// changed on key stroke by key stroke
-		// When escape is pressed we need to rollback to the previous value
-		// which is done with a key listener, however
-		// the bufferedValue (the last remembered change value) must be changed
-		// on focus lost
-		if (updateEventType == SWT.Modify) {
-			text.addListener(SWT.FocusOut, new Listener() {
-				public void handleEvent(Event event) {
-					if (!updating) {
-						bufferedValue = text.getText();
-					}
-				}
-			});
-		}
+		
+		oldValue = text.getText();
+		
 		verifyListener = new VerifyListener() {
 			public void verifyText(VerifyEvent e) {
 				if (!updating) {
@@ -165,32 +132,6 @@ public class TextObservableValue extends AbstractSWTVetoableValue {
 			}
 		};
 		text.addVerifyListener(verifyListener);
-		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);
-				}
-			}
-
-			public void keyReleased(KeyEvent e) {
-			}
-		};
-		text.addKeyListener(keyListener);
-		shellListener = new ShellAdapter() {
-			public void shellClosed(ShellEvent e) {
-				if (!text.isDisposed()) {
-					String oldValue = bufferedValue;
-					String newValue = text.getText();
-
-					if (!newValue.equals(oldValue)) {
-						fireValueChange(Diffs.createValueDiff(oldValue,
-								newValue));
-					}
-				}
-			}
-		};
-		text.getShell().addShellListener(shellListener);
 	}
 
 	/**
@@ -202,11 +143,11 @@ public class TextObservableValue extends AbstractSWTVetoableValue {
 	 * @throws ClassCastException
 	 *             if the value is anything other than a String
 	 */
-	public void doSetApprovedValue(final Object value) {
+	protected void doSetApprovedValue(final Object value) {
 		try {
 			updating = true;
-			bufferedValue = (String) value;
 			text.setText(value == null ? "" : value.toString()); //$NON-NLS-1$
+			oldValue = text.getText();
 		} finally {
 			updating = false;
 		}
@@ -218,7 +159,7 @@ public class TextObservableValue extends AbstractSWTVetoableValue {
 	 * @see org.eclipse.core.databinding.observable.value.AbstractVetoableValue#doGetValue()
 	 */
 	public Object doGetValue() {
-		return text.getText();
+		return oldValue = text.getText();
 	}
 
 	/**
@@ -233,12 +174,10 @@ public class TextObservableValue extends AbstractSWTVetoableValue {
 
 	public void dispose() {
 		if (!text.isDisposed()) {
-			text.removeKeyListener(keyListener);
 			if (updateEventType != SWT.None) {
 				text.removeListener(updateEventType, updateListener);
 			}
 			text.removeVerifyListener(verifyListener);
-			text.getShell().removeShellListener(shellListener);
 		}
 		super.dispose();
 	}
diff --git a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/jface/tests/databinding/scenarios/TextControlScenario.java b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/jface/tests/databinding/scenarios/TextControlScenario.java
index 27f3b6c0..500b09bf 100644
--- a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/jface/tests/databinding/scenarios/TextControlScenario.java
+++ b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/jface/tests/databinding/scenarios/TextControlScenario.java
@@ -20,7 +20,6 @@ import org.eclipse.jface.examples.databinding.model.SampleData;
 import org.eclipse.jface.examples.databinding.model.Transportation;
 import org.eclipse.jface.tests.databinding.BindingTestSuite;
 import org.eclipse.swt.SWT;
-import org.eclipse.swt.widgets.Event;
 import org.eclipse.swt.widgets.Label;
 import org.eclipse.swt.widgets.Text;
 
@@ -99,42 +98,42 @@ public class TextControlScenario extends ScenariosTestCase {
         assertEquals(numberFormat.format(transportation.getPrice()), text.getText());
     }
 
-    public void testScenario03() {
-        // Show that the Escape key can be pressed in the middle of editing and
-        // the value will revert
-        // the updatePolicy for this test is TIME_LATE so it occurs when focus
-        // is lost from the Text control
-        getDbc().bindValue(SWTObservables.observeText(text, SWT.FocusOut),
-                BeansObservables.observeValue(adventure, "name"),
-                null, null);
-
-        String currentText = text.getText();
-        text.setText("Switzerland");
-        // We do not notify FocusOut
-        // Verify that the model hasn't changed
-        assertEquals(currentText, adventure.getName());
-        Event event = new Event();
-        event.character = SWT.ESC;
-        event.keyCode = 27;
-        text.notifyListeners(SWT.KeyDown, event);
-        // Verify that the text has reverted
-        assertEquals(currentText, text.getText());
-        // And that the model didn't change
-        assertEquals(adventure.getName(), currentText);
-        // Now change the GUI and commit this change
-        currentText = "Austria";
-        text.setText(currentText);
-        text.notifyListeners(SWT.FocusOut, null);
-        assertEquals(text.getText(), adventure.getName());
-        // Now change the text again and press escape a second time
-        text.setText("Turkey");
-        // Send escape
-        text.notifyListeners(SWT.KeyDown, event);
-        // Verify it has reverted to "Austria" and not any other value, i.e. the
-        // last value it displayed
-        assertEquals(currentText, text.getText());
-
-    }
+//    public void testScenario03() {
+//        // Show that the Escape key can be pressed in the middle of editing and
+//        // the value will revert
+//        // the updatePolicy for this test is TIME_LATE so it occurs when focus
+//        // is lost from the Text control
+//        getDbc().bindValue(SWTObservables.observeText(text, SWT.FocusOut),
+//                BeansObservables.observeValue(adventure, "name"),
+//                null, null);
+//
+//        String currentText = text.getText();
+//        text.setText("Switzerland");
+//        // We do not notify FocusOut
+//        // Verify that the model hasn't changed
+//        assertEquals(currentText, adventure.getName());
+//        Event event = new Event();
+//        event.character = SWT.ESC;
+//        event.keyCode = 27;
+//        text.notifyListeners(SWT.KeyDown, event);
+//        // Verify that the text has reverted
+//        assertEquals(currentText, text.getText());
+//        // And that the model didn't change
+//        assertEquals(adventure.getName(), currentText);
+//        // Now change the GUI and commit this change
+//        currentText = "Austria";
+//        text.setText(currentText);
+//        text.notifyListeners(SWT.FocusOut, null);
+//        assertEquals(text.getText(), adventure.getName());
+//        // Now change the text again and press escape a second time
+//        text.setText("Turkey");
+//        // Send escape
+//        text.notifyListeners(SWT.KeyDown, event);
+//        // Verify it has reverted to "Austria" and not any other value, i.e. the
+//        // last value it displayed
+//        assertEquals(currentText, text.getText());
+//
+//    }
 
 //    public void testScenario04() {
 //        // Show that the Escape key can be pressed in the middle of editing and
diff --git a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/jface/tests/internal/databinding/internal/swt/TextObservableValueTest.java b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/jface/tests/internal/databinding/internal/swt/TextObservableValueTest.java
index dcea344a..c86bc233 100755
--- a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/jface/tests/internal/databinding/internal/swt/TextObservableValueTest.java
+++ b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/jface/tests/internal/databinding/internal/swt/TextObservableValueTest.java
@@ -13,13 +13,10 @@
 
 package org.eclipse.jface.tests.internal.databinding.internal.swt;
 
-import junit.framework.TestCase;
-
-import org.eclipse.jface.databinding.swt.SWTObservables;
 import org.eclipse.jface.internal.databinding.internal.swt.TextObservableValue;
-import org.eclipse.jface.tests.databinding.RealmTester;
+import org.eclipse.jface.tests.databinding.AbstractDefaultRealmTestCase;
+import org.eclipse.jface.tests.databinding.EventTrackers.ValueChangeEventTracker;
 import org.eclipse.swt.SWT;
-import org.eclipse.swt.widgets.Display;
 import org.eclipse.swt.widgets.Shell;
 import org.eclipse.swt.widgets.Text;
 
@@ -28,15 +25,17 @@ import org.eclipse.swt.widgets.Text;
  * 
  * @since 3.2
  */
-public class TextObservableValueTest extends TestCase {
+public class TextObservableValueTest extends AbstractDefaultRealmTestCase {
 	private Text text;
+	private ValueChangeEventTracker listener;
 
 	protected void setUp() throws Exception {
 		super.setUp();
 
-		RealmTester.setDefault(SWTObservables.getRealm(Display.getDefault()));
 		Shell shell = new Shell();
 		text = new Text(shell, SWT.NONE);
+		
+		listener = new ValueChangeEventTracker();
 	}
 
 	/**
@@ -70,4 +69,96 @@ public class TextObservableValueTest extends TestCase {
 		observableValue.setValue(value);
 		assertEquals("observable value", value, observableValue.getValue());
 	}
+	
+	public void testSetValueValueChangeEvent() throws Exception {
+		String a = "a";
+		String b = "b";
+		
+		TextObservableValue observableValue = new TextObservableValue(text, SWT.NONE);
+		observableValue.addValueChangeListener(listener);
+		
+		observableValue.setValue(a);
+		assertEquals("", listener.event.diff.getOldValue());
+		assertEquals(a, listener.event.diff.getNewValue());
+		
+		observableValue.setValue(b);
+		assertEquals(a, listener.event.diff.getOldValue());
+		assertEquals(b, listener.event.diff.getNewValue());
+	}
+
+	public void testOnModifyValueChangeEvent() throws Exception {
+		TextObservableValue observableValue = new TextObservableValue(text,
+				SWT.Modify);
+
+		String a = "a";
+		String b = "b";
+
+		text.setText(a);
+		
+		observableValue.addValueChangeListener(listener);
+
+		assertEquals(0, listener.count);
+		text.setText(b);
+
+		assertEquals(1, listener.count);
+		assertEquals(a, listener.event.diff.getOldValue());
+		assertEquals(b, listener.event.diff.getNewValue());
+	}
+
+	public void testOnFocusOutValueChangeEvent() throws Exception {
+		String a = "a";
+		String b = "b";
+		
+		text.setText(a);
+
+		TextObservableValue observableValue = new TextObservableValue(text,
+				SWT.FocusOut);
+		
+		observableValue.addValueChangeListener(listener);
+		
+		text.setText(b);
+		assertEquals(0, listener.count);
+		
+		text.notifyListeners(SWT.FocusOut, null);
+		assertEquals(1, listener.count);
+		
+		assertEquals(a, listener.event.diff.getOldValue());
+		assertEquals(b, listener.event.diff.getNewValue());
+	}
+	
+	public void testChangeEventsSuppressedWhenValueDoesNotChange() throws Exception {
+		TextObservableValue observableValue = new TextObservableValue(text, SWT.Modify);
+		
+		observableValue.addValueChangeListener(listener);
+		
+		String value = "value";
+		text.setText(value);
+		assertEquals(1, listener.count);
+		
+		text.setText(value);
+		assertEquals("listener not notified", 1, listener.count);
+	}
+	
+	/**
+	 * https://bugs.eclipse.org/bugs/show_bug.cgi?id=171132
+	 * 
+	 * @throws Exception
+	 */
+	public void testGetValueBeforeFocusOutChangeEventsFire() throws Exception {
+		TextObservableValue observableValue = new TextObservableValue(text, SWT.FocusOut);
+		observableValue.addValueChangeListener(listener);
+		
+		String a = "a";
+		String b = "b";
+		
+		text.setText(a);
+		assertEquals(a, observableValue.getValue()); //fetch the value updating the buffered value
+		
+		text.setText(b);
+		text.notifyListeners(SWT.FocusOut, null);
+		
+		assertEquals(1, listener.count);
+		assertEquals(a, listener.event.diff.getOldValue());
+		assertEquals(b, listener.event.diff.getNewValue());
+	}
 }