Merge "Move comment change handling to gr-diff-host"

polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html

@@ -37,7 +37,6 @@ limitations under the License.
         commit-range="[[commitRange]]"
         hidden$="[[hidden]]"
         no-render-on-prefs-change="[[noRenderOnPrefsChange]]"
-        comments="[[comments]]"
         line-wrapping="[[lineWrapping]]"
         view-mode="[[viewMode]]"
         line-of-interest="[[lineOfInterest]]"

polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js

@@ -208,7 +208,16 @@
     ],
 
     listeners: {
+      // These are named inconsistently for a reason:
+      // The create-comment event is fired to indicate that we should
+      // create a comment.
+      // The comment-* events are just notifying that the comments did already
+      // change in some way, and that we should update any models we may want
+      // to keep in sync.
       'create-comment': '_handleCreateComment',
+      'comment-discard': '_handleCommentDiscard',
+      'comment-update': '_handleCommentUpdate',
+      'comment-save': '_handleCommentSave',
     },
 
     observers: [
@@ -723,5 +732,76 @@
           this.getParentIndex(patchRangeRecord.base.basePatchNum) : null;
     },
 
+    _handleCommentSave(e) {
+      const comment = e.detail.comment;
+      const side = e.detail.comment.__commentSide;
+      const idx = this._findDraftIndex(comment, side);
+      this.set(['comments', side, idx], comment);
+      this._handleCommentSaveOrDiscard();
+    },
+
+    _handleCommentDiscard(e) {
+      const comment = e.detail.comment;
+      this._removeComment(comment);
+      this._handleCommentSaveOrDiscard();
+    },
+
+    /**
+     * Closure annotation for Polymer.prototype.push is off. Submitted PR:
+     * https://github.com/Polymer/polymer/pull/4776
+     * but for not supressing annotations.
+     *
+     * @suppress {checkTypes}
+     */
+    _handleCommentUpdate(e) {
+      const comment = e.detail.comment;
+      const side = e.detail.comment.__commentSide;
+      let idx = this._findCommentIndex(comment, side);
+      if (idx === -1) {
+        idx = this._findDraftIndex(comment, side);
+      }
+      if (idx !== -1) { // Update draft or comment.
+        this.set(['comments', side, idx], comment);
+      } else { // Create new draft.
+        this.push(['comments', side], comment);
+      }
+    },
+
+    _handleCommentSaveOrDiscard() {
+      this.dispatchEvent(new CustomEvent('diff-comments-modified',
+          {bubbles: true}));
+    },
+
+    _removeComment(comment) {
+      const side = comment.__commentSide;
+      this._removeCommentFromSide(comment, side);
+    },
+
+    _removeCommentFromSide(comment, side) {
+      let idx = this._findCommentIndex(comment, side);
+      if (idx === -1) {
+        idx = this._findDraftIndex(comment, side);
+      }
+      if (idx !== -1) {
+        this.splice('comments.' + side, idx, 1);
+      }
+    },
+
+    /** @return {number} */
+    _findCommentIndex(comment, side) {
+      if (!comment.id || !this.comments[side]) {
+        return -1;
+      }
+      return this.comments[side].findIndex(item => item.id === comment.id);
+    },
+
+    /** @return {number} */
+    _findDraftIndex(comment, side) {
+      if (!comment.__draftID || !this.comments[side]) {
+        return -1;
+      }
+      return this.comments[side].findIndex(
+          item => item.__draftID === comment.__draftID);
+    },
   });
 })();

polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html

@@ -52,6 +52,189 @@ limitations under the License.
       sandbox.restore();
     });
 
+    suite('handle comment-update', () => {
+      setup(() => {
+        sandbox.stub(element, '_commentsChanged');
+        element.comments = {
+          meta: {
+            changeNum: '42',
+            patchRange: {
+              basePatchNum: 'PARENT',
+              patchNum: 3,
+            },
+            path: '/path/to/foo',
+            projectConfig: {foo: 'bar'},
+          },
+          left: [
+            {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
+            {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
+            {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
+            {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
+          ],
+          right: [
+            {id: 'c1', __commentSide: 'right'},
+            {id: 'c2', __commentSide: 'right'},
+            {id: 'd1', __draft: true, __commentSide: 'right'},
+            {id: 'd2', __draft: true, __commentSide: 'right'},
+          ],
+        };
+      });
+
+      test('creating a draft', () => {
+        const comment = {__draft: true, __draftID: 'tempID', side: 'PARENT',
+          __commentSide: 'left'};
+        element.fire('comment-update', {comment});
+        assert.include(element.comments.left, comment);
+      });
+
+      test('discarding a draft', () => {
+        const draftID = 'tempID';
+        const id = 'savedID';
+        const comment = {
+          __draft: true,
+          __draftID: draftID,
+          side: 'PARENT',
+          __commentSide: 'left',
+        };
+        const diffCommentsModifiedStub = sandbox.stub();
+        element.addEventListener('diff-comments-modified',
+            diffCommentsModifiedStub);
+        element.comments.left.push(comment);
+        comment.id = id;
+        element.fire('comment-discard', {comment});
+        const drafts = element.comments.left.filter(item => {
+          return item.__draftID === draftID;
+        });
+        assert.equal(drafts.length, 0);
+        assert.isTrue(diffCommentsModifiedStub.called);
+      });
+
+      test('saving a draft', () => {
+        const draftID = 'tempID';
+        const id = 'savedID';
+        const comment = {
+          __draft: true,
+          __draftID: draftID,
+          side: 'PARENT',
+          __commentSide: 'left',
+        };
+        const diffCommentsModifiedStub = sandbox.stub();
+        element.addEventListener('diff-comments-modified',
+            diffCommentsModifiedStub);
+        element.comments.left.push(comment);
+        comment.id = id;
+        element.fire('comment-save', {comment});
+        const drafts = element.comments.left.filter(item => {
+          return item.__draftID === draftID;
+        });
+        assert.equal(drafts.length, 1);
+        assert.equal(drafts[0].id, id);
+        assert.isTrue(diffCommentsModifiedStub.called);
+      });
+    });
+
+    test('remove comment', () => {
+      sandbox.stub(element, '_commentsChanged');
+      element.comments = {
+        meta: {
+          changeNum: '42',
+          patchRange: {
+            basePatchNum: 'PARENT',
+            patchNum: 3,
+          },
+          path: '/path/to/foo',
+          projectConfig: {foo: 'bar'},
+        },
+        left: [
+          {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
+          {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
+          {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
+          {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
+        ],
+        right: [
+          {id: 'c1', __commentSide: 'right'},
+          {id: 'c2', __commentSide: 'right'},
+          {id: 'd1', __draft: true, __commentSide: 'right'},
+          {id: 'd2', __draft: true, __commentSide: 'right'},
+        ],
+      };
+
+      element._removeComment({});
+      // Using JSON.stringify because Safari 9.1 (11601.5.17.1) doesn’t seem
+      // to believe that one object deepEquals another even when they do :-/.
+      assert.equal(JSON.stringify(element.comments), JSON.stringify({
+        meta: {
+          changeNum: '42',
+          patchRange: {
+            basePatchNum: 'PARENT',
+            patchNum: 3,
+          },
+          path: '/path/to/foo',
+          projectConfig: {foo: 'bar'},
+        },
+        left: [
+          {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
+          {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
+          {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
+          {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
+        ],
+        right: [
+          {id: 'c1', __commentSide: 'right'},
+          {id: 'c2', __commentSide: 'right'},
+          {id: 'd1', __draft: true, __commentSide: 'right'},
+          {id: 'd2', __draft: true, __commentSide: 'right'},
+        ],
+      }));
+
+      element._removeComment({id: 'bc2', side: 'PARENT',
+        __commentSide: 'left'});
+      assert.deepEqual(element.comments, {
+        meta: {
+          changeNum: '42',
+          patchRange: {
+            basePatchNum: 'PARENT',
+            patchNum: 3,
+          },
+          path: '/path/to/foo',
+          projectConfig: {foo: 'bar'},
+        },
+        left: [
+          {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
+          {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
+          {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
+        ],
+        right: [
+          {id: 'c1', __commentSide: 'right'},
+          {id: 'c2', __commentSide: 'right'},
+          {id: 'd1', __draft: true, __commentSide: 'right'},
+          {id: 'd2', __draft: true, __commentSide: 'right'},
+        ],
+      });
+
+      element._removeComment({id: 'd2', __commentSide: 'right'});
+      assert.deepEqual(element.comments, {
+        meta: {
+          changeNum: '42',
+          patchRange: {
+            basePatchNum: 'PARENT',
+            patchNum: 3,
+          },
+          path: '/path/to/foo',
+          projectConfig: {foo: 'bar'},
+        },
+        left: [
+          {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
+          {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
+          {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
+        ],
+        right: [
+          {id: 'c1', __commentSide: 'right'},
+          {id: 'c2', __commentSide: 'right'},
+          {id: 'd1', __draft: true, __commentSide: 'right'},
+        ],
+      });
+    });
+
     test('thread-discard handling', () => {
       const threads = [
         {comments: [{id: 4711}]},
@@ -682,12 +865,6 @@ limitations under the License.
       assert.equal(element.$.diff.noRenderOnPrefsChange, value);
     });
 
-    test('passes in comments', () => {
-      const value = {left: [], right: []};
-      element.comments = value;
-      assert.equal(element.$.diff.comments, value);
-    });
-
     test('passes in lineWrapping', () => {
       const value = true;
       element.lineWrapping = value;

polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js

@@ -74,12 +74,6 @@
      * @event show-auth-required
      */
 
-    /**
-     * Fired when a comment is saved or discarded
-     *
-     * @event diff-comments-modified
-     */
-
      /**
       * Fired when a comment is created
       *
@@ -113,10 +107,6 @@
         reflectToAttribute: true,
       },
       noRenderOnPrefsChange: Boolean,
-      comments: {
-        type: Object,
-        value: {left: [], right: []},
-      },
       /** @type {!Array<!Gerrit.HoveredRange>} */
       _commentRanges: {
         type: Array,
@@ -237,9 +227,6 @@
     ],
 
     listeners: {
-      'comment-discard': '_handleCommentDiscard',
-      'comment-update': '_handleCommentUpdate',
-      'comment-save': '_handleCommentSave',
       'create-range-comment': '_handleCreateRangeComment',
       'render-content': '_handleRenderContent',
     },
@@ -256,7 +243,7 @@
     _observeNodes() {
       this._nodeObserver = Polymer.dom(this).observeNodes(info => {
         const addedThreadEls = info.addedNodes.filter(isThreadEl);
-        // In principal we should also handle removed nodes, but I have not
+        // In principle we should also handle removed nodes, but I have not
         // figured out how to do that yet without also catching all the removals
         // caused by further redistribution. Right now, comments are never
         // removed by no longer slotting them in, so I decided to not handle
@@ -319,11 +306,6 @@
       }
     },
 
-    _handleCommentSaveOrDiscard() {
-      this.dispatchEvent(new CustomEvent('diff-comments-modified',
-          {bubbles: true}));
-    },
-
     /** @return {string} */
     _computeContainerClass(loggedIn, viewMode, displayLine) {
       const classes = ['diffContainer'];
@@ -518,75 +500,6 @@
       return side;
     },
 
-    _handleCommentDiscard(e) {
-      const comment = e.detail.comment;
-      this._removeComment(comment);
-      this._handleCommentSaveOrDiscard();
-    },
-
-    _removeComment(comment) {
-      const side = comment.__commentSide;
-      this._removeCommentFromSide(comment, side);
-    },
-
-    _handleCommentSave(e) {
-      const comment = e.detail.comment;
-      const side = e.detail.comment.__commentSide;
-      const idx = this._findDraftIndex(comment, side);
-      this.set(['comments', side, idx], comment);
-      this._handleCommentSaveOrDiscard();
-    },
-
-    /**
-     * Closure annotation for Polymer.prototype.push is off. Submitted PR:
-     * https://github.com/Polymer/polymer/pull/4776
-     * but for not supressing annotations.
-     *
-     * @suppress {checkTypes} */
-    _handleCommentUpdate(e) {
-      const comment = e.detail.comment;
-      const side = e.detail.comment.__commentSide;
-      let idx = this._findCommentIndex(comment, side);
-      if (idx === -1) {
-        idx = this._findDraftIndex(comment, side);
-      }
-      if (idx !== -1) { // Update draft or comment.
-        this.set(['comments', side, idx], comment);
-      } else { // Create new draft.
-        this.push(['comments', side], comment);
-      }
-    },
-
-    _removeCommentFromSide(comment, side) {
-      let idx = this._findCommentIndex(comment, side);
-      if (idx === -1) {
-        idx = this._findDraftIndex(comment, side);
-      }
-      if (idx !== -1) {
-        this.splice('comments.' + side, idx, 1);
-      }
-    },
-
-    /** @return {number} */
-    _findCommentIndex(comment, side) {
-      if (!comment.id || !this.comments[side]) {
-        return -1;
-      }
-      return this.comments[side].findIndex(item => {
-        return item.id === comment.id;
-      });
-    },
-
-    /** @return {number} */
-    _findDraftIndex(comment, side) {
-      if (!comment.__draftID || !this.comments[side]) {
-        return -1;
-      }
-      return this.comments[side].findIndex(item => {
-        return item.__draftID === comment.__draftID;
-      });
-    },
-
     _prefsObserver(newPrefs, oldPrefs) {
       // Scan the preference objects one level deep to see if they differ.
       let differ = !oldPrefs;
@@ -646,7 +559,7 @@
 
       this.updateStyles(stylesToUpdate);
 
-      if (this.diff && this.comments && !this.noRenderOnPrefsChange) {
+      if (this.diff && !this.noRenderOnPrefsChange) {
         this._renderDiffTable();
       }
     },
@@ -684,7 +597,7 @@
     _handleRenderContent() {
       this._incrementalNodeObserver = Polymer.dom(this).observeNodes(info => {
         const addedThreadEls = info.addedNodes.filter(isThreadEl);
-        // In principal we should also handle removed nodes, but I have not
+        // In principle we should also handle removed nodes, but I have not
         // figured out how to do that yet without also catching all the removals
         // caused by further redistribution. Right now, comments are never
         // removed by no longer slotting them in, so I decided to not handle

polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html

@@ -192,107 +192,6 @@ limitations under the License.
             element.$$('.diffContainer').classList.contains('displayLine'));
       });
 
-      test('remove comment', () => {
-        element.comments = {
-          meta: {
-            changeNum: '42',
-            patchRange: {
-              basePatchNum: 'PARENT',
-              patchNum: 3,
-            },
-            path: '/path/to/foo',
-            projectConfig: {foo: 'bar'},
-          },
-          left: [
-            {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
-            {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
-            {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
-            {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
-          ],
-          right: [
-            {id: 'c1', __commentSide: 'right'},
-            {id: 'c2', __commentSide: 'right'},
-            {id: 'd1', __draft: true, __commentSide: 'right'},
-            {id: 'd2', __draft: true, __commentSide: 'right'},
-          ],
-        };
-
-        element._removeComment({});
-        // Using JSON.stringify because Safari 9.1 (11601.5.17.1) doesn’t seem
-        // to believe that one object deepEquals another even when they do :-/.
-        assert.equal(JSON.stringify(element.comments), JSON.stringify({
-          meta: {
-            changeNum: '42',
-            patchRange: {
-              basePatchNum: 'PARENT',
-              patchNum: 3,
-            },
-            path: '/path/to/foo',
-            projectConfig: {foo: 'bar'},
-          },
-          left: [
-            {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
-            {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
-            {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
-            {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
-          ],
-          right: [
-            {id: 'c1', __commentSide: 'right'},
-            {id: 'c2', __commentSide: 'right'},
-            {id: 'd1', __draft: true, __commentSide: 'right'},
-            {id: 'd2', __draft: true, __commentSide: 'right'},
-          ],
-        }));
-
-        element._removeComment({id: 'bc2', side: 'PARENT',
-          __commentSide: 'left'});
-        assert.deepEqual(element.comments, {
-          meta: {
-            changeNum: '42',
-            patchRange: {
-              basePatchNum: 'PARENT',
-              patchNum: 3,
-            },
-            path: '/path/to/foo',
-            projectConfig: {foo: 'bar'},
-          },
-          left: [
-            {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
-            {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
-            {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
-          ],
-          right: [
-            {id: 'c1', __commentSide: 'right'},
-            {id: 'c2', __commentSide: 'right'},
-            {id: 'd1', __draft: true, __commentSide: 'right'},
-            {id: 'd2', __draft: true, __commentSide: 'right'},
-          ],
-        });
-
-        element._removeComment({id: 'd2', __commentSide: 'right'});
-        assert.deepEqual(element.comments, {
-          meta: {
-            changeNum: '42',
-            patchRange: {
-              basePatchNum: 'PARENT',
-              patchNum: 3,
-            },
-            path: '/path/to/foo',
-            projectConfig: {foo: 'bar'},
-          },
-          left: [
-            {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
-            {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
-            {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
-          ],
-          right: [
-            {id: 'c1', __commentSide: 'right'},
-            {id: 'c2', __commentSide: 'right'},
-            {id: 'd1', __draft: true, __commentSide: 'right'},
-          ],
-        });
-      });
-
       test('thread groups', () => {
         const contentEl = document.createElement('div');
 
@@ -333,11 +232,6 @@ limitations under the License.
           };
 
           element.patchRange = {basePatchNum: 'PARENT', patchNum: 1};
-          element.comments = {
-            left: [],
-            right: [],
-            meta: {patchRange: undefined},
-          };
           element.isImageDiff = true;
           element.prefs = {
             auto_hide_diff_table_header: true,
@@ -663,11 +557,6 @@ limitations under the License.
         const setupDiff = function() {
           const mock = document.createElement('mock-diff-response');
           element.diff = mock.diffResponse;
-          element.comments = {
-            left: [],
-            right: [],
-            meta: {patchRange: undefined},
-          };
           element.prefs = {
             context: 10,
             tab_size: 8,
@@ -766,29 +655,6 @@ limitations under the License.
             change_type: 'MODIFIED',
             content: [{skip: 66}],
           };
-          element.comments = {
-            meta: {
-              changeNum: '42',
-              patchRange: {
-                basePatchNum: 'PARENT',
-                patchNum: 3,
-              },
-              path: '/path/to/foo',
-              projectConfig: {foo: 'bar'},
-            },
-            left: [
-              {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
-              {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
-              {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
-              {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
-            ],
-            right: [
-              {id: 'c1', __commentSide: 'right'},
-              {id: 'c2', __commentSide: 'right'},
-              {id: 'd1', __draft: true, __commentSide: 'right'},
-              {id: 'd2', __draft: true, __commentSide: 'right'},
-            ],
-          };
         });
 
         test('change in preferences re-renders diff', () => {
@@ -807,86 +673,6 @@ limitations under the License.
           assert.isFalse(element._renderDiffTable.called);
         });
       });
-
-      suite('handle comment-update', () => {
-        setup(() => {
-          element.comments = {
-            meta: {
-              changeNum: '42',
-              patchRange: {
-                basePatchNum: 'PARENT',
-                patchNum: 3,
-              },
-              path: '/path/to/foo',
-              projectConfig: {foo: 'bar'},
-            },
-            left: [
-              {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
-              {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
-              {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
-              {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
-            ],
-            right: [
-              {id: 'c1', __commentSide: 'right'},
-              {id: 'c2', __commentSide: 'right'},
-              {id: 'd1', __draft: true, __commentSide: 'right'},
-              {id: 'd2', __draft: true, __commentSide: 'right'},
-            ],
-          };
-        });
-
-        test('creating a draft', () => {
-          const comment = {__draft: true, __draftID: 'tempID', side: 'PARENT',
-            __commentSide: 'left'};
-          element.fire('comment-update', {comment});
-          assert.include(element.comments.left, comment);
-        });
-
-        test('discarding a draft', () => {
-          const draftID = 'tempID';
-          const id = 'savedID';
-          const comment = {
-            __draft: true,
-            __draftID: draftID,
-            side: 'PARENT',
-            __commentSide: 'left',
-          };
-          const diffCommentsModifiedStub = sandbox.stub();
-          element.addEventListener('diff-comments-modified',
-              diffCommentsModifiedStub);
-          element.comments.left.push(comment);
-          comment.id = id;
-          element.fire('comment-discard', {comment});
-          const drafts = element.comments.left.filter(item => {
-            return item.__draftID === draftID;
-          });
-          assert.equal(drafts.length, 0);
-          assert.isTrue(diffCommentsModifiedStub.called);
-        });
-
-        test('saving a draft', () => {
-          const draftID = 'tempID';
-          const id = 'savedID';
-          const comment = {
-            __draft: true,
-            __draftID: draftID,
-            side: 'PARENT',
-            __commentSide: 'left',
-          };
-          const diffCommentsModifiedStub = sandbox.stub();
-          element.addEventListener('diff-comments-modified',
-              diffCommentsModifiedStub);
-          element.comments.left.push(comment);
-          comment.id = id;
-          element.fire('comment-save', {comment});
-          const drafts = element.comments.left.filter(item => {
-            return item.__draftID === draftID;
-          });
-          assert.equal(drafts.length, 1);
-          assert.equal(drafts[0].id, id);
-          assert.isTrue(diffCommentsModifiedStub.called);
-        });
-      });
     });
 
     suite('diff header', () => {
@@ -946,7 +732,6 @@ limitations under the License.
         const mock = document.createElement('mock-diff-response');
         sandbox.stub(element.$.diffBuilder, 'getDiffLength').returns(10000);
         element.diff = mock.diffResponse;
-        element.comments = {left: [], right: []};
         element.noRenderOnPrefsChange = true;
       });