Merge changes I826fdc7d,I2e1cbe93

* changes: Implementing UI for range comment Fix handling of comment sides
2013-08-15 06:31:28 +00:00
parent da65c27bc8 8b21a8f169
commit 81a37a868e
13 changed files with 278 additions and 132 deletions
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java
@@ -16,12 +16,18 @@ package com.google.gerrit.client.diff;

 import com.google.gerrit.client.changes.CommentInfo;
 import com.google.gerrit.client.diff.PaddingManager.PaddingWidgetWrapper;
+import com.google.gerrit.client.diff.SideBySide2.DisplaySide;
 import com.google.gerrit.client.diff.SideBySide2.DiffChunkInfo;
 import com.google.gerrit.client.diff.SidePanel.GutterWrapper;
 import com.google.gwt.core.client.Scheduler;
 import com.google.gwt.core.client.Scheduler.ScheduledCommand;
 import com.google.gwt.user.client.ui.Composite;

+import net.codemirror.lib.CodeMirror;
+import net.codemirror.lib.Configuration;
+import net.codemirror.lib.TextMarker;
+import net.codemirror.lib.TextMarker.FromTo;
+
 /** An HtmlPanel for displaying a comment */
 abstract class CommentBox extends Composite {
  static {
@@ -31,8 +37,27 @@ abstract class CommentBox extends Composite {
  private PaddingManager widgetManager;
  private PaddingWidgetWrapper selfWidgetWrapper;
  private SideBySide2 parent;
+  private CodeMirror cm;
+  private DisplaySide side;
  private DiffChunkInfo diffChunkInfo;
  private GutterWrapper gutterWrapper;
+  private FromTo fromTo;
+  private TextMarker rangeMarker;
+  private TextMarker rangeHighlightMarker;
+
+  CommentBox(CodeMirror cm, CommentInfo info, DisplaySide side) {
+    this.cm = cm;
+    this.side = side;
+    CommentRange range = info.range();
+    if (range != null) {
+      fromTo = FromTo.create(range);
+      rangeMarker = cm.markText(
+          fromTo.getFrom(),
+          fromTo.getTo(),
+          Configuration.create()
+              .set("className", DiffTable.style.range()));
+    }
+  }

  @Override
  protected void onLoad() {
@@ -49,8 +74,7 @@ abstract class CommentBox extends Composite {
        assert selfWidgetWrapper != null;
        selfWidgetWrapper.getWidget().changed();
        if (diffChunkInfo != null) {
-          parent.resizePaddingOnOtherSide(getCommentInfo().side(),
-              diffChunkInfo.getEnd());
+          parent.resizePaddingOnOtherSide(side, diffChunkInfo.getEnd());
        } else {
          assert widgetManager != null;
          widgetManager.resizePaddingWidget();
@@ -64,6 +88,7 @@ abstract class CommentBox extends Composite {

  void setOpen(boolean open) {
    resizePaddingWidget();
+    setRangeHighlight(open);
  }

  PaddingManager getPaddingManager() {
@@ -94,7 +119,36 @@ abstract class CommentBox extends Composite {
    gutterWrapper = wrapper;
  }

+  void setRangeHighlight(boolean highlight) {
+    if (fromTo != null) {
+      if (highlight && rangeHighlightMarker == null) {
+        rangeHighlightMarker = cm.markText(
+            fromTo.getFrom(),
+            fromTo.getTo(),
+            Configuration.create()
+                .set("className", DiffTable.style.rangeHighlight()));
+      } else if (!highlight && rangeHighlightMarker != null) {
+        rangeHighlightMarker.clear();
+        rangeHighlightMarker = null;
+      }
+    }
+  }
+
+  void clearRange() {
+    if (rangeMarker != null) {
+      rangeMarker.clear();
+    }
+  }
+
  GutterWrapper getGutterWrapper() {
    return gutterWrapper;
  }
+
+  DisplaySide getSide() {
+    return side;
+  }
+
+  CodeMirror getCm() {
+    return cm;
+  }
 }
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentRange.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentRange.java
@@ -16,6 +16,9 @@ package com.google.gerrit.client.diff;

 import com.google.gwt.core.client.JavaScriptObject;

+import net.codemirror.lib.LineCharacter;
+import net.codemirror.lib.TextMarker.FromTo;
+
 public class CommentRange extends JavaScriptObject {
  public static CommentRange create(int sl, int sc, int el, int ec) {
    CommentRange r = createObject().cast();
@@ -23,6 +26,18 @@ public class CommentRange extends JavaScriptObject {
    return r;
  }

+  public static CommentRange create(FromTo fromTo) {
+    if (fromTo == null) {
+      return null;
+    }
+
+    LineCharacter from = fromTo.getFrom();
+    LineCharacter to = fromTo.getTo();
+    return create(
+        from.getLine() + 1, from.getCh(),
+        to.getLine() + 1, to.getCh());
+  }
+
  public final native int start_line() /*-{ return this.start_line; }-*/;
  public final native int start_character() /*-{ return this.start_character; }-*/;
  public final native int end_line() /*-{ return this.end_line; }-*/;
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java
@@ -14,7 +14,7 @@

 package com.google.gerrit.client.diff;

-import com.google.gerrit.common.changes.Side;
+import com.google.gerrit.client.diff.SideBySide2.DisplaySide;
 import com.google.gwt.core.client.GWT;
 import com.google.gwt.dom.client.Element;
 import com.google.gwt.resources.client.CssResource;
@@ -40,6 +40,8 @@ class DiffTable extends Composite {
    String activeLine();
    String activeLineBg();
    String hideNumber();
+    String range();
+    String rangeHighlight();
  }

  @UiField
@@ -87,10 +89,10 @@ class DiffTable extends Composite {
  private SideBySide2 host;

  DiffTable(SideBySide2 host, String path) {
-    patchSelectBoxA = new PatchSelectBox2(this, Side.PARENT);
-    patchSelectBoxB = new PatchSelectBox2(this, Side.REVISION);
-    fileCommentPanelA = new FileCommentPanel(host, this, path, Side.PARENT);
-    fileCommentPanelB = new FileCommentPanel(host, this, path, Side.REVISION);
+    patchSelectBoxA = new PatchSelectBox2(this, DisplaySide.A);
+    patchSelectBoxB = new PatchSelectBox2(this, DisplaySide.B);
+    fileCommentPanelA = new FileCommentPanel(host, this, path, DisplaySide.A);
+    fileCommentPanelB = new FileCommentPanel(host, this, path, DisplaySide.B);
    initWidget(uiBinder.createAndBindUi(this));
    this.host = host;
  }
@@ -111,21 +113,21 @@ class DiffTable extends Composite {
    host.resizeCodeMirror();
  }

-  private FileCommentPanel getPanelFromSide(Side side) {
-    return side == Side.PARENT ? fileCommentPanelA : fileCommentPanelB;
+  private FileCommentPanel getPanelFromSide(DisplaySide side) {
+    return side == DisplaySide.A ? fileCommentPanelA : fileCommentPanelB;
  }

-  void createOrEditFileComment(Side side) {
+  void createOrEditFileComment(DisplaySide side) {
    getPanelFromSide(side).createOrEditFileComment();
    updateFileCommentVisibility(false);
  }

-  void addFileCommentBox(CommentBox box, Side side) {
-    getPanelFromSide(side).addFileComment(box);
+  void addFileCommentBox(CommentBox box) {
+    getPanelFromSide(box.getSide()).addFileComment(box);
  }

-  void onRemoveDraftBox(DraftBox box, Side side) {
-    getPanelFromSide(side).onRemoveDraftBox(box);
+  void onRemoveDraftBox(DraftBox box) {
+    getPanelFromSide(box.getSide()).onRemoveDraftBox(box);
  }

  int getHeaderHeight() {
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml
@@ -76,6 +76,12 @@ limitations under the License.
    .activeLineBg {
      background-color: #E0FFFF !important;
    }
+    .range {
+      background-color: #ffd500 !important;
+    }
+    .rangeHighlight {
+      background-color: #ffff00 !important;
+    }
    .cm-searching {
      background-color: #ffa !important;
    }
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DraftBox.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DraftBox.java
@@ -18,9 +18,9 @@ import com.google.gerrit.client.FormatUtil;
 import com.google.gerrit.client.changes.CommentApi;
 import com.google.gerrit.client.changes.CommentInfo;
 import com.google.gerrit.client.changes.CommentInput;
+import com.google.gerrit.client.diff.SideBySide2.DisplaySide;
 import com.google.gerrit.client.rpc.GerritCallback;
 import com.google.gerrit.client.ui.CommentLinkProcessor;
-import com.google.gerrit.common.changes.Side;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gwt.core.client.GWT;
 import com.google.gwt.core.client.JavaScriptObject;
@@ -59,7 +59,6 @@ class DraftBox extends CommentBox {
  private static final int MAX_LINES = 30;

  private final SideBySide2 parent;
-  private final CodeMirror cm;
  private final CommentLinkProcessor linkProcessor;
  private final PatchSet.Id psId;
  private CommentInfo comment;
@@ -82,15 +81,17 @@ class DraftBox extends CommentBox {
  @UiField Button discard2;

  DraftBox(
-      SideBySide2 parent,
+      SideBySide2 sideBySide,
      CodeMirror cm,
+      DisplaySide side,
      CommentLinkProcessor clp,
      PatchSet.Id id,
      CommentInfo info) {
-    this.parent = parent;
-    this.cm = cm;
-    this.linkProcessor = clp;
-    this.psId = id;
+    super(cm, info, side);
+
+    parent = sideBySide;
+    linkProcessor = clp;
+    psId = id;
    initWidget(uiBinder.createAndBindUi(this));

    expandTimer = new Timer() {
@@ -136,7 +137,7 @@ class DraftBox extends CommentBox {
      message.setHTML(linkProcessor.apply(
          new SafeHtmlBuilder().append(msg).wikify()));
    }
-    this.comment = info;
+    comment = info;
  }

  @Override
@@ -178,6 +179,7 @@ class DraftBox extends CommentBox {
    UIObject.setVisible(p_view, !edit);
    UIObject.setVisible(p_edit, edit);

+    setRangeHighlight(edit);
    if (edit) {
      final String msg = comment.message() != null
          ? comment.message().trim()
@@ -215,16 +217,17 @@ class DraftBox extends CommentBox {
    if (replyToBox != null) {
      replyToBox.unregisterReplyBox();
    }
-    Side side = comment.side();
+    clearRange();
+    setRangeHighlight(false);
    removeFromParent();
    if (!getCommentInfo().has_line()) {
-      parent.removeFileCommentBox(this, side);
+      parent.removeFileCommentBox(this);
      return;
    }
    PaddingManager manager = getPaddingManager();
    manager.remove(this);
-    parent.removeDraft(this, side, comment.line() - 1);
-    cm.focus();
+    parent.removeDraft(this, comment.line() - 1);
+    getCm().focus();
    getSelfWidgetWrapper().getWidget().clear();
    getGutterWrapper().remove();
    Scheduler.get().scheduleDeferred(new ScheduledCommand() {
@@ -292,7 +295,7 @@ class DraftBox extends CommentBox {
    } else {
      CommentApi.updateDraft(psId, original.id(), input, cb);
    }
-    cm.focus();
+    getCm().focus();
  }

  private void enableEdit(boolean on) {
@@ -309,7 +312,7 @@ class DraftBox extends CommentBox {
      removeUI();
    } else {
      setEdit(false);
-      cm.focus();
+      getCm().focus();
    }
  }

@@ -347,7 +350,7 @@ class DraftBox extends CommentBox {
        return;
      } else {
        setEdit(false);
-        cm.focus();
+        getCm().focus();
        return;
      }
    }
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/FileCommentPanel.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/FileCommentPanel.java
@@ -16,7 +16,7 @@ package com.google.gerrit.client.diff;

 import com.google.gerrit.client.Gerrit;
 import com.google.gerrit.client.changes.CommentInfo;
-import com.google.gerrit.common.changes.Side;
+import com.google.gerrit.client.diff.SideBySide2.DisplaySide;
 import com.google.gwt.user.client.ui.Composite;
 import com.google.gwt.user.client.ui.FlowPanel;

@@ -33,11 +33,11 @@ class FileCommentPanel extends Composite {
  private SideBySide2 parent;
  private DiffTable table;
  private String path;
-  private Side side;
+  private DisplaySide side;
  private List<CommentBox> boxes;
  private FlowPanel body;

-  FileCommentPanel(SideBySide2 host, DiffTable table, String path, Side side) {
+  FileCommentPanel(SideBySide2 host, DiffTable table, String path, DisplaySide side) {
    this.parent = host;
    this.table = table;
    this.path = path;
@@ -54,10 +54,10 @@ class FileCommentPanel extends Composite {
    if (boxes.isEmpty()) {
      CommentInfo info = CommentInfo.createFile(
          path,
-          side,
+          parent.getStoredSideFromDisplaySide(side),
          null,
          null);
-      addFileComment(parent.addDraftBox(info));
+      addFileComment(parent.addDraftBox(info, side));
    } else {
      CommentBox box = boxes.get(boxes.size() - 1);
      if (box instanceof DraftBox) {
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/LineMapper.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/LineMapper.java
@@ -14,7 +14,7 @@

 package com.google.gerrit.client.diff;

-import com.google.gerrit.common.changes.Side;
+import com.google.gerrit.client.diff.SideBySide2.DisplaySide;

 import java.util.ArrayList;
 import java.util.Collections;
@@ -98,8 +98,8 @@ class LineMapper {
   *   -   |   6
   *      ...
   */
-  LineOnOtherInfo lineOnOther(Side mySide, int line) {
-    List<LineGap> lineGaps = mySide == Side.PARENT ? lineMapAtoB : lineMapBtoA;
+  LineOnOtherInfo lineOnOther(DisplaySide mySide, int line) {
+    List<LineGap> lineGaps = mySide == DisplaySide.A ? lineMapAtoB : lineMapBtoA;
    // Create a dummy LineGap for the search.
    int ret = Collections.binarySearch(lineGaps, new LineGap(line));
    if (ret == -1) {
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSelectBox2.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSelectBox2.java
@@ -15,8 +15,8 @@
 package com.google.gerrit.client.diff;

 import com.google.gerrit.client.Gerrit;
+import com.google.gerrit.client.diff.SideBySide2.DisplaySide;
 import com.google.gerrit.client.patches.PatchUtil;
-import com.google.gerrit.common.changes.Side;
 import com.google.gwt.core.client.GWT;
 import com.google.gwt.event.dom.client.ClickEvent;
 import com.google.gwt.uibinder.client.UiBinder;
@@ -38,9 +38,9 @@ class PatchSelectBox2 extends Composite {
  Image icon;

  private DiffTable table;
-  private Side side;
+  private DisplaySide side;

-  PatchSelectBox2(DiffTable table, Side side) {
+  PatchSelectBox2(DiffTable table, DisplaySide side) {
    initWidget(uiBinder.createAndBindUi(this));
    icon.setTitle(PatchUtil.C.addFileCommentToolTip());
    icon.addStyleName(Gerrit.RESOURCES.css().link());
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PublishedBox.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PublishedBox.java
@@ -21,6 +21,7 @@ import com.google.gerrit.client.changes.CommentApi;
 import com.google.gerrit.client.changes.CommentInfo;
 import com.google.gerrit.client.changes.CommentInput;
 import com.google.gerrit.client.changes.Util;
+import com.google.gerrit.client.diff.SideBySide2.DisplaySide;
 import com.google.gerrit.client.patches.PatchUtil;
 import com.google.gerrit.client.rpc.GerritCallback;
 import com.google.gerrit.client.ui.CommentLinkProcessor;
@@ -39,6 +40,8 @@ import com.google.gwt.user.client.ui.UIObject;
 import com.google.gwt.user.client.ui.Widget;
 import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder;

+import net.codemirror.lib.CodeMirror;
+
 /** An HtmlPanel for displaying a published comment */
 class PublishedBox extends CommentBox {
  interface Binder extends UiBinder<HTMLPanel, PublishedBox> {}
@@ -68,9 +71,13 @@ class PublishedBox extends CommentBox {

  PublishedBox(
      SideBySide2 parent,
+      CodeMirror cm,
+      DisplaySide side,
      CommentLinkProcessor clp,
      PatchSet.Id psId,
      CommentInfo info) {
+    super(cm, info, side);
+
    this.parent = parent;
    this.psId = psId;
    this.comment = info;
@@ -137,7 +144,7 @@ class PublishedBox extends CommentBox {
  }

  DraftBox addReplyBox() {
-    DraftBox box = parent.addDraftBox(parent.createReply(comment));
+    DraftBox box = parent.addDraftBox(parent.createReply(comment), getSide());
    registerReplyBox(box);
    return box;
  }
@@ -148,7 +155,7 @@ class PublishedBox extends CommentBox {
    } else if (replyBox == null) {
      DraftBox box = addReplyBox();
      if (!getCommentInfo().has_line()) {
-        parent.addFileCommentBox(box, comment.side());
+        parent.addFileCommentBox(box);
      }
    } else {
      openReplyBox();
@@ -176,10 +183,10 @@ class PublishedBox extends CommentBox {
            public void onSuccess(CommentInfo result) {
              done.setEnabled(true);
              setOpen(false);
-              DraftBox box = parent.addDraftBox(result);
+              DraftBox box = parent.addDraftBox(result, getSide());
              registerReplyBox(box);
              if (!getCommentInfo().has_line()) {
-                parent.addFileCommentBox(box, comment.side());
+                parent.addFileCommentBox(box);
              }
            }
          });
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java
@@ -78,6 +78,7 @@ import net.codemirror.lib.LineCharacter;
 import net.codemirror.lib.LineWidget;
 import net.codemirror.lib.ModeInjector;
 import net.codemirror.lib.ScrollInfo;
+import net.codemirror.lib.TextMarker.FromTo;

 import java.util.ArrayList;
 import java.util.Collections;
@@ -90,6 +91,10 @@ public class SideBySide2 extends Screen {
  interface Binder extends UiBinder<FlowPanel, SideBySide2> {}
  private static Binder uiBinder = GWT.create(Binder.class);

+  enum DisplaySide {
+    A, B;
+  }
+
  private static final JsArrayString EMPTY =
      JavaScriptObject.createArray().cast();

@@ -108,8 +113,10 @@ public class SideBySide2 extends Screen {
  private Timer scrollTimerA;
  private Timer scrollTimerB;
  private HandlerRegistration resizeHandler;
-  private JsArray<CommentInfo> published;
-  private JsArray<CommentInfo> drafts;
+  private JsArray<CommentInfo> publishedBase;
+  private JsArray<CommentInfo> publishedRevision;
+  private JsArray<CommentInfo> draftsBase;
+  private JsArray<CommentInfo> draftsRevision;
  private DiffInfo diff;
  private LineMapper mapper;
  private CommentLinkProcessor commentLinkProcessor;
@@ -176,16 +183,14 @@ public class SideBySide2 extends Screen {
        }
      }));
    if (base != null) {
-      CommentApi.comments(base, group.add(getCommentCallback(false)));
+      CommentApi.comments(base, group.add(getCommentCallback(DisplaySide.A, false)));
    }
-    CommentApi.comments(revision, group.add(getCommentCallback(false)));
+    CommentApi.comments(revision, group.add(getCommentCallback(DisplaySide.B, false)));
    if (Gerrit.isSignedIn()) {
      if (base != null) {
-        CommentApi.drafts(base, group.add(getCommentCallback(true)));
+        CommentApi.drafts(base, group.add(getCommentCallback(DisplaySide.A, true)));
      }
-      CommentApi.drafts(revision, group.add(getCommentCallback(true)));
-    } else {
-      drafts = JsArray.createArray().cast();
+      CommentApi.drafts(revision, group.add(getCommentCallback(DisplaySide.B, true)));
    }
    ConfigInfoCache.get(revision.getParentKey(), group.addFinal(
        new ScreenLoadCallback<ConfigInfoCache.Entry>(SideBySide2.this) {
@@ -321,31 +326,30 @@ public class SideBySide2 extends Screen {
  }

  private GerritCallback<NativeMap<JsArray<CommentInfo>>> getCommentCallback(
-      final boolean toDrafts) {
+      final DisplaySide side, final boolean toDrafts) {
    return new GerritCallback<NativeMap<JsArray<CommentInfo>>>() {
      @Override
      public void onSuccess(NativeMap<JsArray<CommentInfo>> result) {
        JsArray<CommentInfo> in = result.get(path);
        if (in != null) {
-          addAllToCommentList(in, toDrafts);
+          if (toDrafts) {
+            if (side == DisplaySide.A) {
+              draftsBase = in;
+            } else {
+              draftsRevision = in;
+            }
+          } else {
+            if (side == DisplaySide.A) {
+              publishedBase = in;
+            } else {
+              publishedRevision = in;
+            }
+          }
        }
      }
    };
  }

-  private void addAllToCommentList(JsArray<CommentInfo> in, boolean toDrafts) {
-    if (toDrafts && drafts == null) {
-      drafts = in;
-    } else if (!toDrafts && published == null) {
-      published = in;
-    } else {
-      JsArray<CommentInfo> dest = toDrafts ? drafts : published;
-      for (int i = 0; i < in.length(); i++) {
-        dest.push(in.get(i));
-      }
-    }
-  }
-
  private void display(DiffInfo diffInfo) {
    cmA = displaySide(diffInfo.meta_a(), diffInfo.text_a(), diffTable.cmA);
    cmB = displaySide(diffInfo.meta_b(), diffInfo.text_b(), diffTable.cmB);
@@ -356,17 +360,22 @@ public class SideBySide2 extends Screen {
    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 (publishedBase != null || publishedRevision != null) {
+      publishedMap = new HashMap<String, PublishedBox>();
    }
-    if (drafts != null) {
-      renderDrafts();
+    if (publishedBase != null) {
+      renderPublished(publishedBase);
+    }
+    if (publishedRevision != null) {
+      renderPublished(publishedRevision);
+    }
+    if (draftsBase != null) {
+      renderDrafts(draftsBase);
+    }
+    if (draftsRevision != null) {
+      renderDrafts(draftsRevision);
    }
    renderSkips();
-    published = null;
-    drafts = null;
-    skips = null;
    registerCmEvents(cmA);
    registerCmEvents(cmB);
    resizeHandler = Window.addResizeHandler(new ResizeHandler() {
@@ -470,29 +479,29 @@ public class SideBySide2 extends Screen {
    }
  }

-  private DraftBox addNewDraft(CodeMirror cm, int line) {
+  private DraftBox addNewDraft(CodeMirror cm, int line, FromTo fromTo) {
+    DisplaySide side = getSideFromCm(cm);
    return addDraftBox(CommentInfo.createRange(
        path,
-        getSideFromCm(cm),
+        getStoredSideFromDisplaySide(side),
        line + 1,
        null,
        null,
-        null));
+        CommentRange.create(fromTo)), side);
  }

  CommentInfo createReply(CommentInfo replyTo) {
-    if (!replyTo.has_line()) {
+    if (!replyTo.has_line() && replyTo.range() == null) {
      return CommentInfo.createFile(path, replyTo.side(), replyTo.id(), null);
    } else {
      return CommentInfo.createRange(path, replyTo.side(), replyTo.line(),
-          replyTo.id(), null, null);
+          replyTo.id(), null, replyTo.range());
    }
  }

-  DraftBox addDraftBox(CommentInfo info) {
-    Side side = info.side();
+  DraftBox addDraftBox(CommentInfo info, DisplaySide side) {
    CodeMirror cm = getCmFromSide(side);
-    final DraftBox box = new DraftBox(this, cm, commentLinkProcessor,
+    final DraftBox box = new DraftBox(this, cm, side, commentLinkProcessor,
        getPatchSetIdFromSide(side), info);
    if (info.id() == null) {
      Scheduler.get().scheduleDeferred(new ScheduledCommand() {
@@ -514,8 +523,8 @@ public class SideBySide2 extends Screen {

  CommentBox addCommentBox(CommentInfo info, CommentBox box) {
    diffTable.add(box);
-    Side mySide = info.side();
-    CodeMirror cm = mySide == Side.PARENT ? cmA : cmB;
+    DisplaySide side = box.getSide();
+    CodeMirror cm = getCmFromSide(side);
    CodeMirror other = otherCm(cm);
    int line = info.line() - 1; // CommentInfo is 1-based, but CM is 0-based
    LineHandle handle = cm.getLineHandle(line);
@@ -528,9 +537,9 @@ public class SideBySide2 extends Screen {
          addPaddingWidget(cm, DiffTable.style.padding(), line, 0, Unit.PX, 0));
      linePaddingManagerMap.put(handle, manager);
    }
-    int lineToPad = mapper.lineOnOther(mySide, line).getLine();
+    int lineToPad = mapper.lineOnOther(side, line).getLine();
    LineHandle otherHandle = other.getLineHandle(lineToPad);
-    DiffChunkInfo myChunk = getDiffChunk(mySide, line);
+    DiffChunkInfo myChunk = getDiffChunk(side, line);
    DiffChunkInfo otherChunk = getDiffChunk(getSideFromCm(other), lineToPad);
    PaddingManager otherManager;
    if (linePaddingManagerMap.containsKey(otherHandle)) {
@@ -562,20 +571,20 @@ public class SideBySide2 extends Screen {
    return box;
  }

-  void removeDraft(DraftBox box, Side side, int line) {
-    LineHandle handle = getCmFromSide(side).getLineHandle(line);
+  void removeDraft(DraftBox box, int line) {
+    LineHandle handle = getCmFromSide(box.getSide()).getLineHandle(line);
    lineActiveBoxMap.remove(handle);
    if (lineLastPublishedBoxMap.containsKey(handle)) {
      lineActiveBoxMap.put(handle, lineLastPublishedBoxMap.get(handle));
    }
  }

-  void addFileCommentBox(CommentBox box, Side side) {
-    diffTable.addFileCommentBox(box, side);
+  void addFileCommentBox(CommentBox box) {
+    diffTable.addFileCommentBox(box);
  }

-  void removeFileCommentBox(DraftBox box, Side side) {
-    diffTable.onRemoveDraftBox(box, side);
+  void removeFileCommentBox(DraftBox box) {
+    diffTable.onRemoveDraftBox(box);
  }

  private List<CommentInfo> sortComment(JsArray<CommentInfo> unsorted) {
@@ -592,16 +601,24 @@ public class SideBySide2 extends Screen {
    return sorted;
  }

-  private void renderPublished() {
+  private void renderPublished(JsArray<CommentInfo> published) {
    List<CommentInfo> sorted = sortComment(published);
    for (CommentInfo info : sorted) {
-      Side side = info.side();
+      DisplaySide side;
+      if (info.side() == Side.PARENT) {
+        if (base != null) {
+          continue;
+        }
+        side = DisplaySide.A;
+      } else {
+        side = published == publishedBase ? DisplaySide.A : DisplaySide.B;
+      }
      CodeMirror cm = getCmFromSide(side);
-      PublishedBox box = new PublishedBox(this, commentLinkProcessor,
+      PublishedBox box = new PublishedBox(this, cm, side, commentLinkProcessor,
          getPatchSetIdFromSide(side), info);
      publishedMap.put(info.id(), box);
      if (!info.has_line()) {
-        diffTable.addFileCommentBox(box, side);
+        diffTable.addFileCommentBox(box);
        continue;
      }
      int line = info.line() - 1;
@@ -612,21 +629,29 @@ public class SideBySide2 extends Screen {
    }
  }

-  private void renderDrafts() {
+  private void renderDrafts(JsArray<CommentInfo> drafts) {
    List<CommentInfo> sorted = sortComment(drafts);
    for (CommentInfo info : sorted) {
-      Side side = info.side();
+      DisplaySide side;
+      if (info.side() == Side.PARENT) {
+        if (base != null) {
+          continue;
+        }
+        side = DisplaySide.A;
+      } else {
+        side = drafts == draftsBase ? DisplaySide.A : DisplaySide.B;
+      }
      DraftBox box = new DraftBox(
-          this, getCmFromSide(side), commentLinkProcessor,
+          this, getCmFromSide(side), side, commentLinkProcessor,
          getPatchSetIdFromSide(side), info);
-      if (published != null) {
+      if (publishedBase != null || publishedRevision != null) {
        PublishedBox replyToBox = publishedMap.get(info.in_reply_to());
        if (replyToBox != null) {
          replyToBox.registerReplyBox(box);
        }
      }
      if (!info.has_line()) {
-        diffTable.addFileCommentBox(box, side);
+        diffTable.addFileCommentBox(box);
        continue;
      }
      lineActiveBoxMap.put(
@@ -723,16 +748,20 @@ public class SideBySide2 extends Screen {
    return me == cmA ? cmB : cmA;
  }

-  private PatchSet.Id getPatchSetIdFromSide(Side side) {
-    return side == Side.PARENT && base != null ? base : revision;
+  private PatchSet.Id getPatchSetIdFromSide(DisplaySide side) {
+    return side == DisplaySide.A && base != null ? base : revision;
  }

-  private CodeMirror getCmFromSide(Side side) {
-    return side == Side.PARENT ? cmA : cmB;
+  private CodeMirror getCmFromSide(DisplaySide side) {
+    return side == DisplaySide.A ? cmA : cmB;
  }

-  private Side getSideFromCm(CodeMirror cm) {
-    return cm == cmA ? Side.PARENT : Side.REVISION;
+  private DisplaySide getSideFromCm(CodeMirror cm) {
+    return cm == cmA ? DisplaySide.A : DisplaySide.B;
+  }
+
+  Side getStoredSideFromDisplaySide(DisplaySide side) {
+    return side == DisplaySide.A && base == null ? Side.PARENT : Side.REVISION;
  }

  private void markEdit(CodeMirror cm, JsArrayString lines,
@@ -848,7 +877,7 @@ public class SideBySide2 extends Screen {
    if (info.isAligned()) {
      double myHeight = cm.heightAtLine(line);
      double otherHeight = other.heightAtLine(info.getLine());
-      if (myHeight !=  otherHeight) {
+      if (myHeight != otherHeight) {
        other.scrollToY(other.getScrollInfo().getTop() + otherHeight - myHeight);
        other.setScrollSetAt(System.currentTimeMillis());
      }
@@ -888,7 +917,7 @@ public class SideBySide2 extends Screen {
          other.removeLineClass(otherActiveLine,
              LineClassWhere.BACKGROUND, DiffTable.style.activeLineBg());
        }
-        LineHandle handle = cm.getLineHandleVisualStart(cm.getCursor().getLine());
+        LineHandle handle = cm.getLineHandleVisualStart(cm.getCursor("end").getLine());
        cm.setActiveLine(handle);
        if (cm.somethingSelected()) {
          return;
@@ -914,9 +943,12 @@ public class SideBySide2 extends Screen {
      @Override
      public void handle(CodeMirror instance, int line, String gutter,
          NativeEvent clickEvent) {
-        instance.setCursor(LineCharacter.create(line));
-        instance.setActiveLine(instance.getLineHandle(line));
-        insertNewDraft(instance).run();
+        if (!(cm.hasActiveLine() &&
+            instance.getLineNumber(cm.getActiveLine()) == line)) {
+          instance.setCursor(LineCharacter.create(line));
+          instance.setActiveLine(cm.getLineHandle(line));
+        }
+        insertNewDraft(cm).run();
      }
    };
  }
@@ -935,8 +967,12 @@ public class SideBySide2 extends Screen {
        LineHandle handle = cm.getActiveLine();
        int line = cm.getLineNumber(handle);
        CommentBox box = lineActiveBoxMap.get(handle);
-        if (box == null) {
-          lineActiveBoxMap.put(handle, addNewDraft(cm, line));
+        FromTo fromTo = cm.getSelectedRange();
+        if (cm.somethingSelected()) {
+          lineActiveBoxMap.put(handle,
+              addNewDraft(cm, line, fromTo.getTo().getLine() == line ? fromTo : null));
+        } else if (box == null) {
+          lineActiveBoxMap.put(handle, addNewDraft(cm, line, null));
        } else if (box instanceof DraftBox) {
          ((DraftBox) box).setEdit(true);
        } else {
@@ -995,7 +1031,7 @@ public class SideBySide2 extends Screen {
    };
  }

-  private DiffChunkInfo getDiffChunk(Side side, int line) {
+  private DiffChunkInfo getDiffChunk(DisplaySide side, int line) {
    for (DiffChunkInfo info : diffChunks) {
      if (info.getSide() == side && info.getStart() <= line &&
          line <= info.getEnd()) {
@@ -1005,7 +1041,7 @@ public class SideBySide2 extends Screen {
    return null;
  }

-  void resizePaddingOnOtherSide(Side mySide, int line) {
+  void resizePaddingOnOtherSide(DisplaySide mySide, int line) {
    CodeMirror cm = getCmFromSide(mySide);
    LineHandle handle = cm.getLineHandle(line);
    final LinePaddingWidgetWrapper otherWrapper = linePaddingOnOtherSideMap.get(handle);
@@ -1051,7 +1087,7 @@ public class SideBySide2 extends Screen {
  }

  // TODO: Maybe integrate this with PaddingManager.
-  private RenderLineHandler resizeLinePadding(final Side side) {
+  private RenderLineHandler resizeLinePadding(final DisplaySide side) {
    return new RenderLineHandler() {
      @Override
      public void handle(final CodeMirror instance, final LineHandle handle,
@@ -1142,17 +1178,17 @@ public class SideBySide2 extends Screen {
  }

  static class DiffChunkInfo {
-    private Side side;
+    private DisplaySide side;
    private int start;
    private int end;

-    DiffChunkInfo(Side side, int start, int end) {
+    DiffChunkInfo(DisplaySide side, int start, int end) {
      this.side = side;
      this.start = start;
      this.end = end;
    }

-    Side getSide() {
+    DisplaySide getSide() {
      return side;
    }

--- a/gerrit-gwtui/src/main/java/net/codemirror/lib/CodeMirror.java
+++ b/gerrit-gwtui/src/main/java/net/codemirror/lib/CodeMirror.java
@@ -19,6 +19,8 @@ import com.google.gwt.dom.client.Element;
 import com.google.gwt.dom.client.NativeEvent;
 import com.google.gwt.user.client.rpc.AsyncCallback;

+import net.codemirror.lib.TextMarker.FromTo;
+
 /**
 * Glue to connect CodeMirror to be callable from GWT.
 *
@@ -205,6 +207,10 @@ public class CodeMirror extends JavaScriptObject {
    return this.getCursor(start);
  }-*/;

+  public final FromTo getSelectedRange() {
+    return FromTo.create(getCursor("start"), getCursor("end"));
+  };
+
  public final native void setCursor(LineCharacter lineCh) /*-{
    this.setCursor(lineCh);
  }-*/;
--- a/gerrit-gwtui/src/main/java/net/codemirror/lib/TextMarker.java
+++ b/gerrit-gwtui/src/main/java/net/codemirror/lib/TextMarker.java
@@ -14,6 +14,7 @@

 package net.codemirror.lib;

+import com.google.gerrit.client.diff.CommentRange;
 import com.google.gwt.core.client.JavaScriptObject;

 /** Object that represents a text marker within CodeMirror */
@@ -30,9 +31,25 @@ public class TextMarker extends JavaScriptObject {
  }

  public static class FromTo extends JavaScriptObject {
+    public static FromTo create(LineCharacter from, LineCharacter to) {
+      FromTo fromTo = createObject().cast();
+      fromTo.setFrom(from);
+      fromTo.setTo(to);
+      return fromTo;
+    }
+
+    public static FromTo create(CommentRange range) {
+      return create(
+          LineCharacter.create(range.start_line() - 1, range.start_character()),
+          LineCharacter.create(range.end_line() - 1, range.end_character()));
+    }
+
    public final native LineCharacter getFrom() /*-{ return this.from; }-*/;
    public final native LineCharacter getTo() /*-{ return this.to; }-*/;

+    public final native void setFrom(LineCharacter from) /*-{ this.from = from; }-*/;
+    public final native void setTo(LineCharacter to) /*-{ this.to = to; }-*/;
+
    protected FromTo() {
    }
  }
--- a/gerrit-gwtui/src/test/java/com/google/gerrit/client/diff/LineMapperTest.java
+++ b/gerrit-gwtui/src/test/java/com/google/gerrit/client/diff/LineMapperTest.java
@@ -17,7 +17,7 @@ package com.google.gerrit.client.diff;
 import static org.junit.Assert.assertEquals;

 import com.google.gerrit.client.diff.LineMapper.LineOnOtherInfo;
-import com.google.gerrit.common.changes.Side;
+import com.google.gerrit.client.diff.SideBySide2.DisplaySide;

 import org.junit.Test;

@@ -53,9 +53,9 @@ public class LineMapperTest {
    LineMapper mapper = new LineMapper();
    mapper.appendCommon(10);
    assertEquals(new LineOnOtherInfo(9, true),
-        mapper.lineOnOther(Side.PARENT, 9));
+        mapper.lineOnOther(DisplaySide.A, 9));
    assertEquals(new LineOnOtherInfo(9, true),
-        mapper.lineOnOther(Side.REVISION, 9));
+        mapper.lineOnOther(DisplaySide.B, 9));
  }

  @Test
@@ -63,9 +63,9 @@ public class LineMapperTest {
    LineMapper mapper = new LineMapper();
    mapper.appendCommon(10);
    assertEquals(new LineOnOtherInfo(10, true),
-        mapper.lineOnOther(Side.PARENT, 10));
+        mapper.lineOnOther(DisplaySide.A, 10));
    assertEquals(new LineOnOtherInfo(10, true),
-        mapper.lineOnOther(Side.REVISION, 10));
+        mapper.lineOnOther(DisplaySide.B, 10));
  }

  @Test
@@ -73,7 +73,7 @@ public class LineMapperTest {
    LineMapper mapper = new LineMapper();
    mapper.appendInsert(10);
    assertEquals(new LineOnOtherInfo(-1, false),
-        mapper.lineOnOther(Side.REVISION, 9));
+        mapper.lineOnOther(DisplaySide.B, 9));
  }

  @Test
@@ -81,9 +81,9 @@ public class LineMapperTest {
    LineMapper mapper = new LineMapper();
    mapper.appendInsert(10);
    assertEquals(new LineOnOtherInfo(0, true),
-        mapper.lineOnOther(Side.REVISION, 10));
+        mapper.lineOnOther(DisplaySide.B, 10));
    assertEquals(new LineOnOtherInfo(10, true),
-        mapper.lineOnOther(Side.PARENT, 0));
+        mapper.lineOnOther(DisplaySide.A, 0));
  }

  @Test
@@ -91,7 +91,7 @@ public class LineMapperTest {
    LineMapper mapper = new LineMapper();
    mapper.appendDelete(10);
    assertEquals(new LineOnOtherInfo(-1, false),
-        mapper.lineOnOther(Side.PARENT, 9));
+        mapper.lineOnOther(DisplaySide.A, 9));
  }

  @Test
@@ -99,8 +99,8 @@ public class LineMapperTest {
    LineMapper mapper = new LineMapper();
    mapper.appendDelete(10);
    assertEquals(new LineOnOtherInfo(0, true),
-        mapper.lineOnOther(Side.PARENT, 10));
+        mapper.lineOnOther(DisplaySide.A, 10));
    assertEquals(new LineOnOtherInfo(10, true),
-        mapper.lineOnOther(Side.REVISION, 0));
+        mapper.lineOnOther(DisplaySide.B, 0));
  }
 }