Improve the side-by-side viewer table
Move the line number column for the right side to be on the far
right of the table, giving us the layout:
1 | foo | bar | 1
2 | hello | hello | 2
This looks nicer when reading a lot of code, as the line numbers
are less relevant entities than the code itself which is now in
the center of the UI.
Line numbers are still links to create comment editors, but they
use a light shade of gray and skip the underline decoration, making
them less visually distracting.
Skip lines now use a paler shade of blue and also hide the fact they
contain anchors, until you hover over them and the anchor shows up.
The expand before and after are changed to be arrows showing in
which direction the lines will appear above or below the skip
line. This makes it easier for people like myself who get confused
and always hit the wrong link, the arrow shows to which side the
lines will appear.
Change-Id: I7108733358db23dca6f7d5b90cd4b7e8712cf2b2
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/gerrit.css b/gerrit-gwtui/src/main/java/com/google/gerrit/client/gerrit.css
index 1081e47..87208c6 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/gerrit.css
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/gerrit.css
@@ -662,11 +662,17 @@
white-space: pre;
width: 3.5em;
text-align: right;
- border-right: thin solid #b0bdcc;
padding-right: 0.2em;
background: white;
border-bottom: 1px solid white;
}
+.lineNumber.rightmost {
+ border-left: thin solid #b0bdcc;
+}
+.lineNumber a {
+ color: #888;
+ text-decoration: none;
+}
.patchContentTable td.fileColumnHeader {
background: trimColor;
font-family: norm-font;
@@ -689,6 +695,7 @@
padding-left: 0;
padding-right: 0;
white-space: pre;
+ border-left: thin solid #b0bdcc;
}
.fileLineNone {
background: #eeeeee;
@@ -723,11 +730,21 @@
font-family: norm-font;
text-align: center;
font-style: italic;
- background: lightblue;
+ background: #def;
+ color: grey;
}
.patchContentTable .skipLine div {
display: inline;
}
+.patchContentTable a.skipLine {
+ color: grey;
+ text-decoration: none;
+}
+.patchContentTable a:hover.skipLine {
+ background: white;
+ color: #00A;
+ text-decoration: underline;
+}
.patchContentTable .activeRow .iconCell,
.patchContentTable .activeRow .lineNumber {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java
index 8282caa..24a2ae5 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java
@@ -516,22 +516,6 @@
Gerrit.RESOURCES.css().iconCell());
}
- protected void addStyle(final int row, final int col, final String style) {
- table.getCellFormatter().addStyleName(row, col, style);
- }
-
- protected void removeRow(final int row) {
- table.removeRow(row);
- }
-
- protected void setHtml(final int row, final int col, final String html) {
- table.setHTML(row, col, html);
- }
-
- protected void setWidget(final int row, final int col, final Widget widget) {
- table.setWidget(row, col, widget);
- }
-
@Override
protected void onOpenRow(final int row) {
final Object item = getRowItem(row);
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.properties
index 6a1dbc1..23090a2 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.properties
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.properties
@@ -48,5 +48,5 @@
fileTypeSymlink = Type: Symbolic Link
fileTypeGitlink = Type: Git Commit in Subproject
-patchSkipRegionStart = (... skipping
-patchSkipRegionEnd = common lines ...)
+patchSkipRegionStart = ... skipped
+patchSkipRegionEnd = common lines ...
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchMessages.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchMessages.properties
index 52dcba2..2d01e24 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchMessages.properties
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchMessages.properties
@@ -1,4 +1,3 @@
-expandBefore = Expand {0} before
-expandAfter = Expand {0} after
-
+expandBefore = +{0}⇧
+expandAfter = +{0}⇩
draftSaved = Draft saved at {0,time,short}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchMessages_en.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchMessages_en.properties
index 58c4f6c..2f3c20a 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchMessages_en.properties
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchMessages_en.properties
@@ -1,2 +1,2 @@
-expandBefore = Expand {0} before
-expandAfter = Expand {0} after
+expandBefore = +{0}⇧
+expandAfter = +{0}⇩
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/SideBySideTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/SideBySideTable.java
index 6379e23..57320e2 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/SideBySideTable.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/SideBySideTable.java
@@ -26,16 +26,15 @@
import com.google.gerrit.prettify.common.EditList;
import com.google.gerrit.prettify.common.SparseHtmlFile;
import com.google.gerrit.reviewdb.client.Patch;
-import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.Patch.ChangeType;
+import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gwt.core.client.GWT;
import com.google.gwt.event.dom.client.ClickEvent;
import com.google.gwt.event.dom.client.ClickHandler;
import com.google.gwt.user.client.ui.Anchor;
import com.google.gwt.user.client.ui.FlowPanel;
-import com.google.gwt.user.client.ui.HTMLTable.Cell;
import com.google.gwt.user.client.ui.HTMLTable.CellFormatter;
-import com.google.gwt.user.client.ui.Label;
+import com.google.gwt.user.client.ui.InlineLabel;
import com.google.gwtexpui.safehtml.client.SafeHtml;
import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder;
import com.google.gwtorm.client.KeyUtil;
@@ -47,9 +46,8 @@
import java.util.List;
public class SideBySideTable extends AbstractPatchContentTable {
- private static final int COL_A = 2;
- private static final int COL_B = 4;
-
+ private static final int A = 2;
+ private static final int B = 3;
private static final int NUM_ROWS_TO_EXPAND = 10;
private SparseHtmlFile a;
@@ -59,24 +57,17 @@
protected void onCellDoubleClick(final int row, int column) {
if (column > 0 && getRowItem(row) instanceof PatchLine) {
final PatchLine line = (PatchLine) getRowItem(row);
- final short file = (short) ((column - 1) / 2);
- if (column < (1 + file * 2 + 1)) {
- column++;
- }
- switch (file) {
- case 0:
- createCommentEditor(row + 1, column, line.getLineA(), file);
- break;
- case 1:
- createCommentEditor(row + 1, column, line.getLineB(), file);
- break;
+ if (column == 1 || column == A) {
+ createCommentEditor(row + 1, A, line.getLineA(), (short) 0);
+ } else if (column == B || column == 4) {
+ createCommentEditor(row + 1, B, line.getLineB(), (short) 1);
}
}
}
@Override
protected void onCellSingleClick(int row, int column) {
- if (column == 1 || column == 3) {
+ if (column == 1 || column == 4) {
onCellDoubleClick(row, column);
}
}
@@ -84,7 +75,7 @@
@Override
protected void onInsertComment(final PatchLine line) {
final int row = getCurrentRow();
- createCommentEditor(row + 1, 4, line.getLineB(), (short) 1);
+ createCommentEditor(row + 1, B, line.getLineB(), (short) 1);
}
@Override
@@ -100,7 +91,8 @@
appendHeader(script, nc);
lines.add(null);
- if(script.getFileModeA()!=FileMode.FILE||script.getFileModeB()!=FileMode.FILE){
+ if (script.getFileModeA() != FileMode.FILE
+ || script.getFileModeB() != FileMode.FILE) {
openLine(nc);
appendModeLine(nc, script.getFileModeA());
appendModeLine(nc, script.getFileModeB());
@@ -121,13 +113,14 @@
if (hunk.isContextLine()) {
openLine(nc);
final SafeHtml ctx = a.getSafeHtmlLine(hunk.getCurA());
- appendLineText(nc, hunk.getCurA(), CONTEXT, ctx, false, false);
+ appendLineNumber(nc, hunk.getCurA(), false);
+ appendLineText(nc, CONTEXT, ctx, false, false);
if (ignoreWS && b.contains(hunk.getCurB())) {
- appendLineText(nc, hunk.getCurB(), CONTEXT, b, hunk.getCurB(),
- false);
+ appendLineText(nc, CONTEXT, b, hunk.getCurB(), false);
} else {
- appendLineText(nc, hunk.getCurB(), CONTEXT, ctx, false, false);
+ appendLineText(nc, CONTEXT, ctx, false, false);
}
+ appendLineNumber(nc, hunk.getCurB(), true);
closeLine(nc);
hunk.incBoth();
lines.add(new PatchLine(CONTEXT, hunk.getCurA(), hunk.getCurB()));
@@ -140,21 +133,27 @@
openLine(nc);
if (del) {
- appendLineText(nc, hunk.getCurA(), DELETE, a, hunk.getCurA(), full);
+ appendLineNumber(nc, hunk.getCurA(), false);
+ appendLineText(nc, DELETE, a, hunk.getCurA(), full);
hunk.incA();
} else if (hunk.getCurEdit().getType() == Edit.Type.REPLACE) {
+ appendLineNumber(nc, false);
appendLineNone(nc, DELETE);
} else {
+ appendLineNumber(nc, false);
appendLineNone(nc, CONTEXT);
}
if (ins) {
- appendLineText(nc, hunk.getCurB(), INSERT, b, hunk.getCurB(), full);
+ appendLineText(nc, INSERT, b, hunk.getCurB(), full);
+ appendLineNumber(nc, hunk.getCurB(), true);
hunk.incB();
} else if (hunk.getCurEdit().getType() == Edit.Type.REPLACE) {
appendLineNone(nc, INSERT);
+ appendLineNumber(nc, true);
} else {
appendLineNone(nc, CONTEXT);
+ appendLineNumber(nc, true);
}
closeLine(nc);
@@ -229,13 +228,13 @@
final PatchLineComment ac = ai.next();
final PatchLineComment bc = bi.next();
insertRow(row);
- bindComment(row, COL_A, ac, !ai.hasNext(), expandComments);
- bindComment(row, COL_B, bc, !bi.hasNext(), expandComments);
+ bindComment(row, A, ac, !ai.hasNext(), expandComments);
+ bindComment(row, B, bc, !bi.hasNext(), expandComments);
row++;
}
- row = finish(ai, row, COL_A, expandComments);
- row = finish(bi, row, COL_B, expandComments);
+ row = finish(ai, row, A, expandComments);
+ row = finish(bi, row, B, expandComments);
} else {
row++;
}
@@ -246,10 +245,10 @@
protected void insertRow(final int row) {
super.insertRow(row);
final CellFormatter fmt = table.getCellFormatter();
- fmt.addStyleName(row, COL_A - 1, Gerrit.RESOURCES.css().lineNumber());
- fmt.addStyleName(row, COL_A, Gerrit.RESOURCES.css().diffText());
- fmt.addStyleName(row, COL_B - 1, Gerrit.RESOURCES.css().lineNumber());
- fmt.addStyleName(row, COL_B, Gerrit.RESOURCES.css().diffText());
+ fmt.addStyleName(row, A - 1, Gerrit.RESOURCES.css().lineNumber());
+ fmt.addStyleName(row, A, Gerrit.RESOURCES.css().diffText());
+ fmt.addStyleName(row, B, Gerrit.RESOURCES.css().diffText());
+ fmt.addStyleName(row, B + 1, Gerrit.RESOURCES.css().lineNumber());
}
private int finish(final Iterator<PatchLineComment> i, int row, final int col, boolean expandComment) {
@@ -295,11 +294,6 @@
m.closeTd();
m.openTd();
- m.addStyleName(Gerrit.RESOURCES.css().fileColumnHeader());
- m.addStyleName(Gerrit.RESOURCES.css().lineNumber());
- m.closeTd();
-
- m.openTd();
m.setStyleName(Gerrit.RESOURCES.css().fileColumnHeader());
m.setAttribute("width", "50%");
m.append(PatchUtil.C.patchHeaderNew());
@@ -309,6 +303,11 @@
}
m.closeTd();
+ m.openTd();
+ m.addStyleName(Gerrit.RESOURCES.css().fileColumnHeader());
+ m.addStyleName(Gerrit.RESOURCES.css().lineNumber());
+ m.closeTd();
+
m.closeTr();
}
@@ -336,21 +335,21 @@
m.closeTr();
}
- ClickHandler expandAllListener = new ClickHandler() {
+ private ClickHandler expandAllListener = new ClickHandler() {
@Override
public void onClick(ClickEvent event) {
expand(event, 0);
}
};
- ClickHandler expandBeforeListener = new ClickHandler() {
+ private ClickHandler expandBeforeListener = new ClickHandler() {
@Override
public void onClick(ClickEvent event) {
expand(event, NUM_ROWS_TO_EXPAND);
}
};
- ClickHandler expandAfterListener = new ClickHandler() {
+ private ClickHandler expandAfterListener = new ClickHandler() {
@Override
public void onClick(ClickEvent event) {
expand(event, -NUM_ROWS_TO_EXPAND);
@@ -358,11 +357,11 @@
};
private void expand(ClickEvent event, final int numRows) {
- Cell cell = table.getCellForEvent(event);
- int row = cell.getRowIndex();
+ int row = table.getCellForEvent(event).getRowIndex();
if (!(getRowItem(row) instanceof SkippedLine)) {
return;
}
+
SkippedLine line = (SkippedLine) getRowItem(row);
int loopTo = numRows;
if (numRows == 0) {
@@ -374,6 +373,8 @@
if (numRows < 0) {
offset = 1;
}
+
+ CellFormatter fmt = table.getCellFormatter();
for (int i = 0 + offset; i < loopTo + offset; i++) {
insertRow(row + i);
int lineA = line.getStartA() + i;
@@ -382,21 +383,20 @@
lineA = line.getStartA() + line.getSize() + numRows + i - offset;
lineB = line.getStartB() + line.getSize() + numRows + i - offset;
}
- setHtml(row + i, 1, "<a href=\"javascript:void(0)\">" + (lineA + 1)
- + "</a>");
- addStyle(row + i, 1, Gerrit.RESOURCES.css().lineNumber());
- setHtml(row + i, 2, a.getSafeHtmlLine(lineA).asString());
- addStyle(row + i, 2, Gerrit.RESOURCES.css().fileLine());
- addStyle(row + i, 2, Gerrit.RESOURCES.css().fileLineCONTEXT());
+ table.setHTML(row + i, A - 1, "<a href=\"javascript:;\">" + (lineA + 1) + "</a>");
+ fmt.addStyleName(row + i, A - 1, Gerrit.RESOURCES.css().lineNumber());
- setHtml(row + i, 3, "<a href=\"javascript:void(0)\">" + (lineB + 1)
- + "</a>");
- addStyle(row + i, 3, Gerrit.RESOURCES.css().lineNumber());
+ table.setHTML(row + i, A, a.getSafeHtmlLine(lineA).asString());
+ fmt.addStyleName(row + i, A, Gerrit.RESOURCES.css().fileLine());
+ fmt.addStyleName(row + i, A, Gerrit.RESOURCES.css().fileLineCONTEXT());
- setHtml(row + i, 4, b.getSafeHtmlLine(lineB).asString());
- addStyle(row + i, 4, Gerrit.RESOURCES.css().fileLine());
- addStyle(row + i, 4, Gerrit.RESOURCES.css().fileLineCONTEXT());
+ table.setHTML(row + i, B, b.getSafeHtmlLine(lineB).asString());
+ fmt.addStyleName(row + i, B, Gerrit.RESOURCES.css().fileLine());
+ fmt.addStyleName(row + i, B, Gerrit.RESOURCES.css().fileLineCONTEXT());
+
+ table.setHTML(row + i, B + 1, "<a href=\"javascript:;\">" + (lineB + 1) + "</a>");
+ fmt.addStyleName(row + i, B + 1, Gerrit.RESOURCES.css().lineNumber());
setRowItem(row + i, new PatchLine(CONTEXT, lineA, lineB));
}
@@ -408,34 +408,41 @@
line.reduceSize(-numRows);
createSkipLine(row, line);
} else {
- removeRow(row + loopTo);
+ table.removeRow(row + loopTo);
}
}
private void createSkipLine(int row, SkippedLine line) {
FlowPanel p = new FlowPanel();
- Label l1 = new Label(" " + PatchUtil.C.patchSkipRegionStart() + " ");
+ InlineLabel l1 = new InlineLabel(" " + PatchUtil.C.patchSkipRegionStart() + " ");
+ InlineLabel l2 = new InlineLabel(" " + PatchUtil.C.patchSkipRegionEnd() + " ");
+
Anchor all = new Anchor(String.valueOf(line.getSize()));
- Label l2 = new Label(" " + PatchUtil.C.patchSkipRegionEnd() + " ");
all.addClickHandler(expandAllListener);
+ all.setStyleName(Gerrit.RESOURCES.css().skipLine());
+
if (line.getSize() > 30) {
- // We only show the expand before & after links if we skip more than
- // 30 lines.
- Anchor before = new Anchor(PatchUtil.M.expandBefore(NUM_ROWS_TO_EXPAND));
- before.addClickHandler(expandBeforeListener);
- Anchor after = new Anchor(PatchUtil.M.expandAfter(NUM_ROWS_TO_EXPAND));
- after.addClickHandler(expandAfterListener);
- p.add(before);
+ // 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);
+
+ b.addClickHandler(expandBeforeListener);
+ a.addClickHandler(expandAfterListener);
+
+ b.setStyleName(Gerrit.RESOURCES.css().skipLine());
+ a.setStyleName(Gerrit.RESOURCES.css().skipLine());
+
+ p.add(b);
p.add(l1);
p.add(all);
p.add(l2);
- p.add(after);
+ p.add(a);
} else {
p.add(l1);
p.add(all);
p.add(l2);
}
- setWidget(row, 1, p);
+ table.setWidget(row, 1, p);
}
private void openLine(final SafeHtmlBuilder m) {
@@ -447,22 +454,34 @@
m.closeTd();
}
- private void appendLineText(final SafeHtmlBuilder m,
- final int lineNumberMinusOne, final PatchLine.Type type,
- final SparseHtmlFile src, final int i, final boolean fullBlock) {
- appendLineText(m, lineNumberMinusOne, type, //
- src.getSafeHtmlLine(i), src.hasTrailingEdit(i), fullBlock);
+ private void appendLineNumber(SafeHtmlBuilder m, boolean right) {
+ m.openTd();
+ m.setStyleName(Gerrit.RESOURCES.css().lineNumber());
+ if (right) {
+ m.addStyleName(Gerrit.RESOURCES.css().rightmost());
+ }
+ m.closeTd();
+ }
+
+ private void appendLineNumber(SafeHtmlBuilder m, int lineNumberMinusOne, boolean right) {
+ m.openTd();
+ m.setStyleName(Gerrit.RESOURCES.css().lineNumber());
+ if (right) {
+ m.addStyleName(Gerrit.RESOURCES.css().rightmost());
+ }
+ m.append(SafeHtml.asis("<a href=\"javascript:;\">"+ (lineNumberMinusOne + 1) + "</a>"));
+ m.closeTd();
}
private void appendLineText(final SafeHtmlBuilder m,
- final int lineNumberMinusOne, final PatchLine.Type type,
- final SafeHtml lineHtml, final boolean trailingEdit,
+ final PatchLine.Type type, final SparseHtmlFile src, final int i,
final boolean fullBlock) {
- m.openTd();
- m.setStyleName(Gerrit.RESOURCES.css().lineNumber());
- m.append(SafeHtml.asis("<a href=\"javascript:void(0)\">"+ (lineNumberMinusOne + 1) + "</a>"));
- m.closeTd();
+ appendLineText(m, type, src.getSafeHtmlLine(i), src.hasTrailingEdit(i), fullBlock);
+ }
+ private void appendLineText(final SafeHtmlBuilder m,
+ final PatchLine.Type type, final SafeHtml lineHtml,
+ final boolean trailingEdit, final boolean fullBlock) {
m.openTd();
m.addStyleName(Gerrit.RESOURCES.css().fileLine());
switch (type) {
@@ -488,10 +507,6 @@
private void appendLineNone(final SafeHtmlBuilder m, final PatchLine.Type type) {
m.openTd();
- m.setStyleName(Gerrit.RESOURCES.css().lineNumber());
- m.closeTd();
-
- m.openTd();
m.addStyleName(Gerrit.RESOURCES.css().fileLine());
switch (type != null ? type : PatchLine.Type.CONTEXT) {
case DELETE:
