opendev/gerrit
Code Issues Proposed changes

Merge changes I6b58d8ba,Ifa3853aa

Browse Source 
* changes:
  Extract helpers for isOnParent and patchNum
  Move comment threading to builder
This commit is contained in:
Ole Rehmsen
2018-10-12 05:04:52 +00:00
committed by Gerrit Code Review
parent 7e615ff249 f0b33aa489
commit b0ab2c5224
5 changed files with 320 additions and 287 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -352,6 +352,92 @@
return result;
};
/**
* @param {Array<Object>} comments
* @param {string} patchForNewThreads
*/
GrDiffBuilder.prototype._getThreads = function(comments, patchForNewThreads) {
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,
/**
* Determines what the patchNum of a thread should be. Use patchNum from
* comment if it exists, otherwise the property of the thread group.
* This is needed for switching between side-by-side and unified views
* when there are unsaved drafts.
*/
patchNum: comment.patch_set || patchForNewThreads,
rootId: comment.id || comment.__draftID,
};
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;
}
};
/**
* Returns whether the comments on the given line are on a (merge) parent.
*
* @param {string} firstCommentSide
* @param {{basePatchNum: number, patchNum: number}} patchRange
* @param {GrDiffLine} line The line the comments are on.
* @param {string=} opt_side
* @return {boolean} True iff the comments on the given line are on a (merge)
* parent.
*/
GrDiffBuilder.prototype._determineIsOnParent = function(
firstCommentSide, patchRange, line, opt_side) {
return ((line.type === GrDiffLine.Type.REMOVE ||
opt_side === GrDiffBuilder.Side.LEFT) &&
(patchRange.basePatchNum === 'PARENT' ||
Gerrit.PatchSetBehavior.isMergeParent(
patchRange.basePatchNum))) ||
firstCommentSide === 'PARENT';
};
/**
* @param {GrDiffLine} line
* @param {string=} opt_side
@@ -360,26 +446,19 @@
GrDiffBuilder.prototype._commentThreadGroupForLine = function(
line, opt_side) {
const comments =
this._getCommentsForLine(this._comments, line, opt_side);
this._getCommentsForLine(this._comments, line, opt_side);
if (!comments || comments.length === 0) {
return null;
}
let patchNum = this._comments.meta.patchRange.patchNum;
let isOnParent = comments[0].side === 'PARENT' || false;
if (line.type === GrDiffLine.Type.REMOVE ||
opt_side === GrDiffBuilder.Side.LEFT) {
if (this._comments.meta.patchRange.basePatchNum === 'PARENT' ||
Gerrit.PatchSetBehavior.isMergeParent(
this._comments.meta.patchRange.basePatchNum)) {
isOnParent = true;
} else {
patchNum = this._comments.meta.patchRange.basePatchNum;
}
}
const patchNum = this._determinePatchNumForNewThreads(
this._comments.meta.patchRange, line, opt_side);
const isOnParent = this._determineIsOnParent(
comments[0].side, this._comments.meta.patchRange, line, opt_side);
const threadGroupEl = this._createThreadGroupFn(patchNum, isOnParent,
opt_side);
threadGroupEl.comments = comments;
threadGroupEl.threads = this._getThreads(comments, patchNum);
if (opt_side) {
threadGroupEl.setAttribute('data-side', opt_side);
}

174
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
View File

@@ -84,6 +84,166 @@ limitations under the License.
teardown(() => { sandbox.restore(); });
test('_getThreads', () => {
const patchForNewThreads = 3;
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',
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',
},
];
let expectedThreadGroups = [
{
start_datetime: '2015-12-23 15:00:20.396000000',
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',
}],
rootId: 'sallys_confession',
patchNum: 3,
},
{
start_datetime: '2015-12-20 15:01:20.396000000',
commentSide: 'left',
comments: [
{
id: 'new_draft',
message: 'i do not like either of you',
__commentSide: 'left',
__draft: true,
updated: '2015-12-20 15:01:20.396000000',
},
],
rootId: 'new_draft',
patchNum: 3,
},
];
assert.deepEqual(
builder._getThreads(comments, patchForNewThreads),
expectedThreadGroups);
// Patch num should get inherited from comment rather
comments.push({
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,
},
__commentSide: 'left',
});
expectedThreadGroups = [
{
start_datetime: '2015-12-23 15:00:20.396000000',
commentSide: 'left',
comments: [{
id: 'sallys_confession',
message: 'i like you, jack',
updated: '2015-12-23 15:00:20.396000000',
__commentSide: 'left',
}, {
id: 'jacks_reply',
in_reply_to: 'sallys_confession',
message: 'i like you, too',
updated: '2015-12-24 15:01:20.396000000',
__commentSide: 'left',
}],
patchNum: 3,
rootId: 'sallys_confession',
},
{
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,
},
__commentSide: 'left',
}],
patchNum: 3,
rootId: 'betsys_confession',
range: {
start_line: 1,
start_character: 1,
end_line: 1,
end_character: 2,
},
},
{
start_datetime: '2015-12-20 15:01:20.396000000',
commentSide: 'left',
comments: [
{
id: 'new_draft',
message: 'i do not like either of you',
__commentSide: 'left',
__draft: true,
updated: '2015-12-20 15:01:20.396000000',
},
],
rootId: 'new_draft',
patchNum: 3,
},
];
assert.deepEqual(
builder._getThreads(comments, patchForNewThreads),
expectedThreadGroups);
});
test('multiple comments at same location but not threaded', () => {
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._getThreads(comments, '3').length, 2);
});
test('_createElement classStr applies all classes', () => {
const node = builder._createElement('div', 'test classes');
assert.isTrue(node.classList.contains('gr-diff'));
@@ -312,11 +472,23 @@ limitations under the License.
right: [r5],
};
function threadForComment(c, patchNum) {
return {
commentSide: c.__commentSide,
comments: [c],
patchNum,
rootId: c.id,
start_datetime: c.updated,
};
}
function checkThreadGroupProps(threadGroupEl, patchNum, isOnParent,
comments) {
assert.equal(createThreadGroupFn.lastCall.args[0], patchNum);
assert.equal(createThreadGroupFn.lastCall.args[1], isOnParent);
assert.deepEqual(threadGroupEl.comments, comments);
assert.deepEqual(
threadGroupEl.threads,
comments.map(c => threadForComment(c, patchNum)));
}
let line = new GrDiffLine(GrDiffLine.Type.BOTH);

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

@@ -31,7 +31,7 @@ limitations under the License.
margin-top: .2em;
}
</style>
<template is="dom-repeat" items="[[_threads]]" as="thread">
<template is="dom-repeat" items="[[threads]]" as="thread">
<gr-diff-comment-thread
comments="[[thread.comments]]"
comment-side="[[thread.commentSide]]"

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

@@ -22,10 +22,6 @@
properties: {
changeNum: String,
comments: {
type: Array,
value() { return []; },
},
projectName: String,
patchForNewThreads: String,
range: Object,
@@ -37,16 +33,12 @@
type: Number,
value: null,
},
_threads: {
threads: {
type: Array,
value() { return []; },
},
},
observers: [
'_commentsChanged(comments.*)',
],
get threadEls() {
return Polymer.dom(this.root).querySelectorAll('gr-diff-comment-thread');
},
@@ -58,7 +50,7 @@
* @param {!Object} opt_range
*/
addNewThread(commentSide, opt_range) {
this.push('_threads', {
this.push('threads', {
comments: [],
commentSide,
patchNum: this.patchForNewThreads,
@@ -67,9 +59,9 @@
},
removeThread(rootId) {
for (let i = 0; i < this._threads.length; i++) {
if (this._threads[i].rootId === rootId) {
this.splice('_threads', i, 1);
for (let i = 0; i < this.threads.length; i++) {
if (this.threads[i].rootId === rootId) {
this.splice('threads', i, 1);
return;
}
}
@@ -114,10 +106,6 @@
a.endChar === b.endChar;
},
_commentsChanged() {
this._threads = this._getThreads(this.comments);
},
_sortByDate(threadGroups) {
if (!threadGroups.length) { return; }
return threadGroups.sort((a, b) => {
@@ -141,51 +129,5 @@
range.end_character + '-' +
comment.__commentSide;
},
/**
* Determines what the patchNum of a thread should be. Use patchNum from
* comment if it exists, otherwise the property of the thread group.
* This is needed for switching between side-by-side and unified views when
* there are unsaved drafts.
*/
_getPatchNum(comment) {
return comment.patch_set || this.patchForNewThreads;
},
_getThreads(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: this._getPatchNum(comment),
rootId: comment.id || comment.__draftID,
};
if (comment.range) {
newThread.range = Object.assign({}, comment.range);
}
threads.push(newThread);
}
return threads;
},
});
})();

256
polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html
View File

@@ -51,145 +51,6 @@ limitations under the License.
sandbox.restore();
});
test('_getThreads', () => {
element.patchForNewThreads = 3;
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',
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',
},
];
let expectedThreadGroups = [
{
start_datetime: '2015-12-23 15:00:20.396000000',
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',
}],
rootId: 'sallys_confession',
patchNum: 3,
},
{
start_datetime: '2015-12-20 15:01:20.396000000',
commentSide: 'left',
comments: [
{
id: 'new_draft',
message: 'i do not like either of you',
__commentSide: 'left',
__draft: true,
updated: '2015-12-20 15:01:20.396000000',
},
],
rootId: 'new_draft',
patchNum: 3,
},
];
assert.deepEqual(element._getThreads(comments), expectedThreadGroups);
// Patch num should get inherited from comment rather
comments.push({
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,
},
__commentSide: 'left',
});
expectedThreadGroups = [
{
start_datetime: '2015-12-23 15:00:20.396000000',
commentSide: 'left',
comments: [{
id: 'sallys_confession',
message: 'i like you, jack',
updated: '2015-12-23 15:00:20.396000000',
__commentSide: 'left',
}, {
id: 'jacks_reply',
in_reply_to: 'sallys_confession',
message: 'i like you, too',
updated: '2015-12-24 15:01:20.396000000',
__commentSide: 'left',
}],
patchNum: 3,
rootId: 'sallys_confession',
},
{
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,
},
__commentSide: 'left',
}],
patchNum: 3,
rootId: 'betsys_confession',
range: {
start_line: 1,
start_character: 1,
end_line: 1,
end_character: 2,
},
},
{
start_datetime: '2015-12-20 15:01:20.396000000',
commentSide: 'left',
comments: [
{
id: 'new_draft',
message: 'i do not like either of you',
__commentSide: 'left',
__draft: true,
updated: '2015-12-20 15:01:20.396000000',
},
],
rootId: 'new_draft',
patchNum: 3,
},
];
assert.deepEqual(element._getThreads(comments), expectedThreadGroups);
});
test('getThread', () => {
const range = {
start_line: 1,
@@ -197,37 +58,57 @@ limitations under the License.
end_line: 1,
end_character: 2,
};
element.comments = [
element.threads = [
{
id: 'sallys_confession',
message: 'i like you, jack',
updated: '2015-12-23 15:00:20.396000000',
__commentSide: 'left',
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',
},
],
}, {
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',
}, {
id: 'right_side_comment',
message: 'right side comment',
__commentSide: 'right',
__draft: true,
updated: '2015-12-20 15:01:20.396000000',
}, {
id: 'betsys_confession',
message: 'i like you more, jack',
updated: '2015-12-24 15:00:10.396000000',
rootId: 'betsys_confession',
commentSide: 'left',
range,
__commentSide: 'left',
comments: [
{
id: 'betsys_confession',
message: 'i like you more, jack',
updated: '2015-12-24 15:00:10.396000000',
range,
__commentSide: 'left',
},
],
},
];
@@ -241,24 +122,6 @@ limitations under the License.
assert.deepEqual(element.getThread('left', range).comments.length, 1);
});
test('multiple comments at same location but not threaded', () => {
element.patchForNewThreads = 3;
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._getThreads(comments).length, 2);
});
test('_sortByDate', () => {
let threadGroups = [
{
@@ -329,17 +192,6 @@ limitations under the License.
'range-1-2-3-4-left');
});
test('thread groups are updated when comments change', () => {
const commentsChangedStub = sandbox.stub(element, '_commentsChanged');
element.comments = [];
element.comments.push({
id: 'sallys_confession',
message: 'i like you, jack',
updated: '2015-12-23 15:00:20.396000000',
});
assert(commentsChangedStub.called);
});
test('addNewThread', () => {
const locationRange = 'range-1-2-3-4';
element._threads = [{locationRange: 'line'}];
@@ -347,18 +199,6 @@ limitations under the License.
assert(element._threads.length, 2);
});
test('_getPatchNum', () => {
element.patchForNewThreads = 3;
const comment = {
id: 'sallys_confession',
message: 'i like you, jack',
updated: '2015-12-23 15:00:20.396000000',
};
assert.equal(element._getPatchNum(comment), 3);
comment.patch_set = 4;
assert.equal(element._getPatchNum(comment), 4);
});
test('removeThread', () => {
const locationRange = 'range-1-2-3-4';
element._threads = [