Do not use the lightweight decorator in the staging view

This reverts commit a1a7ffd2c2467b4033b83307903a9bf54f560348.

Keep the simplifications in the StagingViewLabelProvider. Change the
ColorsAndFonts cache to track color and font changes itself (instead
of having the GitLightweightDecorator do so). Give the staging view's
label provider such a ChangeTrackingColorsAndFonts.

In the staging view, update buttons and messages only in response to
events from our own ProblemLabelDecorator; we're not interested in
other events there.

The staging view cannot use the GitLightweightDecorator because it
doesn't decorate entries for non-workspace files, and making it do so
leads to double decorations on entries that _do_ have resources. Also,
going via IResource when a StagingEntry has all the needed information
ready is less efficient. For staging entries, decorating on the UI
thread is actually feasible.

Bug: 565990
Change-Id: Ic32682334884a75a6e52a33039dc72cac464e0fd
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

3 files changed, 180 insertions, 45 deletions

diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/decorators/GitLightweightDecorator.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/decorators/GitLightweightDecorator.java
index 9472e58f59..4d1054fdc7 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/decorators/GitLightweightDecorator.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/decorators/GitLightweightDecorator.java
@@ -29,6 +29,7 @@ import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 import org.eclipse.core.resources.IResource;
 import org.eclipse.core.resources.ResourcesPlugin;
@@ -37,6 +38,7 @@ import org.eclipse.core.runtime.Adapters;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IAdaptable;
 import org.eclipse.core.runtime.IStatus;
+import org.eclipse.egit.core.internal.SafeRunnable;
 import org.eclipse.egit.core.internal.indexdiff.IndexDiffData;
 import org.eclipse.egit.core.internal.util.ExceptionCollector;
 import org.eclipse.egit.core.project.GitProjectData;
@@ -108,7 +110,7 @@ public class GitLightweightDecorator extends GitDecorator
 			UIPreferences.THEME_IgnoredResourceBackgroundColor,
 			UIPreferences.THEME_IgnoredResourceForegroundColor);
 
-	private final ReloadableColorsAndFonts resources;
+	private final ChangeTrackingColorsAndFonts resources;
 
 	private final DecorationHelper helper;
 
@@ -120,14 +122,12 @@ public class GitLightweightDecorator extends GitDecorator
 	public GitLightweightDecorator() {
 		// This is an optimization to ensure that while decorating our fonts and
 		// colors are pre-created and decoration can occur without having to syncExec.
-		resources = new ReloadableColorsAndFonts();
-		resources.reload();
+		resources = new ChangeTrackingColorsAndFonts();
 		helper = new DecorationHelper(
 				Activator.getDefault().getPreferenceStore(), resources);
+		resources.addListener(this::postLabelEvent);
 		TeamUI.addPropertyChangeListener(this);
 		Activator.addPropertyChangeListener(this);
-		PlatformUI.getWorkbench().getThemeManager().getCurrentTheme()
-				.addPropertyChangeListener(this);
 
 		GitProjectData.addRepositoryChangeListener(mappingChangeListener);
 	}
@@ -135,8 +135,7 @@ public class GitLightweightDecorator extends GitDecorator
 	@Override
 	public void dispose() {
 		super.dispose();
-		PlatformUI.getWorkbench().getThemeManager().getCurrentTheme()
-				.removePropertyChangeListener(this);
+		resources.dispose();
 		TeamUI.removePropertyChangeListener(this);
 		Activator.removePropertyChangeListener(this);
 		GitProjectData.removeRepositoryChangeListener(mappingChangeListener);
@@ -245,36 +244,95 @@ public class GitLightweightDecorator extends GitDecorator
 		helper.decorate(decoration, decoRes);
 	}
 
-	private static interface ColorsAndFonts {
+	/**
+	 * A repository of colors and fonts.
+	 */
+	public static interface ColorsAndFonts {
 
+		/**
+		 * Retrieves the {@link Color} identified by {@code id}.
+		 *
+		 * @param id
+		 *            of the color to get
+		 * @return the color, or {@code null} if none
+		 */
 		Color getColor(String id);
 
+		/**
+		 * Retrieves the {@link Font} identified by {@code id}.
+		 *
+		 * @param id
+		 *            of the font to get
+		 * @return the font, or {@code null} if none
+		 */
 		Font getFont(String id);
 
+		/**
+		 * Retrieves the default font.
+		 *
+		 * @return the font
+		 */
 		Font getDefaultFont();
 
+		/**
+		 * Retrieves the default background color.
+		 *
+		 * @return the {@link RGB} of the color
+		 */
 		RGB getDefaultBackground();
 
 	}
 
 	/**
-	 * Our own private cache of current colors and fonts. Do not rely on the
-	 * platform's current theme caching them: when a theme switch occurs, the
-	 * decorator running asynchronously in the background may get not only wrong
-	 * but even disposed values, and somehow it never really worked for the
-	 * default font. The main idea behind caching colors and fonts is to do load
-	 * them on the UI thread once, and then access them from the decorators
-	 * background thread. Otherwise the decorator would need to syncExec(),
-	 * which kind of obviates running in the background.
+	 * A font and color cache that tracks changes to the fonts and colors used
+	 * by Git decoration and that fires a notification when they have changed.
+	 * <p>
+	 * The main idea behind caching colors and fonts is to do load them on the
+	 * UI thread once, and then access them from the decorators background
+	 * thread. Otherwise the decorator would need to syncExec(), which kind of
+	 * obviates running in the background.
+	 * </p>
 	 */
-	private static class ReloadableColorsAndFonts implements ColorsAndFonts {
+	public static class ChangeTrackingColorsAndFonts implements ColorsAndFonts {
+
+		private final CopyOnWriteArrayList<Runnable> listeners = new CopyOnWriteArrayList<>();
+
+		private final IPropertyChangeListener themeListener = event -> {
+			final String prop = event.getProperty();
+			if (prop == null) {
+				return;
+			}
+			switch (prop) {
+			case IThemeManager.CHANGE_CURRENT_THEME:
+			case TREE_TABLE_FONT:
+			case UIPreferences.THEME_UncommittedChangeBackgroundColor:
+			case UIPreferences.THEME_UncommittedChangeFont:
+			case UIPreferences.THEME_UncommittedChangeForegroundColor:
+			case UIPreferences.THEME_IgnoredResourceFont:
+			case UIPreferences.THEME_IgnoredResourceBackgroundColor:
+			case UIPreferences.THEME_IgnoredResourceForegroundColor:
+				reload();
+				break;
+			default:
+				break;
+			}
+		};
 
 		private volatile Map<String, Object> colorsOrFonts = new HashMap<>();
 
 		private volatile RGB defaultBackground;
 
-		public void reload() {
-			final Display display = PlatformUI.getWorkbench().getDisplay();
+		/**
+		 * Creates a new {@link ChangeTrackingColorsAndFonts} object.
+		 */
+		public ChangeTrackingColorsAndFonts() {
+			PlatformUI.getWorkbench().getThemeManager().getCurrentTheme()
+					.addPropertyChangeListener(themeListener);
+			reload();
+		}
+
+		private void reload() {
+			Display display = PlatformUI.getWorkbench().getDisplay();
 			display.syncExec(() -> {
 				Map<String, Object> newResources = new HashMap<>();
 				ITheme theme = PlatformUI.getWorkbench().getThemeManager()
@@ -293,6 +351,45 @@ public class GitLightweightDecorator extends GitDecorator
 				defaultBackground = display
 						.getSystemColor(SWT.COLOR_LIST_BACKGROUND).getRGB();
 			});
+			notifyListeners();
+		}
+
+		private void notifyListeners() {
+			for (Runnable listener : listeners) {
+				SafeRunnable.run(listener::run);
+			}
+		}
+
+		/**
+		 * Register a listener to be called when the colors or fonts have
+		 * changed. A no-op if the listener is already registered.
+		 *
+		 * @param runnable
+		 *            to invoke
+		 */
+		public void addListener(Runnable runnable) {
+			listeners.addIfAbsent(runnable);
+		}
+
+		/**
+		 * Unregister a previously registered listener. A no-op if the listener
+		 * is not currently registered.
+		 *
+		 * @param runnable
+		 *            to unregister
+		 */
+		public void removeListener(Runnable runnable) {
+			listeners.remove(runnable);
+		}
+
+		/**
+		 * Disposes internal resources, removes all listeners, and stops
+		 * reacting on color or font changes.
+		 */
+		public void dispose() {
+			listeners.clear();
+			PlatformUI.getWorkbench().getThemeManager().getCurrentTheme()
+					.removePropertyChangeListener(themeListener);
 		}
 
 		@Override
@@ -482,16 +579,15 @@ public class GitLightweightDecorator extends GitDecorator
 		}
 
 		/**
-		 * Internal constructor; also used by the
-		 * {@link GitLightweightDecorator} to provide a {@link ColorsAndFonts}
-		 * registry that reacts to color, font, or theme changes.
+		 * Creates a new {@link DecorationHelper} based on the given
+		 * {@link IPreferenceStore} using the given {@link ColorsAndFonts}.
 		 *
 		 * @param preferencesStore
 		 *            to get decoration settings from
 		 * @param resources
 		 *            to get colors and fonts from
 		 */
-		private DecorationHelper(IPreferenceStore preferencesStore,
+		public DecorationHelper(IPreferenceStore preferencesStore,
 				ColorsAndFonts resources) {
 			store = preferencesStore;
 			this.resources = resources;
@@ -796,17 +892,6 @@ public class GitLightweightDecorator extends GitDecorator
 		case Activator.DECORATORS_CHANGED:
 			postLabelEvent();
 			break;
-		case IThemeManager.CHANGE_CURRENT_THEME:
-		case TREE_TABLE_FONT:
-		case UIPreferences.THEME_UncommittedChangeBackgroundColor:
-		case UIPreferences.THEME_UncommittedChangeFont:
-		case UIPreferences.THEME_UncommittedChangeForegroundColor:
-		case UIPreferences.THEME_IgnoredResourceFont:
-		case UIPreferences.THEME_IgnoredResourceBackgroundColor:
-		case UIPreferences.THEME_IgnoredResourceForegroundColor:
-			resources.reload();
-			postLabelEvent();
-			break;
 		default:
 			break;
 		}
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/staging/StagingView.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/staging/StagingView.java
index 10a459ed6f..0ae2107150 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/staging/StagingView.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/staging/StagingView.java
@@ -584,10 +584,7 @@ public class StagingView extends ViewPart
 		public TreeDecoratingLabelProvider(StagingViewLabelProvider base,
 				ILabelDecorator decorator) {
 			this.base = base;
-			this.provider = new DecoratingLabelProvider(
-					new DecoratingLabelProvider(base, decorator),
-					PlatformUI.getWorkbench().getDecoratorManager()
-							.getLabelDecorator());
+			this.provider = new DecoratingLabelProvider(base, decorator);
 		}
 
 		public StagingViewLabelProvider getBaseLabelProvider() {
@@ -1203,8 +1200,12 @@ public class StagingView extends ViewPart
 		stagedViewer = createViewer(stagedComposite, false,
 				selection -> stage(selection.toList()), unstageAction);
 		stagedViewer.getLabelProvider().addListener(event -> {
-			updateMessage();
-			updateCommitButtons();
+			if (event.getSource() instanceof ProblemLabelDecorator
+					&& !isDisposed()) {
+				// Update UI when problem markers on staged items change
+				updateMessage();
+				updateCommitButtons();
+			}
 		});
 		stagedViewer.addSelectionChangedListener(event -> {
 			boolean hasSelection = !event.getSelection().isEmpty();
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/staging/StagingViewLabelProvider.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/staging/StagingViewLabelProvider.java
index 276e8170e2..4d82722334 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/staging/StagingViewLabelProvider.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/staging/StagingViewLabelProvider.java
@@ -14,17 +14,24 @@ package org.eclipse.egit.ui.internal.staging;
 
 import org.eclipse.core.runtime.IPath;
 import org.eclipse.core.runtime.Path;
+import org.eclipse.egit.ui.Activator;
 import org.eclipse.egit.ui.UIUtils;
 import org.eclipse.egit.ui.internal.UIIcons;
+import org.eclipse.egit.ui.internal.decorators.DecorationResult;
+import org.eclipse.egit.ui.internal.decorators.GitLightweightDecorator.ChangeTrackingColorsAndFonts;
+import org.eclipse.egit.ui.internal.decorators.GitLightweightDecorator.DecorationHelper;
 import org.eclipse.egit.ui.internal.staging.StagingView.Presentation;
+import org.eclipse.jface.resource.ImageDescriptor;
 import org.eclipse.jface.resource.JFaceResources;
 import org.eclipse.jface.resource.LocalResourceManager;
 import org.eclipse.jface.resource.ResourceManager;
 import org.eclipse.jface.viewers.DecorationOverlayIcon;
 import org.eclipse.jface.viewers.IDecoration;
 import org.eclipse.jface.viewers.LabelProvider;
+import org.eclipse.jface.viewers.LabelProviderChangedEvent;
 import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.swt.graphics.Image;
+import org.eclipse.swt.widgets.Display;
 import org.eclipse.swt.widgets.TreeItem;
 import org.eclipse.ui.ISharedImages;
 import org.eclipse.ui.PlatformUI;
@@ -45,6 +52,10 @@ public class StagingViewLabelProvider extends LabelProvider {
 	private ResourceManager resourceManager = new LocalResourceManager(
 			JFaceResources.getResources());
 
+	private final DecorationHelper decorationHelper;
+
+	private final ChangeTrackingColorsAndFonts colorsAndFonts;
+
 	private boolean fileNameMode = false;
 
 	/**
@@ -52,6 +63,10 @@ public class StagingViewLabelProvider extends LabelProvider {
 	 */
 	public StagingViewLabelProvider(StagingView stagingView) {
 		super();
+		colorsAndFonts = new ChangeTrackingColorsAndFonts();
+		decorationHelper = new DecorationHelper(
+				Activator.getDefault().getPreferenceStore(), colorsAndFonts);
+		colorsAndFonts.addListener(this::postLabelEvent);
 		this.stagingView = stagingView;
 	}
 
@@ -70,10 +85,18 @@ public class StagingViewLabelProvider extends LabelProvider {
 
 	@Override
 	public void dispose() {
+		this.colorsAndFonts.dispose();
 		this.resourceManager.dispose();
 		super.dispose();
 	}
 
+	private void postLabelEvent() {
+		Display display = PlatformUI.getWorkbench().getDisplay();
+		display.asyncExec(() -> {
+			fireLabelProviderChanged(new LabelProviderChangedEvent(this));
+		});
+	}
+
 	private Image getEditorImage(StagingEntry diff) {
 		if (diff.isSubmodule()) {
 			return (Image) resourceManager.get(UIIcons.REPOSITORY);
@@ -95,6 +118,12 @@ public class StagingViewLabelProvider extends LabelProvider {
 		return image;
 	}
 
+	private Image getDecoratedImage(Image base, ImageDescriptor decorator) {
+		DecorationOverlayIcon decorated = new DecorationOverlayIcon(base,
+				decorator, IDecoration.BOTTOM_RIGHT);
+		return (Image) this.resourceManager.get(decorated);
+	}
+
 	private Image addSymlinkDecorationToImage(Image base) {
 		DecorationOverlayIcon decorated = new DecorationOverlayIcon(base,
 				UIIcons.OVR_SYMLINK, IDecoration.TOP_RIGHT);
@@ -114,7 +143,9 @@ public class StagingViewLabelProvider extends LabelProvider {
 		}
 
 		StagingEntry c = (StagingEntry) element;
-		return getEditorImage(c);
+		DecorationResult decoration = new DecorationResult();
+		decorationHelper.decorate(decoration, c);
+		return getDecoratedImage(getEditorImage(c), decoration.getOverlay());
 	}
 
 	@Override
@@ -131,17 +162,35 @@ public class StagingViewLabelProvider extends LabelProvider {
 			return ""; //$NON-NLS-1$
 		}
 
+		DecorationResult decoration = new DecorationResult();
+		decorationHelper.decorate(decoration, stagingEntry);
+		String prefix = decoration.getPrefix();
+		String suffix = decoration.getSuffix();
+		StringBuilder label = new StringBuilder();
+		if (prefix != null) {
+			label.append(prefix);
+		}
 		if (stagingView.getPresentation() == Presentation.LIST) {
 			if (fileNameMode) {
 				IPath parsed = Path.fromOSString(stagingEntry.getPath());
 				if (parsed.segmentCount() > 1) {
-					return parsed.lastSegment() + " - " //$NON-NLS-1$
-							+ parsed.removeLastSegments(1).toString();
+					label.append(parsed.lastSegment());
+					if (suffix != null) {
+						label.append(suffix);
+					}
+					label.append(" - ") //$NON-NLS-1$
+							.append(parsed.removeLastSegments(1).toString());
+					return label.toString();
 				}
 			}
-			return stagingEntry.getPath();
+			label.append(stagingEntry.getPath());
+		} else {
+			label.append(stagingEntry.getName());
+		}
+		if (suffix != null) {
+			label.append(suffix);
 		}
-		return stagingEntry.getName();
+		return label.toString();
 	}
 
 	@Nullable