Released patch for Bug 238909 [DataBinding] Correctly set the staleness state of the validation status observable of the MultiValidator
diff --git a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/validation/MultiValidator.java b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/validation/MultiValidator.java
index 385d549..3c459e1 100644
--- a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/validation/MultiValidator.java
+++ b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/validation/MultiValidator.java
@@ -9,21 +9,26 @@
* Matthew Hall - initial API and implementation (bug 218269)
* Boris Bokowski - bug 218269
* Matthew Hall - bug 237884, 240590
+ * Ovidio Mallo - bug 238909
******************************************************************************/
package org.eclipse.core.databinding.validation;
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Iterator;
import java.util.List;
import org.eclipse.core.databinding.ValidationStatusProvider;
import org.eclipse.core.databinding.observable.ChangeEvent;
+import org.eclipse.core.databinding.observable.Diffs;
import org.eclipse.core.databinding.observable.IChangeListener;
import org.eclipse.core.databinding.observable.IObservable;
+import org.eclipse.core.databinding.observable.IStaleListener;
import org.eclipse.core.databinding.observable.ObservableTracker;
import org.eclipse.core.databinding.observable.Observables;
import org.eclipse.core.databinding.observable.Realm;
+import org.eclipse.core.databinding.observable.StaleEvent;
import org.eclipse.core.databinding.observable.list.IListChangeListener;
import org.eclipse.core.databinding.observable.list.IObservableList;
import org.eclipse.core.databinding.observable.list.ListChangeEvent;
@@ -31,8 +36,9 @@
import org.eclipse.core.databinding.observable.list.WritableList;
import org.eclipse.core.databinding.observable.map.IObservableMap;
import org.eclipse.core.databinding.observable.set.IObservableSet;
+import org.eclipse.core.databinding.observable.value.AbstractObservableValue;
import org.eclipse.core.databinding.observable.value.IObservableValue;
-import org.eclipse.core.databinding.observable.value.WritableValue;
+import org.eclipse.core.internal.databinding.Util;
import org.eclipse.core.internal.databinding.observable.ValidatedObservableList;
import org.eclipse.core.internal.databinding.observable.ValidatedObservableMap;
import org.eclipse.core.internal.databinding.observable.ValidatedObservableSet;
@@ -117,7 +123,7 @@
*/
public abstract class MultiValidator extends ValidationStatusProvider {
private Realm realm;
- private IObservableValue validationStatus;
+ private ValidationStatusObservableValue validationStatus;
private IObservableValue unmodifiableValidationStatus;
private WritableList targets;
private IObservableList unmodifiableTargets;
@@ -127,23 +133,31 @@
public void handleListChange(ListChangeEvent event) {
event.diff.accept(new ListDiffVisitor() {
public void handleAdd(int index, Object element) {
- ((IObservable) element)
- .addChangeListener(dependencyListener);
+ IObservable dependency = (IObservable) element;
+ dependency.addChangeListener(dependencyListener);
+ dependency.addStaleListener(dependencyListener);
}
public void handleRemove(int index, Object element) {
- ((IObservable) element)
- .removeChangeListener(dependencyListener);
+ IObservable dependency = (IObservable) element;
+ dependency.removeChangeListener(dependencyListener);
+ dependency.removeStaleListener(dependencyListener);
}
});
}
};
- private IChangeListener dependencyListener = new IChangeListener() {
+ private class DependencyListener implements IChangeListener, IStaleListener {
public void handleChange(ChangeEvent event) {
revalidate();
}
- };
+
+ public void handleStale(StaleEvent staleEvent) {
+ validationStatus.makeStale();
+ }
+ }
+
+ private DependencyListener dependencyListener = new DependencyListener();
/**
* Constructs a MultiValidator on the default realm.
@@ -162,8 +176,7 @@
Assert.isNotNull(realm, "Realm cannot be null"); //$NON-NLS-1$
this.realm = realm;
- validationStatus = new WritableValue(realm, ValidationStatus.ok(),
- IStatus.class);
+ validationStatus = new ValidationStatusObservableValue(realm);
targets = new WritableList(realm, new ArrayList(), IObservable.class);
targets.addListChangeListener(targetsListener);
@@ -197,21 +210,26 @@
}
private void revalidate() {
+ class ValidationRunnable implements Runnable {
+ IStatus validationResult;
+
+ public void run() {
+ try {
+ validationResult = validate();
+ if (validationResult == null)
+ validationResult = ValidationStatus.ok();
+ } catch (RuntimeException e) {
+ // Usually an NPE as dependencies are init'ed
+ validationResult = ValidationStatus
+ .error(e.getMessage(), e);
+ }
+ }
+ }
+
+ ValidationRunnable validationRunnable = new ValidationRunnable();
final IObservable[] dependencies = ObservableTracker.runAndMonitor(
- new Runnable() {
- public void run() {
- try {
- IStatus status = validate();
- if (status == null)
- status = ValidationStatus.ok();
- setStatus(status);
- } catch (RuntimeException e) {
- // Usually an NPE as dependencies are
- // init'ed
- setStatus(ValidationStatus.error(e.getMessage(), e));
- }
- }
- }, null, null);
+ validationRunnable, null, null);
+
ObservableTracker.runAndIgnore(new Runnable() {
public void run() {
List newTargets = new ArrayList(Arrays.asList(dependencies));
@@ -228,14 +246,9 @@
targets.addAll(newTargets);
}
});
- }
- private void setStatus(final IStatus status) {
- ObservableTracker.runAndIgnore(new Runnable() {
- public void run() {
- validationStatus.setValue(status);
- }
- });
+ // Once the dependencies are up-to-date, we set the new status.
+ validationStatus.setValue(validationRunnable.validationResult);
}
/**
@@ -401,4 +414,60 @@
super.dispose();
}
+ private class ValidationStatusObservableValue extends
+ AbstractObservableValue {
+ private Object value = ValidationStatus.ok();
+
+ private boolean stale = false;
+
+ public ValidationStatusObservableValue(Realm realm) {
+ super(realm);
+ }
+
+ protected Object doGetValue() {
+ return value;
+ }
+
+ protected void doSetValue(Object value) {
+ boolean oldStale = stale;
+
+ // Update the staleness state by checking whether any of the current
+ // dependencies is stale.
+ stale = false;
+ for (Iterator iter = targets.iterator(); iter.hasNext();) {
+ IObservable dependency = (IObservable) iter.next();
+ if (dependency.isStale()) {
+ stale = true;
+ break;
+ }
+ }
+
+ Object oldValue = this.value;
+ this.value = value;
+
+ // If either becoming non-stale or setting a new value, we must fire
+ // a value change event.
+ if ((oldStale && !stale) || !Util.equals(oldValue, value)) {
+ fireValueChange(Diffs.createValueDiff(oldValue, value));
+ } else if (!oldStale && stale) {
+ fireStale();
+ }
+ }
+
+ void makeStale() {
+ if (!stale) {
+ stale = true;
+ fireStale();
+ }
+ }
+
+ public boolean isStale() {
+ ObservableTracker.getterCalled(this);
+ return stale;
+ }
+
+ public Object getValueType() {
+ return IStatus.class;
+ }
+ }
}
diff --git a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/validation/MultiValidatorTest.java b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/validation/MultiValidatorTest.java
index 9457db9..2dc8c6f 100644
--- a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/validation/MultiValidatorTest.java
+++ b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/validation/MultiValidatorTest.java
@@ -8,16 +8,19 @@
* Contributors:
* Matthew Hall - initial API and implementation (bug 218269)
* Matthew Hall - bug 237884
- * Ovidio Mallo - bug 240590
+ * Ovidio Mallo - bugs 240590, 238909
******************************************************************************/
package org.eclipse.core.tests.databinding.validation;
import org.eclipse.core.databinding.DataBindingContext;
import org.eclipse.core.databinding.observable.ChangeEvent;
+import org.eclipse.core.databinding.observable.Diffs;
import org.eclipse.core.databinding.observable.IChangeListener;
+import org.eclipse.core.databinding.observable.IStaleListener;
import org.eclipse.core.databinding.observable.ObservableTracker;
import org.eclipse.core.databinding.observable.Realm;
+import org.eclipse.core.databinding.observable.StaleEvent;
import org.eclipse.core.databinding.observable.value.IObservableValue;
import org.eclipse.core.databinding.observable.value.IValueChangeListener;
import org.eclipse.core.databinding.observable.value.ValueChangeEvent;
@@ -28,16 +31,17 @@
import org.eclipse.core.runtime.AssertionFailedException;
import org.eclipse.core.runtime.IStatus;
import org.eclipse.jface.databinding.conformance.util.CurrentRealm;
+import org.eclipse.jface.databinding.conformance.util.ValueChangeEventTracker;
import org.eclipse.jface.tests.databinding.AbstractDefaultRealmTestCase;
public class MultiValidatorTest extends AbstractDefaultRealmTestCase {
- private WritableValue dependency;
+ private DependencyObservableValue dependency;
private MultiValidator validator;
private IObservableValue validationStatus;
protected void setUp() throws Exception {
super.setUp();
- dependency = new WritableValue(null, IStatus.class);
+ dependency = new DependencyObservableValue(null, IStatus.class);
validator = new MultiValidator() {
protected IStatus validate() {
return (IStatus) dependency.getValue();
@@ -142,7 +146,7 @@
}
public void testBug237884_Comment3_ValidationStatusAsDependencyCausesStackOverflow() {
- dependency = new WritableValue(new Object(), Object.class);
+ dependency = new DependencyObservableValue(new Object(), Object.class);
validator = new MultiValidator() {
private int counter;
@@ -213,4 +217,129 @@
// dependency set (the validator's targets).
assertFalse(validator.getTargets().contains(noDependency));
}
+
+ public void testValidationStaleness() {
+ ValueChangeEventTracker validationChangeCounter = ValueChangeEventTracker
+ .observe(validationStatus);
+
+ StaleCounter validationStaleCounter = new StaleCounter();
+ validationStatus.addStaleListener(validationStaleCounter);
+
+ // Assert initial state.
+ assertFalse(validationStatus.isStale());
+ assertEquals(0, validationChangeCounter.count);
+ assertEquals(0, validationStaleCounter.count);
+
+ // Change to a stale state.
+ dependency.setStale(true);
+ assertTrue(validationStatus.isStale());
+ assertEquals(0, validationChangeCounter.count);
+ assertEquals(1, validationStaleCounter.count); // +1
+
+ // The validation status is already stale so even if it gets another
+ // stale event from its dependencies, it should not propagate that
+ // event.
+ dependency.fireStale();
+ assertTrue(validationStatus.isStale());
+ assertEquals(0, validationChangeCounter.count);
+ assertEquals(1, validationStaleCounter.count);
+
+ // Change the validation status while remaining stale.
+ dependency.setValue(ValidationStatus.error("e1"));
+ assertTrue(validationStatus.isStale());
+ assertEquals(1, validationChangeCounter.count); // +1
+ assertEquals(1, validationStaleCounter.count);
+
+ // Move back to a non-stale state.
+ dependency.setStale(false);
+ assertFalse(dependency.isStale());
+ assertFalse(validationStatus.isStale());
+ assertEquals(2, validationChangeCounter.count); // +1
+ assertEquals(1, validationStaleCounter.count);
+ }
+
+ public void testStatusValueChangeWhileValidationStale() {
+ // Change to a stale state.
+ dependency.setStale(true);
+ assertTrue(validationStatus.isStale());
+
+ // Even if the validation is stale, we want the current value to be
+ // tracked.
+ dependency.setValue(ValidationStatus.error("e1"));
+ assertTrue(validationStatus.isStale());
+ assertEquals(dependency.getValue(), validationStatus.getValue());
+ dependency.setValue(ValidationStatus.error("e2"));
+ assertTrue(validationStatus.isStale());
+ assertEquals(dependency.getValue(), validationStatus.getValue());
+ }
+
+ public void testValidationStatusBecomesStaleThroughNewDependency() {
+ final DependencyObservableValue nonStaleDependency = new DependencyObservableValue(
+ ValidationStatus.ok(), IStatus.class);
+ nonStaleDependency.setStale(false);
+
+ final DependencyObservableValue staleDependency = new DependencyObservableValue(
+ ValidationStatus.ok(), IStatus.class);
+ staleDependency.setStale(true);
+
+ validator = new MultiValidator() {
+ protected IStatus validate() {
+ if (nonStaleDependency.getValue() != null) {
+ return (IStatus) nonStaleDependency.getValue();
+ }
+ return (IStatus) staleDependency.getValue();
+ }
+ };
+ validationStatus = validator.getValidationStatus();
+
+ assertFalse(validationStatus.isStale());
+
+ StaleCounter validationStaleCounter = new StaleCounter();
+ validationStatus.addStaleListener(validationStaleCounter);
+ assertEquals(0, validationStaleCounter.count);
+
+ // Setting the status of the non-stale dependency to null leads to the
+ // new stale dependency being accessed which in turn should trigger a
+ // stale event.
+ nonStaleDependency.setValue(null);
+ assertTrue(validationStatus.isStale());
+ assertEquals(1, validationStaleCounter.count);
+ }
+
+ private static class DependencyObservableValue extends WritableValue {
+ private boolean stale = false;
+
+ public DependencyObservableValue(Object initialValue, Object valueType) {
+ super(initialValue, valueType);
+ }
+
+ public boolean isStale() {
+ ObservableTracker.getterCalled(this);
+ return stale;
+ }
+
+ public void setStale(boolean stale) {
+ if (this.stale != stale) {
+ this.stale = stale;
+ if (stale) {
+ fireStale();
+ } else {
+ fireValueChange(Diffs.createValueDiff(doGetValue(),
+ doGetValue()));
+ }
+ }
+ }
+
+ protected void fireStale() {
+ super.fireStale();
+ }
+ }
+
+ private static class StaleCounter implements IStaleListener {
+ int count;
+
+ public void handleStale(StaleEvent event) {
+ count++;
+ }
+ }
}
