opendev/gerrit
Code Issues Proposed changes

Merge changes Icb5d3ba1,Ia7970122

Browse Source 
* changes:
  Properly remove discarded comments from model in gr-new-diff
  Add intraline highlights to gr-new-diff
This commit is contained in:
Andrew Bonventre
2016-03-25 18:44:33 +00:00
committed by Gerrit Code Review
parent d4dae5c16d 6a9312f7b1
commit cced99ae2d
13 changed files with 374 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html
View File

@@ -36,7 +36,7 @@ limitations under the License.
}
</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 @@ limitations under the License.
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>

8
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
View File

@@ -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);

4
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
View File

@@ -235,7 +235,7 @@ limitations under the License.
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 @@ limitations under the License.
assert.equal(drafts.length, 0);
done();
});
draftEl.fire('discard', null, {bubbles: false});
draftEl.fire('comment-discard', null, {bubbles: false});
});
});
</script>

8
polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
View File

@@ -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.');

2
polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
View File

@@ -205,7 +205,7 @@ limitations under the License.
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();

2
polygerrit-ui/app/elements/diff/gr-diff-side/gr-diff-side.js
View File

@@ -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;

4
polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
View File

@@ -220,7 +220,7 @@ limitations under the License.
// 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 @@ limitations under the License.
done();
});
var commentEl = newThreadEls[1].$$('gr-diff-comment');
commentEl.fire('discard', null, {bubbles: false});
commentEl.fire('comment-discard');
});
});
});

142
polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder.js
View File

@@ -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;

81
polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder_test.html
View File

@@ -431,5 +431,86 @@ limitations under the License.
}
]);
});
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>

1
polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-line.js
View File

@@ -17,6 +17,7 @@
function GrDiffLine(type) {
this.type = type;
this.contextLines = [];
this.highlights = [];
}
GrDiffLine.prototype.beforeNumber = 0;

14
polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.html
View File

@@ -92,7 +92,7 @@ limitations under the License.
}
.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 @@ limitations under the License.
-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;

39
polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff.js
View File

@@ -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;
}
},
});
})();

98
polygerrit-ui/app/elements/diff/gr-new-diff/gr-new-diff_test.html
View File

@@ -142,5 +142,103 @@ limitations under the License.
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>