opendev/gerrit
Code Issues Proposed changes

Allow large patches to function reasonably in diffs

Browse Source 
This allows extremely large patches to function in the diff view
reasonably.  ChangeScreen doesn't like expanding a large number
of lines at once, so the maximum context is still limited, and
every JavaScript engine but Chrome is extremely slow when running
Prettify over a large patch.  CodeMirror doesn't have these same
issues, so the changes are not applied to ChangeScreen2.  Because
CodeMirror expects the whole file context to be available, this
fixes the behavior of large patches with ChangeScreen2.

Bug: Issue 1233
Bug: Issue 2166
Change-Id: Idcb6991ccba41d4139aa85014eef496f9f2c035c
This commit is contained in:
Doug Kelly
2013-10-04 18:23:09 -05:00
parent ea2272ce36
commit 0b2f09be68
4 changed files with 52 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java
View File

@@ -50,6 +50,7 @@ import com.google.gwtexpui.globalkey.client.KeyCommandSet;
public abstract class PatchScreen extends Screen implements
CommentEditorContainer {
static final PrettyFactory PRETTY = ClientSideFormatter.FACTORY;
static final short LARGE_FILE_CONTEXT = 100;
public static class SideBySide extends PatchScreen {
public SideBySide(final Patch.Key id, final int patchIndex,
@@ -460,6 +461,19 @@ public abstract class PatchScreen extends Screen implements
setToken(Dispatcher.toPatchUnified(idSideA, patchKey));
}
if (script.isHugeFile()) {
AccountDiffPreference dp = script.getDiffPrefs();
int context = dp.getContext();
if (context == AccountDiffPreference.WHOLE_FILE_CONTEXT) {
context = Short.MAX_VALUE;
} else if (context > Short.MAX_VALUE) {
context = Short.MAX_VALUE;
}
dp.setContext((short) Math.min(context, LARGE_FILE_CONTEXT));
dp.setSyntaxHighlighting(false);
script.setDiffPrefs(dp);
}
contentTable.display(patchKey, idSideA, idSideB, script, patchSetDetail);
contentTable.display(script.getCommentDetail(), script.isExpandAllComments());
contentTable.finishDisplay();

18
gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScriptSettingsPanel.java
View File

@@ -23,6 +23,9 @@ import com.google.gerrit.client.ui.NpIntTextBox;
import com.google.gerrit.reviewdb.client.AccountDiffPreference;
import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace;
import com.google.gwt.core.client.GWT;
import com.google.gwt.dom.client.NodeList;
import com.google.gwt.dom.client.OptionElement;
import com.google.gwt.dom.client.SelectElement;
import com.google.gwt.event.dom.client.ClickEvent;
import com.google.gwt.event.dom.client.KeyCodes;
import com.google.gwt.event.dom.client.KeyPressEvent;
@@ -157,6 +160,21 @@ public class PatchScriptSettingsPanel extends Composite {
} else {
syntaxHighlighting.setValue(false);
}
NodeList<OptionElement> options =
context.getElement().<SelectElement>cast().getOptions();
// WHOLE_FILE_CONTEXT is the last option in the list.
int lastIndex = options.getLength() - 1;
OptionElement currOption = options.getItem(lastIndex);
if (enableSmallFileFeatures) {
currOption.setDisabled(false);
} else {
currOption.setDisabled(true);
if (context.getSelectedIndex() == lastIndex) {
// Select the next longest context from WHOLE_FILE_CONTEXT
context.setSelectedIndex(lastIndex - 1);
}
}
toggleEnabledStatus(save.isEnabled());
}

26
gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/SideBySideTable.java
View File

@@ -52,6 +52,7 @@ public class SideBySideTable extends AbstractPatchContentTable {
private SparseHtmlFile a;
private SparseHtmlFile b;
private boolean isHugeFile;
protected boolean isFileCommentBorderRowExist;
protected void createFileCommentEditorOnSideA() {
@@ -94,6 +95,7 @@ public class SideBySideTable extends AbstractPatchContentTable {
protected void render(final PatchScript script, final PatchSetDetail detail) {
final ArrayList<Object> lines = new ArrayList<Object>();
final SafeHtmlBuilder nc = new SafeHtmlBuilder();
isHugeFile = script.isHugeFile();
allocateTableHeader(script, nc);
lines.add(null);
if (!isDisplayBinary) {
@@ -209,7 +211,7 @@ public class SideBySideTable extends AbstractPatchContentTable {
for (int row = 0; row < lines.size(); row++) {
setRowItem(row, lines.get(row));
if (lines.get(row) instanceof SkippedLine) {
createSkipLine(row, (SkippedLine) lines.get(row), script.getA().isWholeFile());
createSkipLine(row, (SkippedLine) lines.get(row), isHugeFile);
}
}
}
@@ -540,18 +542,16 @@ public class SideBySideTable extends AbstractPatchContentTable {
if (numRows > 0) {
line.incrementStart(numRows);
// If we got here, we must have the whole file anyway.
createSkipLine(row + loopTo, line, true);
createSkipLine(row + loopTo, line, isHugeFile);
} else if (numRows < 0) {
line.reduceSize(-numRows);
// If we got here, we must have the whole file anyway.
createSkipLine(row, line, true);
createSkipLine(row, line, isHugeFile);
} else {
table.removeRow(row + loopTo);
}
}
private void createSkipLine(int row, SkippedLine line, boolean isWholeFile) {
private void createSkipLine(int row, SkippedLine line, boolean isHugeFile) {
FlowPanel p = new FlowPanel();
InlineLabel l1 = new InlineLabel(" " + PatchUtil.C.patchSkipRegionStart() + " ");
InlineLabel l2 = new InlineLabel(" " + PatchUtil.C.patchSkipRegionEnd() + " ");
@@ -560,7 +560,7 @@ public class SideBySideTable extends AbstractPatchContentTable {
all.addClickHandler(expandAllListener);
all.setStyleName(Gerrit.RESOURCES.css().skipLine());
if (line.getSize() > 30 && isWholeFile) {
if (line.getSize() > 30) {
// Only show the expand before/after if skipped more than 30 lines.
Anchor b = new Anchor(PatchUtil.M.expandBefore(NUM_ROWS_TO_EXPAND), true);
Anchor a = new Anchor(PatchUtil.M.expandAfter(NUM_ROWS_TO_EXPAND), true);
@@ -573,16 +573,16 @@ public class SideBySideTable extends AbstractPatchContentTable {
p.add(b);
p.add(l1);
p.add(all);
if (isHugeFile) {
p.add(new InlineLabel(" " + line.getSize() + " "));
} else {
p.add(all);
}
p.add(l2);
p.add(a);
} else if (isWholeFile) {
p.add(l1);
p.add(all);
p.add(l2);
} else {
p.add(l1);
p.add(new InlineLabel(" " + line.getSize() + " "));
p.add(all);
p.add(l2);
}
table.setWidget(row, 1, p);