From 735b606577bf28d74873454428b06322b4dad100 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Tue, 14 Aug 2018 10:24:43 +0200 Subject: [PATCH] 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 --- .../repository/RepositoryTreeNodeDecorator.java | 122 ++++++--------------- .../RepositoryTreeNodeLabelProvider.java | 20 +++- .../RepositoryTreeNodeWorkbenchAdapter.java | 96 ++++++++++++++++ 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 206a2dee3..139f990be 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 ebee104fb..18fb1d4b1 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.) + *

+ * 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 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 442ec5938..479aaec14 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; -- 2.11.4.GIT