Move triggering thread creation to gr-diff-host This is a step towards allowing code using gr-diff pick their own comment widget. The threads are still created by gr-diff-comment-thread-group in this commit - only the triggering is moved. But that will change in a future CL. Change-Id: If42b07df11422b69e7e1421c15e923d208012b47
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js index cee3cad..4fe4ab08 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
@@ -35,7 +35,7 @@ listeners: { 'comment-mouse-out': '_handleCommentMouseOut', 'comment-mouse-over': '_handleCommentMouseOver', - 'create-comment': '_createComment', + 'create-range-comment': '_createRangeComment', }, observers: [ @@ -311,7 +311,7 @@ } }, - _createComment(e) { + _createRangeComment(e) { this._removeActionBox(); },
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html index 7b19338..8c758fe 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
@@ -215,9 +215,9 @@ assert.isFalse(builder.getContentsByLineRange.called); }); - test('on create-comment action box is removed', () => { + test('on create-range-comment action box is removed', () => { sandbox.stub(element, '_removeActionBox'); - element.fire('create-comment', { + element.fire('create-range-comment', { comment: { range: {}, },
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 6f61fb9..19fb308 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
@@ -177,7 +177,7 @@ }, listeners: { - 'draft-interaction': '_handleDraftInteraction', + 'create-comment': '_handleCreateComment', }, observers: [ @@ -445,11 +445,35 @@ this.patchRange); }, - _handleDraftInteraction() { + /** @param {CustomEvent} e */ + _handleCreateComment(e) { + const {threadGroupEl, lineNum, side, range} = e.detail; + const threadEl = this._getOrCreateThread(threadGroupEl, side, range); + threadEl.addOrEditDraft(lineNum, range); this.$.reporting.recordDraftInteraction(); }, /** + * Gets or creates a comment thread from a specific thread group. + * May include a range, if the comment is a range comment. + * + * @param {!Object} threadGroupEl + * @param {string} commentSide + * @param {!Object=} range + * @return {!Object} + */ + _getOrCreateThread(threadGroupEl, commentSide, range=undefined) { + let threadEl = threadGroupEl.getThread(commentSide, range); + + if (!threadEl) { + threadGroupEl.addNewThread(commentSide, range); + Polymer.dom.flush(); + threadEl = threadGroupEl.getThread(commentSide, range); + } + return threadEl; + }, + + /** * Take a diff that was loaded with a ignore-whitespace other than * IGNORE_NONE, and convert delta chunks labeled as common into shared * chunks.
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 f83253e..3374686 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
@@ -776,6 +776,29 @@ }); }); + test('_getOrCreateThread', () => { + const threadGroupEl = + document.createElement('gr-diff-comment-thread-group'); + const commentSide = 'left'; + + assert.isOk(element._getOrCreateThread(threadGroupEl, + commentSide)); + + // Try to fetch a thread with a different range. + range = { + startLine: 1, + startChar: 1, + endLine: 1, + endChar: 3, + }; + + assert.isOk(element._getOrCreateThread( + threadGroupEl, commentSide, range)); + const threadCount = Polymer.dom(threadGroupEl.root). + querySelectorAll('gr-diff-comment-thread').length; + assert.equal(threadCount, 2); + }); + suite('_translateChunksToIgnore', () => { let content;
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 003e024..69ac283 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -60,9 +60,9 @@ */ /** - * Fired when a draft is added or edited. + * Fired when a comment is created * - * @event draft-interaction + * @event create-comment */ properties: { @@ -207,7 +207,7 @@ 'comment-discard': '_handleCommentDiscard', 'comment-update': '_handleCommentUpdate', 'comment-save': '_handleCommentSave', - 'create-comment': '_handleCreateComment', + 'create-range-comment': '_handleCreateRangeComment', }, /** Cancel any remaining diff builder rendering work. */ @@ -322,7 +322,7 @@ this._createComment(el, lineNum); }, - _handleCreateComment(e) { + _handleCreateRangeComment(e) { const range = e.detail.range; const side = e.detail.side; const lineNum = range.endLine; @@ -359,23 +359,29 @@ /** * @param {!Object} lineEl - * @param {number=} opt_lineNum - * @param {string=} opt_side - * @param {!Object=} opt_range + * @param {number=} lineNum + * @param {string=} side + * @param {!Object=} range */ - _createComment(lineEl, opt_lineNum, opt_side, opt_range) { - this.dispatchEvent(new CustomEvent('draft-interaction', {bubbles: true})); + _createComment(lineEl, lineNum=undefined, side=undefined, range=undefined) { const contentText = this.$.diffBuilder.getContentByLineEl(lineEl); const contentEl = contentText.parentElement; - const side = opt_side || + side = side || this._getCommentSideByLineAndContent(lineEl, contentEl); const patchNum = this._getPatchNumByLineAndContent(lineEl, contentEl); const isOnParent = this._getIsParentCommentByLineAndContent(lineEl, contentEl); const threadGroupEl = this._getOrCreateThreadGroup(contentEl, patchNum, side, isOnParent); - const threadEl = this._getOrCreateThread(threadGroupEl, side, opt_range); - threadEl.addOrEditDraft(opt_lineNum, opt_range); + this.dispatchEvent(new CustomEvent('create-comment', { + bubbles: true, + detail: { + threadGroupEl, + lineNum, + side, + range, + }, + })); }, _getThreadGroupForLine(contentEl) { @@ -403,26 +409,6 @@ }, /** - * Gets or creates a comment thread from a specific thread group. - * May include a range, if the comment is a range comment. - * - * @param {!Object} threadGroupEl - * @param {string} commentSide - * @param {!Object=} range - * @return {!Object} - */ - _getOrCreateThread(threadGroupEl, commentSide, range=undefined) { - let threadEl = threadGroupEl.getThread(commentSide, range); - - if (!threadEl) { - threadGroupEl.addNewThread(commentSide, range); - Polymer.dom.flush(); - threadEl = threadGroupEl.getThread(commentSide, range); - } - return threadEl; - }, - - /** * @param {number} patchNum * @param {boolean} isOnParent * @param {!string} commentSide
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 aa28713..07584c7 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
@@ -298,12 +298,6 @@ const commentSide = 'left'; const patchNum = 1; const side = 'PARENT'; - let range = { - startLine: 1, - startChar: 1, - endLine: 1, - endChar: 2, - }; element.changeNum = 123; element.patchRange = {basePatchNum: 1, patchNum: 2}; @@ -322,30 +316,11 @@ patchNum, commentSide, side); assert.isOk(threadGroupEl); - assert.isOk(element._getOrCreateThread(threadGroupEl, - commentSide, side)); - - // Try to fetch a thread with a different range. - range = { - startLine: 1, - startChar: 1, - endLine: 1, - endChar: 3, - }; - - assert.isOk(element._getOrCreateThread( - threadGroupEl, commentSide, range)); // The new thread group can be fetched. assert.isOk(element._getThreadGroupForLine(contentEl)); assert.equal(contentEl.querySelectorAll( 'gr-diff-comment-thread-group').length, 1); - - const threadGroup = contentEl.querySelector( - 'gr-diff-comment-thread-group'); - const threadLength = Polymer.dom(threadGroup.root). - querySelectorAll('gr-diff-comment-thread').length; - assert.equal(threadLength, 2); }); suite('image diffs', () => {
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js index 6349ab6..0f84877 100644 --- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js +++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js
@@ -23,7 +23,7 @@ /** * Fired when the comment creation action was taken (hotkey, click). * - * @event create-comment + * @event create-range-comment */ properties: { @@ -110,7 +110,7 @@ }, _fireCreateComment() { - this.fire('create-comment', {side: this.side, range: this.range}); + this.fire('create-range-comment', {side: this.side, range: this.range}); }, }); })();
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.html b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.html index 19155e4..4f1065a 100644 --- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.html +++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.html
@@ -98,7 +98,7 @@ element.range = range; MockInteractions.pressAndReleaseKeyOn(document.body, 67, null, 'c'); assert(element.fire.calledWithExactly( - 'create-comment', + 'create-range-comment', { side, range,