Minor clean ups in new CommitMessageViewer

* Former inner class CommitMessageViewer.ObjectLink has become global
  class GitCommitReference.
* On JFacePreference change, it suffices to update the view; the model
  is not affected. (CommitMessageViewer)
* Reduce code duplication in CommitInfoBuilder.

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

5 files changed, 157 insertions, 114 deletions

diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitInfoBuilder.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitInfoBuilder.java
index f991f3d543..2378a8d32c 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitInfoBuilder.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitInfoBuilder.java
@@ -31,7 +31,6 @@ import org.eclipse.core.runtime.OperationCanceledException;
 import org.eclipse.egit.ui.Activator;
 import org.eclipse.egit.ui.UIPreferences;
 import org.eclipse.egit.ui.internal.UIText;
-import org.eclipse.egit.ui.internal.history.CommitMessageViewer.ObjectLink;
 import org.eclipse.egit.ui.internal.history.FormatJob.FormatResult;
 import org.eclipse.egit.ui.internal.trace.GitTraceLocation;
 import org.eclipse.jface.text.Region;
@@ -54,8 +53,6 @@ import org.eclipse.osgi.util.NLS;
  */
 public class CommitInfoBuilder {
 
-	private static final String SPACE = " "; //$NON-NLS-1$
-
 	private static final String LF = "\n"; //$NON-NLS-1$
 
 	private static final int MAXBRANCHES = 20;
@@ -104,56 +101,23 @@ public class CommitInfoBuilder {
 		final StringBuilder d = new StringBuilder();
 		final PersonIdent author = commit.getAuthorIdent();
 		final PersonIdent committer = commit.getCommitterIdent();
-		List<ObjectLink> hyperlinks = new ArrayList<>();
+		List<GitCommitReference> hyperlinks = new ArrayList<>();
 		d.append(UIText.CommitMessageViewer_commit);
-		d.append(SPACE);
+		d.append(' ');
 		d.append(commit.getId().name());
 		d.append(LF);
 
-		if (author != null) {
-			d.append(UIText.CommitMessageViewer_author);
-			d.append(": "); //$NON-NLS-1$
-			d.append(author.getName());
-			d.append(" <"); //$NON-NLS-1$
-			d.append(author.getEmailAddress());
-			d.append("> "); //$NON-NLS-1$
-			d.append(fmt.format(author.getWhen()));
-			d.append(LF);
-		}
-
-		if (committer != null) {
-			d.append(UIText.CommitMessageViewer_committer);
-			d.append(": "); //$NON-NLS-1$
-			d.append(committer.getName());
-			d.append(" <"); //$NON-NLS-1$
-			d.append(committer.getEmailAddress());
-			d.append("> "); //$NON-NLS-1$
-			d.append(fmt.format(committer.getWhen()));
-			d.append(LF);
-		}
+		addPersonIdent(d, author, UIText.CommitMessageViewer_author);
+		addPersonIdent(d, committer, UIText.CommitMessageViewer_committer);
 
 		for (int i = 0; i < commit.getParentCount(); i++) {
-			final SWTCommit p = (SWTCommit)commit.getParent(i);
-			p.parseBody();
-			d.append(UIText.CommitMessageViewer_parent);
-			d.append(": "); //$NON-NLS-1$
-			addLink(d, hyperlinks, p);
-			d.append(" ("); //$NON-NLS-1$
-			d.append(p.getShortMessage());
-			d.append(")"); //$NON-NLS-1$
-			d.append(LF);
+			addCommit(d, (SWTCommit) commit.getParent(i),
+					UIText.CommitMessageViewer_parent, hyperlinks);
 		}
 
 		for (int i = 0; i < commit.getChildCount(); i++) {
-			final SWTCommit p = (SWTCommit)commit.getChild(i);
-			p.parseBody();
-			d.append(UIText.CommitMessageViewer_child);
-			d.append(": "); //$NON-NLS-1$
-			addLink(d, hyperlinks, p);
-			d.append(" ("); //$NON-NLS-1$
-			d.append(p.getShortMessage());
-			d.append(")"); //$NON-NLS-1$
-			d.append(LF);
+			addCommit(d, (SWTCommit) commit.getChild(i),
+					UIText.CommitMessageViewer_child, hyperlinks);
 		}
 
 		if(Activator.getDefault().getPreferenceStore().getBoolean(
@@ -197,30 +161,16 @@ public class CommitInfoBuilder {
 				UIPreferences.HISTORY_SHOW_TAG_SEQUENCE)) {
 			try (RevWalk rw = new RevWalk(db)) {
 				monitor.setTaskName(UIText.CommitMessageViewer_GettingPreviousTagTaskName);
-				Ref followingTag = getNextTag(false, monitor);
-				if (followingTag != null) {
-					d.append(UIText.CommitMessageViewer_follows);
-					d.append(": "); //$NON-NLS-1$
-					RevCommit p = rw.parseCommit(followingTag
-							.getObjectId());
-					addLink(d, formatTagRef(followingTag), hyperlinks, p);
-					d.append(LF);
-				}
+				addTag(d, UIText.CommitMessageViewer_follows, rw,
+						getNextTag(false, monitor), hyperlinks);
 			} catch (IOException e) {
 				Activator.logError(e.getMessage(), e);
 			}
 
 			try (RevWalk rw = new RevWalk(db)) {
 				monitor.setTaskName(UIText.CommitMessageViewer_GettingNextTagTaskName);
-				Ref precedingTag = getNextTag(true, monitor);
-				if (precedingTag != null) {
-					d.append(UIText.CommitMessageViewer_precedes);
-					d.append(": "); //$NON-NLS-1$
-					RevCommit p = rw.parseCommit(precedingTag
-							.getObjectId());
-					addLink(d, formatTagRef(precedingTag), hyperlinks, p);
-					d.append(LF);
-				}
+				addTag(d, UIText.CommitMessageViewer_precedes, rw,
+						getNextTag(true, monitor), hyperlinks);
 			} catch (IOException e) {
 				Activator.logError(e.getMessage(), e);
 			}
@@ -258,17 +208,49 @@ public class CommitInfoBuilder {
 	}
 
 	private void addLink(StringBuilder d, String linkLabel,
-			Collection<ObjectLink> hyperlinks, RevCommit to) {
+			Collection<GitCommitReference> hyperlinks, RevCommit to) {
 		hyperlinks.add(
-				new ObjectLink(to, new Region(d.length(), linkLabel.length())));
+				new GitCommitReference(to, new Region(d.length(), linkLabel.length())));
 		d.append(linkLabel);
 	}
 
-	private void addLink(StringBuilder d, Collection<ObjectLink> hyperlinks,
+	private void addLink(StringBuilder d, Collection<GitCommitReference> hyperlinks,
 			RevCommit to) {
 		addLink(d, to.getId().name(), hyperlinks, to);
 	}
 
+	private void addPersonIdent(StringBuilder d, PersonIdent ident,
+			String label) {
+		if (ident != null) {
+			d.append(label).append(": "); //$NON-NLS-1$
+			d.append(ident.getName().trim());
+			d.append(" <").append(ident.getEmailAddress().trim()).append("> "); //$NON-NLS-1$ //$NON-NLS-2$
+			d.append(fmt.format(ident.getWhen()));
+			d.append(LF);
+		}
+	}
+
+	private void addCommit(StringBuilder d, SWTCommit gitcommit, String label,
+			List<GitCommitReference> hyperlinks) throws IOException {
+		if (gitcommit != null) {
+			d.append(label).append(": "); //$NON-NLS-1$
+			gitcommit.parseBody();
+			addLink(d, hyperlinks, gitcommit);
+			d.append(" (").append(gitcommit.getShortMessage()).append(')'); //$NON-NLS-1$
+			d.append(LF);
+		}
+	}
+
+	private void addTag(StringBuilder d, String label, RevWalk walk, Ref tag,
+			List<GitCommitReference> hyperlinks) throws IOException {
+		if (tag != null) {
+			d.append(label).append(": "); //$NON-NLS-1$
+			RevCommit p = walk.parseCommit(tag.getObjectId());
+			addLink(d, formatTagRef(tag), hyperlinks, p);
+			d.append(LF);
+		}
+	}
+
 	/**
 	 * @param commit
 	 * @param allRefs
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitMessageViewer.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitMessageViewer.java
index fe4d7ea412..d3c1283e4e 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitMessageViewer.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitMessageViewer.java
@@ -61,7 +61,6 @@ import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefDatabase;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revplot.PlotCommit;
-import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.swt.SWT;
 import org.eclipse.swt.custom.StyledText;
 import org.eclipse.swt.events.FocusEvent;
@@ -147,13 +146,22 @@ class CommitMessageViewer extends SourceViewer {
 		fill = store
 				.getBoolean(UIPreferences.RESOURCEHISTORY_SHOW_COMMENT_FILL);
 
-		// React on changes in the JFace color preferences
+		// React on changes in the JFace color preferences by updating the view
 		syntaxColoringListener = new IPropertyChangeListener() {
 			@Override
 			public void propertyChange(PropertyChangeEvent event) {
 				if (JFacePreferences.HYPERLINK_COLOR
 						.equals(event.getProperty())) {
-					format();
+					if (!t.isDisposed()) {
+						t.getDisplay().asyncExec(new Runnable() {
+							@Override
+							public void run() {
+								if (!t.isDisposed()) {
+									refresh();
+								}
+							}
+						});
+					}
 				}
 			}
 		};
@@ -255,7 +263,6 @@ class CommitMessageViewer extends SourceViewer {
 
 	void addDoneListenerToFormatJob() {
 		formatJob.addJobChangeListener(new JobChangeAdapter() {
-
 			@Override
 			public void done(IJobChangeEvent event) {
 				if (!event.getResult().isOK())
@@ -263,11 +270,12 @@ class CommitMessageViewer extends SourceViewer {
 				final StyledText text = getTextWidget();
 				if (text == null || text.isDisposed())
 					return;
-				final FormatJob job = (FormatJob) event.getJob();
+				final FormatResult result = ((FormatJob) event.getJob())
+						.getFormatResult();
 				text.getDisplay().asyncExec(new Runnable() {
 					@Override
 					public void run() {
-						applyFormatJobResultInUI(job.getFormatResult());
+						applyFormatJobResultInUI(result);
 					}
 				});
 			}
@@ -388,30 +396,11 @@ class CommitMessageViewer extends SourceViewer {
 		setDocument(new CommitDocument(formatResult));
 	}
 
-	static class ObjectLink {
-		private final RevCommit target;
-		private final IRegion region;
-
-		public ObjectLink(RevCommit target, IRegion region) {
-			this.target = target;
-			this.region = region;
-		}
-
-		public IRegion getRegion() {
-			return region;
-		}
-
-		public RevCommit getTarget() {
-			return target;
-		}
-
-	}
-
 	private class ObjectHyperlink implements IHyperlink {
 
-		ObjectLink link;
+		private final GitCommitReference link;
 
-		public ObjectHyperlink(ObjectLink link) {
+		public ObjectHyperlink(GitCommitReference link) {
 			this.link = link;
 		}
 
@@ -432,8 +421,9 @@ class CommitMessageViewer extends SourceViewer {
 
 		@Override
 		public void open() {
-			for (final Object l : navListeners.getListeners())
+			for (final Object l : navListeners.getListeners()) {
 				((CommitNavigationListener) l).showCommit(link.getTarget());
+			}
 		}
 
 	}
@@ -450,9 +440,9 @@ class CommitMessageViewer extends SourceViewer {
 			super(format.getCommitInfo());
 			headerEnd = format.getHeaderEnd();
 			footerStart = format.getFooterStart();
-			List<ObjectLink> knownLinks = format.getKnownLinks();
+			List<GitCommitReference> knownLinks = format.getKnownLinks();
 			hyperlinks = new ArrayList<>(knownLinks.size());
-			for (ObjectLink o : knownLinks) {
+			for (GitCommitReference o : knownLinks) {
 				hyperlinks.add(new ObjectHyperlink(o));
 			}
 			IDocumentPartitioner partitioner = new FastPartitioner(
@@ -513,8 +503,8 @@ class CommitMessageViewer extends SourceViewer {
 
 		@Override
 		public IToken nextToken() {
+			tokenStart = currentOffset;
 			if (currentOffset < end) {
-				tokenStart = currentOffset;
 				if (currentOffset < headerEnd) {
 					currentOffset = Math.min(headerEnd, end);
 					return HEADER;
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/FormatJob.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/FormatJob.java
index b93b1a8924..d51c9bbf1a 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/FormatJob.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/FormatJob.java
@@ -21,7 +21,6 @@ import org.eclipse.core.runtime.jobs.Job;
 import org.eclipse.egit.ui.Activator;
 import org.eclipse.egit.ui.JobFamilies;
 import org.eclipse.egit.ui.internal.UIText;
-import org.eclipse.egit.ui.internal.history.CommitMessageViewer.ObjectLink;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revplot.PlotCommit;
@@ -52,9 +51,10 @@ class FormatJob extends Job {
 
 	@Override
 	protected IStatus run(IProgressMonitor monitor) {
-		if(monitor.isCanceled())
+		if (monitor.isCanceled()) {
 			return Status.CANCEL_STATUS;
-		final FormatResult commitInfo;
+		}
+		FormatResult commitInfo;
 		CommitInfoBuilder builder;
 		try {
 			synchronized(lock) {
@@ -68,8 +68,9 @@ class FormatJob extends Job {
 		} catch (IOException e) {
 			return Activator.createErrorStatus(e.getMessage(), e);
 		}
-		if(monitor.isCanceled())
+		if (monitor.isCanceled()) {
 			return Status.CANCEL_STATUS;
+		}
 		synchronized(lock) {
 			formatResult = commitInfo;
 		}
@@ -119,13 +120,13 @@ class FormatJob extends Job {
 	static class FormatResult{
 		private final String commitInfo;
 
-		private final List<ObjectLink> knownLinks;
+		private final List<GitCommitReference> knownLinks;
 
 		private final int headerEnd;
 
 		private final int footerStart;
 
-		FormatResult(String commmitInfo, List<ObjectLink> links,
+		FormatResult(String commmitInfo, List<GitCommitReference> links,
 				int headerEnd, int footerStart) {
 			this.commitInfo = commmitInfo;
 			this.knownLinks = links;
@@ -137,7 +138,7 @@ class FormatJob extends Job {
 			return commitInfo;
 		}
 
-		public List<ObjectLink> getKnownLinks() {
+		public List<GitCommitReference> getKnownLinks() {
 			return knownLinks;
 		}
 
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitCommitReference.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitCommitReference.java
new file mode 100644
index 0000000000..e2e633b3f9
--- /dev/null
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitCommitReference.java
@@ -0,0 +1,73 @@
+/*******************************************************************************
+ * Copyright (C) 2015, Thomas Wolf <thomas.wolf@paranor.ch>
+ *
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *******************************************************************************/
+package org.eclipse.egit.ui.internal.history;
+
+import org.eclipse.jdt.annotation.NonNull;
+import org.eclipse.jface.text.IRegion;
+import org.eclipse.jgit.revwalk.RevCommit;
+
+/**
+ * Represents an {@link IRegion} in some text that references a
+ * {@link RevCommit}.
+ */
+public class GitCommitReference {
+
+	private final RevCommit target;
+
+	private final IRegion region;
+
+	/**
+	 * Creates a new instance.
+	 *
+	 * @param target
+	 *            the {@link RevCommit} referenced
+	 * @param region
+	 *            the {@link IRegion} of the text referencing the commit
+	 */
+	public GitCommitReference(@NonNull RevCommit target, @NonNull IRegion region) {
+		this.target = target;
+		this.region = region;
+	}
+
+	/**
+	 * Retrieves the text region of this reference.
+	 *
+	 * @return the region
+	 */
+	public IRegion getRegion() {
+		return region;
+	}
+
+	/**
+	 * Retrieves the referenced commit.
+	 *
+	 * @return the commit
+	 */
+	public RevCommit getTarget() {
+		return target;
+	}
+
+	@Override
+	public boolean equals(Object obj) {
+		if (this == obj) {
+			return true;
+		}
+		if (obj instanceof GitCommitReference) {
+			GitCommitReference other = (GitCommitReference) obj;
+			return target.equals(other.target) && region.equals(other.region);
+		}
+		return false;
+	}
+
+	@Override
+	public int hashCode() {
+		return target.hashCode() ^ region.hashCode();
+	}
+
+}
\ No newline at end of file
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitHistoryPage.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitHistoryPage.java
index 9ca47acbab..81df7372eb 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitHistoryPage.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitHistoryPage.java
@@ -937,11 +937,10 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener,
 					ISourceViewer viewer) {
 				PresentationReconciler reconciler = new PresentationReconciler();
 				reconciler.setDocumentPartitioning(
-						getConfiguredDocumentPartitioning(commentViewer));
+						getConfiguredDocumentPartitioning(viewer));
 				DefaultDamagerRepairer hyperlinkDamagerRepairer = new DefaultDamagerRepairer(
-						new HyperlinkTokenScanner(
-								getHyperlinkDetectors(commentViewer),
-								commentViewer));
+						new HyperlinkTokenScanner(getHyperlinkDetectors(viewer),
+								viewer));
 				reconciler.setDamager(hyperlinkDamagerRepairer,
 						IDocument.DEFAULT_CONTENT_TYPE);
 				reconciler.setRepairer(hyperlinkDamagerRepairer,
@@ -950,17 +949,15 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener,
 						PlatformUI.getWorkbench().getDisplay()
 								.getSystemColor(SWT.COLOR_DARK_GRAY));
 				DefaultDamagerRepairer headerDamagerRepairer = new DefaultDamagerRepairer(
-						new HyperlinkTokenScanner(
-								getHyperlinkDetectors(commentViewer),
-								commentViewer, headerDefault));
+						new HyperlinkTokenScanner(getHyperlinkDetectors(viewer),
+								viewer, headerDefault));
 				reconciler.setDamager(headerDamagerRepairer,
 						CommitMessageViewer.HEADER_CONTENT_TYPE);
 				reconciler.setRepairer(headerDamagerRepairer,
 						CommitMessageViewer.HEADER_CONTENT_TYPE);
 				DefaultDamagerRepairer footerDamagerRepairer = new DefaultDamagerRepairer(
-						new FooterTokenScanner(
-								getHyperlinkDetectors(commentViewer),
-								commentViewer));
+						new FooterTokenScanner(getHyperlinkDetectors(viewer),
+								viewer));
 				reconciler.setDamager(footerDamagerRepairer,
 						CommitMessageViewer.FOOTER_CONTENT_TYPE);
 				reconciler.setRepairer(footerDamagerRepairer,
@@ -1672,7 +1669,7 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener,
 	}
 
 	@Override
-	public Object getAdapter(final Class adapter) {
+	public <T> T getAdapter(final Class<T> adapter) {
 		return null;
 	}