Fix bugs with the new lightweight decorator for RepositoryTreeNodes

1. The flickering of image decorations was too annoying. Move it
   back from the asynchronous decorator to the synchronous label
   provider. This means image decorations are computed on the UI
   thread again, but since we only decorate ref and tag icons and
   these nodes are initially not expanded, it will at least not
   have an impact during Eclipse startup. (And in any case it won't
   be worse than with the old label provider.)
2. Fix the update of undecorated labels. The caching logic added
   for text labels works only if a decorated label is always
   different from an undecorated one, even when there are no
   decorations. Otherwise we end up mistakenly showing a previously
   decorated label. (User-visible effect: toggling off the display
   of latest branch commits in the repositories view didn't work.)
   Fix this by making our asynchronous decorator always add
   something: when there's no decoration, append a single blank.
   That's a bit hacky, but works.

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

3 files changed, 147 insertions, 91 deletions

diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/repository/RepositoryTreeNodeDecorator.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/repository/RepositoryTreeNodeDecorator.java
index 206a2dee3e..139f990bee 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/repository/RepositoryTreeNodeDecorator.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/repository/RepositoryTreeNodeDecorator.java
@@ -19,7 +19,6 @@ import org.eclipse.egit.core.RepositoryUtil;
 import org.eclipse.egit.ui.Activator;
 import org.eclipse.egit.ui.internal.CommonUtils;
 import org.eclipse.egit.ui.internal.GitLabels;
-import org.eclipse.egit.ui.internal.UIIcons;
 import org.eclipse.egit.ui.internal.UIText;
 import org.eclipse.egit.ui.internal.decorators.GitDecorator;
 import org.eclipse.egit.ui.internal.repository.tree.AdditionalRefNode;
@@ -101,7 +100,6 @@ public class RepositoryTreeNodeDecorator extends GitDecorator
 		if (repository != null) {
 			try {
 				decorateText(node, repository, decoration);
-				decorateImage(node, repository, decoration);
 			} catch (IOException e) {
 				Activator.logError(MessageFormat.format(
 						UIText.GitLabelProvider_UnableToRetrieveLabel,
@@ -113,31 +111,38 @@ public class RepositoryTreeNodeDecorator extends GitDecorator
 	private void decorateText(RepositoryTreeNode<?> node,
 			@NonNull Repository repository, IDecoration decoration)
 			throws IOException {
+		boolean decorated = false;
 		switch (node.getType()) {
 		case REPO:
-			decorateRepository(node, repository, decoration);
+			decorated = decorateRepository(node, repository, decoration);
 			break;
 		case ADDITIONALREF:
-			decorateAdditionalRef((AdditionalRefNode) node, decoration);
+			decorated = decorateAdditionalRef((AdditionalRefNode) node,
+					decoration);
 			break;
 		case REF:
-			decorateRef((RefNode) node, decoration);
+			decorated = decorateRef((RefNode) node, decoration);
 			break;
 		case TAG:
-			decorateTag((TagNode) node, decoration);
+			decorated = decorateTag((TagNode) node, decoration);
 			break;
 		case STASHED_COMMIT:
-			decorateStash((StashedCommitNode) node, decoration);
+			decorated = decorateStash((StashedCommitNode) node, decoration);
 			break;
 		case SUBMODULES:
-			decorateSubmodules(repository, decoration);
+			decorated = decorateSubmodules(repository, decoration);
 			break;
 		default:
-			break;
+			return;
+		}
+		if (!decorated) {
+			// Ensure the caching of last labels in
+			// RepositoryTreeNodeLabelProvider works
+			decoration.addSuffix(" "); //$NON-NLS-1$
 		}
 	}
 
-	private void decorateAdditionalRef(AdditionalRefNode node,
+	private boolean decorateAdditionalRef(AdditionalRefNode node,
 			IDecoration decoration) {
 		Ref ref = node.getObject();
 		StringBuilder suffix = new StringBuilder();
@@ -157,19 +162,22 @@ public class RepositoryTreeNodeDecorator extends GitDecorator
 					UIText.RepositoriesViewLabelProvider_UnbornBranchText);
 		}
 		decoration.addSuffix(suffix.toString());
+		return true;
 	}
 
-	private void decorateRef(RefNode node, IDecoration decoration) {
+	private boolean decorateRef(RefNode node, IDecoration decoration) {
 		if (verboseBranchMode) {
 			RevCommit latest = getLatestCommit(node);
 			if (latest != null) {
 				decoration.addSuffix(" " + abbreviate(latest) + ' ' //$NON-NLS-1$
 						+ latest.getShortMessage());
+				return true;
 			}
 		}
+		return false;
 	}
 
-	private void decorateRepository(RepositoryTreeNode<?> node,
+	private boolean decorateRepository(RepositoryTreeNode<?> node,
 			@NonNull Repository repository, IDecoration decoration)
 			throws IOException {
 		boolean isSubModule = node.getParent() != null && node.getParent()
@@ -181,7 +189,7 @@ public class RepositoryTreeNodeDecorator extends GitDecorator
 		if (isSubModule) {
 			Ref head = repository.exactRef(Constants.HEAD);
 			if (head == null) {
-				return;
+				return false;
 			}
 			suffix.append(" ["); //$NON-NLS-1$
 			if (head.isSymbolic()) {
@@ -204,7 +212,7 @@ public class RepositoryTreeNodeDecorator extends GitDecorator
 			String branch = Activator.getDefault().getRepositoryUtil()
 					.getShortBranch(repository);
 			if (branch == null) {
-				return;
+				return false;
 			}
 			suffix.append(" ["); //$NON-NLS-1$
 			suffix.append(branch);
@@ -226,100 +234,34 @@ public class RepositoryTreeNodeDecorator extends GitDecorator
 			suffix.append(']');
 		}
 		decoration.addSuffix(suffix.toString());
+		return true;
 	}
 
-	private void decorateStash(StashedCommitNode node, IDecoration decoration) {
+	private boolean decorateStash(StashedCommitNode node,
+			IDecoration decoration) {
 		RevCommit commit = node.getObject();
 		decoration.addSuffix(
 				" [" + abbreviate(commit) + "] " + commit.getShortMessage()); //$NON-NLS-1$ //$NON-NLS-2$
+		return true;
 	}
 
-	private void decorateSubmodules(@NonNull Repository repository,
+	private boolean decorateSubmodules(@NonNull Repository repository,
 			IDecoration decoration) throws IOException {
 		if (haveSubmoduleChanges(repository)) {
 			decoration.addPrefix("> "); //$NON-NLS-1$
+			return true;
 		}
+		return false;
 	}
 
-	private void decorateTag(TagNode node, IDecoration decoration) {
+	private boolean decorateTag(TagNode node, IDecoration decoration) {
 		if (verboseBranchMode && node.getCommitId() != null
 				&& node.getCommitId().length() > 0) {
 			decoration.addSuffix(" " + node.getCommitId().substring(0, 7) + ' ' //$NON-NLS-1$
 					+ node.getCommitShortMessage());
+			return true;
 		}
-	}
-
-	private void decorateImage(RepositoryTreeNode<?> node,
-			@NonNull Repository repository, IDecoration decoration)
-			throws IOException {
-
-		switch (node.getType()) {
-		case TAG:
-		case ADDITIONALREF:
-		case REF:
-			// if the branch or tag is checked out,
-			// we want to decorate the corresponding
-			// node with a little check indicator
-			String refName = ((Ref) node.getObject()).getName();
-			Ref leaf = ((Ref) node.getObject()).getLeaf();
-
-			String compareString = null;
-
-			String branchName = repository.getFullBranch();
-			if (branchName == null) {
-				return;
-			}
-			if (refName.startsWith(Constants.R_HEADS)) {
-				// local branch: HEAD would be on the branch
-				compareString = refName;
-			} else if (refName.startsWith(Constants.R_TAGS)) {
-				// tag: HEAD would be on the commit id to which the tag is
-				// pointing
-				TagNode tagNode = (TagNode) node;
-				compareString = tagNode.getCommitId();
-			} else if (refName.startsWith(Constants.R_REMOTES)) {
-				// remote branch: HEAD would be on the commit id to which
-				// the branch is pointing
-				ObjectId id = repository.resolve(refName);
-				if (id == null) {
-					return;
-				}
-				try (RevWalk rw = new RevWalk(repository)) {
-					RevCommit commit = rw.parseCommit(id);
-					compareString = commit.getId().name();
-				}
-			} else if (refName.equals(Constants.HEAD)) {
-				decoration.addOverlay(UIIcons.OVR_CHECKEDOUT,
-						IDecoration.TOP_LEFT);
-				return;
-			} else {
-				String leafname = leaf.getName();
-				if (leafname.startsWith(Constants.R_REFS)
-						&& leafname.equals(branchName)) {
-					decoration.addOverlay(UIIcons.OVR_CHECKEDOUT,
-							IDecoration.TOP_LEFT);
-					return;
-				}
-				ObjectId objectId = leaf.getObjectId();
-				if (objectId != null && objectId
-						.equals(repository.resolve(Constants.HEAD))) {
-					decoration.addOverlay(UIIcons.OVR_CHECKEDOUT,
-							IDecoration.TOP_LEFT);
-					return;
-				}
-				// some other symbolic reference
-				return;
-			}
-
-			if (compareString != null && compareString.equals(branchName)) {
-				decoration.addOverlay(UIIcons.OVR_CHECKEDOUT,
-						IDecoration.TOP_LEFT);
-			}
-
-			break;
-		default:
-			break;
-		}
+		return false;
 	}
 
 	private RevCommit getLatestCommit(RepositoryTreeNode node) {
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/repository/RepositoryTreeNodeLabelProvider.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/repository/RepositoryTreeNodeLabelProvider.java
index ebee104fb0..18fb1d4b10 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/repository/RepositoryTreeNodeLabelProvider.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/repository/RepositoryTreeNodeLabelProvider.java
@@ -38,6 +38,19 @@ public class RepositoryTreeNodeLabelProvider
 
 	private final WorkbenchLabelProvider labelProvider;
 
+	/**
+	 * Keeps the last label. If the label we originally get is undecorated, we
+	 * return this last decorated label instead to prevent flickering. When the
+	 * asynchronous lightweight decorator then has computed the decoration, the
+	 * label will be updated. Note that this works only because our
+	 * RepositoryTreeNodeDecorator always decorates! (If there's no decoration,
+	 * it appends a single blank to ensure the decorated label is different from
+	 * the undecorated one.)
+	 * <p>
+	 * For images, there is no such work-around, and thus we need to do the
+	 * image decorations in the label provider (in the
+	 * RepositoryTreeNodeWorkbenchAdapter in our case) in the UI thread.
+	 */
 	private final WeakHashMap<Object, StyledString> previousDecoratedLabels = new WeakHashMap<>();
 
 	/**
@@ -63,7 +76,9 @@ public class RepositoryTreeNodeLabelProvider
 	@Override
 	public StyledString getStyledText(Object element) {
 		StyledString decoratedLabel = super.getStyledText(element);
-		if (decoratedLabel.getString().equals(labelProvider.getText(element))) {
+		String decoratedValue = decoratedLabel.getString();
+		String simpleValue = labelProvider.getText(element);
+		if (decoratedValue.equals(simpleValue)) {
 			// Decoration not available yet... but may be shortly. Try to
 			// prevent flickering by returning the previous decorated label, if
 			// any.
@@ -71,6 +86,9 @@ public class RepositoryTreeNodeLabelProvider
 			if (previousLabel != null) {
 				return previousLabel;
 			}
+		} else if (decoratedValue.trim().equals(simpleValue)) {
+			// No decoration...
+			decoratedLabel = labelProvider.getStyledText(element);
 		}
 		if (element instanceof RepositoryNode) {
 			Repository repository = ((RepositoryNode) element).getRepository();
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/repository/RepositoryTreeNodeWorkbenchAdapter.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/repository/RepositoryTreeNodeWorkbenchAdapter.java
index 442ec59388..479aaec146 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/repository/RepositoryTreeNodeWorkbenchAdapter.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/repository/RepositoryTreeNodeWorkbenchAdapter.java
@@ -11,9 +11,11 @@
 package org.eclipse.egit.ui.internal.repository;
 
 import java.io.File;
+import java.io.IOException;
 import java.text.MessageFormat;
 
 import org.eclipse.core.runtime.IPath;
+import org.eclipse.egit.ui.internal.DecorationOverlayDescriptor;
 import org.eclipse.egit.ui.internal.GitLabels;
 import org.eclipse.egit.ui.internal.ResourcePropertyTester;
 import org.eclipse.egit.ui.internal.UIIcons;
@@ -23,9 +25,14 @@ import org.eclipse.egit.ui.internal.repository.tree.RepositoryTreeNodeType;
 import org.eclipse.egit.ui.internal.repository.tree.StashedCommitNode;
 import org.eclipse.egit.ui.internal.repository.tree.TagNode;
 import org.eclipse.jface.resource.ImageDescriptor;
+import org.eclipse.jface.viewers.IDecoration;
+import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.ui.PlatformUI;
 import org.eclipse.ui.model.WorkbenchAdapter;
 
@@ -53,7 +60,26 @@ public class RepositoryTreeNodeWorkbenchAdapter extends WorkbenchAdapter {
 
 	@Override
 	public ImageDescriptor getImageDescriptor(Object object) {
+		if (object == null) {
+			return null;
+		}
 		RepositoryTreeNode<?> node = (RepositoryTreeNode) object;
+		ImageDescriptor base = getBaseImageDescriptor(node);
+		if (base == null) {
+			return null;
+		}
+		// We have to decorate here: if we let an asynchronous lightweight
+		// decorator do it, image decorations may flicker in the repositories
+		// view and elsewhere where we'd refresh viewers.
+		try {
+			return decorateImageDescriptor(base, node);
+		} catch (IOException e) {
+			return base;
+		}
+	}
+
+	private ImageDescriptor getBaseImageDescriptor(
+			@NonNull RepositoryTreeNode<?> node) {
 		switch (node.getType()) {
 		case FILE: {
 			Object item = node.getObject();
@@ -82,6 +108,76 @@ public class RepositoryTreeNodeWorkbenchAdapter extends WorkbenchAdapter {
 		return node.getType().getIcon();
 	}
 
+	private ImageDescriptor decorateImageDescriptor(
+			@NonNull ImageDescriptor base, @NonNull RepositoryTreeNode<?> node)
+			throws IOException {
+		switch (node.getType()) {
+		case TAG:
+		case ADDITIONALREF:
+		case REF:
+			// if the branch or tag is checked out,
+			// we want to decorate the corresponding
+			// node with a little check indicator
+			String refName = ((Ref) node.getObject()).getName();
+			Ref leaf = ((Ref) node.getObject()).getLeaf();
+
+			String compareString = null;
+			Repository repository = node.getRepository();
+			String branchName = repository.getFullBranch();
+			if (branchName == null) {
+				return base;
+			}
+			if (refName.startsWith(Constants.R_HEADS)) {
+				// local branch: HEAD would be on the branch
+				compareString = refName;
+			} else if (refName.startsWith(Constants.R_TAGS)) {
+				// tag: HEAD would be on the commit id to which the tag is
+				// pointing
+				TagNode tagNode = (TagNode) node;
+				compareString = tagNode.getCommitId();
+			} else if (refName.startsWith(Constants.R_REMOTES)) {
+				// remote branch: HEAD would be on the commit id to which
+				// the branch is pointing
+				ObjectId id = repository.resolve(refName);
+				if (id == null) {
+					return base;
+				}
+				try (RevWalk rw = new RevWalk(repository)) {
+					RevCommit commit = rw.parseCommit(id);
+					compareString = commit.getId().name();
+				}
+			} else if (refName.equals(Constants.HEAD)) {
+				return new DecorationOverlayDescriptor(base,
+						UIIcons.OVR_CHECKEDOUT, IDecoration.TOP_LEFT);
+			} else {
+				String leafname = leaf.getName();
+				if (leafname.startsWith(Constants.R_REFS)
+						&& leafname.equals(branchName)) {
+					return new DecorationOverlayDescriptor(base,
+							UIIcons.OVR_CHECKEDOUT, IDecoration.TOP_LEFT);
+				}
+				ObjectId objectId = leaf.getObjectId();
+				if (objectId != null && objectId
+						.equals(repository.resolve(Constants.HEAD))) {
+					return new DecorationOverlayDescriptor(base,
+							UIIcons.OVR_CHECKEDOUT, IDecoration.TOP_LEFT);
+				}
+				// some other symbolic reference
+				return base;
+			}
+
+			if (compareString != null && compareString.equals(branchName)) {
+				return new DecorationOverlayDescriptor(base,
+						UIIcons.OVR_CHECKEDOUT, IDecoration.TOP_LEFT);
+			}
+
+			break;
+		default:
+			break;
+		}
+		return base;
+	}
+
 	@Override
 	public String getLabel(Object object) {
 		RepositoryTreeNode<?> node = (RepositoryTreeNode) object;