opendev/gerrit
Code Issues Proposed changes

Revert "Make DiffChunkInfo implement Comparable to simplify code"

Browse Source 
This reverts commit 007a21aa22.

Reverting this commit fixes navigation to the next diff chunk on the
side-by-side diff view by typing 'n'. For some diffs navigation went
in circles between the first diffs, but didn't advance further (e.g.
see [1]).

This bug is very annoying for users that rely on this feature and we
got many bug reports for it.

Since the commit that is reverted only simplified code, reverting it
should not cause any other regressions.

[1] https://gerrit-review.googlesource.com/#/c/78117/3/gitiles-servlet/src/main/java/com/google/gitiles/ReadmeHelper.java

Change-Id: I3eb405032b185826558e087675317a0b49140686
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2016-05-31 10:34:30 +02:00
parent 7eea8e6bff
commit bed19b1334
7 changed files with 95 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/ChunkManager.java
View File

@@ -25,6 +25,7 @@ import net.codemirror.lib.Pos;
import net.codemirror.lib.TextMarker;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
/** Colors modified regions for {@link SideBySide} and {@link Unified}. */
@@ -101,7 +102,7 @@ abstract class ChunkManager {
DiffChunkInfo lookUp = chunks.get(res);
// If edit, skip the deletion chunk and set focus on the insertion one.
if (lookUp.edit && lookUp.side == A) {
if (lookUp.isEdit() && lookUp.getSide() == A) {
res = res + (dir == Direction.PREV ? -1 : 1);
if (res < 0 || chunks.size() <= res) {
return;
@@ -109,8 +110,8 @@ abstract class ChunkManager {
}
DiffChunkInfo target = chunks.get(res);
CodeMirror targetCm = host.getCmFromSide(target.side);
int cmLine = getCmLine(target.start, target.side);
CodeMirror targetCm = host.getCmFromSide(target.getSide());
int cmLine = getCmLine(target.getStart(), target.getSide());
targetCm.setCursor(Pos.create(cmLine));
targetCm.focus();
targetCm.scrollToY(
@@ -118,5 +119,28 @@ abstract class ChunkManager {
- 0.5 * targetCm.scrollbarV().getClientHeight());
}
Comparator<DiffChunkInfo> getDiffChunkComparator() {
// Chunks are ordered by their starting line. If it's a deletion,
// use its corresponding line on the revision side for comparison.
// In the edit case, put the deletion chunk right before the
// insertion chunk. This placement guarantees well-ordering.
return new Comparator<DiffChunkInfo>() {
@Override
public int compare(DiffChunkInfo a, DiffChunkInfo b) {
if (a.getSide() == b.getSide()) {
return a.getStart() - b.getStart();
} else if (a.getSide() == A) {
int comp = lineMapper.lineOnOther(a.getSide(), a.getStart())
.getLine() - b.getStart();
return comp == 0 ? -1 : comp;
} else {
int comp = a.getStart() -
lineMapper.lineOnOther(b.getSide(), b.getStart()).getLine();
return comp == 0 ? 1 : comp;
}
}
};
}
abstract int getCmLine(int line, DisplaySide side);
}

45
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffChunkInfo.java
View File

@@ -15,39 +15,32 @@
package com.google.gerrit.client.diff;
/** Object recording the position of a diff chunk and whether it's an edit */
class DiffChunkInfo implements Comparable<DiffChunkInfo> {
final DisplaySide side;
final int start;
final int end;
final boolean edit;
class DiffChunkInfo {
private DisplaySide side;
private int start;
private int end;
private boolean edit;
private final int startOnOther;
DiffChunkInfo(DisplaySide side, int start, int startOnOther, int end,
boolean edit) {
DiffChunkInfo(DisplaySide side, int start, int end, boolean edit) {
this.side = side;
this.start = start;
this.startOnOther = startOnOther;
this.end = end;
this.edit = edit;
}
/**
* Chunks are ordered by their starting line. If it's a deletion, use its
* corresponding line on the revision side for comparison. In the edit case,
* put the deletion chunk right before the insertion chunk. This placement
* guarantees well-ordering.
*/
@Override
public int compareTo(DiffChunkInfo o) {
if (side == o.side) {
return start - o.start;
} else if (side == DisplaySide.A) {
int comp = startOnOther - o.start;
return comp == 0 ? -1 : comp;
} else {
int comp = start - o.startOnOther;
return comp == 0 ? 1 : comp;
DisplaySide getSide() {
return side;
}
int getStart() {
return start;
}
int getEnd() {
return end;
}
boolean isEdit() {
return edit;
}
}

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

@@ -118,12 +118,12 @@ public class SideBySide extends DiffScreen {
if (getStartLine() == 0) {
DiffChunkInfo d = chunkManager.getFirst();
if (d != null) {
if (d.edit && d.side == DisplaySide.A) {
if (d.isEdit() && d.getSide() == DisplaySide.A) {
setStartSide(DisplaySide.B);
setStartLine(lineOnOther(d.side, d.start).getLine() + 1);
setStartLine(lineOnOther(d.getSide(), d.getStart()).getLine() + 1);
} else {
setStartSide(d.side);
setStartLine(d.start + 1);
setStartSide(d.getSide());
setStartLine(d.getStart() + 1);
}
}
}

11
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideChunkManager.java
View File

@@ -170,10 +170,10 @@ class SideBySideChunkManager extends ChunkManager {
int endA = lineMapper.getLineA() - 1;
int endB = lineMapper.getLineB() - 1;
if (aLen > 0) {
addDiffChunk(cmB, endB, endA, aLen, bLen > 0);
addDiffChunk(cmB, endA, aLen, bLen > 0);
}
if (bLen > 0) {
addDiffChunk(cmA, endA, endB, bLen, aLen > 0);
addDiffChunk(cmA, endB, bLen, aLen > 0);
}
}
@@ -245,10 +245,10 @@ class SideBySideChunkManager extends ChunkManager {
}
}
private void addDiffChunk(CodeMirror cmToPad, int line, int lineOnOther,
private void addDiffChunk(CodeMirror cmToPad, int lineOnOther,
int chunkSize, boolean edit) {
chunks.add(new DiffChunkInfo(host.otherCm(cmToPad).side(),
lineOnOther - chunkSize + 1, line - chunkSize + 1, lineOnOther, edit));
lineOnOther - chunkSize + 1, lineOnOther, edit));
}
@Override
@@ -261,7 +261,8 @@ class SideBySideChunkManager extends ChunkManager {
: 0;
int res = Collections.binarySearch(
chunks,
new DiffChunkInfo(cm.side(), line, 0, 0, false));
new DiffChunkInfo(cm.side(), line, 0, false),
getDiffChunkComparator());
diffChunkNavHelper(chunks, host, res, dir);
}
};

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

@@ -116,12 +116,12 @@ public class Unified extends DiffScreen {
if (getStartLine() == 0) {
DiffChunkInfo d = chunkManager.getFirst();
if (d != null) {
if (d.edit && d.side == DisplaySide.A) {
if (d.isEdit() && d.getSide() == DisplaySide.A) {
setStartSide(DisplaySide.B);
} else {
setStartSide(d.side);
setStartSide(d.getSide());
}
setStartLine(chunkManager.getCmLine(d.start, d.side) + 1);
setStartLine(chunkManager.getCmLine(d.getStart(), d.getSide()) + 1);
}
}
if (getStartSide() != null && getStartLine() > 0) {

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

@@ -140,14 +140,14 @@ class UnifiedChunkManager extends ChunkManager {
int endA = lineMapper.getLineA() - 1;
int endB = lineMapper.getLineB() - 1;
if (aLen > 0) {
addDiffChunk(DisplaySide.A, endA, endB, aLen, cmLine, bLen > 0);
addDiffChunk(DisplaySide.A, endA, aLen, cmLine, bLen > 0);
for (int j = 0; j < aLen; j++) {
host.setLineNumber(DisplaySide.A, cmLine + j, startA + j + 1);
host.setLineNumberEmpty(DisplaySide.B, cmLine + j);
}
}
if (bLen > 0) {
addDiffChunk(DisplaySide.B, endB, endA, bLen, cmLine + aLen, aLen > 0);
addDiffChunk(DisplaySide.B, endB, bLen, cmLine + aLen, aLen > 0);
for (int j = 0; j < bLen; j++) {
host.setLineNumberEmpty(DisplaySide.A, cmLine + aLen + j);
host.setLineNumber(DisplaySide.B, cmLine + aLen + j, startB + j + 1);
@@ -208,10 +208,10 @@ class UnifiedChunkManager extends ChunkManager {
: UnifiedTable.style.diffInsert();
}
private void addDiffChunk(DisplaySide side, int chunkEnd, int otherChunkEnd,
int chunkSize, int cmLine, boolean edit) {
chunks.add(new UnifiedDiffChunkInfo(side, chunkEnd - chunkSize + 1,
otherChunkEnd - chunkSize + 1, chunkEnd, cmLine, edit));
private void addDiffChunk(DisplaySide side, int chunkEnd, int chunkSize,
int cmLine, boolean edit) {
chunks.add(new UnifiedDiffChunkInfo(side, chunkEnd - chunkSize + 1, chunkEnd,
cmLine, edit));
}
@Override
@@ -224,7 +224,7 @@ class UnifiedChunkManager extends ChunkManager {
: 0;
int res = Collections.binarySearch(
chunks,
new UnifiedDiffChunkInfo(cm.side(), 0, 0, 0, line, false),
new UnifiedDiffChunkInfo(cm.side(), 0, 0, line, false),
getDiffChunkComparatorCmLine());
diffChunkNavHelper(chunks, host, res, dir);
}
@@ -236,7 +236,7 @@ class UnifiedChunkManager extends ChunkManager {
return new Comparator<UnifiedDiffChunkInfo>() {
@Override
public int compare(UnifiedDiffChunkInfo o1, UnifiedDiffChunkInfo o2) {
return o1.cmLine - o2.cmLine;
return o1.getCmLine() - o2.getCmLine();
}
};
}
@@ -246,30 +246,31 @@ class UnifiedChunkManager extends ChunkManager {
int res =
Collections.binarySearch(chunks,
new UnifiedDiffChunkInfo(
side, line, 0, 0, 0, false)); // Dummy DiffChunkInfo
side, line, 0, 0, false), // Dummy DiffChunkInfo
getDiffChunkComparator());
if (res >= 0) {
return chunks.get(res).cmLine;
return chunks.get(res).getCmLine();
} else { // The line might be within a DiffChunk
res = -res - 1;
if (res > 0) {
UnifiedDiffChunkInfo info = chunks.get(res - 1);
if (side == DisplaySide.A && info.edit
&& info.side == DisplaySide.B) {
if (side == DisplaySide.A && info.isEdit()
&& info.getSide() == DisplaySide.B) {
// Need to use the start and cmLine of the deletion chunk
UnifiedDiffChunkInfo delete = chunks.get(res - 2);
if (line <= delete.end) {
return delete.cmLine + line - delete.start;
if (line <= delete.getEnd()) {
return delete.getCmLine() + line - delete.getStart();
} else {
// Need to add the length of the insertion chunk
return delete.cmLine + line - delete.start
+ info.end - info.start + 1;
return delete.getCmLine() + line - delete.getStart()
+ info.getEnd() - info.getStart() + 1;
}
} else if (side == info.side) {
return info.cmLine + line - info.start;
} else if (side == info.getSide()) {
return info.getCmLine() + line - info.getStart();
} else {
return info.cmLine
return info.getCmLine()
+ lineMapper.lineOnOther(side, line).getLine()
- info.start;
- info.getStart();
}
} else {
return line;
@@ -281,19 +282,19 @@ class UnifiedChunkManager extends ChunkManager {
int res =
Collections.binarySearch(chunks,
new UnifiedDiffChunkInfo(
DisplaySide.A, 0, 0, 0, cmLine, false), // Dummy DiffChunkInfo
DisplaySide.A, 0, 0, cmLine, false), // Dummy DiffChunkInfo
getDiffChunkComparatorCmLine());
if (res >= 0) { // The line is right at the start of a diff chunk.
UnifiedDiffChunkInfo info = chunks.get(res);
return new LineRegionInfo(
info.start, displaySideToRegionType(info.side));
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.start + cmLine - info.cmLine;
if (lineOnInfoSide > info.end) { // After a diff chunk
if (info.side == DisplaySide.A) {
int lineOnInfoSide = info.getStart() + cmLine - info.getCmLine();
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(
@@ -304,7 +305,7 @@ class UnifiedChunkManager extends ChunkManager {
}
} else { // Within a diff chunk
return new LineRegionInfo(
lineOnInfoSide, displaySideToRegionType(info.side));
lineOnInfoSide, displaySideToRegionType(info.getSide()));
}
} else {
// The line is before any diff chunk, so it always equals cmLine and

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

@@ -16,11 +16,15 @@ package com.google.gerrit.client.diff;
public class UnifiedDiffChunkInfo extends DiffChunkInfo {
final int cmLine;
private int cmLine;
UnifiedDiffChunkInfo(DisplaySide side, int start, int startOnOther, int end,
int cmLine, boolean edit) {
super(side, start, startOnOther, end, edit);
UnifiedDiffChunkInfo(DisplaySide side,
int start, int end, int cmLine, boolean edit) {
super(side, start, end, edit);
this.cmLine = cmLine;
}
int getCmLine() {
return cmLine;
}
}