diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html index d335e7a833..3eb7b37c66 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html +++ b/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]]" diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js index 8a2f2cdb18..5839141597 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js +++ b/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); + }, }); })(); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html index 88d7ab6828..53442560dd 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html +++ b/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; diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js index 12aa628bf0..a8cf320b89 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js +++ b/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} */ _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 diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html index 0577d5ab28..99f2ded955 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html +++ b/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; });