Refactor padding for comment boxes

Added PaddingManager that takes care of calculating and adjusting
paddings introduced by CommentBoxes.

Change-Id: Iff58822edeeb724f649a347197ec0beec15c5a82

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CodeMirrorDemo.java

@@ -19,6 +19,7 @@ import com.google.gerrit.client.changes.ChangeApi;
 import com.google.gerrit.client.changes.ChangeInfo;
 import com.google.gerrit.client.changes.CommentApi;
 import com.google.gerrit.client.changes.CommentInfo;
+import com.google.gerrit.client.diff.PaddingManager.LineWidgetElementPair;
 import com.google.gerrit.client.diff.DiffInfo.Region;
 import com.google.gerrit.client.diff.DiffInfo.Span;
 import com.google.gerrit.client.diff.LineMapper.LineOnOtherInfo;
@@ -46,6 +47,7 @@ import com.google.gwt.user.client.rpc.AsyncCallback;
 
 import net.codemirror.lib.CodeMirror;
 import net.codemirror.lib.CodeMirror.LineClassWhere;
+import net.codemirror.lib.CodeMirror.LineHandle;
 import net.codemirror.lib.Configuration;
 import net.codemirror.lib.KeyMap;
 import net.codemirror.lib.LineCharacter;
@@ -53,6 +55,8 @@ import net.codemirror.lib.LineWidget;
 import net.codemirror.lib.ModeInjector;
 
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -61,13 +65,6 @@ public class CodeMirrorDemo extends Screen {
   private static final int HEADER_FOOTER = 60 + 15 * 2 + 38;
   private static final JsArrayString EMPTY =
       JavaScriptObject.createArray().cast();
-  /**
-   * TODO: Handle ordering of line widgets. CodeMirror seems to have a bug when
-   * multiple line widgets are added to a line: it puts the first widget above
-   * the line when the line is clicked upon.
-   */
-  private static final Configuration COMMENT_BOX_CONFIG =
-      Configuration.create().set("coverGutter", true);
 
   private final PatchSet.Id base;
   private final PatchSet.Id revision;
@@ -84,8 +81,9 @@ public class CodeMirrorDemo extends Screen {
   private LineMapper mapper;
   private CommentLinkProcessor commentLinkProcessor;
   private Map<String, PublishedBox> publishedMap;
-  private Map<Integer, CommentBox> lineBoxMapA;
+  private Map<LineHandle, CommentBox> lineActiveBoxMap;
-  private Map<Integer, CommentBox> lineBoxMapB;
+  private Map<LineHandle, PublishedBox> lineLastPublishedBoxMap;
+  private Map<LineHandle, PaddingManager> linePaddingManagerMap;
 
   public CodeMirrorDemo(
       PatchSet.Id base,
@@ -201,13 +199,16 @@ public class CodeMirrorDemo extends Screen {
     cmB = displaySide(diffInfo.meta_b(), diffInfo.text_b(), diffTable.getCmB());
     render(diffInfo);
     initialBoxes = new ArrayList<CommentBox>();
+    lineActiveBoxMap = new HashMap<LineHandle, CommentBox>();
+    lineLastPublishedBoxMap = new HashMap<LineHandle, PublishedBox>();
+    linePaddingManagerMap = new HashMap<LineHandle, PaddingManager>();
     if (published != null) {
       publishedMap = new HashMap<String, PublishedBox>(published.length());
+      renderPublished();
+    }
+    if (drafts != null) {
+      renderDrafts();
     }
-    lineBoxMapA = new HashMap<Integer, CommentBox>();
-    lineBoxMapB = new HashMap<Integer, CommentBox>();
-    renderPublished();
-    renderDrafts();
     published = null;
     drafts = null;
     cmA.on("cursorActivity", updateActiveLine(cmA));
@@ -317,7 +318,8 @@ public class CodeMirrorDemo extends Screen {
     if (!doSave) {
       box.setEdit(true);
     }
-    getLineBoxMapFromSide(info.side()).put(info.line() - 1, box);
+    LineHandle handle = getCmFromSide(info.side()).getLineHandle(info.line() - 1);
+    lineActiveBoxMap.put(handle, box);
     return box;
   }
 
@@ -327,42 +329,82 @@ public class CodeMirrorDemo extends Screen {
     CodeMirror cm = mySide == Side.PARENT ? cmA : cmB;
     CodeMirror other = otherCM(cm);
     int line = info.line() - 1; // CommentInfo is 1-based, but CM is 0-based
-    LineWidget boxWidget =
+    LineHandle handle = cm.getLineHandle(line);
-        cm.addLineWidget(line, box.getElement(), COMMENT_BOX_CONFIG);
+    PaddingManager manager;
+    if (linePaddingManagerMap.containsKey(handle)) {
+      manager = linePaddingManagerMap.get(handle);
+    } else {
+      // Estimated height at 21px, fixed by deferring after display
+      manager = new PaddingManager(
+          addPaddingWidget(cm, diffTable.style.padding(), line, 21, Unit.PX, 0));
+      linePaddingManagerMap.put(handle, manager);
+    }
     int lineToPad = mapper.lineOnOther(mySide, line).getLine();
-    // Estimated height at 21px, fixed by deferring after display
+    LineHandle otherHandle = other.getLineHandle(lineToPad);
-    LineWidgetElementPair padding = addPaddingWidget(
+    if (linePaddingManagerMap.containsKey(otherHandle)) {
-        other, diffTable.style.padding(), lineToPad, 21, Unit.PX);
+      PaddingManager.link(manager, linePaddingManagerMap.get(otherHandle));
+    } else {
+      PaddingManager otherManager = new PaddingManager(
+          addPaddingWidget(other, diffTable.style.padding(), lineToPad, 21, Unit.PX, 0));
+      linePaddingManagerMap.put(otherHandle, otherManager);
+      PaddingManager.link(manager, otherManager);
+    }
+    int index = manager.getCurrentCount();
+    manager.insert(box, index);
+    Configuration config = Configuration.create()
+      .set("coverGutter", true)
+      .set("insertAt", index);
+    LineWidget boxWidget = cm.addLineWidget(line, box.getElement(), config);
+    box.setPaddingManager(manager);
     box.setSelfWidget(boxWidget);
-    box.setPadding(padding.widget, padding.element);
     return box;
   }
 
-  void removeCommentBox(Side side, int line) {
+  void removeDraft(Side side, int line) {
-    getLineBoxMapFromSide(side).remove(line);
+    LineHandle handle = getCmFromSide(side).getLineHandle(line);
+    lineActiveBoxMap.remove(handle);
+    if (lineLastPublishedBoxMap.containsKey(handle)) {
+      lineActiveBoxMap.put(handle, lineLastPublishedBoxMap.get(handle));
+    }
+  }
+
+  private List<CommentInfo> sortComment(JsArray<CommentInfo> unsorted) {
+    List<CommentInfo> sorted = new ArrayList<CommentInfo>();
+    for (int i = 0; i < unsorted.length(); i++) {
+      sorted.add(unsorted.get(i));
+    }
+    Collections.sort(sorted, new Comparator<CommentInfo>() {
+      @Override
+      public int compare(CommentInfo o1, CommentInfo o2) {
+        return o1.updated().compareTo(o2.updated());
+      }
+    });
+    return sorted;
   }
 
   private void renderPublished() {
-    for (int i = 0; published != null && i < published.length(); i++) {
+    List<CommentInfo> sorted = sortComment(published);
-      CommentInfo info = published.get(i);
+    for (CommentInfo info : sorted) {
       final PublishedBox box =
           new PublishedBox(this, revision, info, commentLinkProcessor);
       box.setOpen(false);
-      addCommentBox(info, box);
       initialBoxes.add(box);
       publishedMap.put(info.id(), box);
-      getLineBoxMapFromSide(info.side()).put(info.line() - 1, box);
+      int line = info.line() - 1;
+      LineHandle handle = getCmFromSide(info.side()).getLineHandle(line);
+      lineLastPublishedBoxMap.put(handle, box);
+      lineActiveBoxMap.put(handle, box);
+      addCommentBox(info, box);
     }
   }
 
   private void renderDrafts() {
-    for (int i = 0; drafts != null && i < drafts.length(); i++) {
+    List<CommentInfo> sorted = sortComment(drafts);
-      CommentInfo info = drafts.get(i);
+    for (CommentInfo info : sorted) {
       final DraftBox box =
           new DraftBox(this, revision, info, commentLinkProcessor, false, false);
       box.setOpen(false);
       box.setEdit(false);
-      addCommentBox(info, box);
       initialBoxes.add(box);
       if (published != null) {
         PublishedBox replyToBox = publishedMap.get(info.in_reply_to());
@@ -370,7 +412,9 @@ public class CodeMirrorDemo extends Screen {
           replyToBox.registerReplyBox(box);
         }
       }
-      getLineBoxMapFromSide(info.side()).put(info.line() - 1, box);
+      lineActiveBoxMap.put(
+          getCmFromSide(info.side()).getLineHandle(info.line() - 1), box);
+      addCommentBox(info, box);
     }
   }
 
@@ -378,8 +422,8 @@ public class CodeMirrorDemo extends Screen {
     return me == cmA ? cmB : cmA;
   }
 
-  private Map<Integer, CommentBox> getLineBoxMapFromSide(Side side) {
+  private CodeMirror getCmFromSide(Side side) {
-    return side == Side.PARENT ? lineBoxMapA : lineBoxMapB;
+    return side == Side.PARENT ? cmA : cmB;
   }
 
   private void markEdit(CodeMirror cm, JsArrayString lines,
@@ -423,17 +467,20 @@ public class CodeMirrorDemo extends Screen {
   private void insertEmptyLines(CodeMirror cm, int nextLine, int cnt) {
     // -1 to compensate for the line we went past when this method is called.
     addPaddingWidget(cm, diffTable.style.padding(), nextLine - 1,
-        cnt, Unit.EM);
+        cnt, Unit.EM, null);
   }
 
   private LineWidgetElementPair addPaddingWidget(CodeMirror cm, String style,
-      int line, int height, Unit unit) {
+      int line, int height, Unit unit, Integer index) {
     Element div = DOM.createDiv();
     div.setClassName(style);
     div.getStyle().setHeight(height, unit);
     Configuration config = Configuration.create()
         .set("coverGutter", true)
         .set("above", line == -1);
+    if (index != null) {
+      config = config.set("insertAt", index);
+    }
     LineWidget widget = cm.addLineWidget(line == -1 ? 0 : line, div, config);
     return new LineWidgetElementPair(widget, div);
   }
@@ -484,14 +531,13 @@ public class CodeMirrorDemo extends Screen {
   private Runnable insertNewDraft(final CodeMirror cm) {
     return new Runnable() {
       public void run() {
-        Map<Integer, CommentBox> lineBoxMap = cm == cmA ?
-            lineBoxMapA : lineBoxMapB;
         int line = cm.getActiveLine();
-        CommentBox box = lineBoxMap.get(line);
+        LineHandle handle = cm.getLineHandle(line);
+        CommentBox box = lineActiveBoxMap.get(handle);
         if (box == null) {
-          lineBoxMap.put(line, addNewDraft(cm, line));
+          lineActiveBoxMap.put(handle, addNewDraft(cm, line));
         } else if (box.isDraft()) {
-          ((DraftBox) lineBoxMap.get(line)).setEdit(true);
+          ((DraftBox) lineActiveBoxMap.get(handle)).setEdit(true);
         } else {
           ((PublishedBox) box).onReply(null);
         }
@@ -505,16 +551,6 @@ public class CodeMirrorDemo extends Screen {
         : null;
   }
 
-  private static class LineWidgetElementPair {
-    private LineWidget widget;
-    private Element element;
-
-    private LineWidgetElementPair(LineWidget w, Element e) {
-      widget = w;
-      element = e;
-    }
-  }
-
   static class EditIterator {
     private final JsArrayString lines;
     private final int startLine;

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java

@@ -19,8 +19,6 @@ import com.google.gerrit.client.ui.CommentLinkProcessor;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gwt.core.client.Scheduler;
 import com.google.gwt.core.client.Scheduler.ScheduledCommand;
-import com.google.gwt.dom.client.Element;
-import com.google.gwt.dom.client.Style.Unit;
 import com.google.gwt.event.dom.client.ClickEvent;
 import com.google.gwt.event.dom.client.ClickHandler;
 import com.google.gwt.event.shared.HandlerRegistration;
@@ -48,11 +46,10 @@ abstract class CommentBox extends Composite {
   private HandlerRegistration headerClick;
   private CommentInfo original;
   private PatchSet.Id patchSetId;
-  private LineWidget selfWidget;
+  private PaddingManager widgetManager;
-  private LineWidget paddingWidget;
-  private Element paddingWidgetEle;
   private CodeMirrorDemo diffView;
   private boolean draft;
+  private LineWidget selfWidget;
 
   @UiField(provided=true)
   CommentBoxHeader header;
@@ -63,7 +60,7 @@ abstract class CommentBox extends Composite {
   @UiField
   CommentBoxResources res;
 
-  protected CommentBox(
+  CommentBox(
       CodeMirrorDemo host,
       UiBinder<? extends Widget, CommentBox> binder,
       PatchSet.Id id, CommentInfo info, CommentLinkProcessor linkProcessor,
@@ -101,27 +98,20 @@ abstract class CommentBox extends Composite {
     }
   }
 
-  void setSelfWidget(LineWidget widget) {
-    selfWidget = widget;
-  }
-
-  void setPadding(LineWidget widget, Element element) {
-    paddingWidget = widget;
-    paddingWidgetEle = element;
-  }
-
   void resizePaddingWidget() {
-    Scheduler.get().scheduleDeferred(new ScheduledCommand() {
+    Scheduler.get().scheduleDeferred(new ScheduledCommand(){
-      @Override
       public void execute() {
-        paddingWidgetEle.getStyle().setHeight(getOffsetHeight(), Unit.PX);
+        if (selfWidget == null || widgetManager == null) {
-        paddingWidget.changed();
+          throw new IllegalStateException(
+              "resizePaddingWidget() called before setting up widgets");
+        }
         selfWidget.changed();
+        widgetManager.resizePaddingWidget();
       }
     });
   }
 
-  protected void setMessageText(String message) {
+  void setMessageText(String message) {
     if (message == null) {
       message = "";
     } else {
@@ -133,11 +123,11 @@ abstract class CommentBox extends Composite {
     SafeHtml.set(contentPanelMessage, buf);
   }
 
-  protected void setDate(Timestamp when) {
+  void setDate(Timestamp when) {
     header.setDate(when);
   }
 
-  protected void setOpen(boolean open) {
+  void setOpen(boolean open) {
     if (open) {
       removeStyleName(res.style().close());
       addStyleName(res.style().open());
@@ -152,31 +142,39 @@ abstract class CommentBox extends Composite {
     return getStyleName().contains(res.style().open());
   }
 
-  protected CodeMirrorDemo getDiffView() {
+  CodeMirrorDemo getDiffView() {
     return diffView;
   }
 
-  protected PatchSet.Id getPatchSetId() {
+  PatchSet.Id getPatchSetId() {
     return patchSetId;
   }
 
-  protected CommentInfo getOriginal() {
+  CommentInfo getOriginal() {
     return original;
   }
 
-  protected LineWidget getSelfWidget() {
+  void updateOriginal(CommentInfo newInfo) {
-    return selfWidget;
-  }
-
-  protected LineWidget getPaddingWidget() {
-    return paddingWidget;
-  }
-
-  protected void updateOriginal(CommentInfo newInfo) {
     original = newInfo;
   }
 
+  PaddingManager getPaddingManager() {
+    return widgetManager;
+  }
+
   boolean isDraft() {
     return draft;
   }
+
+  void setPaddingManager(PaddingManager manager) {
+    widgetManager = manager;
+  }
+
+  void setSelfWidget(LineWidget widget) {
+    selfWidget = widget;
+  }
+
+  LineWidget getSelfWidget() {
+    return selfWidget;
+  }
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DraftBox.java

@@ -139,10 +139,12 @@ class DraftBox extends CommentBox {
       replyToBox.unregisterReplyBox();
     }
     CommentInfo info = getOriginal();
-    getDiffView().removeCommentBox(info.side(), info.line() - 1);
+    getDiffView().removeDraft(info.side(), info.line() - 1);
     removeFromParent();
     getSelfWidget().clear();
-    getPaddingWidget().clear();
+    PaddingManager manager = getPaddingManager();
+    manager.remove(this);
+    manager.resizePaddingWidget();
   }
 
   @UiHandler("edit")

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PaddingManager.java

@@ -0,0 +1,138 @@
+// Copyright (C) 2013 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.client.diff;
+
+import com.google.gwt.dom.client.Element;
+import com.google.gwt.dom.client.Style.Unit;
+
+import net.codemirror.lib.LineWidget;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Manages paddings for CommentBoxes. Each line that may need to be padded owns
+ * a PaddingManager instance, which maintains a padding widget whose height
+ * changes as necessary. PaddingManager calculates padding by taking the
+ * difference of the sum of CommentBox heights on the two sides.
+ *
+ * Note that in the case of an insertion or deletion gap, A PaddingManager
+ * can map to a list of managers on the other side. The padding needed is then
+ * calculated from the sum of all their heights.
+ *
+ * TODO: Let PaddingManager also take care of the paddings introduced by
+ * insertions and deletions.
+ */
+class PaddingManager {
+  private List<CommentBox> comments;
+  private LineWidgetElementPair padding;
+  private List<PaddingManager> others;
+
+  PaddingManager(LineWidgetElementPair padding) {
+    comments = new ArrayList<CommentBox>();
+    others = new ArrayList<PaddingManager>();
+    this.padding = padding;
+  }
+
+  static void link(PaddingManager a, PaddingManager b) {
+    if (!a.others.contains(b)) {
+      a.others.add(b);
+    }
+    if (!b.others.contains(a)) {
+      b.others.add(a);
+    }
+  }
+
+  private int getMyTotalHeight() {
+    int total = 0;
+    for (CommentBox box : comments) {
+      total += box.getOffsetHeight();
+    }
+    return total;
+  }
+
+  /**
+   * If this instance is on the insertion side, its counterpart on the other
+   * side will map to a group of PaddingManagers on this side, so we calculate
+   * the group's total height instead of an individual one's.
+   */
+  private int getGroupTotalHeight() {
+    if (others.size() > 1) {
+      return getMyTotalHeight();
+    } else {
+      return others.get(0).getOthersTotalHeight();
+    }
+  }
+
+  private int getOthersTotalHeight() {
+    int total = 0;
+    for (PaddingManager manager : others) {
+      total += manager.getMyTotalHeight();
+    }
+    return total;
+  }
+
+  private void setPaddingHeight(int height) {
+    padding.element.getStyle().setHeight(height, Unit.PX);
+    padding.widget.changed();
+  }
+
+  void resizePaddingWidget() {
+    if (others.isEmpty()) {
+      throw new IllegalStateException("resizePaddingWidget() called before linking");
+    }
+    int myHeight = getGroupTotalHeight();
+    int othersHeight = getOthersTotalHeight();
+    int paddingNeeded = othersHeight - myHeight;
+    if (paddingNeeded < 0) {
+      for (PaddingManager manager : others.get(0).others) {
+        manager.setPaddingHeight(0);
+      }
+      others.get(others.size() - 1).setPaddingHeight(-paddingNeeded);
+    } else {
+      setPaddingHeight(paddingNeeded);
+      for (PaddingManager other : others) {
+        other.setPaddingHeight(0);
+      }
+    }
+  }
+
+  /** This is unused now because threading info is ignored. */
+  int getReplyIndex(CommentBox box) {
+    return comments.indexOf(box) + 1;
+  }
+
+  int getCurrentCount() {
+    return comments.size();
+  }
+
+  void insert(CommentBox box, int index) {
+    comments.add(index, box);
+  }
+
+  void remove(CommentBox box) {
+    comments.remove(box);
+  }
+
+  static class LineWidgetElementPair {
+    private LineWidget widget;
+    private Element element;
+
+    LineWidgetElementPair(LineWidget w, Element e) {
+      widget = w;
+      element = e;
+    }
+  }
+}

gerrit-gwtui/src/main/java/net/codemirror/lib/CodeMirror.java

@@ -130,6 +130,15 @@ public class CodeMirror extends JavaScriptObject {
 
   public final native void addKeyMap(KeyMap map) /*-{ this.addKeyMap(map); }-*/;
 
+  public final native LineHandle getLineHandle(int line) /*-{
+    return this.getLineHandle(line);
+  }-*/;
+
   protected CodeMirror() {
   }
+
+  public static class LineHandle extends JavaScriptObject {
+    protected LineHandle(){
+    }
+  }
 }