From 5fd7813e72c37ccf14bae374586bf0f5951bbd85 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 30 Apr 2015 10:07:29 -0700 Subject: [PATCH] Always publish comments on all revisions from the reply box Fetch comments for all revisions when populating the history table, so it shows what was published. This actually allows us to simplify the history table considerably, since we don't need to lazily fetch comments for different patch set IDs. For this to work we need to move the History population up one level of callbacks, which is doable because it doesn't actually depend on the revision callback returning. Instead of resorting comments by line, trust the order that comes back from the handler. Bug: issue 1100 Change-Id: I2749d9f693adca22855958208d810e0231bb3325 --- .../gerrit/client/change/ChangeScreen.java | 42 +++++-- .../gerrit/client/change/FileComments.java | 15 +-- .../google/gerrit/client/change/History.java | 104 ++---------------- .../gerrit/client/change/LineComment.java | 17 ++- .../gerrit/client/change/LineComment.ui.xml | 9 +- .../google/gerrit/client/change/Message.java | 11 +- .../google/gerrit/client/change/ReplyBox.java | 34 +++--- .../gerrit/client/changes/ChangeApi.java | 8 ++ .../gerrit/client/changes/CommentInfo.java | 1 + .../gerrit/client/changes/ReviewInput.java | 2 +- 10 files changed, 95 insertions(+), 148 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java index 6d237938a6..9545b84548 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java @@ -908,16 +908,19 @@ public class ChangeScreen extends Screen { } private List>> loadComments( - RevisionInfo rev, CallbackGroup group) { - final int id = rev._number(); + final RevisionInfo rev, CallbackGroup group) { final List>> r = new ArrayList<>(1); - ChangeApi.revision(changeId.get(), rev.name()) - .view("comments") + // TODO(dborowitz): Could eliminate this call by adding an option to include + // inline comments in the change detail. + ChangeApi.comments(changeId.get()) .get(group.add(new AsyncCallback>>() { @Override public void onSuccess(NativeMap> result) { - r.add(result); - history.addComments(id, result); + // Return value is used for populating the file table, so only count + // comments for the current revision. Still include all comments in + // the history table. + r.add(filterForRevision(result, rev._number())); + history.addComments(result); } @Override @@ -927,6 +930,23 @@ public class ChangeScreen extends Screen { return r; } + private static NativeMap> filterForRevision( + NativeMap> comments, int id) { + NativeMap> filtered = NativeMap.create(); + for (String k : comments.keySet()) { + JsArray allRevisions = comments.get(k); + JsArray thisRevision = JsArray.createArray().cast(); + for (int i = 0; i < allRevisions.length(); i++) { + CommentInfo c = allRevisions.get(i); + if (c.patch_set() == id) { + thisRevision.push(c); + } + } + filtered.put(k, thisRevision); + } + return filtered; + } + private List>> loadDrafts( RevisionInfo rev, CallbackGroup group) { final List>> r = new ArrayList<>(1); @@ -1129,10 +1149,7 @@ public class ChangeScreen extends Screen { initRevisionsAction(info, revision, emptyMap); quickApprove.setVisible(false); actions.reloadRevisionActions(emptyMap); - } - private void renderRevisionInfo(ChangeInfo info, - NativeMap actionMap) { RevisionInfo revisionInfo = info.revision(revision); boolean current = revision.equals(info.current_revision()) && !revisionInfo.is_edit(); @@ -1146,8 +1163,6 @@ public class ChangeScreen extends Screen { statusText.setInnerText(Util.toLongString(info.status())); } - initRevisionsAction(info, revision, actionMap); - if (Gerrit.isSignedIn()) { replyAction = new ReplyAction(info, revision, hasDraftComments, style, commentLinkProcessor, reply, quickApprove); @@ -1160,6 +1175,11 @@ public class ChangeScreen extends Screen { } else { quickApprove.setVisible(false); } + } + + private void renderRevisionInfo(ChangeInfo info, + NativeMap actionMap) { + initRevisionsAction(info, revision, actionMap); actions.reloadRevisionActions(actionMap); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java index 71720119ec..e73c70a0ba 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java @@ -26,8 +26,6 @@ import com.google.gwt.user.client.ui.Composite; import com.google.gwt.user.client.ui.FlowPanel; import com.google.gwt.user.client.ui.HTMLPanel; -import java.util.Collections; -import java.util.Comparator; import java.util.List; class FileComments extends Composite { @@ -38,22 +36,15 @@ class FileComments extends Composite { @UiField FlowPanel comments; FileComments(CommentLinkProcessor clp, - PatchSet.Id ps, + PatchSet.Id defaultPs, String title, List list) { initWidget(uiBinder.createAndBindUi(this)); - path.setTargetHistoryToken(url(ps, list.get(0))); + path.setTargetHistoryToken(url(defaultPs, list.get(0))); path.setText(title); - - Collections.sort(list, new Comparator() { - @Override - public int compare(CommentInfo a, CommentInfo b) { - return a.line() - b.line(); - } - }); for (CommentInfo c : list) { - comments.add(new LineComment(clp, ps, c)); + comments.add(new LineComment(clp, defaultPs, c)); } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java index 7635d81a1b..d1f6f2e4fc 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java @@ -14,8 +14,6 @@ package com.google.gerrit.client.change; -import com.google.gerrit.client.account.AccountInfo; -import com.google.gerrit.client.changes.ChangeApi; import com.google.gerrit.client.changes.ChangeInfo; import com.google.gerrit.client.changes.ChangeInfo.MessageInfo; import com.google.gerrit.client.changes.CommentInfo; @@ -23,9 +21,7 @@ import com.google.gerrit.client.rpc.NativeMap; import com.google.gerrit.client.rpc.Natives; import com.google.gerrit.client.ui.CommentLinkProcessor; import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gwt.core.client.JsArray; -import com.google.gwt.user.client.rpc.AsyncCallback; import com.google.gwt.user.client.ui.FlowPanel; import com.google.gwt.user.client.ui.Widget; @@ -33,22 +29,15 @@ import java.sql.Timestamp; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; class History extends FlowPanel { private CommentLinkProcessor clp; private ReplyAction replyAction; private Change.Id changeId; - private final Set loaded = new HashSet<>(); - private final Map> byAuthor = - new HashMap<>(); - - private final List toLoad = new ArrayList<>(4); - private int active; + private final Map> byAuthor = new HashMap<>(); void set(CommentLinkProcessor clp, ReplyAction ra, Change.Id id, ChangeInfo info) { @@ -60,9 +49,7 @@ class History extends FlowPanel { if (messages != null) { for (MessageInfo msg : Natives.asList(messages)) { Message ui = new Message(this, msg); - if (loaded.contains(msg._revisionNumber())) { - ui.addComments(comments(msg)); - } + ui.addComments(comments(msg)); add(ui); } autoOpen(ChangeScreen.myLastReply(info)); @@ -99,18 +86,16 @@ class History extends FlowPanel { replyAction.onReply(info); } - void addComments(int id, NativeMap> map) { - loaded.add(id); - + void addComments(NativeMap> map) { for (String path : map.keySet()) { for (CommentInfo c : Natives.asList(map.get(path))) { c.path(path); if (c.author() != null) { - AuthorRevision k = new AuthorRevision(c.author(), id); - List l = byAuthor.get(k); + int authorId = c.author()._account_id(); + List l = byAuthor.get(authorId); if (l == null) { l = new ArrayList<>(); - byAuthor.put(k, l); + byAuthor.put(authorId, l); } l.add(c); } @@ -118,58 +103,13 @@ class History extends FlowPanel { } } - void load(int revisionNumber) { - if (revisionNumber > 0 && loaded.add(revisionNumber)) { - toLoad.add(revisionNumber); - start(); - } - } - - private void start() { - if (active >= 2 || toLoad.isEmpty() || !isAttached()) { - return; - } - - final int revisionNumber = toLoad.remove(0); - active++; - ChangeApi.revision(new PatchSet.Id(changeId, revisionNumber)) - .view("comments") - .get(new AsyncCallback>>() { - @Override - public void onSuccess(NativeMap> result) { - addComments(revisionNumber, result); - update(revisionNumber); - --active; - start(); - } - - @Override - public void onFailure(Throwable caught) { - loaded.remove(revisionNumber); - loaded.removeAll(toLoad); - toLoad.clear(); - active--; - } - }); - } - - private void update(int revisionNumber) { - for (Widget child : getChildren()) { - Message ui = (Message) child; - MessageInfo info = ui.getMessageInfo(); - if (info._revisionNumber() == revisionNumber) { - ui.addComments(comments(info)); - } - } - } - private List comments(MessageInfo msg) { if (msg.author() == null) { return Collections.emptyList(); } - AuthorRevision k = new AuthorRevision(msg.author(), msg._revisionNumber()); - List list = byAuthor.get(k); + int authorId = msg.author()._account_id(); + List list = byAuthor.get(authorId); if (list == null) { return Collections.emptyList(); } @@ -187,34 +127,10 @@ class History extends FlowPanel { if (match.isEmpty()) { return Collections.emptyList(); } else if (other.isEmpty()) { - byAuthor.remove(k); + byAuthor.remove(authorId); } else { - byAuthor.put(k, other); + byAuthor.put(authorId, other); } return match; } - - private static final class AuthorRevision { - final int author; - final int revision; - - AuthorRevision(AccountInfo author, int revision) { - this.author = author._account_id(); - this.revision = revision; - } - - @Override - public int hashCode() { - return author * 31 + revision; - } - - @Override - public boolean equals(Object o) { - if (!(o instanceof AuthorRevision)) { - return false; - } - AuthorRevision b = (AuthorRevision) o; - return author == b.author && revision == b.revision; - } - } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java index 8fa5a68e15..7b3c310954 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java @@ -33,14 +33,29 @@ class LineComment extends Composite { interface Binder extends UiBinder {} private static final Binder uiBinder = GWT.create(Binder.class); + @UiField Element psLoc; + @UiField Element psNum; @UiField Element fileLoc; @UiField Element lineLoc; @UiField InlineHyperlink line; @UiField Element message; - LineComment(CommentLinkProcessor clp, PatchSet.Id ps, CommentInfo info) { + LineComment(CommentLinkProcessor clp, + PatchSet.Id defaultPs, + CommentInfo info) { initWidget(uiBinder.createAndBindUi(this)); + PatchSet.Id ps; + if (info.patch_set() != defaultPs.get()) { + ps = new PatchSet.Id(defaultPs.getParentKey(), info.patch_set()); + psNum.setInnerText(Integer.toString(info.patch_set())); + } else { + ps = defaultPs; + psLoc.removeFromParent(); + psLoc = null; + psNum= null; + } + if (info.has_line()) { fileLoc.removeFromParent(); fileLoc = null; diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml index 4926a07788..2890832739 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml @@ -29,13 +29,16 @@ limitations under the License. font-weight: bold; } .message { - margin-left: 111px; + margin-left: 135px; } -
File Comment
-
Line :
+
+ PS, + File Comment + Line : +
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java index 22d39a7dea..bc1ac3034b 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java @@ -121,13 +121,9 @@ class Message extends Composite { } void setOpen(boolean open) { - if (open && info._revisionNumber() > 0) { - if (commentList == null) { - history.load(info._revisionNumber()); - } else if (!commentList.isEmpty()) { - renderComments(commentList); - commentList = Collections.emptyList(); - } + if (open && info._revisionNumber() > 0 && !commentList.isEmpty()) { + renderComments(commentList); + commentList = Collections.emptyList(); } setName(open); @@ -156,7 +152,6 @@ class Message extends Composite { void autoOpen() { if (commentList == null) { autoOpen = true; - history.load(info._revisionNumber()); } else if (!commentList.isEmpty()) { setOpen(true); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java index f67860eac4..d6e7159248 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java @@ -21,9 +21,9 @@ import com.google.gerrit.client.changes.ChangeApi; import com.google.gerrit.client.changes.ChangeInfo.ApprovalInfo; import com.google.gerrit.client.changes.ChangeInfo.LabelInfo; import com.google.gerrit.client.changes.ChangeInfo.MessageInfo; -import com.google.gerrit.client.changes.CommentApi; import com.google.gerrit.client.changes.CommentInfo; import com.google.gerrit.client.changes.ReviewInput; +import com.google.gerrit.client.changes.ReviewInput.DraftHandling; import com.google.gerrit.client.changes.Util; import com.google.gerrit.client.rpc.GerritCallback; import com.google.gerrit.client.rpc.NativeMap; @@ -140,19 +140,19 @@ class ReplyBox extends Composite { protected void onLoad() { commentsPanel.setVisible(false); post.setEnabled(false); - CommentApi.drafts(psId, new AsyncCallback>>() { - @Override - public void onSuccess(NativeMap> result) { - attachComments(result); - displayComments(result); - post.setEnabled(true); - } + ChangeApi.drafts(psId.getParentKey().get()) + .get(new AsyncCallback>>() { + @Override + public void onSuccess(NativeMap> result) { + displayComments(result); + post.setEnabled(true); + } - @Override - public void onFailure(Throwable caught) { - post.setEnabled(true); - } - }); + @Override + public void onFailure(Throwable caught) { + post.setEnabled(true); + } + }); Scheduler.get().scheduleDeferred(new ScheduledCommand() { @Override @@ -186,6 +186,9 @@ class ReplyBox extends Composite { private void postReview() { in.message(message.getText().trim()); + // Don't send any comments in the request; just publish everything, even if + // e.g. a draft was modified in another tab since we last looked it up. + in.drafts(DraftHandling.PUBLISH_ALL_REVISIONS); in.prePost(); ChangeApi.revision(psId.getParentKey().get(), revision) .view("review") @@ -379,11 +382,6 @@ class ReplyBox extends Composite { && values.contains((short) 1); } - private void attachComments(NativeMap> result) { - in.drafts(ReviewInput.DraftHandling.KEEP); - in.comments(result); - } - private void displayComments(NativeMap> m) { comments.clear(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java index 1135491d4d..652e4bd39e 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java @@ -102,6 +102,14 @@ public class ChangeApi { return call(id, revision, "actions"); } + public static RestApi comments(int id) { + return call(id, "comments"); + } + + public static RestApi drafts(int id) { + return call(id, "drafts"); + } + public static void edit(int id, AsyncCallback cb) { edit(id).get(cb); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java index 52e5d40837..fed041ca66 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java @@ -82,6 +82,7 @@ public class CommentInfo extends JavaScriptObject { public final native String path() /*-{ return this.path }-*/; public final native String id() /*-{ return this.id }-*/; public final native String in_reply_to() /*-{ return this.in_reply_to }-*/; + public final native int patch_set() /*-{ return this.patch_set }-*/; public final Side side() { String s = sideRaw(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java index 7651495015..096dbd0df7 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java @@ -24,7 +24,7 @@ public class ReviewInput extends JavaScriptObject { } public static enum DraftHandling { - DELETE, PUBLISH, KEEP + DELETE, PUBLISH, KEEP, PUBLISH_ALL_REVISIONS } public static ReviewInput create() {