Bug 545686 - Keyboard navigation of tables with many validation problems is slow

Performance improvements in:
- rendering checkbox columns in the grid control
- do not change the child context when a selection change in the
  grid is within the same row (side-to-side between columns only)
- compute exactly which row(s) to update for validation changes
  in a table view instead of all of them every time
- optimize DiagnosticHelper for access to standard diagnostic
  data pattern

Change-Id: Id723c43ba4b237f9b5bae01ee3be98213263f2cc
Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>

7 files changed, 667 insertions, 89 deletions

diff --git a/bundles/org.eclipse.emf.ecp.view.table.celleditor.rcp/src/org/eclipse/emf/ecp/view/spi/table/celleditor/rcp/NativeWidgetHelper.java b/bundles/org.eclipse.emf.ecp.view.table.celleditor.rcp/src/org/eclipse/emf/ecp/view/spi/table/celleditor/rcp/NativeWidgetHelper.java
index 30f423b20c..2b28310f8c 100644
--- a/bundles/org.eclipse.emf.ecp.view.table.celleditor.rcp/src/org/eclipse/emf/ecp/view/spi/table/celleditor/rcp/NativeWidgetHelper.java
+++ b/bundles/org.eclipse.emf.ecp.view.table.celleditor.rcp/src/org/eclipse/emf/ecp/view/spi/table/celleditor/rcp/NativeWidgetHelper.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2011-2015 EclipseSource Muenchen GmbH and others.
+ * Copyright (c) 2011-2019 EclipseSource Muenchen GmbH and others.
  *
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
@@ -8,6 +8,7 @@
  *
  * Contributors:
  * jfaltermeier - initial API and implementation
+ * Christian W. Damus - bug 545686
  ******************************************************************************/
 /*******************************************************************************
  * Copyright (c) 2006, 2014 Tom Schindl and others.
@@ -24,7 +25,8 @@
  *******************************************************************************/
 package org.eclipse.emf.ecp.view.spi.table.celleditor.rcp;
 
-import java.util.concurrent.Semaphore;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 
 import org.eclipse.emf.ecp.view.spi.util.swt.ImageRegistryService;
 import org.eclipse.swt.SWT;
@@ -40,6 +42,8 @@ import org.eclipse.swt.widgets.Shell;
 import org.osgi.framework.Bundle;
 import org.osgi.framework.FrameworkUtil;
 import org.osgi.framework.ServiceReference;
+import org.osgi.util.tracker.ServiceTracker;
+import org.osgi.util.tracker.ServiceTrackerCustomizer;
 
 /**
  * Util class for faking native widgets.
@@ -55,14 +59,25 @@ public final class NativeWidgetHelper {
 	private static final String CHECKED_DEFAULT = "icons/checked.png"; //$NON-NLS-1$
 	private static final String UNCHECKED_DEFAULT = "icons/unchecked.png"; //$NON-NLS-1$
 
-	private static final Semaphore LOCK = new Semaphore(1, true);
+	private static final Lock LOCK = new ReentrantLock(true);
 	private static boolean initalized;
 
-	private static ServiceReference<ImageRegistryService> imageRegistryServiceReference;
+	private static final Bundle BUNDLE;
+	private static final ServiceTracker<ImageRegistryService, ImageRegistryService> IMAGE_REGISTRY_SERVICE_TRACKER;
 
 	private static ImageData checked;
 	private static ImageData unchecked;
 
+	private static Image defaultCheckedImage;
+	private static Image defaultUncheckedImage;
+
+	static {
+		BUNDLE = FrameworkUtil.getBundle(NativeWidgetHelper.class);
+		IMAGE_REGISTRY_SERVICE_TRACKER = new ServiceTracker<>(BUNDLE.getBundleContext(), ImageRegistryService.class,
+			new ImageRegistryServiceCustomizer());
+		IMAGE_REGISTRY_SERVICE_TRACKER.open();
+	}
+
 	private NativeWidgetHelper() {
 		// util
 	}
@@ -76,16 +91,14 @@ public final class NativeWidgetHelper {
 	 */
 	public static void initCheckBoxImages(Control control) {
 		try {
-			LOCK.acquire();
-		} catch (final InterruptedException ex) {
-			if (initalized) {
-				return;
+			LOCK.lock();
+			if (!initalized) {
+				createCheckBoxImage(control, true);
+				createCheckBoxImage(control, false);
+				initalized = true;
 			}
-			createCheckBoxImage(control, true);
-			createCheckBoxImage(control, false);
-			initalized = true;
 		} finally {
-			LOCK.release();
+			LOCK.unlock();
 		}
 	}
 
@@ -100,27 +113,25 @@ public final class NativeWidgetHelper {
 	 */
 	public static Image getCheckBoxImage(Control control, CheckBoxState state) {
 		try {
-			LOCK.acquire();
+			LOCK.lock();
 			switch (state) {
 
 			case checked:
 				if (checked != null) {
 					return new Image(control.getDisplay(), checked);
 				}
-				return getImage(CHECKED_DEFAULT);
+				return defaultCheckedImage;
 			case unchecked:
 				if (unchecked != null) {
 					return new Image(control.getDisplay(), unchecked);
 				}
-				return getImage(UNCHECKED_DEFAULT);
+				return defaultUncheckedImage;
 			default:
 				break;
 
 			}
-		} catch (final InterruptedException ex) {
-
 		} finally {
-			LOCK.release();
+			LOCK.unlock();
 		}
 
 		return null;
@@ -204,22 +215,6 @@ public final class NativeWidgetHelper {
 		return WS_CARBON.equals(ws) || WS_COCOA.equals(ws);
 	}
 
-	private static Image getImage(String path) {
-		final Bundle bundle = FrameworkUtil.getBundle(NativeWidgetHelper.class);
-		final Image image = getImageRegistryService().getImage(bundle, path);
-		bundle.getBundleContext().ungetService(imageRegistryServiceReference);
-		return image;
-	}
-
-	private static ImageRegistryService getImageRegistryService() {
-		final Bundle bundle = FrameworkUtil.getBundle(NativeWidgetHelper.class);
-		if (imageRegistryServiceReference == null) {
-			imageRegistryServiceReference = bundle.getBundleContext()
-				.getServiceReference(ImageRegistryService.class);
-		}
-		return bundle.getBundleContext().getService(imageRegistryServiceReference);
-	}
-
 	/**
 	 * Enum describing the state of a checkbox.
 	 *
@@ -238,4 +233,55 @@ public final class NativeWidgetHelper {
 		unchecked
 	}
 
+	/**
+	 * Service-tracker customizer that uses the image-registry service, when it is
+	 * discovered or updated, to get and cache the default checked and unchecked images.
+	 */
+	private static final class ImageRegistryServiceCustomizer
+		implements ServiceTrackerCustomizer<ImageRegistryService, ImageRegistryService> {
+
+		@Override
+		public ImageRegistryService addingService(ServiceReference<ImageRegistryService> reference) {
+			final ImageRegistryService result = BUNDLE.getBundleContext().getService(reference);
+
+			onUI(() -> updateImages(result));
+
+			return result;
+		}
+
+		@Override
+		public void modifiedService(ServiceReference<ImageRegistryService> reference, ImageRegistryService service) {
+			onUI(() -> updateImages(service));
+		}
+
+		@Override
+		public void removedService(ServiceReference<ImageRegistryService> reference, ImageRegistryService service) {
+			onUI(() -> updateImages(null));
+		}
+
+		private void onUI(Runnable action) {
+			final Display display = Display.getCurrent();
+			if (display != null) {
+				action.run();
+			} else {
+				Display.getDefault().syncExec(action);
+			}
+		}
+
+		private void updateImages(ImageRegistryService imageRegistryService) {
+			LOCK.lock();
+
+			try {
+				if (imageRegistryService != null) {
+					defaultCheckedImage = imageRegistryService.getImage(BUNDLE, CHECKED_DEFAULT);
+					defaultUncheckedImage = imageRegistryService.getImage(BUNDLE, UNCHECKED_DEFAULT);
+				} else {
+					defaultCheckedImage = null;
+					defaultUncheckedImage = null;
+				}
+			} finally {
+				LOCK.unlock();
+			}
+		}
+	}
 }
diff --git a/bundles/org.eclipse.emf.ecp.view.table.ui.nebula.grid/src/org/eclipse/emf/ecp/view/spi/table/nebula/grid/GridControlDetailPanelRenderer.java b/bundles/org.eclipse.emf.ecp.view.table.ui.nebula.grid/src/org/eclipse/emf/ecp/view/spi/table/nebula/grid/GridControlDetailPanelRenderer.java
index 41c397d62a..55e71032a9 100644
--- a/bundles/org.eclipse.emf.ecp.view.table.ui.nebula.grid/src/org/eclipse/emf/ecp/view/spi/table/nebula/grid/GridControlDetailPanelRenderer.java
+++ b/bundles/org.eclipse.emf.ecp.view.table.ui.nebula.grid/src/org/eclipse/emf/ecp/view/spi/table/nebula/grid/GridControlDetailPanelRenderer.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2011-2018 EclipseSource Muenchen GmbH and others.
+ * Copyright (c) 2011-2019 EclipseSource Muenchen GmbH and others.
  *
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
@@ -8,6 +8,7 @@
  *
  * Contributors:
  * Johannes Faltermeier - initial API and implementation
+ * Christian W. Damus - bug 545686
  ******************************************************************************/
 package org.eclipse.emf.ecp.view.spi.table.nebula.grid;
 
@@ -286,12 +287,17 @@ public class GridControlDetailPanelRenderer extends GridControlSWTRenderer {
 	 * @param selection the selection
 	 */
 	protected void handleSingleSelection(IStructuredSelection selection) {
+		// Did the selection actionally change? We may have stepped sideways in a row
+		final EObject object = (EObject) selection.getFirstElement();
+		if (ecpView != null && ecpView.getViewModelContext().getDomainModel() == object) {
+			return;
+		}
+
 		disposeDetail();
 		final Composite compositeToRenderOn = new Composite(detailPanel, SWT.NONE);
 		GridLayoutFactory.fillDefaults().numColumns(1).equalWidth(false).applyTo(compositeToRenderOn);
 		GridDataFactory.fillDefaults().grab(true, true).align(SWT.FILL, SWT.FILL).applyTo(compositeToRenderOn);
 
-		final EObject object = (EObject) selection.getFirstElement();
 		renderSelectedObject(compositeToRenderOn, object);
 		border.layout(true, true);
 		scrolledComposite.setMinSize(detailPanel.computeSize(SWT.DEFAULT, SWT.DEFAULT));
diff --git a/bundles/org.eclipse.emf.ecp.view.table.ui.swt/src/org/eclipse/emf/ecp/view/internal/table/swt/RunnableManager.java b/bundles/org.eclipse.emf.ecp.view.table.ui.swt/src/org/eclipse/emf/ecp/view/internal/table/swt/RunnableManager.java
index 3d17830366..3e8c564eab 100644
--- a/bundles/org.eclipse.emf.ecp.view.table.ui.swt/src/org/eclipse/emf/ecp/view/internal/table/swt/RunnableManager.java
+++ b/bundles/org.eclipse.emf.ecp.view.table.ui.swt/src/org/eclipse/emf/ecp/view/internal/table/swt/RunnableManager.java
@@ -8,26 +8,41 @@
  *
  * Contributors:
  * Edgar Mueller - initial API and implementation
- * Christian W. Damus - bug 544116
+ * Christian W. Damus - bugs 544116, 545686
  ******************************************************************************/
 package org.eclipse.emf.ecp.view.internal.table.swt;
 
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.locks.Condition;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 
 import org.eclipse.swt.widgets.Display;
 
 /**
+ * <p>
  * A utility class that is capable of maintaining the running
  * state of a {@link Runnable}, i.e. frequent calls of the {@link #executeAsync(Runnable)}
  * with long running {@link Runnable}s will not result in each {@link Runnable} being called.
  * This is useful, for instance, if the same {@link Runnable} is
  * submitted multiple times unnecessarily.
- *
+ * </p>
+ * <p>
+ * If a runnable additionally implements the {@link BackgroundStage} interface, then
+ * it will be invoked on a background thread and when complete will be asked to
+ * {@linkplain BackgroundStage#getNextStage() provide a subsequent stage} to execute.
+ * This may similarly be a background stage, iterating the process, or may be a terminal
+ * runnable that then is posted on the UI thread to update the UI.
+ * </p>
  */
 public class RunnableManager {
 
+	private final Lock lock = new ReentrantLock();
 	private final AtomicBoolean isRunning = new AtomicBoolean(false);
+	private final Condition runningCond = lock.newCondition();
 	private final AtomicReference<Runnable> pending = new AtomicReference<>();
 	private final Display display;
 
@@ -41,12 +56,22 @@ public class RunnableManager {
 	}
 
 	private void finish(Runnable runnable) {
-		// validation finished
-		isRunning.compareAndSet(true, false);
-		// re-trigger validation if we have a pending request
-		final Runnable next = pending.getAndSet(null);
-		if (next != null) {
-			executeAsync(next);
+		lock.lock();
+
+		try {
+			// task has finished
+			isRunning.compareAndSet(true, false);
+
+			// Running state has changed
+			runningCond.signalAll();
+
+			// trigger next task if we have a pending request
+			final Runnable next = pending.getAndSet(null);
+			if (next != null) {
+				executeAsync(next);
+			}
+		} finally {
+			lock.unlock();
 		}
 	}
 
@@ -69,10 +94,27 @@ public class RunnableManager {
 	 * @param runnable the {@link Runnable} to be executed asynchronously
 	 */
 	public void executeAsync(final Runnable runnable) {
-		if (isRunning.compareAndSet(false, true)) {
-			getDisplay().asyncExec(createWrapperRunnable(runnable));
+		lock.lock();
+
+		try {
+			if (isRunning.compareAndSet(false, true)) {
+				doExecuteAsync(runnable);
+
+				// Running state has changed
+				runningCond.signalAll();
+			} else {
+				pending.compareAndSet(null, runnable);
+			}
+		} finally {
+			lock.unlock();
+		}
+	}
+
+	private void doExecuteAsync(final Runnable runnable) {
+		if (runnable instanceof BackgroundStage) {
+			runStage(runnable, (BackgroundStage) runnable);
 		} else {
-			pending.compareAndSet(null, runnable);
+			getDisplay().asyncExec(createWrapperRunnable(runnable));
 		}
 	}
 
@@ -95,4 +137,157 @@ public class RunnableManager {
 		return isRunning.get();
 	}
 
+	/**
+	 * Wait until no task is running. Returns immediately if there is currently
+	 * no task running.
+	 *
+	 * @throws InterruptedException if interrupted while waiting
+	 * @since 1.21
+	 */
+	public final void waitForIdle() throws InterruptedException {
+		if (!isRunning()) {
+			// Short-circuit
+			return;
+		}
+
+		final boolean busyWait = Display.getCurrent() == display;
+
+		lock.lock();
+
+		try {
+			out: while (isRunning()) {
+				if (busyWait) {
+					try {
+						lock.unlock();
+
+						// We can only finish on the display thread, which is
+						// performed by a runnable in this queue
+						do {
+							if (!isRunning()) {
+								break out;
+							}
+						} while (display.readAndDispatch());
+
+						display.sleep();
+					} finally {
+						lock.lock();
+					}
+				} else {
+					runningCond.await();
+				}
+			}
+		} finally {
+			lock.unlock();
+		}
+	}
+
+	/**
+	 * Wait until no task is running or the given {@code time} elapses.
+	 * Returns immediately if there is currently no task running. Note that if called on the
+	 * UI thread, then the actual wait time in case of time-out can be longer than requested
+	 * because of UI event queue processing.
+	 *
+	 * @param time the amount of time to wait, or a non-positive amount to wait indefinitely
+	 *            as in {@link #waitForIdle()}
+	 * @param unit the unit of measure of the {@code time} to wait
+	 * @return {@code true} if on return there is no task running; {@code false} on time-out
+	 *         (which does not mean that since the time-out occurred the manager did not become idle)
+	 *
+	 * @throws InterruptedException if interrupted while waiting
+	 * @since 1.21
+	 */
+	public final boolean waitForIdle(long time, TimeUnit unit) throws InterruptedException {
+		if (time <= 0L) {
+			waitForIdle();
+			return true;
+		}
+
+		final long deadline = System.nanoTime() + unit.toNanos(time);
+
+		if (!isRunning()) {
+			// Short-circuit
+			return true;
+		}
+
+		final boolean busyWait = Display.getCurrent() == display;
+
+		lock.lock();
+
+		try {
+			out: while (isRunning()) {
+				final long remaining = deadline - System.nanoTime();
+				if (remaining <= 0L) {
+					return false;
+				}
+
+				if (busyWait) {
+					try {
+						lock.unlock();
+
+						// We can only finish on the display thread, which is
+						// performed by a runnable in this queue
+						do {
+							if (!isRunning()) {
+								break out;
+							}
+						} while (display.readAndDispatch());
+					} finally {
+						lock.lock();
+					}
+
+					runningCond.await(50L, TimeUnit.MILLISECONDS);
+				} else {
+					runningCond.awaitNanos(remaining);
+				}
+			}
+		} finally {
+			lock.unlock();
+		}
+
+		return true;
+	}
+
+	private void runStage(Runnable computation, BackgroundStage stage) {
+		CompletableFuture.runAsync(computation)
+			.whenComplete((result, exception) -> {
+				if (exception != null) {
+					// Computation failed. Just finish
+					finish(computation);
+					Activator.getInstance().log(exception);
+				} else {
+					// Send the next stage
+					final Runnable next = stage.getNextStage();
+					if (next == null) {
+						// Okay, then. Just finish
+						finish(computation);
+					} else {
+						doExecuteAsync(next);
+					}
+				}
+			});
+	}
+
+	//
+	// Nested types
+	//
+
+	/**
+	 * An optional mix-in interface for a {@link Runnable} scheduled on the
+	 * {@link RunnableManager} that should be run in a background thread and which
+	 * produces a subsequent stage for further execution.
+	 *
+	 * @since 1.21
+	 */
+	public interface BackgroundStage {
+		/**
+		 * Provides the next stage of computation. If the result is another
+		 * {@code BackgroundStage}, then it, too, will run in the background.
+		 * The final stage is some {@link Runnable} that is not a {@code BackgroundStage}
+		 * which then is posted on the display thread to update the UI.
+		 *
+		 * @return the next stage of computation/update
+		 */
+		Runnable getNextStage();
+	}
+
 }
diff --git a/bundles/org.eclipse.emf.ecp.view.table.ui.swt/src/org/eclipse/emf/ecp/view/spi/table/swt/TableControlSWTRenderer.java b/bundles/org.eclipse.emf.ecp.view.table.ui.swt/src/org/eclipse/emf/ecp/view/spi/table/swt/TableControlSWTRenderer.java
index a0e3c2831e..0185987783 100644
--- a/bundles/org.eclipse.emf.ecp.view.table.ui.swt/src/org/eclipse/emf/ecp/view/spi/table/swt/TableControlSWTRenderer.java
+++ b/bundles/org.eclipse.emf.ecp.view.table.ui.swt/src/org/eclipse/emf/ecp/view/spi/table/swt/TableControlSWTRenderer.java
@@ -9,25 +9,25 @@
  * Contributors:
  * Eugen Neufeld - initial API and implementation
  * Johannes Faltermeier - refactorings
- * Christian W. Damus - bugs 544116, 544537
+ * Christian W. Damus - bugs 544116, 544537, 545686
  ******************************************************************************/
 package org.eclipse.emf.ecp.view.spi.table.swt;
 
-import static java.util.stream.Collectors.toCollection;
-
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.SortedSet;
+import java.util.TreeSet;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 import javax.inject.Inject;
 
@@ -1491,36 +1491,26 @@ public class TableControlSWTRenderer extends AbstractControlSWTRenderer<VTableCo
 
 	@Override
 	protected void applyValidation(VDiagnostic oldDiagnostic, VDiagnostic newDiagnostic) {
-		final Stream<Object> oldDiagnostics = oldDiagnostic == null ? Stream.empty()
-			: oldDiagnostic.getDiagnostics().stream();
-		final Stream<Object> newDiagnostics = newDiagnostic == null ? Stream.empty()
-			: newDiagnostic.getDiagnostics().stream();
-
-		final Set<Object> updates = Stream.concat(oldDiagnostics, newDiagnostics)
-			.map(this::getSubject).filter(Objects::nonNull).collect(toCollection(LinkedHashSet::new));
-
-		runnableManager.executeAsync(new ApplyValidationRunnable(updates));
-	}
-
-	private Object getSubject(Object diagnostic) {
-		Object result = null;
-
-		if (diagnostic instanceof Diagnostic) {
-			final List<?> data = ((Diagnostic) diagnostic).getData();
-			result = data.isEmpty() ? null : data.get(0);
-		}
-
-		return result;
+		getRunnableManager().executeAsync(new ComputeValidationUpdate(oldDiagnostic, newDiagnostic));
 	}
 
 	@Override
 	protected void applyValidation() {
-		if (!runnableManager.isRunning()) {
-			runnableManager.executeAsync(new ApplyValidationRunnable());
+		if (!getRunnableManager().isRunning()) {
+			getRunnableManager().executeAsync(new ApplyValidationRunnable());
 		}
 	}
 
 	/**
+	 * Obtain my runnable manager (visible for testability).
+	 *
+	 * @return my runnable manager
+	 */
+	final RunnableManager getRunnableManager() {
+		return runnableManager;
+	}
+
+	/**
 	 * Returns the add button created by the framework.
 	 *
 	 * @deprecated use {@link #getControlForAction(String)} instead
@@ -1999,6 +1989,149 @@ public class TableControlSWTRenderer extends AbstractControlSWTRenderer<VTableCo
 	}
 
 	/**
+	 * A two-stage asynchronous task that first, in the background, computes the objects
+	 * that have actually seen validation changes and then post the viewer update
+	 * on the UI thread.
+	 */
+	private final class ComputeValidationUpdate implements Runnable, RunnableManager.BackgroundStage {
+		private final Set<Diagnostic> oldDiagnostics;
+		private final Set<Diagnostic> newDiagnostics;
+
+		private Runnable update;
+
+		/**
+		 * Initializes me with the old and new diagnostics from which to compute the objects
+		 * that need to be updated in my viewer.
+		 *
+		 * @param oldDiagnostic the old validation state
+		 * @param newDiagnostic the new validation state
+		 */
+		ComputeValidationUpdate(VDiagnostic oldDiagnostic, VDiagnostic newDiagnostic) {
+			super();
+
+			// These have to be extracted on the calling thread because the ELists
+			// are not thread-safe
+			oldDiagnostics = getDiagnostics(oldDiagnostic);
+			newDiagnostics = getDiagnostics(newDiagnostic);
+		}
+
+		@Override
+		public Runnable getNextStage() {
+			return update;
+		}
+
+		@Override
+		public void run() {
+			// Compute the difference between the diagnostics to find what actually
+			// are the logical changes in validation that will need updates in the UI
+			final Set<Diagnostic> difference = difference(oldDiagnostics, newDiagnostics);
+			final Set<Object> updates = difference.stream().map(this::getSubject).filter(Objects::nonNull)
+				.collect(Collectors.toSet());
+			if (!updates.isEmpty()) {
+				update = new ApplyValidationRunnable(updates);
+			} // Otherwise we don't need the UI update stage
+		}
+
+		private Set<Diagnostic> getDiagnostics(VDiagnostic container) {
+			return container == null ? Collections.emptySet()
+				: container.getDiagnostics().stream()
+					.filter(Diagnostic.class::isInstance).map(Diagnostic.class::cast)
+					.collect(Collectors.toSet());
+		}
+
+		private Object getSubject(Diagnostic diagnostic) {
+			final List<?> data = diagnostic.getData();
+			return data.isEmpty() ? null : data.get(0);
+		}
+
+		private Set<Diagnostic> difference(Set<Diagnostic> set1, Set<Diagnostic> set2) {
+			// Diagnostics do not implement equals(), so we have to do it for them.
+			// Most straightforward approach is to use a tree set, which uses the
+			// comparator's zero result to test equality
+			final SortedSet<Diagnostic> sorted1 = new TreeSet<>(this::compare);
+			sorted1.addAll(set1);
+			final SortedSet<Diagnostic> sorted2 = new TreeSet<>(this::compare);
+			sorted2.addAll(set2);
+
+			// Difference each side
+			sorted1.removeAll(set2);
+			sorted2.removeAll(set1);
+
+			// And the union of that
+			sorted1.addAll(sorted2);
+
+			return sorted1;
+		}
+
+		private int compare(Diagnostic d1, Diagnostic d2) {
+			int result = d1.getSeverity() - d2.getSeverity();
+			if (result != 0) {
+				return result;
+			}
+
+			result = d1.getCode() - d2.getCode();
+			if (result != 0) {
+				return result;
+			}
+
+			result = compare(d1.getSource(), d2.getSource());
+			if (result != 0) {
+				return result;
+			}
+
+			result = compare(d1.getMessage(), d2.getMessage());
+			if (result != 0) {
+				return result;
+			}
+
+			result = compare(d1.getData(), d2.getData());
+
+			// The diagnostics from validation should not have exceptions and
+			// the children are immaterial because we've already flattened them
+
+			return result;
+		}
+
+		private int compare(String s1, String s2) {
+			if (s1 == null) {
+				return s2 == null ? 0 : -1;
+			}
+			if (s2 == null) {
+				return +1;
+			}
+			return s1.compareTo(s2);
+		}
+
+		private int compare(List<?> s1, List<?> s2) {
+			if (s1 == null) {
+				return s2 == null ? 0 : -1;
+			}
+			if (s2 == null) {
+				return +1;
+			}
+
+			final int size = s1.size();
+			int result = size - s2.size();
+			if (result != 0) {
+				return result;
+			}
+
+			int i = 0;
+			for (i = 0; i < size; i++) {
+				final Object e1 = s1.get(i);
+				final Object e2 = s2.get(i);
+				if (e1 != e2) {
+					// Arbitrary but stable order
+					result = System.identityHashCode(e1) - System.identityHashCode(e2);
+					break;
+				}
+			}
+
+			return result;
+		}
+	}
+
+	/**
 	 * Runnable which is called by {@link TableControlSWTRenderer#applyValidation() applyValidation}.
 	 *
 	 */
diff --git a/bundles/org.eclipse.emfforms.common.validation/src/org/eclipse/emfforms/common/internal/validation/DiagnosticHelper.java b/bundles/org.eclipse.emfforms.common.validation/src/org/eclipse/emfforms/common/internal/validation/DiagnosticHelper.java
index 00e248070f..288cbdb7a9 100644
--- a/bundles/org.eclipse.emfforms.common.validation/src/org/eclipse/emfforms/common/internal/validation/DiagnosticHelper.java
+++ b/bundles/org.eclipse.emfforms.common.validation/src/org/eclipse/emfforms/common/internal/validation/DiagnosticHelper.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2017 EclipseSource Muenchen GmbH and others.
+ * Copyright (c) 2017-2019 EclipseSource Muenchen GmbH and others.
  *
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
@@ -8,6 +8,7 @@
  *
  * Contributors:
  * Mat Hansen - initial API and implementation
+ * Christian W. Damus - bug 545686
  ******************************************************************************/
 package org.eclipse.emfforms.common.internal.validation;
 
@@ -35,15 +36,25 @@ public final class DiagnosticHelper {
 	 * @return the found {@link EStructuralFeature}, null if no {@link EStructuralFeature} is found
 	 */
 	public static EStructuralFeature getEStructuralFeature(List<?> data) {
-		if (data == null || data.isEmpty()) {
+		// Exclude first object for cases when we validate an EStructuralFeature.
+		if (data == null || data.size() < 2) {
 			return null;
 		}
-		// Exclude first object for cases when we validate an EStructuralFeature.
-		for (final Object object : data.subList(1, data.size())) {
-			if (EStructuralFeature.class.isInstance(object)) {
-				return EStructuralFeature.class.cast(object);
+
+		// The usual case is that the structural feature is the second object.
+		final Object usual = data.get(1);
+		if (usual instanceof EStructuralFeature) {
+			return (EStructuralFeature) usual;
+		}
+
+		if (data.size() > 2) {
+			for (final Object object : data.subList(2, data.size())) {
+				if (EStructuralFeature.class.isInstance(object)) {
+					return EStructuralFeature.class.cast(object);
+				}
 			}
 		}
+
 		return null;
 	}
 
@@ -54,11 +65,24 @@ public final class DiagnosticHelper {
 	 * @return the found {@link InternalEObject}, null if no {@link InternalEObject} is found
 	 */
 	public static InternalEObject getFirstInternalEObject(List<?> data) {
-		for (final Object object : data) {
-			if (InternalEObject.class.isInstance(object)) {
-				return InternalEObject.class.cast(object);
+		if (data == null || data.isEmpty()) {
+			return null;
+		}
+
+		// The usual case is that the subject of the problem is the first object
+		final Object usual = data.get(0);
+		if (usual instanceof InternalEObject) {
+			return (InternalEObject) usual;
+		}
+
+		if (data.size() > 1) {
+			for (final Object object : data) {
+				if (InternalEObject.class.isInstance(object)) {
+					return InternalEObject.class.cast(object);
+				}
 			}
 		}
+
 		return null;
 	}
 
diff --git a/tests/org.eclipse.emf.ecp.view.table.ui.swt.test/src/org/eclipse/emf/ecp/view/spi/table/swt/SWTTable_PTest.java b/tests/org.eclipse.emf.ecp.view.table.ui.swt.test/src/org/eclipse/emf/ecp/view/spi/table/swt/SWTTable_PTest.java
index 6e6a2f7170..477fc4fadb 100644
--- a/tests/org.eclipse.emf.ecp.view.table.ui.swt.test/src/org/eclipse/emf/ecp/view/spi/table/swt/SWTTable_PTest.java
+++ b/tests/org.eclipse.emf.ecp.view.table.ui.swt.test/src/org/eclipse/emf/ecp/view/spi/table/swt/SWTTable_PTest.java
@@ -8,7 +8,7 @@
  *
  * Contributors:
  * Johannes Faltermeier
- * Christian W. Damus - bugs 527740, 544116
+ * Christian W. Damus - bugs 527740, 544116, 545686
  *
  *******************************************************************************/
 package org.eclipse.emf.ecp.view.spi.table.swt;
@@ -32,13 +32,13 @@ import java.io.ByteArrayOutputStream;
 import java.io.PrintStream;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
-import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashSet;
 import java.util.Hashtable;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
 
 import javax.inject.Inject;
 
@@ -1056,7 +1056,7 @@ public class SWTTable_PTest {
 
 		// render
 		final ViewModelContext context = new ViewModelContextImpl(tableControl, domainElement);
-		final List<String> requestedCells = new ArrayList<>();
+		final Set<String> requestedCells = new HashSet<>(); // Cell updates are unordered
 		final TableControlSWTRenderer tableRenderer = new TableControlSWTRenderer(tableControl, context,
 			context.getService(ReportService.class),
 			context.getService(EMFFormsDatabindingEMF.class),
@@ -1112,10 +1112,10 @@ public class SWTTable_PTest {
 		vdiag.getDiagnostics().add(diag);
 		tableControl.setDiagnostic(vdiag);
 
-		SWTTestUtil.waitForUIThread();
+		waitFor(tableRenderer);
 
 		// Notably, we updated these two rows in order and *neither* "a" nor "d"
-		assertThat(requestedCells, equalTo(Arrays.asList("b", "c")));
+		assertThat(requestedCells, equalTo(set("b", "c")));
 	}
 
 	@Test
@@ -1147,6 +1147,27 @@ public class SWTTable_PTest {
 		registration.unregister();
 	}
 
+	@SafeVarargs
+	static <T> Set<T> set(T... elements) {
+		return new HashSet<>(Arrays.asList(elements));
+	}
+
+	/**
+	 * Wait for any pending updates in a table renderer to be completed.
+	 *
+	 * @param tableRenderer a table renderer to wait for
+	 */
+	final void waitFor(TableControlSWTRenderer tableRenderer) {
+		try {
+			if (!tableRenderer.getRunnableManager().waitForIdle(1L, TimeUnit.SECONDS)) {
+				fail("Timed out waiting for table updates");
+			}
+		} catch (final InterruptedException e) {
+			fail("Interrupted waiting for table updates");
+		}
+
+	}
+
 	private static class ColumnImageTableRenderer extends TableControlSWTRenderer {
 		// BEGIN COMPLEX CODE
 		@Inject
diff --git a/tests/org.eclipse.emf.ecp.view.table.ui.swt.test/src/org/eclipse/emf/ecp/view/table/ui/swt/test/RunnableManagerTest.java b/tests/org.eclipse.emf.ecp.view.table.ui.swt.test/src/org/eclipse/emf/ecp/view/table/ui/swt/test/RunnableManagerTest.java
index 489a95e9f4..761fdf24cb 100644
--- a/tests/org.eclipse.emf.ecp.view.table.ui.swt.test/src/org/eclipse/emf/ecp/view/table/ui/swt/test/RunnableManagerTest.java
+++ b/tests/org.eclipse.emf.ecp.view.table.ui.swt.test/src/org/eclipse/emf/ecp/view/table/ui/swt/test/RunnableManagerTest.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2011-2017 EclipseSource Muenchen GmbH and others.
+ * Copyright (c) 2011-2019 EclipseSource Muenchen GmbH and others.
  *
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
@@ -8,10 +8,17 @@
  *
  * Contributors:
  * edgar - initial API and implementation
+ * Christian W. Damus - bug 545686
  ******************************************************************************/
 package org.eclipse.emf.ecp.view.table.ui.swt.test;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.greaterThanOrEqualTo;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.lessThanOrEqualTo;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
@@ -19,10 +26,13 @@ import static org.mockito.Mockito.mock;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import org.eclipse.emf.ecp.view.internal.table.swt.RunnableManager;
+import org.eclipse.emf.ecp.view.internal.table.swt.RunnableManager.BackgroundStage;
 import org.eclipse.swt.widgets.Display;
+import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.mockito.invocation.InvocationOnMock;
@@ -60,6 +70,12 @@ public class RunnableManagerTest {
 
 	}
 
+	@AfterClass
+	public static void afterClass() {
+		ASYNC_POOL.shutdown();
+		POOL.shutdown();
+	}
+
 	@Test
 	public void managedRunnable() throws InterruptedException {
 		// setup
@@ -81,4 +97,141 @@ public class RunnableManagerTest {
 		assertTrue(counter.intValue() > asyncCounter.intValue());
 	}
 
+	@Test
+	public void backgroundStage() throws InterruptedException {
+		final AtomicInteger stageCount = new AtomicInteger();
+		final Stage stage = new Stage(stageCount::incrementAndGet);
+
+		final RunnableManager runnableManager = new RunnableManager(DISPLAY);
+
+		runnableManager.executeAsync(stage);
+		stage.join();
+
+		assertThat("Multi-step execution not completed", stageCount.get(), greaterThanOrEqualTo(3));
+	}
+
+	@Test
+	public void waitForIdle() throws InterruptedException {
+		final AtomicInteger countdown = new AtomicInteger(Stage.STAGE_COUNT);
+		final Runnable callback = () -> {
+			try {
+				Thread.sleep(100L);
+			} catch (final InterruptedException e) {
+				fail("Call-back interrupted");
+			}
+			countdown.decrementAndGet();
+		};
+
+		final Stage stage = new Stage(callback);
+
+		final RunnableManager runnableManager = new RunnableManager(DISPLAY);
+
+		runnableManager.executeAsync(stage);
+
+		runnableManager.waitForIdle();
+
+		assertThat("Wait returned early", countdown.get(), lessThanOrEqualTo(0));
+	}
+
+	@Test
+	public void waitForIdle_time() throws InterruptedException {
+		final AtomicInteger countdown = new AtomicInteger(Stage.STAGE_COUNT);
+		final Runnable callback = () -> {
+			try {
+				Thread.sleep(100L);
+			} catch (final InterruptedException e) {
+				fail("Call-back interrupted");
+			}
+			countdown.decrementAndGet();
+		};
+
+		final Stage stage = new Stage(callback);
+
+		final RunnableManager runnableManager = new RunnableManager(DISPLAY);
+
+		runnableManager.executeAsync(stage);
+
+		assertThat("Timed out", runnableManager.waitForIdle(5L, TimeUnit.SECONDS), is(true));
+
+		assertThat("Wait returned early", countdown.get(), lessThanOrEqualTo(0));
+	}
+
+	@Test
+	public void waitForIdle_timeout() throws InterruptedException {
+		final AtomicInteger countdown = new AtomicInteger(Stage.STAGE_COUNT);
+		final Runnable callback = () -> {
+			try {
+				Thread.sleep(1000L);
+			} catch (final InterruptedException e) {
+				fail("Call-back interrupted");
+			}
+			countdown.decrementAndGet();
+		};
+
+		final Stage stage = new Stage(callback);
+
+		final RunnableManager runnableManager = new RunnableManager(DISPLAY);
+
+		runnableManager.executeAsync(stage);
+
+		assertThat("Not timed out", runnableManager.waitForIdle(1L, TimeUnit.SECONDS), is(false));
+
+		assertThat("Task completed", countdown.get(), greaterThan(0));
+	}
+
+	//
+	// Nested types
+	//
+
+	private static class Stage implements Runnable, BackgroundStage {
+		static final int STAGE_COUNT = 3;
+
+		private final Runnable callback;
+
+		private int backgroundStages = STAGE_COUNT - 1;
+		private Runnable next;
+
+		private final CountDownLatch done = new CountDownLatch(1);
+
+		Stage(Runnable callback) {
+			super();
+
+			this.callback = callback;
+		}
+
+		@Override
+		public void run() {
+			// This is a stage
+			try {
+				callback.run();
+			} finally {
+				// This is a background stage
+				backgroundStages--;
+
+				if (backgroundStages <= 0) {
+					// Last stage is foreground
+					next = () -> {
+						try {
+							callback.run();
+						} finally {
+							done.countDown();
+						}
+					};
+				} else {
+					// Next stage is background
+					next = this;
+				}
+			}
+		}
+
+		@Override
+		public Runnable getNextStage() {
+			return next;
+		}
+
+		void join() throws InterruptedException {
+			done.await();
+		}
+	}
+
 }