From 8c8ce2dabacc25cafecb7ad5e727345fb23fb493 Mon Sep 17 00:00:00 2001 From: Ole Rehmsen Date: Fri, 12 Oct 2018 16:20:57 +0200 Subject: [PATCH 1/3] Move the actual comment element creation to host Uses slotting to pass the created comment thread elements to gr-diff. In the initial rendering phase, the thread elements are redistributed to the gr-diff-builder, who filters and attaches them as before. After that, added comments are listened for by gr-diff, and attached to the appropriate thread group. With this change, gr-diff and descendants no longer depend on gr-diff-comment-thread and decendants. However, they still depend on the specifics of the Gerrit comment model (for computing lines of interest and for the ranged comment layer), which is a dependency we will cut next. Change-Id: Ifdb5da86d6bf6160efba9bc59dd347b1c5e55e85 --- .../gr-diff-builder/gr-diff-builder-binary.js | 8 +- .../gr-diff-builder/gr-diff-builder-image.js | 8 +- .../gr-diff-builder-side-by-side.js | 8 +- .../gr-diff-builder-unified.js | 8 +- .../diff/gr-diff-builder/gr-diff-builder.html | 36 ++- .../diff/gr-diff-builder/gr-diff-builder.js | 103 +------ .../gr-diff-builder/gr-diff-builder_test.html | 276 ++++-------------- .../gr-diff-comment-thread-group.html | 6 +- .../gr-diff-comment-thread-group.js | 42 +-- .../gr-diff-comment-thread-group_test.html | 114 +------- .../gr-diff-cursor/gr-diff-cursor_test.html | 6 +- .../diff/gr-diff-host/gr-diff-host.html | 5 +- .../diff/gr-diff-host/gr-diff-host.js | 198 +++++++++++-- .../diff/gr-diff-host/gr-diff-host_test.html | 175 ++++++++++- .../diff/gr-diff-view/gr-diff-view.html | 1 - .../app/elements/diff/gr-diff/gr-diff.html | 4 +- .../app/elements/diff/gr-diff/gr-diff.js | 60 ++-- .../elements/diff/gr-diff/gr-diff_test.html | 7 +- 18 files changed, 512 insertions(+), 553 deletions(-) diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-binary.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-binary.js index 11a36b35e6..a9242be5eb 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-binary.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-binary.js @@ -20,10 +20,10 @@ // Prevent redefinition. if (window.GrDiffBuilderBinary) { return; } - function GrDiffBuilderBinary(diff, comments, parentIndex, changeNum, path, - projectName, prefs, outputEl) { - GrDiffBuilder.call(this, diff, comments, parentIndex, changeNum, path, - projectName, prefs, outputEl); + function GrDiffBuilderBinary(diff, patchRange, commentThreadEls, prefs, + outputEl) { + GrDiffBuilder.call(this, diff, patchRange, commentThreadEls, prefs, + outputEl); } GrDiffBuilderBinary.prototype = Object.create(GrDiffBuilder.prototype); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-image.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-image.js index dce66e12d7..c52a504466 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-image.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-image.js @@ -22,10 +22,10 @@ const IMAGE_MIME_PATTERN = /^image\/(bmp|gif|jpeg|jpg|png|tiff|webp)$/; - function GrDiffBuilderImage(diff, comments, parentIndex, changeNum, path, - projectName, prefs, outputEl, baseImage, revisionImage) { - GrDiffBuilderSideBySide.call(this, diff, comments, parentIndex, changeNum, - path, projectName, prefs, outputEl, []); + function GrDiffBuilderImage(diff, patchRange, commentThreadEls, prefs, + outputEl, baseImage, revisionImage) { + GrDiffBuilderSideBySide.call(this, diff, patchRange, commentThreadEls, + prefs, outputEl, []); this._baseImage = baseImage; this._revisionImage = revisionImage; } diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js index 90688fc5d0..da085c239a 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js @@ -20,10 +20,10 @@ // Prevent redefinition. if (window.GrDiffBuilderSideBySide) { return; } - function GrDiffBuilderSideBySide(diff, comments, parentIndex, changeNum, path, - projectName, prefs, outputEl, layers) { - GrDiffBuilder.call(this, diff, comments, parentIndex, changeNum, path, - projectName, prefs, outputEl, layers); + function GrDiffBuilderSideBySide(diff, patchRange, commentThreadEls, + prefs, outputEl, layers) { + GrDiffBuilder.call(this, diff, patchRange, commentThreadEls, prefs, + outputEl, layers); } GrDiffBuilderSideBySide.prototype = Object.create(GrDiffBuilder.prototype); GrDiffBuilderSideBySide.prototype.constructor = GrDiffBuilderSideBySide; diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js index 1c9c1b2d4e..0657ee4565 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js @@ -20,10 +20,10 @@ // Prevent redefinition. if (window.GrDiffBuilderUnified) { return; } - function GrDiffBuilderUnified(diff, comments, parentIndex, changeNum, path, - projectName, prefs, outputEl, layers) { - GrDiffBuilder.call(this, diff, comments, parentIndex, changeNum, path, - projectName, prefs, outputEl, layers); + function GrDiffBuilderUnified(diff, patchRange, commentThreadEls, prefs, + outputEl, layers) { + GrDiffBuilder.call(this, diff, patchRange, commentThreadEls, prefs, + outputEl, layers); } GrDiffBuilderUnified.prototype = Object.create(GrDiffBuilder.prototype); GrDiffBuilderUnified.prototype.constructor = GrDiffBuilderUnified; diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html index 91cd0cf94a..33e2a19115 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html @@ -16,9 +16,9 @@ limitations under the License. --> - - + + @@ -131,6 +131,10 @@ limitations under the License. return this.queryEffectiveChildren('#diffTable'); }, + get _commentThreadElements() { + return this.queryAllEffectiveChildren('.comment-thread'); + }, + observers: [ '_groupsChanged(_groups.splices)', ], @@ -166,7 +170,8 @@ limitations under the License. // Stop the processor and syntax layer (if they're running). this.cancel(); - this._builder = this._getDiffBuilder(this.diff, comments, prefs); + this._builder = this._getDiffBuilder( + this.diff, comments.meta.patchRange, prefs); this.$.processor.context = prefs.context; this.$.processor.keyLocations = this._getKeyLocations(comments, @@ -290,7 +295,7 @@ limitations under the License. throw Error(`Invalid preference value: ${pref}`); }, - _getDiffBuilder(diff, comments, prefs) { + _getDiffBuilder(diff, patchRange, prefs) { if (isNaN(prefs.tab_size) || prefs.tab_size <= 0) { this._handlePreferenceError('tab size'); return; @@ -303,22 +308,21 @@ limitations under the License. let builder = null; if (this.isImageDiff) { - builder = new GrDiffBuilderImage(diff, comments, this.parentIndex, - this.changeNum, this.path, this.projectName, prefs, - this.diffElement, this.baseImage, this.revisionImage); + builder = new GrDiffBuilderImage(diff, patchRange, + this._commentThreadElements, prefs, this.diffElement, + this.baseImage, this.revisionImage); } else if (diff.binary) { // If the diff is binary, but not an image. - return new GrDiffBuilderBinary(diff, comments, this.parentIndex, - this.changeNum, this.path, this.projectName, prefs, - this.diffElement); + return new GrDiffBuilderBinary(diff, patchRange, + this._commentThreadElements, prefs, this.diffElement); } else if (this.viewMode === DiffViewMode.SIDE_BY_SIDE) { - builder = new GrDiffBuilderSideBySide(diff, comments, - this.parentIndex, this.changeNum, this.path, this.projectName, - prefs, this.diffElement, this._layers); + builder = new GrDiffBuilderSideBySide(diff, patchRange, + this._commentThreadElements, prefs, this.diffElement, + this._layers); } else if (this.viewMode === DiffViewMode.UNIFIED) { - builder = new GrDiffBuilderUnified(diff, comments, this.parentIndex, - this.changeNum, this.path, this.projectName, prefs, - this.diffElement, this._layers); + builder = new GrDiffBuilderUnified(diff, patchRange, + this._commentThreadElements, prefs, this.diffElement, + this._layers); } if (!builder) { throw Error('Unsupported diff view mode: ' + this.viewMode); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js index 7b568d17de..bfd2125df1 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js @@ -96,14 +96,10 @@ */ const REGEX_TAB_OR_SURROGATE_PAIR = /\t|[\uD800-\uDBFF][\uDC00-\uDFFF]/; - function GrDiffBuilder(diff, comments, parentIndex, changeNum, path, - projectName, prefs, outputEl, layers) { + function GrDiffBuilder(diff, patchRange, commentThreadEls, prefs, + outputEl, layers) { this._diff = diff; - this._comments = comments; - this._parentIndex = parentIndex; - this._changeNum = changeNum; - this._path = path; - this._projectName = projectName; + this._patchRange = patchRange; this._prefs = prefs; this._outputEl = outputEl; this.groups = []; @@ -125,25 +121,7 @@ } } - if (!this._comments) { - this._threadEls = []; - return; - } - - const allComments = []; - for (const side of [GrDiffBuilder.Side.LEFT, GrDiffBuilder.Side.RIGHT]) { - // This is needed by the threading. - for (const comment of this._comments[side]) { - comment.__commentSide = side; - } - allComments.push(...this._comments[side]); - } - const threads = this._createThreads(allComments); - this._threadEls = threads.map(thread => { - return Gerrit.createThreadElement( - thread, this._parentIndex, this._changeNum, - this._path, this._projectName); - }); + this._threadEls = commentThreadEls; } GrDiffBuilder.GroupType = { @@ -396,77 +374,16 @@ return button; }; - /** - * @param {Array} comments - */ - GrDiffBuilder.prototype._createThreads = function(comments) { - const sortedComments = comments.slice(0).sort((a, b) => { - if (b.__draft && !a.__draft ) { return 0; } - if (a.__draft && !b.__draft ) { return 1; } - return util.parseDate(a.updated) - util.parseDate(b.updated); - }); - - const threads = []; - for (const comment of sortedComments) { - // If the comment is in reply to another comment, find that comment's - // thread and append to it. - if (comment.in_reply_to) { - const thread = threads.find(thread => - thread.comments.some(c => c.id === comment.in_reply_to)); - if (thread) { - thread.comments.push(comment); - continue; - } - } - - // Otherwise, this comment starts its own thread. - const newThread = { - start_datetime: comment.updated, - comments: [comment], - commentSide: comment.__commentSide, - patchNum: comment.patch_set, - rootId: comment.id || comment.__draftID, - lineNum: comment.line, - isOnParent: comment.side === 'PARENT', - }; - if (comment.range) { - newThread.range = Object.assign({}, comment.range); - } - threads.push(newThread); - } - return threads; - }; - - /** - * Returns the patch number that new comment threads should be attached to. - * - * @param {GrDiffLine} line The line new thread will be attached to. - * @param {string=} opt_side Set to LEFT to force adding it to the LEFT side - - * will be ignored if the left is a parent or a merge parent - * @return {number} Patch set to attach the new thread to - */ - GrDiffBuilder.prototype._determinePatchNumForNewThreads = function( - patchRange, line, opt_side) { - if ((line.type === GrDiffLine.Type.REMOVE || - opt_side === GrDiffBuilder.Side.LEFT) && - patchRange.basePatchNum !== 'PARENT' && - !Gerrit.PatchSetBehavior.isMergeParent(patchRange.basePatchNum)) { - return patchRange.basePatchNum; - } else { - return patchRange.patchNum; - } - }; - /** * @param {!GrDiffLine} line - * @param {!GrDiffBuilder.Side=} side The side (LEFT, RIGHT, BOTH) for which to return - * the thread group (default: BOTH). + * @param {!GrDiffBuilder.Side=} side The side (LEFT, RIGHT, BOTH) for which + * to return the thread group (default: BOTH). * @return {!Object} */ GrDiffBuilder.prototype._commentThreadGroupForLine = function( - line, side = GrDiffBuilder.Side.BOTH) { + line, commentSide = GrDiffBuilder.Side.BOTH) { const threadElsForGroup = - Gerrit.filterThreadElsForLocation(this._threadEls, line, side); + Gerrit.filterThreadElsForLocation(this._threadEls, line, commentSide); if (!threadElsForGroup || threadElsForGroup.length === 0) { return null; } @@ -476,8 +393,8 @@ for (const threadEl of threadElsForGroup) { Polymer.dom(threadGroupEl).appendChild(threadEl); } - if (side) { - threadGroupEl.setAttribute('data-side', side); + if (commentSide) { + threadGroupEl.setAttribute('data-side', commentSide); } return threadGroupEl; }; diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html index bd65703aec..a855833e63 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html @@ -62,10 +62,6 @@ limitations under the License. let builder; let sandbox; const LINE_FEED_HTML = ''; - const parentIndex = 3; - const changeNum = 4; - const path = 'some/path.ext'; - const projectName = 'Awesome Project'; setup(() => { sandbox = sinon.sandbox.create(); @@ -80,8 +76,7 @@ limitations under the License. tab_size: 4, }; builder = new GrDiffBuilder( - {content: []}, {left: [], right: []}, parentIndex, changeNum, path, - projectName, prefs); + {content: []}, {left: [], right: []}, [], prefs); }); teardown(() => { sandbox.restore(); }); @@ -145,153 +140,6 @@ limitations under the License. Gerrit.DiffSide.RIGHT), [r]); }); - test('_createThreads', () => { - const comments = [ - { - id: 'sallys_confession', - message: 'i like you, jack', - updated: '2015-12-23 15:00:20.396000000', - line: 1, - __commentSide: 'left', - }, { - id: 'jacks_reply', - message: 'i like you, too', - updated: '2015-12-24 15:01:20.396000000', - __commentSide: 'left', - line: 1, - in_reply_to: 'sallys_confession', - }, - { - id: 'new_draft', - message: 'i do not like either of you', - __commentSide: 'left', - __draft: true, - updated: '2015-12-20 15:01:20.396000000', - }, - ]; - - const actualThreads = builder._createThreads(comments); - - assert.equal(actualThreads.length, 2); - - assert.equal( - actualThreads[0].start_datetime, '2015-12-23 15:00:20.396000000'); - assert.equal(actualThreads[0].commentSide, 'left'); - assert.equal(actualThreads[0].comments.length, 2); - assert.deepEqual(actualThreads[0].comments[0], comments[0]); - assert.deepEqual(actualThreads[0].comments[1], comments[1]); - assert.equal(actualThreads[0].patchNum, undefined); - assert.equal(actualThreads[0].rootId, 'sallys_confession'); - assert.equal(actualThreads[0].lineNum, 1); - - assert.equal( - actualThreads[1].start_datetime, '2015-12-20 15:01:20.396000000'); - assert.equal(actualThreads[1].commentSide, 'left'); - assert.equal(actualThreads[1].comments.length, 1); - assert.deepEqual(actualThreads[1].comments[0], comments[2]); - assert.equal(actualThreads[1].patchNum, undefined); - assert.equal(actualThreads[1].rootId, 'new_draft'); - assert.equal(actualThreads[1].lineNum, undefined); - }); - - test('_createThreads inherits patchNum and range', () => { - const comments = [{ - id: 'betsys_confession', - message: 'i like you, jack', - updated: '2015-12-24 15:00:10.396000000', - range: { - start_line: 1, - start_character: 1, - end_line: 1, - end_character: 2, - }, - patch_set: 5, - __commentSide: 'left', - line: 1, - }]; - - expectedThreads = [ - { - start_datetime: '2015-12-24 15:00:10.396000000', - commentSide: 'left', - comments: [{ - id: 'betsys_confession', - message: 'i like you, jack', - updated: '2015-12-24 15:00:10.396000000', - range: { - start_line: 1, - start_character: 1, - end_line: 1, - end_character: 2, - }, - patch_set: 5, - __commentSide: 'left', - line: 1, - }], - patchNum: 5, - rootId: 'betsys_confession', - range: { - start_line: 1, - start_character: 1, - end_line: 1, - end_character: 2, - }, - lineNum: 1, - isOnParent: false, - }, - ]; - - assert.deepEqual( - builder._createThreads(comments), - expectedThreads); - }); - - test('_createThreads does not thread unrelated comments at same location', - () => { - const comments = [ - { - id: 'sallys_confession', - message: 'i like you, jack', - updated: '2015-12-23 15:00:20.396000000', - __commentSide: 'left', - }, { - id: 'jacks_reply', - message: 'i like you, too', - updated: '2015-12-24 15:01:20.396000000', - __commentSide: 'left', - }, - ]; - assert.equal(builder._createThreads(comments).length, 2); - }); - - test('_createThreads derives isOnParent using side from first comment', - () => { - const comments = [ - { - id: 'sallys_confession', - message: 'i like you, jack', - updated: '2015-12-23 15:00:20.396000000', - // line: 1, - // __commentSide: 'left', - }, { - id: 'jacks_reply', - message: 'i like you, too', - updated: '2015-12-24 15:01:20.396000000', - // __commentSide: 'left', - // line: 1, - in_reply_to: 'sallys_confession', - }, - ]; - - assert.equal(builder._createThreads(comments)[0].isOnParent, false); - - comments[0].side = 'REVISION'; - assert.equal(builder._createThreads(comments)[0].isOnParent, false); - - comments[0].side = 'PARENT'; - assert.equal(builder._createThreads(comments)[0].isOnParent, true); - }); - test('_createElement classStr applies all classes', () => { const node = builder._createElement('div', 'test classes'); assert.isTrue(node.classList.contains('gr-diff')); @@ -466,42 +314,32 @@ limitations under the License. }); test('comment thread group creation', () => { - const l3 = {id: 'l3', line: 3, updated: '2016-08-09 00:42:32.000000000', - __commentSide: 'left', side: 'PARENT'}; - const l5 = {id: 'l5', line: 5, updated: '2016-08-09 00:42:32.000000000', - __commentSide: 'left', patch_set: '2', side: 'REVISION'}; - const r5 = {id: 'r5', line: 5, updated: '2016-08-09 00:42:32.000000000', - __commentSide: 'right', patch_set: '3'}; + const l3 = document.createElement('div'); + l3.className = 'comment-thread'; + l3.setAttribute('comment-side', 'left'); + l3.setAttribute('line-num', 3); + + const l5 = document.createElement('div'); + l5.className = 'comment-thread'; + l5.setAttribute('comment-side', 'left'); + l5.setAttribute('line-num', 5); + + const r5 = document.createElement('div'); + r5.className = 'comment-thread'; + r5.setAttribute('comment-side', 'right'); + r5.setAttribute('line-num', 5); builder = new GrDiffBuilder( - {content: []}, { - meta: { - changeNum: '42', - patchRange: { - basePatchNum: 'PARENT', - patchNum: '3', - }, - path: '/path/to/foo', - projectConfig: {foo: 'bar'}, - }, - left: [l3, l5], - right: [r5], - }, parentIndex, changeNum, path, projectName, prefs); + {content: []}, {basePatchNum: 'PARENT', patchNum: '3'}, [l3, l5, r5], + prefs); - function checkThreadGroupProps(threadGroupEl, patchNum, - comments) { + function checkThreadGroupProps(threadGroupEl, + expectedThreadEls) { const threadEls = Polymer.dom(threadGroupEl).queryDistributedElements( - 'gr-diff-comment-thread'); - assert.equal(threadEls.length, comments.length); - for (let i=0; i < comments.length; i++) { - const threadEl = threadEls[i]; - - const comment = comments[i]; - assert.equal(threadEl.patchNum, comment.patch_set); - assert.equal(threadEl.commentSide, comment.__commentSide); - assert.equal(threadEl.comments.length, 1); - assert.equal(threadEl.comments[0], comment); - assert.equal(threadEl.rootId, comment.id); + '.comment-thread'); + assert.equal(threadEls.length, expectedThreadEls.length); + for (let i=0; i { @@ -1029,7 +869,7 @@ limitations under the License. processStub = sandbox.stub(element.$.processor, 'process') .returns(Promise.resolve()); sandbox.stub(element, '_anyLineTooLong').returns(true); - comments = {left: [], right: []}; + comments = {left: [], right: [], meta: {patchRange: undefined}}; prefs = { line_length: 10, show_tabs: true, @@ -1077,6 +917,7 @@ limitations under the License. suite('rendering', () => { let content; let outputEl; + let comments; setup(done => { const prefs = { @@ -1104,10 +945,10 @@ limitations under the License. }); element = fixture('basic'); outputEl = element.queryEffectiveChildren('#diffTable'); + comments = {left: [], right: [], meta: {patchRange: undefined}}; sandbox.stub(element, '_getDiffBuilder', () => { const builder = new GrDiffBuilder( - {content}, {left: [], right: []}, parentIndex, changeNum, path, - projectName, prefs, outputEl); + {content}, undefined, [], prefs, outputEl); sandbox.stub(builder, 'addColumns'); builder.buildSectionElement = function(group) { const section = document.createElement('stub'); @@ -1119,7 +960,7 @@ limitations under the License. return builder; }); element.diff = {content}; - element.render({left: [], right: []}, prefs).then(done); + element.render(comments, prefs).then(done); }); test('reporting', done => { @@ -1144,7 +985,7 @@ limitations under the License. }); test('addColumns is called', done => { - element.render({left: [], right: []}, {}).then(done); + element.render(comments, {}).then(done); assert.isTrue(element._builder.addColumns.called); }); @@ -1168,7 +1009,7 @@ limitations under the License. test('render-start and render are fired', done => { const dispatchEventStub = sandbox.stub(element, 'dispatchEvent'); - element.render({left: [], right: []}, {}).then(() => { + element.render(comments, {}).then(() => { const firedEventTypes = dispatchEventStub.getCalls() .map(c => { return c.args[0].type; }); assert.include(firedEventTypes, 'render-start'); @@ -1196,7 +1037,7 @@ limitations under the License. context: -1, syntax_highlighting: true, }; - element.render({left: [], right: []}, prefs); + element.render(comments, prefs); }); test('cancel', () => { @@ -1213,6 +1054,7 @@ limitations under the License. let builder; let diff; let prefs; + let comments; setup(done => { element = fixture('mock-diff'); @@ -1224,12 +1066,12 @@ limitations under the License. show_tabs: true, tab_size: 4, }; + comments = {left: [], right: [], meta: {patchRange: undefined}}; - element.render( - undefined, prefs).then(() => { - builder = element._builder; - done(); - }); + element.render(comments, prefs).then(() => { + builder = element._builder; + done(); + }); }); test('getDiffLength', () => { @@ -1336,7 +1178,7 @@ limitations under the License. test('_getNextContentOnSide unified left', done => { // Re-render as unified: element.viewMode = 'UNIFIED_DIFF'; - element.render({left: [], right: []}, prefs).then(() => { + element.render(comments, prefs).then(() => { builder = element._builder; const startElem = builder.getContentByLine(5, 'left', @@ -1356,7 +1198,7 @@ limitations under the License. test('_getNextContentOnSide unified right', done => { // Re-render as unified: element.viewMode = 'UNIFIED_DIFF'; - element.render({left: [], right: []}, prefs).then(() => { + element.render(comments, prefs).then(() => { builder = element._builder; const startElem = builder.getContentByLine(5, 'right', diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html index 9ecc2b154c..1f77c0d24d 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html @@ -27,11 +27,13 @@ limitations under the License. max-width: var(--content-width, 80ch); white-space: normal; } - gr-diff-comment-thread + gr-diff-comment-thread { + div ::slotted(*) + div ::slotted(*) { margin-top: .2em; } - +
+ +
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js index 9eb3fa2b9f..e7641cd652 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js @@ -46,7 +46,7 @@ const threadEl = document.createElement('gr-diff-comment-thread'); threadEl.comments = thread.comments; threadEl.commentSide = thread.commentSide; - threadEl.isOnParent = thread.isOnParent; + threadEl.isOnParent = !!thread.isOnParent; threadEl.parentIndex = parentIndex; threadEl.changeNum = changeNum; threadEl.patchNum = thread.patchNum; @@ -74,45 +74,5 @@ properties: { }, - - get threadEls() { - return Polymer.dom(this).queryDistributedElements( - 'gr-diff-comment-thread'); - }, - - /** - * Fetch the thread element at the given range, or the range-less thread - * element on the line if no range is provided, lineNum, and side. - * - * @param {string} side - * @param {!Object=} opt_range - * @return {!Object|undefined} - */ - getThreadEl(side, opt_range) { - const threads = [].filter.call(this.threadEls, - thread => this._rangesEqual(thread.range, opt_range)) - .filter(thread => thread.commentSide === side); - if (threads.length === 1) { - return threads[0]; - } - }, - - /** - * Compare two ranges. Either argument may be falsy, but will only return - * true if both are falsy or if neither are falsy and have the same position - * values. - * - * @param {Object=} a range 1 - * @param {Object=} b range 2 - * @return {boolean} - */ - _rangesEqual(a, b) { - if (!a && !b) { return true; } - if (!a || !b) { return false; } - return a.startLine === b.startLine && - a.startChar === b.startChar && - a.endLine === b.endLine && - a.endChar === b.endChar; - }, }); })(); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html index 90f50a8e7a..d1150ed7cf 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html @@ -41,9 +41,6 @@ limitations under the License. setup(() => { sandbox = sinon.sandbox.create(); - stub('gr-rest-api-interface', { - getLoggedIn() { return Promise.resolve(false); }, - }); element = fixture('basic'); }); @@ -51,115 +48,26 @@ limitations under the License. sandbox.restore(); }); - test('getThreadEl', () => { - const range = { - start_line: 1, - start_character: 1, - end_line: 1, - end_character: 2, - }; - const threads = [ - { - rootId: 'sallys_confession', - commentSide: 'left', - comments: [ - { - id: 'sallys_confession', - message: 'i like you, jack', - updated: '2015-12-23 15:00:20.396000000', - __commentSide: 'left', - }, { - id: 'jacks_reply', - message: 'i like you, too', - updated: '2015-12-24 15:01:20.396000000', - __commentSide: 'left', - in_reply_to: 'sallys_confession', - }, { - id: 'new_draft', - message: 'i do not like either of you', - __commentSide: 'left', - __draft: true, - in_reply_to: 'sallys_confession', - updated: '2015-12-20 15:01:20.396000000', - }, - ], - }, - { - rootId: 'right_side_comment', - commentSide: 'right', - comments: [ - { - id: 'right_side_comment', - message: 'right side comment', - __commentSide: 'right', - __draft: true, - updated: '2015-12-20 15:01:20.396000000', - }, - ], - }, { - rootId: 'betsys_confession', - commentSide: 'left', - range, - comments: [ - { - id: 'betsys_confession', - message: 'i like you more, jack', - updated: '2015-12-24 15:00:10.396000000', - range, - __commentSide: 'left', - }, - ], - }, - ]; - for (const thread of threads) { - Polymer.dom(element).appendChild(Gerrit.createThreadElement( - thread, 1, '2', 'some/path', 'some_project')); - } - - flushAsynchronousOperations(); - assert.deepEqual( - element.getThreadEl('right').rootId, 'right_side_comment'); - assert.deepEqual(element.getThreadEl('right').comments.length, 1); - assert.deepEqual(element.getThreadEl('left').rootId, 'sallys_confession'); - assert.deepEqual(element.getThreadEl('left').comments.length, 3); - assert.deepEqual(element.getThreadEl('left', range).rootId, - 'betsys_confession'); - assert.deepEqual(element.getThreadEl('left', range).comments.length, 1); - }); - test('thread-discard handling', () => { const threads = [ {comments: [{id: 4711}]}, {comments: [{id: 42}]}, ]; - for (const thread of threads) { - const threadEl = Gerrit.createThreadElement( - thread, 1, 2, 'some/path', 'Some Project'); + const threadEls = threads.map(thread => Gerrit.createThreadElement( + thread, 1, 2, 'some/path', 'Some Project')); + assert.equal(threadEls.length, 2); + assert.equal(threadEls[0].rootId, 4711); + assert.equal(threadEls[1].rootId, 42); + for (const threadEl of threadEls) { Polymer.dom(element).appendChild(threadEl); } - element.threadEls[0].dispatchEvent( + threadEls[0].dispatchEvent( new CustomEvent('thread-discard', {detail: {rootId: 4711}})); - assert.equal(element.threadEls.length, 1); - assert.equal(element.threadEls[0].rootId, 42); - }); - - test('_rangesEqual', () => { - const range1 = - {startLine: 123, startChar: 345, endLine: 234, endChar: 456}; - const range2 = - {startLine: 1, startChar: 2, endLine: 3, endChar: 4}; - - assert.isTrue(element._rangesEqual(null, null)); - assert.isTrue(element._rangesEqual(null, undefined)); - assert.isTrue(element._rangesEqual(undefined, null)); - assert.isTrue(element._rangesEqual(undefined, undefined)); - - assert.isFalse(element._rangesEqual(range1, null)); - assert.isFalse(element._rangesEqual(null, range1)); - assert.isFalse(element._rangesEqual(range1, range2)); - - assert.isTrue(element._rangesEqual(range1, Object.assign({}, range1))); + const attachedThreads = element.queryAllEffectiveChildren( + 'gr-diff-comment-thread'); + assert.equal(attachedThreads.length, 1); + assert.equal(attachedThreads[0].rootId, 42); }); }); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html index 9668a54d48..f111378a74 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html @@ -60,7 +60,11 @@ limitations under the License. diffElement.loggedIn = false; diffElement.patchRange = {basePatchNum: 1, patchNum: 2}; - diffElement.comments = {left: [], right: []}; + diffElement.comments = { + left: [], + right: [], + meta: {patchRange: undefined}, + }; const setupDone = () => { cursorElement._updateStops(); cursorElement.moveToFirstChunk(); 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 a5f5fd918c..028e44c225 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 @@ -19,6 +19,8 @@ limitations under the License. + + @@ -46,8 +48,7 @@ limitations under the License. base-image="[[_baseImage]]" revision-image=[[_revisionImage]] blame="[[_blame]]" - diff="[[_diff]]" - parent-index="[[_parentIndex]]"> + diff="[[_diff]]"> 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 68b4616edf..2b91060e54 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 @@ -45,13 +45,35 @@ return !!(diff.binary && (isA || isB)); } + /** @typedef {{startLine: number, startChar: number, + * endLine: number, endChar: number}} */ + Gerrit.Range; + + + /** + * Compare two ranges. Either argument may be falsy, but will only return + * true if both are falsy or if neither are falsy and have the same position + * values. + * + * @param {Gerrit.Range=} a range 1 + * @param {Gerrit.Range=} b range 2 + * @return {boolean} + */ + function rangesEqual(a, b) { + if (!a && !b) { return true; } + if (!a || !b) { return false; } + return a.startLine === b.startLine && + a.startChar === b.startChar && + a.endLine === b.endLine && + a.endChar === b.endChar; + } + /** * Wrapper around gr-diff. * * Webcomponent fetching diffs and related data from restAPI and passing them * to the presentational gr-diff for rendering. */ - // TODO(oler): Move all calls to restAPI from gr-diff here. Polymer({ is: 'gr-diff-host', @@ -108,7 +130,10 @@ type: Boolean, value: false, }, - comments: Object, + comments: { + type: Object, + observer: '_commentsChanged', + }, lineWrapping: { type: Boolean, value: false, @@ -176,6 +201,11 @@ type: Number, computed: '_computeParentIndex(patchRange.*)', }, + + _threadEls: { + type: Array, + value: [], + }, }, behaviors: [ @@ -310,7 +340,7 @@ * @return {!Array} */ getThreadEls() { - return this.$.diff.getThreadEls(); + return this._threadEls; }, /** @param {HTMLElement} el */ @@ -437,6 +467,70 @@ return isImageDiff(diff); }, + + _commentsChanged(newComments) { + const allComments = []; + for (const side of [GrDiffBuilder.Side.LEFT, GrDiffBuilder.Side.RIGHT]) { + // This is needed by the threading. + for (const comment of newComments[side]) { + comment.__commentSide = side; + } + allComments.push(...newComments[side]); + } + // Currently, the only way this is ever changed here is when the initial + // comments are loaded, so it's okay performance wise to clear the threads + // and recreate them. If this changes in future, we might want to reuse + // some DOM nodes here. + this._clearThreads(); + const threads = this._createThreads(allComments); + for (const thread of threads) { + const threadEl = this._createThreadElement(thread); + this._attachThreadElement(threadEl); + } + }, + + /** + * @param {!Array} comments + * @return {!Array} Threads for the given comments. + */ + _createThreads(comments) { + const sortedComments = comments.slice(0).sort((a, b) => { + if (b.__draft && !a.__draft ) { return 0; } + if (a.__draft && !b.__draft ) { return 1; } + return util.parseDate(a.updated) - util.parseDate(b.updated); + }); + + const threads = []; + for (const comment of sortedComments) { + // If the comment is in reply to another comment, find that comment's + // thread and append to it. + if (comment.in_reply_to) { + const thread = threads.find(thread => + thread.comments.some(c => c.id === comment.in_reply_to)); + if (thread) { + thread.comments.push(comment); + continue; + } + } + + // Otherwise, this comment starts its own thread. + const newThread = { + start_datetime: comment.updated, + comments: [comment], + commentSide: comment.__commentSide, + patchNum: comment.patch_set, + rootId: comment.id || comment.__draftID, + lineNum: comment.line, + isOnParent: comment.side === 'PARENT', + }; + if (comment.range) { + newThread.range = Object.assign({}, comment.range); + } + threads.push(newThread); + } + return threads; + }, + /** * @param {Object} blame * @return {boolean} @@ -456,44 +550,96 @@ /** @param {CustomEvent} e */ _handleCreateComment(e) { - const {threadGroupEl, lineNum, side, range, isOnParent, - patchForNewThreads} = e.detail; - const threadEl = this._getOrCreateThread(threadGroupEl, side, range, - isOnParent, patchForNewThreads); + const {lineNum, side, patchNum, isOnParent, range} = e.detail; + const threadEl = this._getOrCreateThread(patchNum, lineNum, side, range, + isOnParent); 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. + * Gets or creates a comment thread at a given location. + * May provide a range, to get/create a range comment. * - * @param {!Node} threadGroupEl + * @param {string} patchNum + * @param {?number} lineNum * @param {string} commentSide - * @param {?Object} range + * @param {Gerrit.Range|undefined} range * @param {boolean} isOnParent - * @param {string} patchForNewThreads * @return {!Object} */ - _getOrCreateThread(threadGroupEl, commentSide, range, isOnParent, - patchForNewThreads) { - let threadEl = threadGroupEl.getThreadEl(commentSide, range); - + _getOrCreateThread(patchNum, lineNum, commentSide, range, isOnParent) { + let threadEl = this._getThreadEl(lineNum, commentSide, range); if (!threadEl) { - threadEl = Gerrit.createThreadElement( - { - comments: [], - commentSide, - patchNum: patchForNewThreads, - range, - isOnParent, - }, this._parentIndex, this.changeNum, - this.path, this.projectName); - Polymer.dom(threadGroupEl).appendChild(threadEl); + threadEl = this._createThreadElement({ + comments: [], + commentSide, + patchNum, + lineNum, + range, + isOnParent, + }); + this._attachThreadElement(threadEl); } return threadEl; }, + _attachThreadElement(threadEl) { + this._threadEls.push(threadEl); + Polymer.dom(this.$.diff).appendChild(threadEl); + }, + + _clearThreads() { + for (const threadEl of this._threadEls) { + const parent = Polymer.dom(threadEl).parentNode; + Polymer.dom(parent).removeChild(threadEl); + } + this._threadEls = []; + }, + + _createThreadElement(thread) { + const threadEl = Gerrit.createThreadElement( + thread, this._parentIndex, this.changeNum, this.path, + this.projectName); + threadEl.className = 'comment-thread'; + threadEl.addEventListener('thread-discard', e => { + const i = this._threadEls.findIndex( + threadEl => threadEl.rootId == e.detail.rootId); + this._threadEls.splice(i, 1); + }); + return threadEl; + }, + + /** + * Gets a comment thread element at a given location. + * May provide a range, to get a range comment. + * + * @param {?number} lineNum + * @param {string} commentSide + * @param {!Gerrit.Range=} range + * @return {?Node} + */ + _getThreadEl(lineNum, commentSide, range=undefined) { + let line; + if (commentSide === GrDiffBuilder.Side.LEFT) { + line = {beforeNumber: lineNum}; + } else if (commentSide === GrDiffBuilder.Side.RIGHT) { + line = {afterNumber: lineNum}; + } else { + throw new Error(`Unknown side: ${commentSide}`); + } + function matchesRange(threadEl) { + const threadRange = /** @type {!Gerrit.Range} */( + JSON.parse(threadEl.getAttribute('range'))); + return rangesEqual(threadRange, range); + } + + const filteredThreadEls = Gerrit.filterThreadElsForLocation( + this._threadEls, line, commentSide).filter(matchesRange); + return filteredThreadEls.length ? filteredThreadEls[0] : null; + }, + /** * Take a diff that was loaded with a ignore-whitespace other than * IGNORE_NONE, and convert delta chunks labeled as common into shared 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 711757461d..790a71f513 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 @@ -46,6 +46,9 @@ limitations under the License. async getLoggedIn() { return getLoggedIn; }, }); element = fixture('basic'); + // For reasons beyond me, fixture reuses elements, cleans out some + // stuff but not that list. + element._threadEls = []; }); teardown(() => { @@ -182,7 +185,11 @@ limitations under the License. }); element.patchRange = {basePatchNum: 'PARENT', patchNum: 1}; - element.comments = {left: [], right: []}; + element.comments = { + left: [], + right: [], + meta: {patchRange: element.patchRange}, + }; }); test('renders image diffs with same file name', done => { @@ -556,13 +563,10 @@ limitations under the License. }); }); - test('delegates getThreadEls()', () => { + test('getThreadEls() returns _threadEls', () => { const returnValue = [document.createElement('b')]; - const stub = sandbox.stub(element.$.diff, 'getThreadEls') - .returns(returnValue); + element._threadEls = returnValue; assert.equal(element.getThreadEls(), returnValue); - assert.isTrue(stub.calledOnce); - assert.equal(stub.lastCall.args.length, 0); }); test('delegates addDraftAtLine(el)', () => { @@ -771,15 +775,160 @@ limitations under the License. }); }); + test('_createThreads', () => { + const comments = [ + { + id: 'sallys_confession', + message: 'i like you, jack', + updated: '2015-12-23 15:00:20.396000000', + line: 1, + __commentSide: 'left', + }, { + id: 'jacks_reply', + message: 'i like you, too', + updated: '2015-12-24 15:01:20.396000000', + __commentSide: 'left', + line: 1, + in_reply_to: 'sallys_confession', + }, + { + id: 'new_draft', + message: 'i do not like either of you', + __commentSide: 'left', + __draft: true, + updated: '2015-12-20 15:01:20.396000000', + }, + ]; + + const actualThreads = element._createThreads(comments); + + assert.equal(actualThreads.length, 2); + + assert.equal( + actualThreads[0].start_datetime, '2015-12-23 15:00:20.396000000'); + assert.equal(actualThreads[0].commentSide, 'left'); + assert.equal(actualThreads[0].comments.length, 2); + assert.deepEqual(actualThreads[0].comments[0], comments[0]); + assert.deepEqual(actualThreads[0].comments[1], comments[1]); + assert.equal(actualThreads[0].patchNum, undefined); + assert.equal(actualThreads[0].rootId, 'sallys_confession'); + assert.equal(actualThreads[0].lineNum, 1); + + assert.equal( + actualThreads[1].start_datetime, '2015-12-20 15:01:20.396000000'); + assert.equal(actualThreads[1].commentSide, 'left'); + assert.equal(actualThreads[1].comments.length, 1); + assert.deepEqual(actualThreads[1].comments[0], comments[2]); + assert.equal(actualThreads[1].patchNum, undefined); + assert.equal(actualThreads[1].rootId, 'new_draft'); + assert.equal(actualThreads[1].lineNum, undefined); + }); + + test('_createThreads inherits patchNum and range', () => { + const comments = [{ + id: 'betsys_confession', + message: 'i like you, jack', + updated: '2015-12-24 15:00:10.396000000', + range: { + start_line: 1, + start_character: 1, + end_line: 1, + end_character: 2, + }, + patch_set: 5, + __commentSide: 'left', + line: 1, + }]; + + expectedThreads = [ + { + start_datetime: '2015-12-24 15:00:10.396000000', + commentSide: 'left', + comments: [{ + id: 'betsys_confession', + message: 'i like you, jack', + updated: '2015-12-24 15:00:10.396000000', + range: { + start_line: 1, + start_character: 1, + end_line: 1, + end_character: 2, + }, + patch_set: 5, + __commentSide: 'left', + line: 1, + }], + patchNum: 5, + rootId: 'betsys_confession', + range: { + start_line: 1, + start_character: 1, + end_line: 1, + end_character: 2, + }, + lineNum: 1, + isOnParent: false, + }, + ]; + + assert.deepEqual( + element._createThreads(comments), + expectedThreads); + }); + + test('_createThreads does not thread unrelated comments at same location', + () => { + const comments = [ + { + id: 'sallys_confession', + message: 'i like you, jack', + updated: '2015-12-23 15:00:20.396000000', + __commentSide: 'left', + }, { + id: 'jacks_reply', + message: 'i like you, too', + updated: '2015-12-24 15:01:20.396000000', + __commentSide: 'left', + }, + ]; + assert.equal(element._createThreads(comments).length, 2); + }); + + test('_createThreads derives isOnParent using side from first comment', + () => { + const comments = [ + { + id: 'sallys_confession', + message: 'i like you, jack', + updated: '2015-12-23 15:00:20.396000000', + // line: 1, + // __commentSide: 'left', + }, { + id: 'jacks_reply', + message: 'i like you, too', + updated: '2015-12-24 15:01:20.396000000', + // __commentSide: 'left', + // line: 1, + in_reply_to: 'sallys_confession', + }, + ]; + + assert.equal(element._createThreads(comments)[0].isOnParent, false); + + comments[0].side = 'REVISION'; + assert.equal(element._createThreads(comments)[0].isOnParent, false); + + comments[0].side = 'PARENT'; + assert.equal(element._createThreads(comments)[0].isOnParent, true); + }); + test('_getOrCreateThread', () => { - const threadGroupEl = - document.createElement('gr-diff-comment-thread-group'); const commentSide = 'left'; - assert.isOk(element._getOrCreateThread(threadGroupEl, - commentSide, undefined, false, '2')); + assert.isOk(element._getOrCreateThread('2', 3, + commentSide, undefined, false)); - let threads = Polymer.dom(threadGroupEl) + let threads = Polymer.dom(element.$.diff) .queryDistributedElements('gr-diff-comment-thread'); assert.equal(threads.length, 1); @@ -798,9 +947,9 @@ limitations under the License. }; assert.isOk(element._getOrCreateThread( - threadGroupEl, commentSide, range, true, '3')); + '3', 1, commentSide, range, true)); - threads = Polymer.dom(threadGroupEl) + threads = Polymer.dom(element.$.diff) .queryDistributedElements('gr-diff-comment-thread'); assert.equal(threads.length, 2); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html index 0866849d37..0e55b12ca4 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html @@ -332,7 +332,6 @@ limitations under the License. patch-range="[[_patchRange]]" path="[[_path]]" prefs="[[_prefs]]" - project-config="[[_projectConfig]]" project-name="[[_change.project]]" view-mode="[[_diffMode]]" is-blame-loaded="{{_isBlameLoaded}}" diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html index a639d5107e..e58540110a 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html @@ -20,7 +20,6 @@ limitations under the License. - @@ -290,9 +289,8 @@ limitations under the License. is-image-diff="[[isImageDiff]]" base-image="[[baseImage]]" revision-image="[[revisionImage]]" - parentIndex="[[parentIndex]]" - path="[[path]]" line-of-interest="[[lineOfInterest]]"> + } */ - getThreadEls() { - let threads = []; - const threadGroupEls = Polymer.dom(this.root) - .querySelectorAll('gr-diff-comment-thread-group'); - for (const threadGroupEl of threadGroupEls) { - threads = threads.concat(threadGroupEl.threadEls); - } - return threads; - }, - /** @return {string} */ _computeContainerClass(loggedIn, viewMode, displayLine) { const classes = ['diffContainer']; @@ -354,17 +351,15 @@ const patchForNewThreads = this._getPatchNumByLineAndContent( lineEl, contentEl); const isOnParent = - this._getIsParentCommentByLineAndContent(lineEl, contentEl); - const threadGroupEl = this._getOrCreateThreadGroup(contentEl); + this._getIsParentCommentByLineAndContent(lineEl, contentEl); this.dispatchEvent(new CustomEvent('create-comment', { bubbles: true, detail: { - threadGroupEl, lineNum, side, - range, + patchNum: patchForNewThreads, isOnParent, - patchForNewThreads, + range, }, })); }, @@ -377,7 +372,7 @@ * Gets or creates a comment thread group for a specific line and side on a * diff. * @param {!Object} contentEl - * @return {!Object} + * @return {!Node} */ _getOrCreateThreadGroup(contentEl) { // Check if thread group exists. @@ -389,7 +384,6 @@ return threadGroupEl; }, - /** * The value to be used for the patch number of new comments created at the * given line and content elements. @@ -578,6 +572,7 @@ }, _renderDiffTable() { + this._unobserveNodes(); if (!this.prefs) { this.dispatchEvent(new CustomEvent('render', {bubbles: true})); return; @@ -594,6 +589,34 @@ this.$.diffBuilder.render(this.comments, this._getBypassPrefs()); }, + _handleRenderContent() { + this._nodeObserver = Polymer.dom(this).observeNodes(info => { + const addedThreadEls = info.addedNodes.filter( + node => node.nodeType === Node.ELEMENT_NODE); + // In principal 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 + // this situation until it occurs. + for (const threadEl of addedThreadEls) { + const lineNum = Number(threadEl.getAttribute('line-num')); + const commentSide = threadEl.getAttribute('comment-side'); + const lineEl = this.$.diffBuilder.getLineElByNumber( + lineNum, commentSide); + const contentText = this.$.diffBuilder.getContentByLineEl(lineEl); + const contentEl = contentText.parentElement; + const threadGroupEl = this._getOrCreateThreadGroup(contentEl); + Polymer.dom(threadGroupEl).appendChild(threadEl); + } + }); + }, + + _unobserveNodes() { + if (this._nodeObserver) { + Polymer.dom(this).unobserveNodes(this._nodeObserver); + } + }, + /** * Get the preferences object including the safety bypass context (if any). */ @@ -605,6 +628,7 @@ }, clearDiffContent() { + this._unobserveNodes(); this.$.diffTable.innerHTML = null; }, 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 193e7eb026..316990d0dc 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 @@ -335,7 +335,11 @@ limitations under the License. }; element.patchRange = {basePatchNum: 'PARENT', patchNum: 1}; - element.comments = {left: [], right: []}; + element.comments = { + left: [], + right: [], + meta: {patchRange: undefined}, + }; element.isImageDiff = true; element.prefs = { auto_hide_diff_table_header: true, @@ -664,6 +668,7 @@ limitations under the License. element.comments = { left: [], right: [], + meta: {patchRange: undefined}, }; element.prefs = { context: 10, From c4d6257d4e210ee560d7dfb22d5ff0fb2adf530c Mon Sep 17 00:00:00 2001 From: Ole Rehmsen Date: Thu, 8 Nov 2018 09:07:35 +0100 Subject: [PATCH 2/3] Move gr-diff-comment-thread factory to diff-host This is now only used from there, so it no longer needs to be globally accessible on Gerrit. Change-Id: I6facd64ece360f5b8ddf7b801621e86bd5f39697 --- .../diff/gr-diff-builder/gr-diff-builder.html | 1 + .../gr-diff-comment-thread-group.js | 52 ------------------- .../gr-diff-comment-thread-group_test.html | 24 --------- .../diff/gr-diff-host/gr-diff-host.html | 1 - .../diff/gr-diff-host/gr-diff-host.js | 31 +++++++++-- .../diff/gr-diff-host/gr-diff-host_test.html | 26 ++++++++++ .../app/elements/diff/gr-diff/gr-diff.html | 1 + 7 files changed, 54 insertions(+), 82 deletions(-) diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html index 33e2a19115..6db54117e0 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html @@ -19,6 +19,7 @@ limitations under the License. + diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js index e7641cd652..03218267c7 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js @@ -17,58 +17,6 @@ (function() { 'use strict'; - window.Gerrit = window.Gerrit || {}; - - // This method will eventually move to gr-diff-host (so that gr-diff and it's - // descendants, including gr-diff-comment-thread-group, do not depend on a - // specific comment thread implementation, but can instead be used with other - // comment widgets). I cannot move it there yet, because it is still called - // from this file, and this file cannot depend on gr-diff-host. I decided to - // make it a function on the global Gerrit namespace, so that - // 1) I can move the call-side in the next change without moving this code, - // and thereby reduce the number of moving parts per commit. - // 2) To already now cut the ties to the this object - if it was an element - // method, I would probably want to use isOnParent etc. from `this`, and - // thus be required to change the code when I move it to gr-diff host. - // Making it a free function first requires me to catch any references to - // `this` and instead pass those in as parameter, which then allows me - // to move it later without any other changes, which makes the diff - // easier to read. - /** - * @param {Object} thread - * @param {number} parentIndex - * @param {number} changeNum - * @param {string} path - * @param {string} projectName - */ - window.Gerrit.createThreadElement = function( - thread, parentIndex, changeNum, path, projectName) { - const threadEl = document.createElement('gr-diff-comment-thread'); - threadEl.comments = thread.comments; - threadEl.commentSide = thread.commentSide; - threadEl.isOnParent = !!thread.isOnParent; - threadEl.parentIndex = parentIndex; - threadEl.changeNum = changeNum; - threadEl.patchNum = thread.patchNum; - threadEl.lineNum = thread.lineNum; - const rootIdChangedListener = changeEvent => { - thread.rootId = changeEvent.detail.value; - }; - threadEl.addEventListener('root-id-changed', rootIdChangedListener); - threadEl.path = path; - threadEl.projectName = projectName; - threadEl.range = thread.range; - const threadDiscardListener = e => { - const threadEl = /** @type {!Node} */ (e.currentTarget); - const parent = Polymer.dom(threadEl).parentNode; - threadEl.removeEventListener('root-id-changed', rootIdChangedListener); - threadEl.removeEventListener('thread-discard', threadDiscardListener); - Polymer.dom(parent).removeChild(threadEl); - }; - threadEl.addEventListener('thread-discard', threadDiscardListener); - return threadEl; - }; - Polymer({ is: 'gr-diff-comment-thread-group', diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html index d1150ed7cf..8219d1b972 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html @@ -36,38 +36,14 @@ limitations under the License. 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 028e44c225..d335e7a833 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 @@ -20,7 +20,6 @@ limitations under the License. - 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 2b91060e54..5e4a3fd701 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 @@ -599,15 +599,36 @@ }, _createThreadElement(thread) { - const threadEl = Gerrit.createThreadElement( - thread, this._parentIndex, this.changeNum, this.path, - this.projectName); + const threadEl = document.createElement('gr-diff-comment-thread'); threadEl.className = 'comment-thread'; - threadEl.addEventListener('thread-discard', e => { + threadEl.comments = thread.comments; + threadEl.commentSide = thread.commentSide; + threadEl.isOnParent = !!thread.isOnParent; + threadEl.parentIndex = this._parentIndex; + threadEl.changeNum = this.changeNum; + threadEl.patchNum = thread.patchNum; + threadEl.lineNum = thread.lineNum; + const rootIdChangedListener = changeEvent => { + thread.rootId = changeEvent.detail.value; + }; + threadEl.addEventListener('root-id-changed', rootIdChangedListener); + threadEl.path = this.path; + threadEl.projectName = this.projectName; + threadEl.range = thread.range; + const threadDiscardListener = e => { + const threadEl = /** @type {!Node} */ (e.currentTarget); + + const parent = Polymer.dom(threadEl).parentNode; + Polymer.dom(parent).removeChild(threadEl); + const i = this._threadEls.findIndex( threadEl => threadEl.rootId == e.detail.rootId); this._threadEls.splice(i, 1); - }); + + threadEl.removeEventListener('root-id-changed', rootIdChangedListener); + threadEl.removeEventListener('thread-discard', threadDiscardListener); + }; + threadEl.addEventListener('thread-discard', threadDiscardListener); return threadEl; }, 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 790a71f513..c7ee1c2811 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 @@ -55,6 +55,32 @@ limitations under the License. sandbox.restore(); }); + test('thread-discard handling', () => { + const threads = [ + {comments: [{id: 4711}]}, + {comments: [{id: 42}]}, + ]; + element._parentIndex = 1; + element.changeNum = '2'; + element.path = 'some/path'; + element.projectName = 'Some project'; + const threadEls = threads.map( + thread => element._createThreadElement(thread)); + assert.equal(threadEls.length, 2); + assert.equal(threadEls[0].rootId, 4711); + assert.equal(threadEls[1].rootId, 42); + for (const threadEl of threadEls) { + Polymer.dom(element).appendChild(threadEl); + } + + threadEls[0].dispatchEvent( + new CustomEvent('thread-discard', {detail: {rootId: 4711}})); + const attachedThreads = element.queryAllEffectiveChildren( + 'gr-diff-comment-thread'); + assert.equal(attachedThreads.length, 1); + assert.equal(attachedThreads[0].rootId, 42); + }); + test('reload() cancels before network resolves', () => { const cancelStub = sandbox.stub(element.$.diff, 'cancel'); diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html index e58540110a..b2584557f8 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html @@ -20,6 +20,7 @@ limitations under the License. + From ec1b351a13ca35b9a65ce788c11ef2f84950fcbf Mon Sep 17 00:00:00 2001 From: Ole Rehmsen Date: Thu, 8 Nov 2018 09:12:48 +0100 Subject: [PATCH 3/3] Replace gr-diff-comment-thread-group with div This element is now nothing but an empty container, so I think it's better to just use a div. Change-Id: I58da47854d2c50608f9e217e7a12e30aad9aca98 --- .../diff/gr-diff-builder/gr-diff-builder.js | 4 +- .../gr-diff-comment-thread-group.html | 39 --------------- .../gr-diff-comment-thread-group.js | 26 ---------- .../gr-diff-comment-thread-group_test.html | 49 ------------------- .../app/elements/diff/gr-diff/gr-diff.html | 5 ++ .../app/elements/diff/gr-diff/gr-diff.js | 5 +- .../elements/diff/gr-diff/gr-diff_test.html | 3 +- polygerrit-ui/app/test/index.html | 1 - 8 files changed, 11 insertions(+), 121 deletions(-) delete mode 100644 polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html delete mode 100644 polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js delete mode 100644 polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js index bfd2125df1..6ea48ac182 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js @@ -388,8 +388,8 @@ return null; } - const threadGroupEl = - document.createElement('gr-diff-comment-thread-group'); + const threadGroupEl = document.createElement('div'); + threadGroupEl.className = 'thread-group'; for (const threadEl of threadElsForGroup) { Polymer.dom(threadGroupEl).appendChild(threadEl); } diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html deleted file mode 100644 index 1f77c0d24d..0000000000 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html +++ /dev/null @@ -1,39 +0,0 @@ - - - - - - - - - - diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js deleted file mode 100644 index 03218267c7..0000000000 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js +++ /dev/null @@ -1,26 +0,0 @@ -/** - * @license - * Copyright (C) 2017 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -(function() { - 'use strict'; - - Polymer({ - is: 'gr-diff-comment-thread-group', - - properties: { - }, - }); -})(); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html deleted file mode 100644 index 8219d1b972..0000000000 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html +++ /dev/null @@ -1,49 +0,0 @@ - - - - -gr-diff-comment-thread-group - - - - - - - - - - - - - - - diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html index b2584557f8..80ca01dd27 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html @@ -36,6 +36,11 @@ limitations under the License. :host(.no-left) .sideBySide ::content .right:not([data-value]) + td { display: none; } + ::slotted(*) .thread-group { + display: block; + max-width: var(--content-width, 80ch); + white-space: normal; + } .diffContainer { display: flex; font-family: var(--monospace-font-family); 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 557a652e8d..76a62b8b64 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js @@ -365,7 +365,7 @@ }, _getThreadGroupForLine(contentEl) { - return contentEl.querySelector('gr-diff-comment-thread-group'); + return contentEl.querySelector('.thread-group'); }, /** @@ -378,7 +378,8 @@ // Check if thread group exists. let threadGroupEl = this._getThreadGroupForLine(contentEl); if (!threadGroupEl) { - threadGroupEl = document.createElement('gr-diff-comment-thread-group'); + threadGroupEl = document.createElement('div'); + threadGroupEl.className = 'thread-group'; contentEl.appendChild(threadGroupEl); } return threadGroupEl; 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 316990d0dc..4befd2f320 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 @@ -315,8 +315,7 @@ limitations under the License. // The new thread group can be fetched. assert.isOk(element._getThreadGroupForLine(contentEl)); - assert.equal(contentEl.querySelectorAll( - 'gr-diff-comment-thread-group').length, 1); + assert.equal(contentEl.querySelectorAll('.thread-group').length, 1); }); suite('image diffs', () => { diff --git a/polygerrit-ui/app/test/index.html b/polygerrit-ui/app/test/index.html index 5b9ae1576e..fa44a74817 100644 --- a/polygerrit-ui/app/test/index.html +++ b/polygerrit-ui/app/test/index.html @@ -103,7 +103,6 @@ limitations under the License. 'core/gr-smart-search/gr-smart-search_test.html', 'diff/gr-comment-api/gr-comment-api_test.html', 'diff/gr-diff-builder/gr-diff-builder_test.html', - 'diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html', 'diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html', 'diff/gr-diff-comment/gr-diff-comment_test.html', 'diff/gr-diff-cursor/gr-diff-cursor_test.html',