Add 'Reply' button to comments on diff screen
The last comment shown on the diff page now has a Reply button,
which will pop up the comment editor if pressed.
This change also introduces a new parent_uuid column in the
patch_comments table that maintains the ordering of comments for
each line. The first comment on any given line always has a null
value as parent_uuid.
Potential known issues with this implementation:
- When the user presses Reply and the comment editor pops up,
the Reply button remains visible. Pressing it again activates
the existing draft editor, but we might consider disabling it
(and re-enabling it if the user discards their draft comment)
- Pressing Reply, saving a draft, then returning to the same page
and discarding the draft does not restore the Reply button.
Leaving and returning to the file does however create it again.
- Two users can potentially press Reply on the same comment at
the same time, but this has no significant effect on this
implementation. These comments will end up with the same
parent_uuid but they will correctly be displayed in the UI,
sorted by their date of publication, which is the same as the
prior behavior.
- Reply does not quote the parent text, in either the comment
editor that opens or in the generated email when the comments
are published.
diff --git a/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java b/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java
index 0859e53..e77aef3 100644
--- a/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java
+++ b/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java
@@ -104,4 +104,6 @@
String pagedChangeListPrev();
String pagedChangeListNext();
+
+ String reply();
}
diff --git a/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties b/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties
index 74228e1..9e6592b 100644
--- a/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties
+++ b/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties
@@ -85,3 +85,5 @@
upToChangeIconLink = ⇧Up to change
prevPatchLinkIcon = ⇦
nextPatchLinkIcon = ⇨
+
+reply = Reply
diff --git a/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java b/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java
index 5b5779c..da373c3 100644
--- a/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java
+++ b/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java
@@ -266,8 +266,28 @@
/** Invoked when the user clicks on a table cell. */
protected abstract void onCellDoubleClick(int row, int column);
+ /**
+ * Invokes createCommentEditor() with an empty string as value for the comment
+ * parent UUID. This method is invoked by callers that want to create an
+ * editor for a comment that is not a reply.
+ */
protected void createCommentEditor(final int suggestRow, final int column,
final int line, final short file) {
+ createCommentEditor(suggestRow, column, line, file, null /* no parent */);
+ }
+
+ protected void createReplyEditor(final LineCommentPanel currentPanel) {
+ final int row = rowOf(currentPanel.getElement());
+ if (row >= 0) {
+ final int column = columnOf(currentPanel.getElement());
+ final PatchLineComment c = currentPanel.comment;
+ final PatchLineComment.Key k = c.getKey();
+ createCommentEditor(row, column, c.getLine(), c.getSide(), k.get());
+ }
+ }
+
+ private void createCommentEditor(final int suggestRow, final int column,
+ final int line, final short file, final String parentUuid) {
int row = suggestRow;
int spans[] = new int[column + 1];
OUTER: while (row < table.getRowCount()) {
@@ -322,7 +342,7 @@
final PatchLineComment newComment =
new PatchLineComment(new PatchLineComment.Key(parentKey, null), line,
- Gerrit.getUserAccount().getId());
+ Gerrit.getUserAccount().getId(), parentUuid);
newComment.setSide(side);
newComment.setMessage("");
@@ -437,7 +457,14 @@
return;
}
- final LineCommentPanel mp = new LineCommentPanel(line);
+ final LineCommentPanel mp;
+ if (isLast) {
+ // Create a panel with a Reply button if we are looking at the
+ // last comment for this line.
+ mp = new LineCommentPanel(line, this);
+ } else {
+ mp = new LineCommentPanel(line);
+ }
String panelHeader;
final ComplexDisclosurePanel panel;
@@ -491,9 +518,7 @@
if (td == null) {
break;
}
- final Element tr = DOM.getParent(td);
- final Element body = DOM.getParent(tr);
- final int row = DOM.getChildIndex(body, tr);
+ final int row = rowOf(td);
if (getRowItem(row) != null) {
movePointerTo(row);
return;
@@ -506,11 +531,7 @@
if (td == null) {
return;
}
- Element tr = DOM.getParent(td);
- Element body = DOM.getParent(tr);
- int row = DOM.getChildIndex(body, tr);
- int column = DOM.getChildIndex(tr, td);
- onCellDoubleClick(row, column);
+ onCellDoubleClick(rowOf(td), columnOf(td));
return;
}
}
diff --git a/src/main/java/com/google/gerrit/client/patches/CommentDetail.java b/src/main/java/com/google/gerrit/client/patches/CommentDetail.java
index f3c69bd..7e41760 100644
--- a/src/main/java/com/google/gerrit/client/patches/CommentDetail.java
+++ b/src/main/java/com/google/gerrit/client/patches/CommentDetail.java
@@ -123,7 +123,59 @@
private static List<PatchLineComment> get(
final Map<Integer, List<PatchLineComment>> m, final int i) {
final List<PatchLineComment> r = m.get(i);
- return r != null ? r : Collections.<PatchLineComment> emptyList();
+ return r != null ? orderComments(r) : Collections.<PatchLineComment> emptyList();
+ }
+
+ /**
+ * Order the comments based on their parent_uuid parent. It is possible to do this by
+ * iterating over the list only once but it's probably overkill since the number of comments
+ * on a given line will be small most of the time.
+ *
+ * @param comments The list of comments for a given line.
+ * @return The comments sorted as they should appear in the UI
+ */
+ private static List<PatchLineComment> orderComments(List<PatchLineComment> comments) {
+ // Map of comments keyed by their parent. The values are lists of comments since it is
+ // possible for several comments to have the same parent (this can happen if two reviewers
+ // click Reply on the same comment at the same time). Such comments will be displayed under
+ // their correct parent in chronological order.
+ Map<String, List<PatchLineComment>> parentMap = new HashMap<String, List<PatchLineComment>>();
+
+ // It's possible to have more than one root comment if two reviewers create a comment on the
+ // same line at the same time
+ List<PatchLineComment> rootComments = new ArrayList<PatchLineComment>();
+
+ // Store all the comments in parentMap, keyed by their parent
+ for (PatchLineComment c : comments) {
+ String parentUuid = c.getParentUuid();
+ List<PatchLineComment> l = parentMap.get(parentUuid);
+ if (l == null) {
+ l = new ArrayList<PatchLineComment>();
+ parentMap.put(parentUuid, l);
+ }
+ l.add(c);
+ if (parentUuid == null) rootComments.add(c);
+ }
+
+ // Add the comments in the list, starting with the head and then going through all the
+ // comments that have it as a parent, and so on
+ List<PatchLineComment> result = new ArrayList<PatchLineComment>();
+ addChildren(parentMap, rootComments, result);
+
+ return result;
+ }
+
+ /**
+ * Add the comments to <code>outResult</code>, depth first
+ */
+ private static void addChildren(Map<String, List<PatchLineComment>> parentMap,
+ List<PatchLineComment> children, List<PatchLineComment> outResult) {
+ if (children != null) {
+ for (PatchLineComment c : children) {
+ outResult.add(c);
+ addChildren(parentMap, parentMap.get(c.getKey().get()), outResult);
+ }
+ }
}
private Map<Integer, List<PatchLineComment>> index(
diff --git a/src/main/java/com/google/gerrit/client/patches/LineCommentPanel.java b/src/main/java/com/google/gerrit/client/patches/LineCommentPanel.java
index 1369112..00fc5fe 100644
--- a/src/main/java/com/google/gerrit/client/patches/LineCommentPanel.java
+++ b/src/main/java/com/google/gerrit/client/patches/LineCommentPanel.java
@@ -14,8 +14,13 @@
package com.google.gerrit.client.patches;
+import com.google.gerrit.client.changes.Util;
import com.google.gerrit.client.reviewdb.PatchLineComment;
+import com.google.gwt.event.dom.client.ClickEvent;
+import com.google.gwt.event.dom.client.ClickHandler;
+import com.google.gwt.user.client.ui.Button;
import com.google.gwt.user.client.ui.Composite;
+import com.google.gwt.user.client.ui.FlowPanel;
import com.google.gwt.user.client.ui.Widget;
import com.google.gwtexpui.safehtml.client.SafeHtml;
import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder;
@@ -27,12 +32,44 @@
PatchLineComment comment;
boolean isRecent;
+ private FlowPanel body;
+ /**
+ * Create a simple line comment panel.
+ */
public LineCommentPanel(final PatchLineComment msg) {
+ init(msg);
+ }
+
+ /**
+ * Create a line comment panel with a Reply button that creates an editor if pressed.
+ */
+ public LineCommentPanel(final PatchLineComment msg,
+ final AbstractPatchContentTable parent) {
+ init(msg);
+
+ final FlowPanel buttons = new FlowPanel();
+ buttons.setStyleName("gerrit-CommentEditor-Buttons");
+ body.add(buttons);
+
+ Button button = new Button(Util.C.reply());
+ button.addClickHandler(new ClickHandler() {
+ @Override
+ public void onClick(ClickEvent arg0) {
+ parent.createReplyEditor(LineCommentPanel.this);
+ }
+ });
+ buttons.add(button);
+ }
+
+ private void init(PatchLineComment msg) {
comment = msg;
final Widget l = toSafeHtml(msg).toBlockWidget();
l.setStyleName("gerrit-PatchLineComment");
- initWidget(l);
+ body = new FlowPanel();
+ body.add(l);
+ body.setStyleName("gerrit-PatchLineCommentPanel");
+ initWidget(body);
}
void update(final PatchLineComment msg) {
diff --git a/src/main/java/com/google/gerrit/client/reviewdb/PatchLineComment.java b/src/main/java/com/google/gerrit/client/reviewdb/PatchLineComment.java
index 47558c7..90dcc2f 100644
--- a/src/main/java/com/google/gerrit/client/reviewdb/PatchLineComment.java
+++ b/src/main/java/com/google/gerrit/client/reviewdb/PatchLineComment.java
@@ -110,14 +110,19 @@
@Column(notNull = false, length = Integer.MAX_VALUE)
protected String message;
+ /** The parent of this comment, or null if this is the first comment on this line */
+ @Column(length = 40, notNull = false)
+ protected String parentUuid;
+
protected PatchLineComment() {
}
public PatchLineComment(final PatchLineComment.Key id, final int line,
- final Account.Id a) {
+ final Account.Id a, String parentUuid) {
key = id;
lineNbr = line;
author = a;
+ this.parentUuid = parentUuid;
setStatus(Status.DRAFT);
updated();
}
@@ -165,4 +170,8 @@
public void updated() {
writtenOn = new Timestamp(System.currentTimeMillis());
}
+
+ public String getParentUuid() {
+ return parentUuid;
+ }
}
diff --git a/src/main/java/com/google/gerrit/client/reviewdb/ReviewDb.java b/src/main/java/com/google/gerrit/client/reviewdb/ReviewDb.java
index 7aef867..0071555 100644
--- a/src/main/java/com/google/gerrit/client/reviewdb/ReviewDb.java
+++ b/src/main/java/com/google/gerrit/client/reviewdb/ReviewDb.java
@@ -31,7 +31,7 @@
* </ul>
*/
public interface ReviewDb extends Schema {
- public static final int VERSION = 14;
+ public static final int VERSION = 15;
@Relation
SchemaVersionAccess schemaVersion();
diff --git a/src/main/java/com/google/gerrit/client/ui/FancyFlexTable.java b/src/main/java/com/google/gerrit/client/ui/FancyFlexTable.java
index 2dd808f..5f000b4 100644
--- a/src/main/java/com/google/gerrit/client/ui/FancyFlexTable.java
+++ b/src/main/java/com/google/gerrit/client/ui/FancyFlexTable.java
@@ -116,6 +116,49 @@
table.getCellFormatter().addStyleName(newRow, C_ARROW, S_LEFT_MOST_CELL);
}
+ /**
+ * Get the td element that contains another element.
+ *
+ * @param target the child element whose parent td is required.
+ * @return the td containing element {@code target}; null if {@code target} is
+ * not a member of this table.
+ */
+ protected Element getParentCell(final Element target) {
+ final Element body = FancyFlexTableImpl.getBodyElement(table);
+ for (Element td = target; td != null && td != body; td = DOM.getParent(td)) {
+ // If it's a TD, it might be the one we're looking for.
+ if ("td".equalsIgnoreCase(td.getTagName())) {
+ // Make sure it's directly a part of this table.
+ Element tr = DOM.getParent(td);
+ if (DOM.getParent(tr) == body) {
+ return td;
+ }
+ }
+ }
+ return null;
+ }
+
+ /** @return the row of the child element; -1 if the child is not in the table. */
+ protected int rowOf(final Element target) {
+ final Element td = getParentCell(target);
+ if (td == null) {
+ return -1;
+ }
+ final Element tr = DOM.getParent(td);
+ final Element body = DOM.getParent(tr);
+ return DOM.getChildIndex(body, tr);
+ }
+
+ /** @return the cell of the child element; -1 if the child is not in the table. */
+ protected int columnOf(final Element target) {
+ final Element td = getParentCell(target);
+ if (td == null) {
+ return -1;
+ }
+ final Element tr = DOM.getParent(td);
+ return DOM.getChildIndex(tr, td);
+ }
+
protected static class MyFlexTable extends FlexTable {
}
diff --git a/src/main/java/com/google/gerrit/public/gerrit.css b/src/main/java/com/google/gerrit/public/gerrit.css
index 3498c09..76a6c48 100644
--- a/src/main/java/com/google/gerrit/public/gerrit.css
+++ b/src/main/java/com/google/gerrit/public/gerrit.css
@@ -387,6 +387,11 @@
border-collapse: separate;
border-spacing: 0;
}
+
+.gerrit-PatchLineCommentPanel {
+ width: 100%;
+}
+
.gerrit-PatchContentTable .Comment td,
.gerrit-PatchLineComment {
font-family: Arial Unicode MS,Arial,sans-serif;
diff --git a/src/main/java/com/google/gerrit/server/patch/PatchDetailServiceImpl.java b/src/main/java/com/google/gerrit/server/patch/PatchDetailServiceImpl.java
index 6c1edf8..839aa50 100644
--- a/src/main/java/com/google/gerrit/server/patch/PatchDetailServiceImpl.java
+++ b/src/main/java/com/google/gerrit/server/patch/PatchDetailServiceImpl.java
@@ -114,7 +114,7 @@
if (comment.getKey().get() == null) {
final PatchLineComment nc =
new PatchLineComment(new PatchLineComment.Key(patch.getKey(),
- ChangeUtil.messageUUID(db)), comment.getLine(), me);
+ ChangeUtil.messageUUID(db)), comment.getLine(), me, comment.getParentUuid());
nc.setSide(comment.getSide());
nc.setMessage(comment.getMessage());
db.patchComments().insert(Collections.singleton(nc));
diff --git a/src/main/webapp/WEB-INF/sql/upgrade014_015.sql b/src/main/webapp/WEB-INF/sql/upgrade014_015.sql
new file mode 100644
index 0000000..26efcb9
--- /dev/null
+++ b/src/main/webapp/WEB-INF/sql/upgrade014_015.sql
@@ -0,0 +1,5 @@
+-- Upgrade: schema_version 14 to 15
+--
+ALTER TABLE patch_comments ADD parent_uuid VARCHAR(40);
+
+UPDATE schema_version SET version_nbr = 15;
