opendev/gerrit
Code Issues Proposed changes

Unified: Support range comments

Browse Source 
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
This commit is contained in:
Michael Zhou 2016-03-27 03:53:10 -04:00
parent b9ed112277
commit 067a7e8dc4
9 changed files with 150 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java
View File

@ -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(),

4
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentGroup.java
View File

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

12
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java
View File

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

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java
View File

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

5
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java
View File

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

9
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java
View File

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

7
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Unified.java
View File

@ -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

70
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedChunkManager.java
View File

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

78
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedCommentManager.java
View File

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