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); } }