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-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java

@@ -908,16 +908,19 @@ public class ChangeScreen extends Screen {
   }
 
   private List<NativeMap<JsArray<CommentInfo>>> loadComments(
-      RevisionInfo rev, CallbackGroup group) {
+      final RevisionInfo rev, CallbackGroup group) {
-    final int id = rev._number();
     final List<NativeMap<JsArray<CommentInfo>>> r = new ArrayList<>(1);
-    ChangeApi.revision(changeId.get(), rev.name())
+    // TODO(dborowitz): Could eliminate this call by adding an option to include
-      .view("comments")
+    // inline comments in the change detail.
+    ChangeApi.comments(changeId.get())
       .get(group.add(new AsyncCallback<NativeMap<JsArray<CommentInfo>>>() {
         @Override
         public void onSuccess(NativeMap<JsArray<CommentInfo>> result) {
-          r.add(result);
+          // Return value is used for populating the file table, so only count
-          history.addComments(id, result);
+          // 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<JsArray<CommentInfo>> filterForRevision(
+      NativeMap<JsArray<CommentInfo>> comments, int id) {
+    NativeMap<JsArray<CommentInfo>> filtered = NativeMap.create();
+    for (String k : comments.keySet()) {
+      JsArray<CommentInfo> allRevisions = comments.get(k);
+      JsArray<CommentInfo> 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<NativeMap<JsArray<CommentInfo>>> loadDrafts(
       RevisionInfo rev, CallbackGroup group) {
     final List<NativeMap<JsArray<CommentInfo>>> 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<ActionInfo> 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<ActionInfo> actionMap) {
+    initRevisionsAction(info, revision, actionMap);
     actions.reloadRevisionActions(actionMap);
   }
 

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<CommentInfo> 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<CommentInfo>() {
-      @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));
     }
   }
 

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<Integer> loaded = new HashSet<>();
+  private final Map<Integer, List<CommentInfo>> byAuthor = new HashMap<>();
-  private final Map<AuthorRevision, List<CommentInfo>> byAuthor =
-      new HashMap<>();
-
-  private final List<Integer> toLoad = new ArrayList<>(4);
-  private int active;
 
   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<JsArray<CommentInfo>> map) {
+  void addComments(NativeMap<JsArray<CommentInfo>> map) {
-    loaded.add(id);
-
     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);
+          int authorId = c.author()._account_id();
-          List<CommentInfo> l = byAuthor.get(k);
+          List<CommentInfo> 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<NativeMap<JsArray<CommentInfo>>>() {
-        @Override
-        public void onSuccess(NativeMap<JsArray<CommentInfo>> 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<CommentInfo> comments(MessageInfo msg) {
     if (msg.author() == null) {
       return Collections.emptyList();
     }
 
-    AuthorRevision k = new AuthorRevision(msg.author(), msg._revisionNumber());
+    int authorId = msg.author()._account_id();
-    List<CommentInfo> list = byAuthor.get(k);
+    List<CommentInfo> 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;
-    }
-  }
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java

@@ -33,14 +33,29 @@ class LineComment extends Composite {
   interface Binder extends UiBinder<HTMLPanel, LineComment> {}
   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;

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;
     }
   </ui:style>
 
   <g:HTMLPanel styleName='{style.box}'>
-    <div class='{style.location}' ui:field='fileLoc'><ui:msg>File Comment</ui:msg></div>
+    <div class='{style.location}'>
-    <div class='{style.location}' ui:field='lineLoc'><ui:msg>Line <c:InlineHyperlink ui:field='line'/>:</ui:msg></div>
+      <span ui:field='psLoc'><ui:msg>PS<span ui:field='psNum'/>, </ui:msg></span>
+      <span ui:field='fileLoc'><ui:msg>File Comment</ui:msg></span>
+      <span ui:field='lineLoc'><ui:msg>Line <c:InlineHyperlink ui:field='line'/>:</ui:msg></span>
+    </div>
     <div class='{style.message}' ui:field='message'/>
   </g:HTMLPanel>
 </ui:UiBinder>

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 (open && info._revisionNumber() > 0 && !commentList.isEmpty()) {
-      if (commentList == null) {
+      renderComments(commentList);
-        history.load(info._revisionNumber());
+      commentList = Collections.emptyList();
-      } else if (!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);
     }

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<NativeMap<JsArray<CommentInfo>>>() {
+    ChangeApi.drafts(psId.getParentKey().get())
-      @Override
+        .get(new AsyncCallback<NativeMap<JsArray<CommentInfo>>>() {
-      public void onSuccess(NativeMap<JsArray<CommentInfo>> result) {
+          @Override
-        attachComments(result);
+          public void onSuccess(NativeMap<JsArray<CommentInfo>> result) {
-        displayComments(result);
+            displayComments(result);
-        post.setEnabled(true);
+            post.setEnabled(true);
-      }
+          }
 
-      @Override
+          @Override
-      public void onFailure(Throwable caught) {
+          public void onFailure(Throwable caught) {
-        post.setEnabled(true);
+            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<JsArray<CommentInfo>> result) {
-    in.drafts(ReviewInput.DraftHandling.KEEP);
-    in.comments(result);
-  }
-
   private void displayComments(NativeMap<JsArray<CommentInfo>> m) {
     comments.clear();
 

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<EditInfo> cb) {
     edit(id).get(cb);
   }

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();

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() {