From 067a7e8dc4e819a74e1ee4ab07b569fdef594414 Mon Sep 17 00:00:00 2001 From: Michael Zhou Date: Sun, 27 Mar 2016 03:53:10 -0400 Subject: [PATCH] Unified: Support range comments Handle range comments in Unified. In general this works the same way as in SideBySide. The interesting cases come up when the user's selection spans multiple diff regions that are impossible to combine in SideBySide. For example, the selection can span a deletion chunk and an insertion chunk. In these cases, we forcibly move the start line to the corresponding line on the other side as if it were displayed on SideBySide. This is a natural "fix" to the lousy selection made by the user and probably reflects what the user means. Note that range comments spanning multiple regions might cause text that aren't actually part of the comment range to be highlighted. For example, if the user puts a range comment that spans the entire file, then every line will be highlighted in Unified, even though the deletion chunks are technically not part of the comment range. While we might be able to refine the highlighting, it will need a more complicated implementation that doesn't seem to worth the trouble. Change-Id: I8ec6ee3f710a31085ebe8c3f28414d76d498286c --- .../google/gerrit/client/diff/CommentBox.java | 9 ++- .../gerrit/client/diff/CommentGroup.java | 4 + .../gerrit/client/diff/CommentManager.java | 12 +++ .../google/gerrit/client/diff/DiffScreen.java | 2 + .../google/gerrit/client/diff/SideBySide.java | 5 ++ .../client/diff/SideBySideCommentManager.java | 9 +-- .../google/gerrit/client/diff/Unified.java | 7 +- .../client/diff/UnifiedChunkManager.java | 70 +++++++++++------ .../client/diff/UnifiedCommentManager.java | 78 ++++++++++++++++--- 9 files changed, 150 insertions(+), 46 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java index 4654a3d8b0..4fe29147be 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java @@ -24,6 +24,7 @@ import com.google.gwt.user.client.ui.Composite; import net.codemirror.lib.CodeMirror; import net.codemirror.lib.Configuration; +import net.codemirror.lib.Pos; import net.codemirror.lib.TextMarker; import net.codemirror.lib.TextMarker.FromTo; @@ -56,7 +57,13 @@ abstract class CommentBox extends Composite { CommentBox(CommentGroup group, CommentRange range) { this.group = group; if (range != null) { - fromTo = FromTo.create(range); + DiffScreen screen = group.getManager().getDiffScreen(); + int startCmLine = + screen.getCmLine(range.startLine() - 1, group.getSide()); + int endCmLine = screen.getCmLine(range.endLine() - 1, group.getSide()); + fromTo = FromTo.create( + Pos.create(startCmLine, range.startCharacter()), + Pos.create(endCmLine, range.endCharacter())); rangeMarker = group.getCm().markText( fromTo.from(), fromTo.to(), diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentGroup.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentGroup.java index d48ada7497..20dd8832b7 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentGroup.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentGroup.java @@ -63,6 +63,10 @@ abstract class CommentGroup extends Composite { return line; } + DisplaySide getSide() { + return side; + } + void add(PublishedBox box) { comments.add(box); comments.setVisible(true); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java index 43864361ae..7f4e446246 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java @@ -24,6 +24,8 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gwt.core.client.JsArray; import net.codemirror.lib.CodeMirror; +import net.codemirror.lib.Pos; +import net.codemirror.lib.TextMarker.FromTo; import java.util.HashMap; import java.util.HashSet; @@ -130,6 +132,16 @@ abstract class CommentManager { return forSide; } + static FromTo adjustSelection(CodeMirror cm) { + FromTo fromTo = cm.getSelectedRange(); + Pos to = fromTo.to(); + if (to.ch() == 0) { + to.line(to.line() - 1); + to.ch(cm.getLine(to.line()).length()); + } + return fromTo; + } + abstract void insertNewDraft(DisplaySide side, int line); abstract Runnable newDraftCallback(final CodeMirror cm); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java index 6d076c9949..01d6b16361 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java @@ -934,6 +934,8 @@ abstract class DiffScreen extends Screen { abstract DiffTable getDiffTable(); + abstract int getCmLine(int line, DisplaySide side); + LineOnOtherInfo lineOnOther(DisplaySide side, int line) { return getChunkManager().getLineMapper().lineOnOther(side, line); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java index ab1b06dc0f..fa7e0e4dec 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java @@ -309,6 +309,11 @@ public class SideBySide extends DiffScreen { return side == DisplaySide.A ? cmA : cmB; } + @Override + int getCmLine(int line, DisplaySide side) { + return line; + } + @Override Runnable updateActiveLine(final CodeMirror cm) { final CodeMirror other = otherCm(cm); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java index bfd4c18ffc..cf9dd0fb27 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java @@ -333,18 +333,13 @@ class SideBySideCommentManager extends CommentManager { private void newDraft(CodeMirror cm) { int line = cm.getLineNumber(cm.extras().activeLine()) + 1; if (cm.somethingSelected()) { - FromTo fromTo = cm.getSelectedRange(); - Pos end = fromTo.to(); - if (end.ch() == 0) { - end.line(end.line() - 1); - end.ch(cm.getLine(end.line()).length()); - } - + FromTo fromTo = adjustSelection(cm); addDraftBox(cm.side(), CommentInfo.create( getPath(), getStoredSideFromDisplaySide(cm.side()), line, CommentRange.create(fromTo))).setEdit(true); + cm.setCursor(fromTo.to()); cm.setSelection(cm.getCursor()); } else { insertNewDraft(cm.side(), line); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Unified.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Unified.java index a1c30d577b..6b48f159b5 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Unified.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Unified.java @@ -18,7 +18,7 @@ import static java.lang.Double.POSITIVE_INFINITY; import com.google.gerrit.client.Dispatcher; import com.google.gerrit.client.Gerrit; -import com.google.gerrit.client.diff.UnifiedChunkManager.LineSidePair; +import com.google.gerrit.client.diff.UnifiedChunkManager.LineRegionInfo; import com.google.gerrit.client.patches.PatchUtil; import com.google.gerrit.client.projects.ConfigInfoCache; import com.google.gerrit.client.rpc.ScreenLoadCallback; @@ -363,12 +363,13 @@ public class Unified extends DiffScreen { return cm; } + @Override int getCmLine(int line, DisplaySide side) { return chunkManager.getCmLine(line, side); } - LineSidePair getLineSidePairFromCmLine(int cmLine) { - return chunkManager.getLineSidePairFromCmLine(cmLine); + LineRegionInfo getLineRegionInfoFromCmLine(int cmLine) { + return chunkManager.getLineRegionInfoFromCmLine(cmLine); } @Override diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedChunkManager.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedChunkManager.java index c7eca0b232..1e6ed0c7ea 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedChunkManager.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedChunkManager.java @@ -279,52 +279,72 @@ class UnifiedChunkManager extends ChunkManager { } } - LineSidePair getLineSidePairFromCmLine(int cmLine) { + LineRegionInfo getLineRegionInfoFromCmLine(int cmLine) { int res = Collections.binarySearch(chunks, new UnifiedDiffChunkInfo( DisplaySide.A, 0, 0, cmLine, false), // Dummy DiffChunkInfo getDiffChunkComparatorCmLine()); - if (res >= 0) { + if (res >= 0) { // The line is right at the start of a diff chunk. UnifiedDiffChunkInfo info = chunks.get(res); - return new LineSidePair(info.getStart(), info.getSide()); - } else { // The line might be within a DiffChunk + return new LineRegionInfo( + info.getStart(), displaySideToRegionType(info.getSide())); + } else { // The line might be within or after a diff chunk. res = -res - 1; if (res > 0) { UnifiedDiffChunkInfo info = chunks.get(res - 1); int lineOnInfoSide = info.getStart() + cmLine - info.getCmLine(); - if (lineOnInfoSide > info.getEnd() - && info.getSide() == DisplaySide.A) { - // For the common region after a deletion chunk, return the line and - // side info on side B - return new LineSidePair( - getLineMapper().lineOnOther(DisplaySide.A, lineOnInfoSide) - .getLine(), DisplaySide.B); - } else { - return new LineSidePair(lineOnInfoSide, info.getSide()); + if (lineOnInfoSide > info.getEnd()) { // After a diff chunk + if (info.getSide() == DisplaySide.A) { + // For the common region after a deletion chunk, associate the line + // on side B with a common region. + return new LineRegionInfo( + getLineMapper().lineOnOther(DisplaySide.A, lineOnInfoSide) + .getLine(), RegionType.COMMON); + } else { + return new LineRegionInfo(lineOnInfoSide, RegionType.COMMON); + } + } else { // Within a diff chunk + return new LineRegionInfo( + lineOnInfoSide, displaySideToRegionType(info.getSide())); } } else { - // Always return side B - return new LineSidePair(cmLine, DisplaySide.B); + // The line is before any diff chunk, so it always equals cmLine and + // belongs to a common region. + return new LineRegionInfo(cmLine, RegionType.COMMON); } } } - static class LineSidePair { - private int line; - private DisplaySide side; + enum RegionType { + INSERT, DELETE, COMMON, + } - LineSidePair(int line, DisplaySide side) { + private static RegionType displaySideToRegionType(DisplaySide side) { + return side == DisplaySide.A ? RegionType.DELETE : RegionType.INSERT; + } + + /** + * Helper class to associate a line in the original file with the type of the + * region it belongs to. + * + * @field line The 0-based line number in the original file. Note that this + * might be different from the line number shown in CodeMirror. + * @field type The type of the region the line belongs to. Can be INSERT, + * DELETE or COMMON. + */ + static class LineRegionInfo { + final int line; + final RegionType type; + + LineRegionInfo(int line, RegionType type) { this.line = line; - this.side = side; - } - - int getLine() { - return line; + this.type = type; } DisplaySide getSide() { - return side; + // Always return DisplaySide.B for INSERT or COMMON + return type == RegionType.DELETE ? DisplaySide.A : DisplaySide.B; } } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedCommentManager.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedCommentManager.java index 9038fb4357..411b64f811 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedCommentManager.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedCommentManager.java @@ -16,7 +16,9 @@ package com.google.gerrit.client.diff; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.changes.CommentInfo; -import com.google.gerrit.client.diff.UnifiedChunkManager.LineSidePair; +import com.google.gerrit.client.diff.LineMapper.LineOnOtherInfo; +import com.google.gerrit.client.diff.UnifiedChunkManager.LineRegionInfo; +import com.google.gerrit.client.diff.UnifiedChunkManager.RegionType; import com.google.gerrit.client.patches.SkippedLine; import com.google.gerrit.client.rpc.Natives; import com.google.gerrit.client.ui.CommentLinkProcessor; @@ -26,6 +28,7 @@ import com.google.gwt.core.client.JsArray; import net.codemirror.lib.CodeMirror; import net.codemirror.lib.CodeMirror.LineHandle; import net.codemirror.lib.Pos; +import net.codemirror.lib.TextMarker.FromTo; import java.util.ArrayList; import java.util.List; @@ -179,10 +182,10 @@ class UnifiedCommentManager extends CommentManager { ((PublishedBox)last).doReply(); } } else { - LineSidePair pair = host.getLineSidePairFromCmLine(cmLinePlusOne - 1); - int line = pair.getLine(); - if (pair.getSide() != side) { - line = host.lineOnOther(pair.getSide(), line).getLine(); + LineRegionInfo info = host.getLineRegionInfoFromCmLine(cmLinePlusOne - 1); + int line = info.line; + if (info.getSide() != side) { + line = host.lineOnOther(info.getSide(), line).getLine(); } addDraftBox(side, CommentInfo.create( getPath(), @@ -335,13 +338,68 @@ class UnifiedCommentManager extends CommentManager { } private void newDraft(CodeMirror cm) { - int cmLine = cm.getLineNumber(cm.extras().activeLine()); - LineSidePair pair = host.getLineSidePairFromCmLine(cmLine); - DisplaySide side = pair.getSide(); if (cm.somethingSelected()) { - // TODO: Handle range comment + FromTo fromTo = adjustSelection(cm); + Pos from = fromTo.from(); + Pos to = fromTo.to(); + UnifiedChunkManager manager = host.getChunkManager(); + LineRegionInfo fromInfo = + host.getLineRegionInfoFromCmLine(from.line()); + LineRegionInfo toInfo = + host.getLineRegionInfoFromCmLine(to.line()); + DisplaySide side = toInfo.getSide(); + + // Handle special cases in selections that span multiple regions. Force + // start line to be on the same side as the end line. + if ((fromInfo.type == RegionType.INSERT + || fromInfo.type == RegionType.COMMON) + && toInfo.type == RegionType.DELETE) { + LineOnOtherInfo infoOnSideA = manager.getLineMapper() + .lineOnOther(DisplaySide.B, fromInfo.line); + int startLineOnSideA = infoOnSideA.getLine(); + if (infoOnSideA.isAligned()) { + from.line(startLineOnSideA); + } else { + from.line(startLineOnSideA + 1); + } + from.ch(0); + to.line(toInfo.line); + } else if (fromInfo.type == RegionType.DELETE + && toInfo.type == RegionType.INSERT) { + LineOnOtherInfo infoOnSideB = manager.getLineMapper() + .lineOnOther(DisplaySide.A, fromInfo.line); + int startLineOnSideB = infoOnSideB.getLine(); + if (infoOnSideB.isAligned()) { + from.line(startLineOnSideB); + } else { + from.line(startLineOnSideB + 1); + } + from.ch(0); + to.line(toInfo.line); + } else if (fromInfo.type == RegionType.DELETE + && toInfo.type == RegionType.COMMON) { + int toLineOnSideA = manager.getLineMapper() + .lineOnOther(DisplaySide.B, toInfo.line).getLine(); + from.line(fromInfo.line); + // Force the end line to be on the same side as the start line. + to.line(toLineOnSideA); + side = DisplaySide.A; + } else { // Common case + from.line(fromInfo.line); + to.line(toInfo.line); + } + + addDraftBox(side, CommentInfo.create( + getPath(), + getStoredSideFromDisplaySide(side), + to.line() + 1, + CommentRange.create(fromTo))).setEdit(true); + cm.setCursor(Pos.create(host.getCmLine(to.line(), side), to.ch())); + cm.setSelection(cm.getCursor()); } else { - insertNewDraft(side, cmLine + 1); + int cmLine = cm.getLineNumber(cm.extras().activeLine()); + LineRegionInfo info = host.getLineRegionInfoFromCmLine(cmLine); + insertNewDraft(info.getSide(), cmLine + 1); } }