From 3b30746f0e7fe44443c7f7d00948d7628f0c4103 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Mon, 10 Jul 2017 15:02:20 +0200 Subject: [PATCH] Reflog view: serialize asynchronous loading jobs Per bug 226343 the DeferredTreeContentManager has some problems. Avoid most of them by serializing jobs asynchronously loading the reflog, and once we have the reflog, don't re-load it but return it directly. Fix auto-updates of the view by re-setting the input on ref changes. Just calling refresh() doesn't work anymore if the reflog is loaded asynchronously. Bug: 519431 Change-Id: I999616ea79068d5f4d0683e655231494d916cd83 Signed-off-by: Thomas Wolf --- .../egit/ui/internal/reflog/ReflogView.java | 7 +- .../internal/reflog/ReflogViewContentProvider.java | 126 ++++++++++++++++++--- 2 files changed, 118 insertions(+), 15 deletions(-) diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/reflog/ReflogView.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/reflog/ReflogView.java index 33361836c..4c7ad0791 100644 --- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/reflog/ReflogView.java +++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/reflog/ReflogView.java @@ -572,7 +572,12 @@ public class ReflogView extends ViewPart implements RefsChangedListener, IShowIn PlatformUI.getWorkbench().getDisplay().syncExec(new Runnable() { @Override public void run() { - refLogTableTreeViewer.refresh(); + Object currentInput = refLogTableTreeViewer.getInput(); + if (currentInput instanceof ReflogInput) { + ReflogInput oldInput = (ReflogInput) currentInput; + refLogTableTreeViewer.setInput(new ReflogInput( + oldInput.getRepository(), oldInput.getRef())); + } } }); } diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/reflog/ReflogViewContentProvider.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/reflog/ReflogViewContentProvider.java index 3dd3ffcbd..01ede86c3 100644 --- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/reflog/ReflogViewContentProvider.java +++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/reflog/ReflogViewContentProvider.java @@ -12,7 +12,9 @@ *******************************************************************************/ package org.eclipse.egit.ui.internal.reflog; +import java.io.File; import java.util.Collection; +import java.util.Objects; import org.eclipse.core.runtime.Assert; import org.eclipse.core.runtime.IProgressMonitor; @@ -25,6 +27,7 @@ import org.eclipse.jface.viewers.Viewer; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.lib.ReflogEntry; import org.eclipse.jgit.lib.Repository; +import org.eclipse.swt.widgets.Control; import org.eclipse.ui.model.WorkbenchAdapter; import org.eclipse.ui.progress.DeferredTreeContentManager; import org.eclipse.ui.progress.IDeferredWorkbenchAdapter; @@ -40,7 +43,42 @@ public class ReflogViewContentProvider implements ITreeContentProvider { private Object currentInput; /** - * Input class for this content provider + * Serializes concurrent attempts to load the reflog. + */ + private static class ReflogSchedulingRule implements ISchedulingRule { + + private final File gitDir; + + public ReflogSchedulingRule(File gitDir) { + this.gitDir = gitDir; + } + + @Override + public boolean contains(ISchedulingRule rule) { + if (rule instanceof ReflogSchedulingRule) { + return Objects.equals(gitDir, + ((ReflogSchedulingRule) rule).gitDir); + } + return false; + } + + @Override + public boolean isConflicting(ISchedulingRule rule) { + return rule instanceof ReflogSchedulingRule; + } + + } + + private static final WorkbenchAdapter ERROR_ELEMENT = new WorkbenchAdapter() { + + @Override + public String getLabel(Object object) { + return UIText.ReflogView_ErrorOnLoad; + } + }; + + /** + * Input class for this content provider. */ public static class ReflogInput extends WorkbenchAdapter implements IDeferredWorkbenchAdapter { @@ -49,6 +87,8 @@ public class ReflogViewContentProvider implements ITreeContentProvider { private final String ref; + private final ISchedulingRule rule; + private Collection refLog; /** @@ -62,6 +102,7 @@ public class ReflogViewContentProvider implements ITreeContentProvider { Assert.isNotNull(ref, "Ref cannot be null"); //$NON-NLS-1$ this.repository = repository; this.ref = ref; + this.rule = new ReflogSchedulingRule(repository.getDirectory()); } /** @@ -73,23 +114,35 @@ public class ReflogViewContentProvider implements ITreeContentProvider { return repository; } + /** + * Retrieves the ref. + * + * @return the ref + */ + public String getRef() { + return ref; + } + @Override public Object[] getChildren(Object o) { if (refLog != null) { return refLog.toArray(); } - return super.getChildren(o); + return null; } @Override public void fetchDeferredChildren(Object object, IElementCollector collector, IProgressMonitor monitor) { + if (refLog != null) { + return; // Already loaded. + } try (Git git = new Git(repository)) { refLog = git.reflog().setRef(ref).call(); collector.add(refLog.toArray(), monitor); } catch (Exception e) { Activator.logError("Error running reflog command", e); //$NON-NLS-1$ - collector.add(new ErrorElement(), monitor); + collector.add(ERROR_ELEMENT, monitor); } } @@ -100,15 +153,7 @@ public class ReflogViewContentProvider implements ITreeContentProvider { @Override public ISchedulingRule getRule(Object object) { - return null; - } - } - - private static class ErrorElement extends WorkbenchAdapter { - - @Override - public String getLabel(Object object) { - return UIText.ReflogView_ErrorOnLoad; + return rule; } } @@ -124,8 +169,7 @@ public class ReflogViewContentProvider implements ITreeContentProvider { } currentInput = newInput; if (viewer instanceof AbstractTreeViewer && newInput != null) { - loader = new DeferredTreeContentManager( - (AbstractTreeViewer) viewer); + loader = new DeferredBatchLoader((AbstractTreeViewer) viewer); } } @@ -141,6 +185,11 @@ public class ReflogViewContentProvider implements ITreeContentProvider { @Override public Object[] getChildren(Object parentElement) { if (parentElement instanceof ReflogInput && loader != null) { + Object[] knownChildren = ((ReflogInput) parentElement) + .getChildren(parentElement); + if (knownChildren != null) { + return knownChildren; + } return loader.getChildren(parentElement); } return new Object[0]; @@ -155,4 +204,53 @@ public class ReflogViewContentProvider implements ITreeContentProvider { public boolean hasChildren(Object element) { return false; } + + /** + * A variant of {@link DeferredTreeContentManager} that doesn't use a + * separate UI job to fill in the tree. With UI jobs, it's simply impossible + * to know what has already been added when there are several loading jobs. + * For our use case (load the whole reflog, then add it to the tree) a + * {@link org.eclipse.swt.widgets.Display#syncExec(Runnable) syncExec()} is + * sufficient. + */ + private static class DeferredBatchLoader + extends DeferredTreeContentManager { + + private AbstractTreeViewer viewer; + + public DeferredBatchLoader(AbstractTreeViewer viewer) { + super(viewer); + this.viewer = viewer; + } + + /** + * Add child nodes, removing the error element if appropriate. Contrary + * to the super implementation, this does not use a UI job but + * a simple {@link org.eclipse.swt.widgets.Display#syncExec(Runnable) + * syncExec()}. + * + * @param parent + * to add the {@code children} to + * @param children + * to add to the {@code parent} + * @param monitor + * is ignored + */ + @Override + protected void addChildren(Object parent, Object[] children, + IProgressMonitor monitor) { + Control control = viewer.getControl(); + if (control == null || control.isDisposed()) { + return; + } + control.getDisplay().syncExec(() -> { + if (!control.isDisposed()) { + if (children.length != 1 || children[0] != ERROR_ELEMENT) { + viewer.remove(ERROR_ELEMENT); + } + viewer.add(parent, children); + } + }); + } + } } -- 2.11.4.GIT