Image diffs

Ticket 88: https://dev.gitblit.com/tickets/gitblit.git/88

Based on Lea Verou's pure CSS slider:
http://lea.verou.me/2014/07/image-comparison-slider-with-pure-css/

* Add a callback interface, pass it through DiffUtils to the
  GitBlitDiffFormatter. Is needed because the rendering needs access
  to the repositoryName and other things that are known only at higher
  levels.
  
* New class ImageDiffHandler responsible for rendering an image diff.
  Called for all binary diffs, doesn't do anything if it's not an
  image. HTML is generated via JSoup: no worries about forgetting to
  close a tag, not about HTML escaping, nor about XSS.
  
* The 3 diff pages set up such an ImageDIffHandler and pass it along.

* CSS changes: from Lea Verou, with some minor improvements.

I think in the long run there'll be no way around rewriting the
HTML diff formatter from scratch, not using the standard JGit
DiffFormatter at all.

4 files changed, 162 insertions, 5 deletions

diff --git a/src/main/java/com/gitblit/wicket/pages/BlobDiffPage.java b/src/main/java/com/gitblit/wicket/pages/BlobDiffPage.java
index 9cc3eae1..517e80b8 100644
--- a/src/main/java/com/gitblit/wicket/pages/BlobDiffPage.java
+++ b/src/main/java/com/gitblit/wicket/pages/BlobDiffPage.java
@@ -15,12 +15,15 @@
  */

 package com.gitblit.wicket.pages;

 

+import java.util.List;

+

 import org.apache.wicket.PageParameters;

 import org.apache.wicket.markup.html.basic.Label;

 import org.apache.wicket.markup.html.link.BookmarkablePageLink;

 import org.eclipse.jgit.lib.Repository;

 import org.eclipse.jgit.revwalk.RevCommit;

 

+import com.gitblit.Keys;

 import com.gitblit.utils.DiffUtils;

 import com.gitblit.utils.DiffUtils.DiffOutputType;

 import com.gitblit.utils.JGitUtils;

@@ -43,16 +46,23 @@ public class BlobDiffPage extends RepositoryPage {
 		Repository r = getRepository();

 		RevCommit commit = getCommit();

 

+		final List<String> imageExtensions = app().settings().getStrings(Keys.web.imageExtensions);

+

 		String diff;

 		if (StringUtils.isEmpty(baseObjectId)) {

 			// use first parent

-			diff = DiffUtils.getDiff(r, commit, blobPath, DiffOutputType.HTML).content;

+			RevCommit parent = commit.getParentCount() == 0 ? null : commit.getParent(0);

+			ImageDiffHandler handler = new ImageDiffHandler(getContextUrl(), repositoryName,

+					parent.getName(), commit.getName(), imageExtensions);

+			diff = DiffUtils.getDiff(r, commit, blobPath, DiffOutputType.HTML, handler).content;

 			add(new BookmarkablePageLink<Void>("patchLink", PatchPage.class,

 					WicketUtils.newPathParameter(repositoryName, objectId, blobPath)));

 		} else {

 			// base commit specified

 			RevCommit baseCommit = JGitUtils.getCommit(r, baseObjectId);

-			diff = DiffUtils.getDiff(r, baseCommit, commit, blobPath, DiffOutputType.HTML).content;

+			ImageDiffHandler handler = new ImageDiffHandler(getContextUrl(), repositoryName,

+					baseCommit.getName(), commit.getName(), imageExtensions);

+			diff = DiffUtils.getDiff(r, baseCommit, commit, blobPath, DiffOutputType.HTML, handler).content;

 			add(new BookmarkablePageLink<Void>("patchLink", PatchPage.class,

 					WicketUtils.newBlobDiffParameter(repositoryName, baseObjectId, objectId,

 							blobPath)));

diff --git a/src/main/java/com/gitblit/wicket/pages/CommitDiffPage.java b/src/main/java/com/gitblit/wicket/pages/CommitDiffPage.java
index 34ff5a29..77d5ccf3 100644
--- a/src/main/java/com/gitblit/wicket/pages/CommitDiffPage.java
+++ b/src/main/java/com/gitblit/wicket/pages/CommitDiffPage.java
@@ -31,6 +31,7 @@ import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 
 import com.gitblit.Constants;
+import com.gitblit.Keys;
 import com.gitblit.models.GitNote;
 import com.gitblit.models.PathModel.PathChangeModel;
 import com.gitblit.models.SubmoduleModel;
@@ -59,8 +60,6 @@ public class CommitDiffPage extends RepositoryPage {
 
 		RevCommit commit = getCommit();
 
-		final DiffOutput diff = DiffUtils.getCommitDiff(r, commit, DiffOutputType.HTML);
-
 		List<String> parents = new ArrayList<String>();
 		if (commit.getParentCount() > 0) {
 			for (RevCommit parent : commit.getParents()) {
@@ -82,6 +81,11 @@ public class CommitDiffPage extends RepositoryPage {
 
 		add(new CommitHeaderPanel("commitHeader", repositoryName, commit));
 
+		final List<String> imageExtensions = app().settings().getStrings(Keys.web.imageExtensions);
+		final ImageDiffHandler handler = new ImageDiffHandler(getContextUrl(), repositoryName,
+				parents.isEmpty() ? null : parents.get(0), commit.getName(), imageExtensions);
+		final DiffOutput diff = DiffUtils.getCommitDiff(r, commit, DiffOutputType.HTML, handler);
+
 		// add commit diffstat
 		int insertions = 0;
 		int deletions = 0;
diff --git a/src/main/java/com/gitblit/wicket/pages/ComparePage.java b/src/main/java/com/gitblit/wicket/pages/ComparePage.java
index 3b8bb03e..dae8d8ea 100644
--- a/src/main/java/com/gitblit/wicket/pages/ComparePage.java
+++ b/src/main/java/com/gitblit/wicket/pages/ComparePage.java
@@ -37,6 +37,7 @@ import org.eclipse.jgit.diff.DiffEntry.ChangeType;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 
+import com.gitblit.Keys;
 import com.gitblit.models.PathModel.PathChangeModel;
 import com.gitblit.models.RefModel;
 import com.gitblit.models.RepositoryModel;
@@ -111,7 +112,11 @@ public class ComparePage extends RepositoryPage {
 			fromCommitId.setObject(startId);
 			toCommitId.setObject(endId);
 
-			final DiffOutput diff = DiffUtils.getDiff(r, fromCommit, toCommit, DiffOutputType.HTML);
+			final List<String> imageExtensions = app().settings().getStrings(Keys.web.imageExtensions);
+			final ImageDiffHandler handler = new ImageDiffHandler(getContextUrl(), repositoryName,
+					fromCommit.getName(), toCommit.getName(), imageExtensions);
+
+			final DiffOutput diff = DiffUtils.getDiff(r, fromCommit, toCommit, DiffOutputType.HTML, handler);
 
 			// add compare diffstat
 			int insertions = 0;
diff --git a/src/main/java/com/gitblit/wicket/pages/ImageDiffHandler.java b/src/main/java/com/gitblit/wicket/pages/ImageDiffHandler.java
new file mode 100644
index 00000000..69d84f4e
--- /dev/null
+++ b/src/main/java/com/gitblit/wicket/pages/ImageDiffHandler.java
@@ -0,0 +1,138 @@
+/*
+ * Copyright 2014 Tom <tw201207@gmail.com>
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.gitblit.wicket.pages;
+
+import java.nio.charset.StandardCharsets;
+import java.util.List;
+
+import org.apache.wicket.protocol.http.WicketURLEncoder;
+import org.eclipse.jgit.diff.DiffEntry;
+import org.eclipse.jgit.diff.DiffEntry.Side;
+import org.jsoup.nodes.Element;
+
+import com.gitblit.servlet.RawServlet;
+import com.gitblit.utils.DiffUtils;
+import com.gitblit.utils.HtmlBuilder;
+
+/**
+ * A {@link DiffUtils.BinaryDiffHandler BinaryDiffHandler} for images.
+ *
+ * @author Tom <tw201207@gmail.com>
+ */
+public class ImageDiffHandler implements DiffUtils.BinaryDiffHandler {
+
+	private final String oldCommitId;
+	private final String newCommitId;
+	private final String repositoryName;
+	private final String baseUrl;
+	private final List<String> imageExtensions;
+
+	public ImageDiffHandler(final String baseUrl, final String repositoryName, final String oldCommitId,
+			final String newCommitId, final List<String> imageExtensions) {
+		this.baseUrl = baseUrl;
+		this.repositoryName = repositoryName;
+		this.oldCommitId = oldCommitId;
+		this.newCommitId = newCommitId;
+		this.imageExtensions = imageExtensions;
+	}
+
+	/** {@inheritDoc} */
+	@Override
+	public String renderBinaryDiff(DiffEntry diffEntry) {
+		switch (diffEntry.getChangeType()) {
+		case MODIFY:
+		case RENAME:
+		case COPY:
+			// TODO: for very small images such as icons, the slider doesn't really help. Two possible
+			// approaches: either upscale them for display (may show blurry upscaled images), or show
+			// them side by side (may still be too small to really make out the differences).
+			String oldUrl = getImageUrl(diffEntry, Side.OLD);
+			String newUrl = getImageUrl(diffEntry, Side.NEW);
+			if (oldUrl != null && newUrl != null) {
+				HtmlBuilder builder = new HtmlBuilder("div");
+				Element container = builder.root().appendElement("div").attr("class", "imgdiff");
+				Element resizeable = container.appendElement("div").attr("class", "imgdiff-left");
+				// style='max-width:640px;' is necessary for ensuring that the browser limits large images
+				// to some reasonable width, and to override the "img { max-width: 100%; }" from bootstrap.css,
+				// which would scale the left image to the width of its resizeable container, which isn't what
+				// we want here. Note that the max-width must be defined directly as inline style on the element,
+				// otherwise browsers ignore it if the image is larger, and we end up with an image display that
+				// is too wide.
+				// XXX: Maybe add a max-height, too, to limit portrait-oriented images to some reasonable height?
+				// (Like a 300x10000px image...)
+				resizeable.appendElement("img").attr("class", "imgdiff imgdiff-left").attr("style", "max-width:640px;").attr("src", oldUrl);
+				container.appendElement("img").attr("class", "imgdiff").attr("style", "max-width:640px;").attr("src", newUrl);
+				return builder.toString();
+			}
+			break;
+		case ADD:
+			String url = getImageUrl(diffEntry, Side.NEW);
+			if (url != null) {
+				return new HtmlBuilder("img").root().attr("class", "diff-img").attr("src", url).toString();
+			}
+			break;
+		default:
+			break;
+		}
+		return null;
+	}
+
+	/**
+	 * Constructs a URL that will fetch the designated resource in the git repository. The returned string will
+	 * contain the URL fully URL-escaped, but note that it may still contain unescaped ampersands, so the result
+	 * must still be run through HTML escaping if it is to be used in HTML.
+	 *
+	 * @return the URL to the image, if the given {@link DiffEntry} and {@link Side} refers to an image, or {@code null} otherwise.
+	 */
+	protected String getImageUrl(DiffEntry entry, Side side) {
+		String path = entry.getPath(side);
+		int i = path.lastIndexOf('.');
+		if (i > 0) {
+			String extension = path.substring(i + 1);
+			for (String ext : imageExtensions) {
+				if (ext.equalsIgnoreCase(extension)) {
+					String commitId = Side.NEW.equals(side) ? newCommitId : oldCommitId;
+					if (commitId != null) {
+						return RawServlet.asLink(baseUrl, urlencode(repositoryName), commitId, urlencode(path));
+					} else {
+						return null;
+					}
+				}
+			}
+		}
+		return null;
+	}
+
+	/**
+	 * Encode a URL component of a {@link RawServlet} URL in the special way that the servlet expects it. Note that
+	 * the %-encoding used does not encode '&amp;' or '&lt;'. Slashes are not encoded in the result.
+	 *
+	 * @param component
+	 *            to encode using %-encoding
+	 * @return the encoded component
+	 */
+	protected String urlencode(final String component) {
+		// RawServlet handles slashes itself. Note that only the PATH_INSTANCE fits the bill here: it encodes
+		// spaces as %20, and we just have to correct for encoded slashes. Java's standard URLEncoder would
+		// encode spaces as '+', and I don't know what effects that would have on other parts of GitBlit. It
+		// would also be wrong for path components (but fine for a query part), so we'd have to correct it, too.
+		//
+		// Actually, this should be done in RawServlet.asLink(). As it is now, this may be incorrect if that
+		// operation ever uses query parameters instead of paths, or if it is fixed to urlencode its path
+		// components. But I don't want to touch that static method in RawServlet.
+		return WicketURLEncoder.PATH_INSTANCE.encode(component, StandardCharsets.UTF_8.name()).replaceAll("%2[fF]", "/");
+	}
+}