opendev/gerrit
Code Issues Proposed changes

SideBySide2: Rewrite padding mechanism to be based on diff chunks

Browse Source 
Calculating padding line-by-line is very slow on large changes.
Rewrite padding to be based on the size of each diff chunk,
queried using the heightAtLine() method provided by CodeMirror.

This new mechanism also takes care of calculating the padding for
any CommentBox appearing in a diff chunk. PaddingManager is now used
only for CommentBoxes not in diff chunks. Eventually the padding
logic will be unified and PaddingManager will be deleted.

Change-Id: I85dd48d5e3448bca3ae7316b4b9c9d13388dd139
This commit is contained in:
Michael Zhou
2013-07-23 11:19:48 -07:00
parent 0a40f5bbff
commit dbeedb11bf
6 changed files with 158 additions and 129 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -15,6 +15,7 @@
package com.google.gerrit.client.diff;
import com.google.gerrit.client.changes.CommentInfo;
import com.google.gerrit.client.diff.SideBySide2.DiffChunkInfo;
import com.google.gwt.core.client.Scheduler;
import com.google.gwt.core.client.Scheduler.ScheduledCommand;
import com.google.gwt.user.client.ui.Composite;
@@ -29,14 +30,22 @@ abstract class CommentBox extends Composite {
private PaddingManager widgetManager;
private LineWidget selfWidget;
private SideBySide2 parent;
private DiffChunkInfo diffChunkInfo;
void resizePaddingWidget() {
Scheduler.get().scheduleDeferred(new ScheduledCommand(){
selfWidget.changed();
Scheduler.get().scheduleDeferred(new ScheduledCommand() {
@Override
public void execute() {
assert selfWidget != null;
assert widgetManager != null;
selfWidget.changed();
widgetManager.resizePaddingWidget();
if (diffChunkInfo != null) {
parent.resizePaddingOnOtherSide(getCommentInfo().side(),
diffChunkInfo.getEnd());
} else {
assert selfWidget != null;
assert widgetManager != null;
widgetManager.resizePaddingWidget();
}
}
});
}
@@ -52,6 +61,14 @@ abstract class CommentBox extends Composite {
return widgetManager;
}
void setDiffChunkInfo(DiffChunkInfo info) {
this.diffChunkInfo = info;
}
void setParent(SideBySide2 parent) {
this.parent = parent;
}
void setPaddingManager(PaddingManager manager) {
widgetManager = manager;
}

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

@@ -25,6 +25,7 @@ import com.google.gwt.core.client.GWT;
import com.google.gwt.core.client.JavaScriptObject;
import com.google.gwt.core.client.Scheduler;
import com.google.gwt.core.client.Scheduler.RepeatingCommand;
import com.google.gwt.core.client.Scheduler.ScheduledCommand;
import com.google.gwt.dom.client.Element;
import com.google.gwt.event.dom.client.ClickEvent;
import com.google.gwt.event.dom.client.ClickHandler;
@@ -213,6 +214,12 @@ class DraftBox extends CommentBox {
parent.removeDraft(this, comment.side(), comment.line() - 1);
removeFromParent();
Scheduler.get().scheduleDeferred(new ScheduledCommand() {
@Override
public void execute() {
resizePaddingWidget();
}
});
getSelfWidget().clear();
PaddingManager manager = getPaddingManager();

71
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PaddingManager.java
View File

@@ -27,31 +27,22 @@ import java.util.List;
* changes as necessary. PaddingManager calculates padding by taking the
* difference of the sum of CommentBox heights on the two sides.
*
* Note that in the case of an insertion or deletion gap, A PaddingManager
* can map to a list of managers on the other side. The padding needed is then
* calculated from the sum of all their heights.
*
* TODO: Let PaddingManager also take care of the paddings introduced by
* insertions and deletions.
*/
class PaddingManager {
private List<CommentBox> comments;
private PaddingWidgetWrapper wrapper;
private List<PaddingManager> others;
private PaddingManager other;
PaddingManager(PaddingWidgetWrapper padding) {
comments = new ArrayList<CommentBox>();
others = new ArrayList<PaddingManager>();
this.wrapper = padding;
}
static void link(PaddingManager a, PaddingManager b) {
if (!a.others.contains(b)) {
a.others.add(b);
}
if (!b.others.contains(a)) {
b.others.add(a);
}
a.other = b;
b.other = a;
}
private int getMyTotalHeight() {
@@ -62,49 +53,22 @@ class PaddingManager {
return total;
}
/**
* If this instance is on the insertion side, its counterpart on the other
* side will map to a group of PaddingManagers on this side, so we calculate
* the group's total height instead of an individual one's.
*/
private int getGroupTotalHeight() {
if (others.size() > 1) {
return getMyTotalHeight();
} else {
return others.get(0).getOthersTotalHeight();
}
}
private int getOthersTotalHeight() {
int total = 0;
for (PaddingManager manager : others) {
total += manager.getMyTotalHeight();
}
return total;
}
private void setPaddingHeight(int height) {
SideBySide2.setHeightInPx(wrapper.element, height);
wrapper.widget.changed();
}
void resizePaddingWidget() {
if (others.isEmpty()) {
throw new IllegalStateException("resizePaddingWidget() called before linking");
}
int myHeight = getGroupTotalHeight();
int othersHeight = getOthersTotalHeight();
assert other != null;
int myHeight = getMyTotalHeight();
int othersHeight = other.getMyTotalHeight();
int paddingNeeded = othersHeight - myHeight;
if (paddingNeeded < 0) {
for (PaddingManager manager : others.get(0).others) {
manager.setPaddingHeight(0);
}
others.get(others.size() - 1).setPaddingHeight(-paddingNeeded);
setPaddingHeight(0);
other.setPaddingHeight(-paddingNeeded);
} else {
setPaddingHeight(paddingNeeded);
for (PaddingManager other : others) {
other.setPaddingHeight(0);
}
other.setPaddingHeight(0);
}
}
@@ -144,15 +108,22 @@ class PaddingManager {
}
static class LinePaddingWidgetWrapper extends PaddingWidgetWrapper {
private boolean common;
private int chunkLength;
private int otherLine;
LinePaddingWidgetWrapper(PaddingWidgetWrapper pair, boolean common) {
LinePaddingWidgetWrapper(PaddingWidgetWrapper pair, int otherLine, int chunkLength) {
super(pair.widget, pair.element);
this.common = common;
this.otherLine = otherLine;
this.chunkLength = chunkLength;
}
boolean isCommon() {
return common;
int getChunkLength() {
return chunkLength;
}
int getOtherLine() {
return otherLine;
}
}
}

156
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java
View File

@@ -111,8 +111,8 @@ public class SideBySide2 extends Screen {
private Map<LineHandle, CommentBox> lineActiveBoxMap;
private Map<LineHandle, PublishedBox> lineLastPublishedBoxMap;
private Map<LineHandle, PaddingManager> linePaddingManagerMap;
private Map<LineHandle, LinePaddingWidgetWrapper> linePaddingWidgetMap;
private Map<LineHandle, Element> lineElementMap;
private Map<LineHandle, LinePaddingWidgetWrapper> linePaddingOnOtherSideMap;
private List<DiffChunkInfo> diffChunks;
private List<SkippedLine> skips;
private int context;
@@ -346,8 +346,8 @@ public class SideBySide2 extends Screen {
cmA = displaySide(diffInfo.meta_a(), diffInfo.text_a(), diffTable.cmA);
cmB = displaySide(diffInfo.meta_b(), diffInfo.text_b(), diffTable.cmB);
skips = new ArrayList<SkippedLine>();
linePaddingWidgetMap = new HashMap<LineHandle, LinePaddingWidgetWrapper>();
lineElementMap = new HashMap<LineHandle, Element>();
linePaddingOnOtherSideMap = new HashMap<LineHandle, LinePaddingWidgetWrapper>();
diffChunks = new ArrayList<DiffChunkInfo>();
render(diffInfo);
allBoxes = new ArrayList<CommentBox>();
lineActiveBoxMap = new HashMap<LineHandle, CommentBox>();
@@ -442,17 +442,19 @@ public class SideBySide2 extends Screen {
mapper.appendCommon(commonCnt);
if (aLength < bLength) { // Edit with insertion
int insertCnt = bLength - aLength;
insertEmptyLines(cmA, mapper.getLineA(), mapper.getLineB(),
insertCnt, false);
mapper.appendInsert(insertCnt);
} else if (aLength > bLength) { // Edit with deletion
int deleteCnt = aLength - bLength;
insertEmptyLines(cmB, mapper.getLineB(), mapper.getLineA(),
deleteCnt, false);
mapper.appendDelete(deleteCnt);
}
insertEmptyLines(cmA, mapper.getLineA(), origLineB, commonCnt, true);
insertEmptyLines(cmB, mapper.getLineB(), origLineA, commonCnt, true);
int chunkEndA = mapper.getLineA() - 1;
int chunkEndB = mapper.getLineB() - 1;
if (bLength > 0) {
addDiffChunkAndPadding(cmA, chunkEndA, chunkEndB, bLength);
}
if (aLength > 0) {
addDiffChunkAndPadding(cmB, chunkEndB, chunkEndA, aLength);
}
markEdit(cmA, currentA, current.edit_a(), origLineA);
markEdit(cmB, currentB, current.edit_b(), origLineB);
}
@@ -503,7 +505,7 @@ public class SideBySide2 extends Screen {
} else {
// Estimated height at 28px, fixed by deferring after display
manager = new PaddingManager(
addPaddingWidget(cm, DiffTable.style.padding(), line, 28, Unit.PX, 0));
addPaddingWidget(cm, DiffTable.style.padding(), line, 0, Unit.PX, 0));
linePaddingManagerMap.put(handle, manager);
}
int lineToPad = mapper.lineOnOther(mySide, line).getLine();
@@ -512,7 +514,7 @@ public class SideBySide2 extends Screen {
PaddingManager.link(manager, linePaddingManagerMap.get(otherHandle));
} else {
PaddingManager otherManager = new PaddingManager(
addPaddingWidget(other, DiffTable.style.padding(), lineToPad, 28, Unit.PX, 0));
addPaddingWidget(other, DiffTable.style.padding(), lineToPad, 0, Unit.PX, 0));
linePaddingManagerMap.put(otherHandle, otherManager);
PaddingManager.link(manager, otherManager);
}
@@ -524,6 +526,8 @@ public class SideBySide2 extends Screen {
LineWidget boxWidget = cm.addLineWidget(line, box.getElement(), config);
box.setPaddingManager(manager);
box.setSelfWidget(boxWidget);
box.setParent(this);
box.setDiffChunkInfo(getDiffChunk(mySide, line));
allBoxes.add(box);
return box;
}
@@ -720,21 +724,14 @@ public class SideBySide2 extends Screen {
}
}
private void insertEmptyLines(CodeMirror cm, int nextLine, int lineOnOther,
int cnt, boolean common) {
for (int i = 0; i < cnt; i++, lineOnOther++) {
LineHandle handle = otherCm(cm).getLineHandle(lineOnOther);
/**
* Link a line with its padding widget on the other side. Note that we
* also add a collapsed padding for common lines, because they may
* linewrap differently due to intraline edits, in which case we detect
* the difference in line height and resize the padding widget.
*/
linePaddingWidgetMap.put(handle, new LinePaddingWidgetWrapper(
// -1 to compensate for the line we went past when this method is called.
addPaddingWidget(cm, DiffTable.style.padding(), nextLine - 1,
common ? 0 : 1.1, Unit.EM, null), common));
}
private void addDiffChunkAndPadding(CodeMirror cmToPad, int lineToPad,
int lineOnOther, int chunkSize) {
CodeMirror otherCm = otherCm(cmToPad);
linePaddingOnOtherSideMap.put(otherCm.getLineHandle(lineOnOther),
new LinePaddingWidgetWrapper(addPaddingWidget(cmToPad, DiffTable.style.padding(),
lineToPad, 0, Unit.EM, null), lineToPad, chunkSize));
diffChunks.add(new DiffChunkInfo(getSideFromCm(otherCm),
lineOnOther - chunkSize + 1, lineOnOther));
}
private PaddingWidgetWrapper addPaddingWidget(CodeMirror cm, String style,
@@ -884,50 +881,59 @@ public class SideBySide2 extends Screen {
};
}
private DiffChunkInfo getDiffChunk(Side side, int line) {
for (DiffChunkInfo info : diffChunks) {
if (info.getSide() == side && info.getStart() <= line &&
line <= info.getEnd()) {
return info;
}
}
return null;
}
void resizePaddingOnOtherSide(Side mySide, int line) {
CodeMirror cm = getCmFromSide(mySide);
LineHandle handle = cm.getLineHandle(line);
final LinePaddingWidgetWrapper otherWrapper = linePaddingOnOtherSideMap.get(handle);
double myChunkHeight = cm.heightAtLine(line + 1) -
cm.heightAtLine(line - otherWrapper.getChunkLength() + 1);
Element otherPadding = otherWrapper.getElement();
CodeMirror otherCm = otherCm(cm);
int otherLine = otherWrapper.getOtherLine();
LineHandle other = otherCm.getLineHandle(otherLine);
if (linePaddingOnOtherSideMap.containsKey(other)) {
LinePaddingWidgetWrapper myWrapper = linePaddingOnOtherSideMap.get(other);
Element myPadding = linePaddingOnOtherSideMap.get(other).getElement();
myChunkHeight -= myPadding.getOffsetHeight();
double otherChunkHeight = otherCm.heightAtLine(otherLine + 1) -
otherCm.heightAtLine(otherLine - myWrapper.getChunkLength() + 1) -
otherPadding.getOffsetHeight();
double delta = myChunkHeight - otherChunkHeight;
if (delta > 0) {
setHeightInPx(myPadding, 0);
setHeightInPx(otherPadding, delta);
} else {
setHeightInPx(myPadding, -delta);
setHeightInPx(otherPadding, 0);
}
myWrapper.getWidget().changed();
} else {
setHeightInPx(otherPadding, myChunkHeight);
}
otherWrapper.getWidget().changed();
}
// TODO: Maybe integrate this with PaddingManager.
private RenderLineHandler resizeEmptyLine(final Side side) {
return new RenderLineHandler() {
@Override
public void handle(final CodeMirror instance, final LineHandle handle,
final Element element) {
if (linePaddingWidgetMap.containsKey(handle)) {
/**
* This needs to be deferred because CM fires "renderLine" **before**
* the line is actually added to the DOM.
*/
Element element) {
if (linePaddingOnOtherSideMap.containsKey(handle)) {
Scheduler.get().scheduleDeferred(new ScheduledCommand() {
@Override
public void execute() {
LinePaddingWidgetWrapper wrapper = linePaddingWidgetMap.get(handle);
int myLineHeight = element.getOffsetHeight();
Element otherPadding = wrapper.getElement();
if (!wrapper.isCommon()) {
setHeightInPx(otherPadding, myLineHeight);
} else {
/**
* We have to pay the cost of keeping track of the actual DOM
* elements ourselves, because CM doesn't provide an interface
* to query for them, and the only place we can ever have legit
* access to the line element is in this event handler.
*/
lineElementMap.put(handle, element);
// The lines are always aligned since they are in a common region.
int otherLine = mapper.lineOnOther(side,
instance.getLineNumber(handle)).getLine();
LineHandle other = otherCm(instance).getLineHandle(otherLine);
if (lineElementMap.containsKey(other)) {
Element otherElement = lineElementMap.get(other);
Element myPadding = linePaddingWidgetMap.get(other).getElement();
int delta = myLineHeight - otherElement.getOffsetHeight();
if (delta >= 0) {
setHeightInPx(otherPadding, delta);
setHeightInPx(myPadding, 0);
} else {
setHeightInPx(otherPadding, 0);
setHeightInPx(myPadding, -delta);
}
}
}
resizePaddingOnOtherSide(side, instance.getLineNumber(handle));
}
});
}
@@ -995,6 +1001,30 @@ public class SideBySide2 extends Screen {
}
}
static class DiffChunkInfo {
private Side side;
private int start;
private int end;
DiffChunkInfo(Side side, int start, int end) {
this.side = side;
this.start = start;
this.end = end;
}
Side getSide() {
return side;
}
int getStart() {
return start;
}
int getEnd() {
return end;
}
}
private static class NoOpKeyCommand extends KeyCommand {
private NoOpKeyCommand(int mask, int key, String help) {
super(mask, key, help);

14
gerrit-gwtui/src/main/java/net/codemirror/lib/CodeMirror.java vendored
View File

@@ -52,9 +52,9 @@ public class CodeMirror extends JavaScriptObject {
public final native void setValue(String v) /*-{ this.setValue(v); }-*/;
public final native void setWidth(int w) /*-{ this.setSize(w, null); }-*/;
public final native void setWidth(double w) /*-{ this.setSize(w, null); }-*/;
public final native void setWidth(String w) /*-{ this.setSize(w, null); }-*/;
public final native void setHeight(int h) /*-{ this.setSize(null, h); }-*/;
public final native void setHeight(double h) /*-{ this.setSize(null, h); }-*/;
public final native void setHeight(String h) /*-{ this.setSize(null, h); }-*/;
public final native void refresh() /*-{ this.refresh(); }-*/;
@@ -120,19 +120,23 @@ public class CodeMirror extends JavaScriptObject {
return this.addLineWidget(line, node, options);
}-*/;
public final native int lineAtHeight(int height) /*-{
public final native int lineAtHeight(double height) /*-{
return this.lineAtHeight(height);
}-*/;
public final native double heightAtLine(int line) /*-{
return this.heightAtLine(line);
}-*/;
public final native CodeMirrorDoc getDoc() /*-{
return this.getDoc();
}-*/;
public final native void scrollTo(int x, int y) /*-{
public final native void scrollTo(double x, double y) /*-{
this.scrollTo(x, y);
}-*/;
public final native void scrollToY(int y) /*-{
public final native void scrollToY(double y) /*-{
this.scrollTo(null, y);
}-*/;

12
gerrit-gwtui/src/main/java/net/codemirror/lib/ScrollInfo.java vendored
View File

@@ -23,12 +23,12 @@ public class ScrollInfo extends JavaScriptObject {
return createObject().cast();
}
public final native int getLeft() /*-{ return this.left; }-*/;
public final native int getTop() /*-{ return this.top; }-*/;
public final native int getWidth() /*-{ return this.width; }-*/;
public final native int getHeight() /*-{ return this.height; }-*/;
public final native int getClientWidth() /*-{ return this.clientWidth; }-*/;
public final native int getClientHeight() /*-{ return this.clientHeight; }-*/;
public final native double getLeft() /*-{ return this.left; }-*/;
public final native double getTop() /*-{ return this.top; }-*/;
public final native double getWidth() /*-{ return this.width; }-*/;
public final native double getHeight() /*-{ return this.height; }-*/;
public final native double getClientWidth() /*-{ return this.clientWidth; }-*/;
public final native double getClientHeight() /*-{ return this.clientHeight; }-*/;
protected ScrollInfo() {