Merge "Add/remove comments in gr-new-diff"

2016-03-24 17:19:05 +00:00
parent 400d1de568 deaf0c217a
commit e0d970083a
8 changed files with 279 additions and 46 deletions
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
@@ -38,6 +38,10 @@
      patchNum: String,
      path: String,
      projectConfig: Object,
+      side: {
+        type: String,
+        value: 'REVISION',
+      },

      _showActions: Boolean,
      _boundWindowResizeHandler: {
@@ -68,6 +72,18 @@
      window.removeEventListener('resize', this._boundWindowResizeHandler);
    },

+    addDraft: function(lineNum) {
+      var lastComment = this.comments[this.comments.length - 1];
+      if (lastComment && lastComment.__draft) {
+        var commentEl = this._commentElWithDraftID(
+            lastComment.id || lastComment.__draftID);
+        commentEl.editing = true;
+        return;
+      }
+
+      this.push('comments', this._newDraft(lineNum));
+    },
+
    _getLoggedIn: function() {
      return this.$.restAPI.getLoggedIn();
    },
@@ -130,8 +146,7 @@
        var quoteStr = msg.split('\n').map(
            function(line) { return ' > ' + line; }).join('\n') + '\n\n';
      }
-      var reply =
-          this._newReply(comment.id, comment.line, this.path, quoteStr);
+      var reply = this._newReply(comment.id, comment.line, quoteStr);
      this.push('comments', reply);

      // Allow the reply to render in the dom-repeat.
@@ -144,7 +159,7 @@

    _handleCommentDone: function(e) {
      var comment = e.detail.comment;
-      var reply = this._newReply(comment.id, comment.line, this.path, 'Done');
+      var reply = this._newReply(comment.id, comment.line, 'Done');
      this.push('comments', reply);

      // Allow the reply to render in the dom-repeat.
@@ -155,30 +170,34 @@
      }.bind(this), 1);
    },

-    _commentElWithDraftID: function(draftID) {
-      var commentEls =
-          Polymer.dom(this.root).querySelectorAll('gr-diff-comment');
-      for (var i = 0; i < commentEls.length; i++) {
-        if (commentEls[i].comment.__draftID == draftID) {
-          return commentEls[i];
+    _commentElWithDraftID: function(id) {
+      var els = Polymer.dom(this.root).querySelectorAll('gr-diff-comment');
+      for (var i = 0; i < els.length; i++) {
+        if (els[i].comment.id === id || els[i].comment.__draftID === id) {
+          return els[i];
        }
      }
      return null;
    },

-    _newReply: function(inReplyTo, line, path, opt_message) {
-      var c = {
+    _newReply: function(inReplyTo, lineNum, opt_message) {
+      var d = this._newDraft(lineNum);
+      d.in_reply_to = inReplyTo;
+      if (opt_message != null) {
+        d.message = opt_message;
+      }
+      return d;
+    },
+
+    _newDraft: function(lineNum) {
+      return {
        __draft: true,
        __draftID: Math.random().toString(36),
        __date: new Date(),
-        line: line,
-        path: path,
-        in_reply_to: inReplyTo,
+        line: lineNum,
+        path: this.path,
+        side: this.side,
      };
-      if (opt_message != null) {
-        c.message = opt_message;
-      }
-      return c;
    },

    _handleCommentDiscard: function(e) {
--- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder-side-by-side.js
+++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder-side-by-side.js
@@ -51,7 +51,7 @@
    } else {
      var textEl = this._createTextEl(line);
      textEl.classList.add(side);
-      var threadEl = this._createCommentThread(line, side);
+      var threadEl = this._commentThreadForLine(line, side);
      if (threadEl) {
        textEl.appendChild(threadEl);
      }
--- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder-unified.js
+++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder-unified.js
@@ -42,7 +42,7 @@
      row.appendChild(action);
    } else {
      var textEl = this._createTextEl(line);
-      var threadEl = this._createCommentThread(line);
+      var threadEl = this._commentThreadForLine(line);
      if (threadEl) {
        textEl.appendChild(threadEl);
      }
--- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder.js
@@ -107,7 +107,7 @@
    for (var side in comments) {
      if (side !== GrDiffBuilder.Side.LEFT &&
          side !== GrDiffBuilder.Side.RIGHT) {
-        throw Error('Invalid side: ' + side);
+        continue;
      }
      comments[side].forEach(function(c) {
        result[side][c.line] = true;
@@ -260,23 +260,52 @@
    return result;
  };

-  GrDiffBuilder.prototype._createCommentThread = function(line, opt_side) {
+  GrDiffBuilder.prototype.createCommentThread = function(changeNum, patchNum,
+      path, side, projectConfig) {
+    var threadEl = document.createElement('gr-diff-comment-thread');
+    threadEl.changeNum = changeNum;
+    threadEl.patchNum = patchNum;
+    threadEl.path = path;
+    threadEl.side = side;
+    threadEl.projectConfig = projectConfig;
+    return threadEl;
+  },
+
+  GrDiffBuilder.prototype._commentThreadForLine = function(line, opt_side) {
    var comments = this._getCommentsForLine(this._comments, line, opt_side);
    if (!comments || comments.length === 0) {
      return null;
    }
-    var threadEl = document.createElement('gr-diff-comment-thread');
+
+    var patchNum = this._comments.meta.patchRange.patchNum;
+    var side = 'REVISION';
+    if (line.type === GrDiffLine.Type.REMOVE ||
+        opt_side === GrDiffBuilder.Side.LEFT) {
+      if (this._comments.meta.patchRange.basePatchNum === 'PARENT') {
+        side = 'PARENT';
+      } else {
+        patchNum = this._comments.meta.patchRange.basePatchNum;
+      }
+    }
+    var threadEl = this.createCommentThread(
+        this._comments.meta.changeNum,
+        patchNum,
+        this._comments.meta.path,
+        side,
+        this._comments.meta.projectConfig);
    threadEl.comments = comments;
    return threadEl;
  };

  GrDiffBuilder.prototype._createLineEl = function(line, number, type) {
-    var td = this._createElement('td', 'lineNum');
+    var td = this._createElement('td');
    if (line.type === GrDiffLine.Type.BLANK) {
      return td;
    } else if (line.type === GrDiffLine.Type.CONTEXT_CONTROL) {
+      td.classList.add('contextLineNum');
      td.setAttribute('data-value', '@@');
    } else if (line.type === GrDiffLine.Type.BOTH || line.type == type) {
+      td.classList.add('lineNum');
      td.setAttribute('data-value', number);
    }
    return td;
--- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder_test.html
@@ -284,6 +284,77 @@ limitations under the License.
          GrDiffBuilder.Side.RIGHT), [{id: 'r5', line: 5}]);
    });

+    test('comment thread creation', function() {
+      builder._comments = {
+        meta: {
+          changeNum: '42',
+          patchRange: {
+            basePatchNum: 'PARENT',
+            patchNum: '3',
+          },
+          path: '/path/to/foo',
+          projectConfig: {foo: 'bar'},
+        },
+        left: [
+          {id: 'l3', line: 3},
+          {id: 'l5', line: 5},
+        ],
+        right: [
+          {id: 'r5', line: 5},
+        ],
+      };
+
+      function checkThreadProps(patchNum, side, comments) {
+        assert.equal(threadEl.changeNum, '42');
+        assert.equal(threadEl.patchNum, patchNum);
+        assert.equal(threadEl.path, '/path/to/foo');
+        assert.equal(threadEl.side, side);
+        assert.deepEqual(threadEl.projectConfig, {foo: 'bar'});
+        assert.deepEqual(threadEl.comments, comments);
+      }
+
+      var line = new GrDiffLine(GrDiffLine.Type.BOTH);
+      line.beforeNumber = 5;
+      line.afterNumber = 5;
+      threadEl = builder._commentThreadForLine(line);
+      checkThreadProps('3', 'REVISION',
+          [{id: 'l5', line: 5}, {id: 'r5', line: 5}]);
+
+      threadEl = builder._commentThreadForLine(line, GrDiffBuilder.Side.RIGHT);
+      checkThreadProps('3', 'REVISION', [{id: 'r5', line: 5}]);
+
+      threadEl = builder._commentThreadForLine(line, GrDiffBuilder.Side.LEFT);
+      checkThreadProps('3', 'PARENT', [{id: 'l5', line: 5}]);
+
+      builder._comments.meta.patchRange.basePatchNum = '1';
+
+      threadEl = builder._commentThreadForLine(line);
+      checkThreadProps('3', 'REVISION',
+          [{id: 'l5', line: 5}, {id: 'r5', line: 5}]);
+
+      threadEl = builder._commentThreadForLine(line, GrDiffBuilder.Side.LEFT);
+      checkThreadProps('1', 'REVISION', [{id: 'l5', line: 5}]);
+
+      threadEl = builder._commentThreadForLine(line, GrDiffBuilder.Side.RIGHT);
+      checkThreadProps('3', 'REVISION', [{id: 'r5', line: 5}]);
+
+      builder._comments.meta.patchRange.basePatchNum = 'PARENT';
+
+      line = new GrDiffLine(GrDiffLine.Type.REMOVE);
+      line.beforeNumber = 5;
+      line.afterNumber = 5;
+      threadEl = builder._commentThreadForLine(line);
+      checkThreadProps('3', 'PARENT',
+          [{id: 'l5', line: 5}, {id: 'r5', line: 5}]);
+
+      line = new GrDiffLine(GrDiffLine.Type.ADD);
+      line.beforeNumber = 3;
+      line.afterNumber = 5;
+      threadEl = builder._commentThreadForLine(line);
+      checkThreadProps('3', 'REVISION',
+          [{id: 'l3', line: 3}, {id: 'r5', line: 5}]);
+    });
+
    test('break up common diff chunks', function() {
      builder._commentLocations = {
        left: {1: true},
--- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.html
@@ -57,25 +57,34 @@ limitations under the License.
        border-collapse: collapse;
        border-right: 1px solid #ddd;
      }
+      .section {
+        background-color: #eee;
+      }
+      .blank,
+      .content {
+        background-color: #fff;
+      }
      .lineNum,
      .content {
        vertical-align: top;
        white-space: pre;
      }
-      .lineNum {
-        background-color: #eee;
+      .contextLineNum:before,
+      .lineNum:before {
+        display: inline-block;
        color: #666;
+        content: attr(data-value);
        padding: 0 .75em;
        text-align: right;
-      }
-      .lineNum:before {
-        content: attr(data-value);
+        width: 100%;
      }
      .canComment .lineNum[data-value] {
        cursor: pointer;
+      }
+      .canComment .lineNum[data-value]:before {
        text-decoration: underline;
      }
-      .canComment .lineNum[data-value]:hover {
+      .canComment .lineNum[data-value]:hover:before {
        background-color: #ccc;
      }
      .content {
@@ -94,18 +103,18 @@ limitations under the License.
        -ms-user-select: var(--right-user-select, text);
        user-select: var(--right-user-select, text);
      }
-      .add {
+      .content.add {
        background-color: var(--dark-add-highlight-color);
      }
-      .remove {
+      .content.remove {
        background-color: var(--dark-remove-highlight-color);
      }
-      .contextControl,
-      .contextControl .lineNum {
+      .contextControl {
        color: #849;
        background-color: #fef;
      }
      .contextControl gr-button {
+        display: block;
        font-family: var(--monospace-font-family);
        text-decoration: none;
      }
@@ -146,7 +155,7 @@ limitations under the License.
            on-cancel="_handlePrefsCancel"></gr-diff-preferences>
      </gr-overlay>

-      <div class$="[[_computeContainerClass(_loggedIn)]]"
+      <div class$="[[_computeContainerClass(_loggedIn, _viewMode)]]"
          on-tap="_handleTap"
          on-mousedown="_handleMouseDown"
          on-copy="_handleCopy">
--- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.js
@@ -19,7 +19,7 @@
    UNIFIED: 'UNIFIED_DIFF',
  };

-  var SelectionSide = {
+  var DiffSide = {
    LEFT: 'left',
    RIGHT: 'right',
  };
@@ -36,7 +36,10 @@
        type: Object,
        notify: true,
      },
-      projectConfig: Object,
+      projectConfig: {
+        type: Object,
+        observer: '_projectConfigChanged',
+      },

      _loggedIn: {
        type: Boolean,
@@ -70,7 +73,7 @@
    },

    reload: function() {
-      this.$.diffTable.innerHTML = null;
+      this._clearDiffContent();
      this._loading = true;

      var promises = [];
@@ -87,8 +90,18 @@
      return Promise.all(promises);
    },

-    _computeContainerClass: function(loggedIn) {
+    _computeContainerClass: function(loggedIn, viewMode) {
      var classes = ['diffContainer'];
+      switch (viewMode) {
+        case DiffViewMode.UNIFIED:
+          classes.push('unified');
+          break;
+        case DiffViewMode.SIDE_BY_SIDE:
+          classes.push('sideBySide');
+          break
+        default:
+          throw Error('Invalid view mode: ', viewMode);
+      }
      if (loggedIn) {
        classes.push('canComment');
      }
@@ -97,20 +110,76 @@

    _handleTap: function(e) {
      var el = Polymer.dom(e).rootTarget;
+
      if (el.classList.contains('showContext')) {
        this._showContext(e.detail.group, e.detail.section);
+      } else if (el.classList.contains('lineNum')) {
+        this._handleLineTap(el);
      }
    },

+    _handleLineTap: function(el) {
+      this._getLoggedIn().then(function(loggedIn) {
+        if (!loggedIn) { return; }
+
+        var value = el.getAttribute('data-value');
+        var lineNum = parseInt(value, 10);
+        if (isNaN(lineNum)) {
+          throw Error('Invalid line number: ' + value);
+        }
+        this._addDraft(el, lineNum);
+      }.bind(this));
+    },
+
+    _addDraft: function(lineEl, lineNum) {
+      var threadEl;
+
+      // Does a thread already exist at this line?
+      var contentEl = lineEl.nextSibling;
+      while (contentEl && !contentEl.classList.contains('content')) {
+        contentEl = contentEl.nextSibling;
+      }
+      if (contentEl.childNodes.length > 0 &&
+          contentEl.lastChild.nodeName === 'GR-DIFF-COMMENT-THREAD') {
+        threadEl = contentEl.lastChild;
+      } else {
+        var patchNum = this.patchRange.patchNum;
+        var side = 'REVISION';
+        if (contentEl.classList.contains(DiffSide.LEFT) ||
+            contentEl.classList.contains('remove')) {
+          if (this.patchRange.basePatchNum === 'PARENT') {
+            side = 'PARENT';
+          } else {
+            patchNum = this.patchRange.basePatchNum;
+          }
+        }
+        threadEl = this._builder.createCommentThread(this.changeNum, patchNum,
+            this.path, side, this.projectConfig);
+        // TODO(andybons): Remove once migration is made to gr-new-diff.
+        threadEl.addEventListener('discard',
+            this._handleThreadDiscard.bind(this));
+        contentEl.appendChild(threadEl);
+      }
+      threadEl.addDraft(lineNum);
+    },
+
+    _handleThreadDiscard: function(e) {
+      e.stopPropagation();
+      var el = Polymer.dom(e).rootTarget;
+      el.parentNode.removeChild(el);
+    },
+
    _handleMouseDown: function(e) {
      var el = Polymer.dom(e).rootTarget;
      var side;
      for (var node = el; node != null; node = node.parentNode) {
-        if (node.classList.contains('left')) {
-          side = SelectionSide.LEFT;
+        if (!node.classList) { continue; }
+
+        if (node.classList.contains(DiffSide.LEFT)) {
+          side = DiffSide.LEFT;
          break;
-        } else if (node.classList.contains('right')) {
-          side = SelectionSide.RIGHT;
+        } else if (node.classList.contains(DiffSide.RIGHT)) {
+          side = DiffSide.RIGHT;
          break;
        }
      }
@@ -119,8 +188,8 @@

    _selectionSideChanged: function(side) {
      if (side) {
-        var oppositeSide = side ==
-            SelectionSide.RIGHT ? SelectionSide.LEFT : SelectionSide.RIGHT;
+        var oppositeSide = side === DiffSide.RIGHT ?
+            DiffSide.LEFT : DiffSide.RIGHT;
        this.customStyle['--' + side + '-user-select'] = 'text';
        this.customStyle['--' + oppositeSide + '-user-select'] = 'none';
      } else {
@@ -163,6 +232,7 @@
    },

    _render: function(diff, comments, prefsChangeRecord) {
+      this._clearDiffContent();
      var prefs = prefsChangeRecord.base;
      this.customStyle['--content-width'] = prefs.line_length + 'ch';
      this.updateStyles();
@@ -170,6 +240,10 @@
      this._builder.emitDiff(diff.content);
    },

+    _clearDiffContent: function() {
+      this.$.diffTable.innerHTML = null;
+    },
+
    _getDiff: function() {
      return this.$.restAPI.getDiff(
          this.changeNum,
@@ -208,7 +282,7 @@
          comments: results[0],
          drafts: results[1],
        });
-      }).then(this._normalizeDiffCommentsAndDrafts);
+      }).then(this._normalizeDiffCommentsAndDrafts.bind(this));
    },

    _normalizeDiffCommentsAndDrafts: function(results) {
@@ -219,6 +293,12 @@
      var baseDrafts = results.drafts.baseComments.map(markAsDraft);
      var drafts = results.drafts.comments.map(markAsDraft);
      return Promise.resolve({
+        meta: {
+          path: this.path,
+          changeNum: this.changeNum,
+          patchRange: this.patchRange,
+          projectConfig: this.projectConfig,
+        },
        left: results.comments.baseComments.concat(baseDrafts),
        right: results.comments.comments.concat(drafts),
      });
@@ -239,5 +319,13 @@
      throw Error('Unsupported diff view mode: ' + this._viewMode);
    },

+    _projectConfigChanged: function(projectConfig) {
+      var threadEls =
+          Polymer.dom(this.root).querySelectorAll('gr-diff-comment-thread');
+      for (var i = 0; i < threadEls.length; i++) {
+        threadEls[i].projectConfig = projectConfig;
+      }
+    },
+
  });
 })();
--- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff_test.html
@@ -106,8 +106,25 @@ limitations under the License.
      var diffDraftsStub = sinon.stub(element, '_getDiffDrafts',
          function() { return Promise.resolve(drafts); });

+      element.changeNum = '42';
+      element.patchRange = {
+        basePatchNum: 'PARENT',
+        patchNum: 3,
+      };
+      element.path = '/path/to/foo';
+      element.projectConfig = {foo: 'bar'};
+
      element._getDiffCommentsAndDrafts().then(function(result) {
        assert.deepEqual(result, {
+          meta: {
+            changeNum: '42',
+            patchRange: {
+              basePatchNum: 'PARENT',
+              patchNum: 3,
+            },
+            path: '/path/to/foo',
+            projectConfig: {foo: 'bar'},
+          },
          left: [
            {id: 'bc1'},
            {id: 'bc2'},