413571: compare with base opens 2 editors

* dispose the compare UI observer directly
* use CopyOnWriteArrayList to avoid ConcurrentModificationException
* remove synchronized blocks around remoteEmfObservers

Bug: 413571
Change-Id: I639f731c6ae7dcdfde677106e55fc3758fcc4c5f
Task-Url: https://bugs.eclipse.org/bugs/show_bug.cgi?id=413571
Signed-off-by: Tomasz Zarna <tomasz.zarna@tasktop.com>

2 files changed, 41 insertions, 36 deletions

diff --git a/org.eclipse.mylyn.reviews.core/src/org/eclipse/mylyn/reviews/core/spi/remote/emf/RemoteENotificationImpl.java b/org.eclipse.mylyn.reviews.core/src/org/eclipse/mylyn/reviews/core/spi/remote/emf/RemoteENotificationImpl.java
index bb159844b..c1813805f 100644
--- a/org.eclipse.mylyn.reviews.core/src/org/eclipse/mylyn/reviews/core/spi/remote/emf/RemoteENotificationImpl.java
+++ b/org.eclipse.mylyn.reviews.core/src/org/eclipse/mylyn/reviews/core/spi/remote/emf/RemoteENotificationImpl.java
@@ -11,6 +11,7 @@
 
 package org.eclipse.mylyn.reviews.core.spi.remote.emf;
 
+import org.apache.commons.lang.ArrayUtils;
 import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.Status;
 import org.eclipse.emf.ecore.EReference;
@@ -166,13 +167,21 @@ public class RemoteENotificationImpl extends ENotificationImpl implements Remote
 	}
 
 	public boolean isDone() {
-		return getEventType() == REMOTE_UPDATE || getEventType() == REMOTE_FAILURE
-				|| getEventType() == REMOTE_MEMBER_UPDATE || getEventType() == REMOTE_MEMBER_FAILURE;
+		int[] failureOrUpdate = new int[] { REMOTE_UPDATE, //
+				REMOTE_FAILURE, //
+				REMOTE_MEMBER_UPDATE, //
+				REMOTE_MEMBER_FAILURE //
+		};
+		return ArrayUtils.contains(failureOrUpdate, getEventType());
 	}
 
 	public boolean isMember() {
-		return getEventType() == REMOTE_MEMBER_CREATE || getEventType() == REMOTE_MEMBER_FAILURE
-				|| getEventType() == REMOTE_MEMBER_UPDATE || getEventType() == REMOTE_MEMBER_UPDATING;
+		int[] member = new int[] { REMOTE_MEMBER_CREATE, //
+				REMOTE_MEMBER_FAILURE, //
+				REMOTE_MEMBER_UPDATE, //
+				REMOTE_MEMBER_UPDATING //
+		};
+		return ArrayUtils.contains(member, getEventType());
 	}
 
 	public IStatus getStatus() {
@@ -180,8 +189,12 @@ public class RemoteENotificationImpl extends ENotificationImpl implements Remote
 	}
 
 	public boolean isModification() {
-		return (modified && getEventType() != REMOTE_MEMBER_FAILURE && getEventType() != REMOTE_FAILURE
-				&& getEventType() != REMOTE_UPDATING && getEventType() != REMOTE_MEMBER_UPDATING)
-				|| getEventType() == REMOTE_MEMBER_CREATE;
+		int[] failureOrUpdating = new int[] { REMOTE_MEMBER_FAILURE, //
+				REMOTE_FAILURE, //
+				REMOTE_UPDATING, //
+				REMOTE_MEMBER_UPDATING //
+		};
+		return getEventType() == REMOTE_MEMBER_CREATE
+				|| (modified && !ArrayUtils.contains(failureOrUpdating, getEventType()));
 	}
 }
diff --git a/org.eclipse.mylyn.reviews.core/src/org/eclipse/mylyn/reviews/core/spi/remote/emf/RemoteEmfConsumer.java b/org.eclipse.mylyn.reviews.core/src/org/eclipse/mylyn/reviews/core/spi/remote/emf/RemoteEmfConsumer.java
index 36342bf91..43599a36e 100644
--- a/org.eclipse.mylyn.reviews.core/src/org/eclipse/mylyn/reviews/core/spi/remote/emf/RemoteEmfConsumer.java
+++ b/org.eclipse.mylyn.reviews.core/src/org/eclipse/mylyn/reviews/core/spi/remote/emf/RemoteEmfConsumer.java
@@ -11,8 +11,8 @@
 
 package org.eclipse.mylyn.reviews.core.spi.remote.emf;
 
-import java.util.ArrayList;
 import java.util.Collection;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IProgressMonitor;
@@ -77,24 +77,22 @@ public class RemoteEmfConsumer<EParentObjectType extends EObject, EObjectType, L
 						&& ((msg.getNewValue() == modelObject && (msg.getEventType() == RemoteNotification.REMOTE_MEMBER_CREATE || msg.getEventType() == RemoteNotification.REMOTE_MEMBER_FAILURE)) || modelObject instanceof Collection);
 				boolean notifyChild = !remoteMessage.isMember() && msg.getNotifier() == modelObject;
 				if (notifyParent || notifyChild) {
-					synchronized (remoteEmfObservers) {
-						for (IRemoteEmfObserver<EParentObjectType, EObjectType, LocalKeyType, ObjectCurrentType> listener : remoteEmfObservers) {
-							switch (msg.getEventType()) {
-							case RemoteNotification.REMOTE_MEMBER_CREATE:
-								listener.created(parentObject, modelObject);
-								break;
-							case RemoteNotification.REMOTE_MEMBER_UPDATING:
-							case RemoteNotification.REMOTE_UPDATING:
-								listener.updating(parentObject, modelObject);
-								break;
-							case RemoteNotification.REMOTE_MEMBER_UPDATE:
-							case RemoteNotification.REMOTE_UPDATE:
-								listener.updated(parentObject, modelObject, remoteMessage.isModification());
-								break;
-							case RemoteNotification.REMOTE_MEMBER_FAILURE:
-							case RemoteNotification.REMOTE_FAILURE:
-								listener.failed(parentObject, modelObject, remoteMessage.getStatus());
-							}
+					for (IRemoteEmfObserver<EParentObjectType, EObjectType, LocalKeyType, ObjectCurrentType> listener : remoteEmfObservers) {
+						switch (msg.getEventType()) {
+						case RemoteNotification.REMOTE_MEMBER_CREATE:
+							listener.created(parentObject, modelObject);
+							break;
+						case RemoteNotification.REMOTE_MEMBER_UPDATING:
+						case RemoteNotification.REMOTE_UPDATING:
+							listener.updating(parentObject, modelObject);
+							break;
+						case RemoteNotification.REMOTE_MEMBER_UPDATE:
+						case RemoteNotification.REMOTE_UPDATE:
+							listener.updated(parentObject, modelObject, remoteMessage.isModification());
+							break;
+						case RemoteNotification.REMOTE_MEMBER_FAILURE:
+						case RemoteNotification.REMOTE_FAILURE:
+							listener.failed(parentObject, modelObject, remoteMessage.getStatus());
 						}
 					}
 				}
@@ -120,7 +118,7 @@ public class RemoteEmfConsumer<EParentObjectType extends EObject, EObjectType, L
 		if (localKey == null && modelObject != null) {
 			this.localKey = factory.getLocalKey(null, modelObject);
 		}
-		remoteEmfObservers = new ArrayList<IRemoteEmfObserver<EParentObjectType, EObjectType, LocalKeyType, ObjectCurrentType>>();
+		remoteEmfObservers = new CopyOnWriteArrayList<IRemoteEmfObserver<EParentObjectType, EObjectType, LocalKeyType, ObjectCurrentType>>();
 		if (modelObject instanceof EObject) {
 			getFactory().getService().modelExec(new Runnable() {
 				public void run() {
@@ -320,9 +318,7 @@ public class RemoteEmfConsumer<EParentObjectType extends EObject, EObjectType, L
 				}
 			}
 		}, false);
-		synchronized (remoteEmfObservers) {
-			remoteEmfObservers.clear();
-		}
+		remoteEmfObservers.clear();
 		getFactory().removeConsumer(this);
 		if (getModelObject() instanceof EObject) {
 			getFactory().getFactoryProvider().close((EObject) getModelObject());
@@ -345,9 +341,7 @@ public class RemoteEmfConsumer<EParentObjectType extends EObject, EObjectType, L
 			}
 			remoteEmfObserver.internalSetConsumer(this);
 		}
-		synchronized (remoteEmfObservers) {
-			remoteEmfObservers.add(observer);
-		}
+		remoteEmfObservers.add(observer);
 		if (modelObject instanceof EObject) {
 			if (!((EObject) modelObject).eAdapters().contains(adapter)) {
 				((EObject) modelObject).eAdapters().add(adapter);
@@ -374,9 +368,7 @@ public class RemoteEmfConsumer<EParentObjectType extends EObject, EObjectType, L
 				remoteEmfObserver.internalSetConsumer(null);
 			}
 		}
-		synchronized (remoteEmfObservers) {
-			remoteEmfObservers.remove(observer);
-		}
+		remoteEmfObservers.remove(observer);
 		release();
 	}