Merge changes Icb5d3ba1,Ia7970122 * changes: Properly remove discarded comments from model in gr-new-diff Add intraline highlights to gr-new-diff
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html index 9e8156b..39477bc 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html
@@ -36,7 +36,7 @@ } </style> <div id="container"> - <template id="commentList" is="dom-repeat" items="{{_orderedComments}}" as="comment"> + <template id="commentList" is="dom-repeat" items="[[_orderedComments]]" as="comment"> <gr-diff-comment comment="{{comment}}" change-num="[[changeNum]]" @@ -46,7 +46,7 @@ project-config="[[projectConfig]]" on-height-change="_handleCommentHeightChange" on-reply="_handleCommentReply" - on-discard="_handleCommentDiscard" + on-comment-discard="_handleCommentDiscard" on-done="_handleCommentDone"></gr-diff-comment> </template> </div>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js index b019730..c7bbff2 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
@@ -26,7 +26,7 @@ /** * Fired when the thread should be discarded. * - * @event discard + * @event thread-discard */ properties: { @@ -204,10 +204,6 @@ }, _handleCommentDiscard: function(e) { - // TODO(andybons): In Shadow DOM, the event bubbles up, while in Shady - // DOM, it respects the bubbles property. - // https://github.com/Polymer/polymer/issues/3226 - e.stopPropagation(); var diffCommentEl = Polymer.dom(e).rootTarget; var idx = this._indexOf(diffCommentEl.comment, this.comments); if (idx == -1) { @@ -216,7 +212,7 @@ } this.splice('comments', idx, 1); if (this.comments.length == 0) { - this.fire('discard', null, {bubbles: false}); + this.fire('thread-discard'); return; } this.async(this._heightChanged.bind(this), 1);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html index 2a6c999..82b0faf 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
@@ -235,7 +235,7 @@ var draftEl = Polymer.dom(element.root).querySelectorAll('gr-diff-comment')[1]; assert.ok(draftEl); - draftEl.addEventListener('discard', function() { + draftEl.addEventListener('comment-discard', function() { server.respond(); var drafts = element.comments.filter(function(c) { return c.__draft == true; @@ -243,7 +243,7 @@ assert.equal(drafts.length, 0); done(); }); - draftEl.fire('discard', null, {bubbles: false}); + draftEl.fire('comment-discard', null, {bubbles: false}); }); }); </script>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js index ca0bedb..dcdb881 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
@@ -38,7 +38,7 @@ /** * Fired when this comment is discarded. * - * @event discard + * @event comment-discard */ properties: { @@ -190,7 +190,7 @@ _handleCancel: function(e) { this._preventDefaultAndBlur(e); if (this.comment.message == null || this.comment.message.length == 0) { - this.fire('discard', null, {bubbles: false}); + this.fire('comment-discard'); return; } this._editDraft = this.comment.message; @@ -205,11 +205,11 @@ this.disabled = true; var commentID = this.comment.id; if (!commentID) { - this.fire('discard', null, {bubbles: false}); + this.fire('comment-discard'); return; } this._send('DELETE', this._restEndpoint(commentID)).then(function(req) { - this.fire('discard', null, {bubbles: false}); + this.fire('comment-discard'); }.bind(this)).catch(function(err) { alert('Your draft couldn’t be deleted. Check the console and ' + 'contact the PolyGerrit team for assistance.');
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html index 799dbf2..c95a9a4 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
@@ -205,7 +205,7 @@ assert.isTrue(disabled, 'save button should be disabled.'); var numDiscardEvents = 0; - element.addEventListener('discard', function(e) { + element.addEventListener('comment-discard', function(e) { numDiscardEvents++; if (numDiscardEvents == 3) { done();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-side/gr-diff-side.js b/polygerrit-ui/app/elements/diff/gr-diff-side/gr-diff-side.js index 98afd78..b97d7b7 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-side/gr-diff-side.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-side/gr-diff-side.js
@@ -404,7 +404,7 @@ var threadEl = document.createElement('gr-diff-comment-thread'); threadEl.addEventListener('height-change', this._handleCommentThreadHeightChange.bind(this)); - threadEl.addEventListener('discard', + threadEl.addEventListener('thread-discard', this._handleCommentThreadDiscard.bind(this)); threadEl.setAttribute('data-index', index); threadEl.changeNum = this.changeNum;
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 91f5f3b..e46bbb8 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
@@ -220,7 +220,7 @@ // Discard the right thread and ensure the left comment heights are // back to their original values. - newThreadEls[1].addEventListener('discard', function() { + newThreadEls[1].addEventListener('thread-discard', function() { rightFillerEls = Polymer.dom(element.$.rightDiff.root).querySelectorAll( '[data-index="' + index + '"]'); @@ -236,7 +236,7 @@ done(); }); var commentEl = newThreadEls[1].$$('gr-diff-comment'); - commentEl.fire('discard', null, {bubbles: false}); + commentEl.fire('comment-discard'); }); }); });
diff --git a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder.js index eb8e525..e49a0a3 100644 --- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder.js +++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder.js
@@ -40,6 +40,11 @@ REMOVED: 'a', }; + GrDiffBuilder.Highlights = { + ADDED: 'edit_b', + REMOVED: 'edit_a', + }; + GrDiffBuilder.Side = { LEFT: 'left', RIGHT: 'right', @@ -90,12 +95,25 @@ } if (group[GrDiffBuilder.GroupType.REMOVED] !== undefined) { + var highlights; + if (group[GrDiffBuilder.Highlights.REMOVED] !== undefined) { + highlights = this._normalizeIntralineHighlights( + group[GrDiffBuilder.GroupType.REMOVED], + group[GrDiffBuilder.Highlights.REMOVED]); + } this._appendRemovedLines(group[GrDiffBuilder.GroupType.REMOVED], lines, - lineNums); + lineNums, highlights); } + if (group[GrDiffBuilder.GroupType.ADDED] !== undefined) { + var highlights; + if (group[GrDiffBuilder.Highlights.ADDED] !== undefined) { + highlights = this._normalizeIntralineHighlights( + group[GrDiffBuilder.GroupType.ADDED], + group[GrDiffBuilder.Highlights.ADDED]); + } this._appendAddedLines(group[GrDiffBuilder.GroupType.ADDED], lines, - lineNums); + lineNums, highlights); } groups.push(new GrDiffGroup(GrDiffGroup.Type.DELTA, lines)); } @@ -171,6 +189,66 @@ return result; }; + // The `highlights` array consists of a list of <skip length, mark length> + // pairs, where the skip length is the number of characters between the + // end of the previous edit and the start of this edit, and the mark + // length is the number of edited characters following the skip. The start + // of the edits is from the beginning of the related diff content lines. + // + // Note that the implied newline character at the end of each line is + // included in the length calculation, and thus it is possible for the + // edits to span newlines. + // + // A line highlight object consists of three fields: + // - contentIndex: The index of the diffChunk `content` field (the line + // being referred to). + // - startIndex: Where the highlight should begin. + // - endIndex: (optional) Where the highlight should end. If omitted, the + // highlight is meant to be a continuation onto the next line. + GrDiffBuilder.prototype._normalizeIntralineHighlights = function(content, + highlights) { + var contentIndex = 0; + var idx = 0; + var normalized = []; + for (var i = 0; i < highlights.length; i++) { + var line = content[contentIndex] + '\n'; + var hl = highlights[i]; + var j = 0; + while (j < hl[0]) { + if (idx === line.length) { + idx = 0; + line = content[++contentIndex] + '\n'; + continue; + } + idx++; + j++; + } + var lineHighlight = { + contentIndex: contentIndex, + startIndex: idx, + }; + + j = 0; + while (line && j < hl[1]) { + if (idx === line.length) { + idx = 0; + line = content[++contentIndex] + '\n'; + normalized.push(lineHighlight); + lineHighlight = { + contentIndex: contentIndex, + startIndex: idx, + }; + continue; + } + idx++; + j++; + } + lineHighlight.endIndex = idx; + normalized.push(lineHighlight); + } + return normalized; + }; + GrDiffBuilder.prototype._insertContextGroups = function(groups, lines, hiddenRange) { var linesBeforeCtx = lines.slice(0, hiddenRange[0]); @@ -201,21 +279,32 @@ } }; - GrDiffBuilder.prototype._appendRemovedLines = function(rows, lines, - lineNums) { + GrDiffBuilder.prototype._appendRemovedLines = function(rows, lines, lineNums, + opt_highlights) { for (var i = 0; i < rows.length; i++) { var line = new GrDiffLine(GrDiffLine.Type.REMOVE); line.text = rows[i]; line.beforeNumber = ++lineNums.left; + if (opt_highlights) { + line.highlights = opt_highlights.filter(function(hl) { + return hl.contentIndex === i; + }); + } lines.push(line); } }; - GrDiffBuilder.prototype._appendAddedLines = function(rows, lines, lineNums) { + GrDiffBuilder.prototype._appendAddedLines = function(rows, lines, lineNums, + opt_highlights) { for (var i = 0; i < rows.length; i++) { var line = new GrDiffLine(GrDiffLine.Type.ADD); line.text = rows[i]; line.afterNumber = ++lineNums.right; + if (opt_highlights) { + line.highlights = opt_highlights.filter(function(hl) { + return hl.contentIndex === i; + }); + } lines.push(line); } }; @@ -334,6 +423,14 @@ td.classList.add(line.type); var text = line.text; var html = util.escapeHTML(text); + + td.classList.add(line.highlights.length > 0 ? + 'lightHighlight' : 'darkHighlight'); + + if (line.highlights.length > 0) { + html = this._addIntralineHighlights(text, html, line.highlights); + } + if (text.length > this._prefs.line_length) { html = this._addNewlines(text, html); } @@ -410,6 +507,41 @@ return html.replace(GrDiffBuilder.TAB_REGEX, htmlStr); }; + GrDiffBuilder.prototype._addIntralineHighlights = function(content, html, + highlights) { + var START_TAG = '<hl class="style-scope gr-new-diff">'; + var END_TAG = '</hl>'; + + for (var i = 0; i < highlights.length; i++) { + var hl = highlights[i]; + + var htmlStartIndex = 0; + // Find the index of the HTML string to insert the start tag. + for (var j = 0; j < hl.startIndex; j++) { + htmlStartIndex = this._advanceChar(html, htmlStartIndex); + } + + var htmlEndIndex = 0; + if (hl.endIndex !== undefined) { + for (var j = 0; j < hl.endIndex; j++) { + htmlEndIndex = this._advanceChar(html, htmlEndIndex); + } + } else { + // If endIndex isn't present, continue to the end of the line. + htmlEndIndex = html.length; + } + // The start and end indices could be the same if a highlight is meant + // to start at the end of a line and continue onto the next one. + // Ignore it. + if (htmlStartIndex !== htmlEndIndex) { + html = html.slice(0, htmlStartIndex) + START_TAG + + html.slice(htmlStartIndex, htmlEndIndex) + END_TAG + + html.slice(htmlEndIndex); + } + } + return html; + }; + GrDiffBuilder.prototype._getTabWrapper = function(tabSize, showTabs) { // Force this to be a number to prevent arbitrary injection. tabSize = +tabSize;
diff --git a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder_test.html index 04aec1e..2e30999 100644 --- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder_test.html +++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder_test.html
@@ -431,5 +431,86 @@ } ]); }); + + test('intraline normalization', function() { + // The content and highlights are in the format returned by the Gerrit + // REST API. + var content = [ + ' <section class="summary">', + ' <gr-linked-text content="' + + '[[_computeCurrentRevisionMessage(change)]]"></gr-linked-text>', + ' </section>', + ]; + var highlights = [ + [31, 34], [42, 26] + ]; + var results = GrDiffBuilder.prototype._normalizeIntralineHighlights( + content, highlights); + assert.deepEqual(results, [ + { + contentIndex: 0, + startIndex: 31, + }, + { + contentIndex: 1, + startIndex: 0, + endIndex: 33, + }, + { + contentIndex: 1, + startIndex: 75, + }, + { + contentIndex: 2, + startIndex: 0, + endIndex: 6, + } + ]); + + content = [ + ' this._path = value.path;', + '', + ' // When navigating away from the page, there is a possibility that the', + ' // patch number is no longer a part of the URL (say when navigating to', + ' // the top-level change info view) and therefore undefined in `params`.', + ' if (!this._patchRange.patchNum) {', + ]; + highlights = [ + [14, 17], + [11, 70], + [12, 67], + [12, 67], + [14, 29], + ]; + results = GrDiffBuilder.prototype._normalizeIntralineHighlights(content, + highlights); + assert.deepEqual(results, [ + { + contentIndex: 0, + startIndex: 14, + endIndex: 31, + }, + { + contentIndex: 2, + startIndex: 8, + endIndex: 78, + }, + { + contentIndex: 3, + startIndex: 11, + endIndex: 78, + }, + { + contentIndex: 4, + startIndex: 11, + endIndex: 78, + }, + { + contentIndex: 5, + startIndex: 12, + endIndex: 41, + } + ]); + }); }); </script>
diff --git a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-line.js b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-line.js index 9807f1d..ea00a3d 100644 --- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-line.js +++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-line.js
@@ -17,6 +17,7 @@ function GrDiffLine(type) { this.type = type; this.contextLines = []; + this.highlights = []; } GrDiffLine.prototype.beforeNumber = 0;
diff --git a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.html b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.html index 7fb4d3f..352092c 100644 --- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.html +++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.html
@@ -92,7 +92,7 @@ } .content { overflow: hidden; - width: var(--content-width, 80ch); + min-width: var(--content-width, 80ch); } .content.left { -webkit-user-select: var(--left-user-select, text); @@ -106,12 +106,20 @@ -ms-user-select: var(--right-user-select, text); user-select: var(--right-user-select, text); } - .content.add { + .content.add hl, + .content.add.darkHighlight { background-color: var(--dark-add-highlight-color); } - .content.remove { + .content.add.lightHighlight { + background-color: var(--light-add-highlight-color); + } + .content.remove hl, + .content.remove.darkHighlight { background-color: var(--dark-remove-highlight-color); } + .content.remove.lightHighlight { + background-color: var(--light-remove.highlight-color); + } .contextControl { color: #849; background-color: #fef;
diff --git a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.js b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.js index 1a24596..aafea43 100644 --- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.js +++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.js
@@ -82,6 +82,11 @@ this._getLoggedIn().then(function(loggedIn) { this._loggedIn = loggedIn; }.bind(this)); + + this.addEventListener('thread-discard', + this._handleThreadDiscard.bind(this)); + this.addEventListener('comment-discard', + this._handleCommentDiscard.bind(this)); }, reload: function() { @@ -277,20 +282,42 @@ } threadEl = this._builder.createCommentThread(this.changeNum, patchNum, this.path, side, this.projectConfig); - // TODO(andybons): Remove once migration is made to gr-new-diff. - threadEl.addEventListener('discard', - this._handleThreadDiscard.bind(this)); contentEl.appendChild(threadEl); } threadEl.addDraft(opt_lineNum); }, _handleThreadDiscard: function(e) { - e.stopPropagation(); var el = Polymer.dom(e).rootTarget; el.parentNode.removeChild(el); }, + _handleCommentDiscard: function(e) { + var comment = Polymer.dom(e).rootTarget.comment; + this._removeComment(comment); + }, + + _removeComment: function(comment) { + if (!comment.id) { return; } + this._removeCommentFromSide(comment, DiffSide.LEFT) || + this._removeCommentFromSide(comment, DiffSide.RIGHT); + }, + + _removeCommentFromSide: function(comment, side) { + var idx = -1; + for (var i = 0; i < this._comments[side].length; i++) { + if (this._comments[side][i].id === comment.id) { + idx = i; + break; + } + } + if (idx !== -1) { + this.splice('_comments.' + side, idx, 1); + return true; + } + return false; + }, + _handleMouseDown: function(e) { var el = Polymer.dom(e).rootTarget; var side; @@ -480,12 +507,10 @@ }, _projectConfigChanged: function(projectConfig) { - var threadEls = - Polymer.dom(this.root).querySelectorAll('gr-diff-comment-thread'); + var threadEls = this._getCommentThreads(); for (var i = 0; i < threadEls.length; i++) { threadEls[i].projectConfig = projectConfig; } }, - }); })();
diff --git a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff_test.html b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff_test.html index eaa0618..b015df4 100644 --- a/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff_test.html +++ b/polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff_test.html
@@ -142,5 +142,103 @@ done(); }); }); + + test('remove comment', function() { + element._comments = { + meta: { + changeNum: '42', + patchRange: { + basePatchNum: 'PARENT', + patchNum: 3, + }, + path: '/path/to/foo', + projectConfig: {foo: 'bar'}, + }, + left: [ + {id: 'bc1'}, + {id: 'bc2'}, + {id: 'bd1', __draft: true}, + {id: 'bd2', __draft: true}, + ], + right: [ + {id: 'c1'}, + {id: 'c2'}, + {id: 'd1', __draft: true}, + {id: 'd2', __draft: true}, + ], + }; + + element._removeComment({}); + assert.deepEqual(element._comments, { + meta: { + changeNum: '42', + patchRange: { + basePatchNum: 'PARENT', + patchNum: 3, + }, + path: '/path/to/foo', + projectConfig: {foo: 'bar'}, + }, + left: [ + {id: 'bc1'}, + {id: 'bc2'}, + {id: 'bd1', __draft: true}, + {id: 'bd2', __draft: true}, + ], + right: [ + {id: 'c1'}, + {id: 'c2'}, + {id: 'd1', __draft: true}, + {id: 'd2', __draft: true}, + ], + }); + + element._removeComment({id: 'bc2'}); + assert.deepEqual(element._comments, { + meta: { + changeNum: '42', + patchRange: { + basePatchNum: 'PARENT', + patchNum: 3, + }, + path: '/path/to/foo', + projectConfig: {foo: 'bar'}, + }, + left: [ + {id: 'bc1'}, + {id: 'bd1', __draft: true}, + {id: 'bd2', __draft: true}, + ], + right: [ + {id: 'c1'}, + {id: 'c2'}, + {id: 'd1', __draft: true}, + {id: 'd2', __draft: true}, + ], + }); + + element._removeComment({id: 'd2'}); + assert.deepEqual(element._comments, { + meta: { + changeNum: '42', + patchRange: { + basePatchNum: 'PARENT', + patchNum: 3, + }, + path: '/path/to/foo', + projectConfig: {foo: 'bar'}, + }, + left: [ + {id: 'bc1'}, + {id: 'bd1', __draft: true}, + {id: 'bd2', __draft: true}, + ], + right: [ + {id: 'c1'}, + {id: 'c2'}, + {id: 'd1', __draft: true}, + ], + }); + }); }); </script>