opendev/gerrit
Code Issues Proposed changes

First thread comments, then filter to line

Browse Source 
I am changing the order of these operations in preparation for other
commenting widgets and models where the threads may already be given.
The threading will in the next steps move out of the gr-diff completely,
while the thread grouping needs to stay because it strongly depends on
the diff's viewMode.

This change makes some assumptions:
 * comments belonging to a thread have the same line

The previous code had special handling for unsaved draft comments, but
even before this change I could not get that to work. Basically, unsaved
drafts are stored in local storage, but never added to the comments
object. As such, they will disappear when you switch between unified and
side-by-side mode, and come back when you create a comment in the same
location. I decided not to try to fix this as part of this refactoring,
because I am not 100% sure what the desired behavior is. I did remove
the code that did not have any affect though.

Change-Id: I297291767603c9d82efb969a6cafc36d7f9a210c
This commit is contained in:
Ole Rehmsen
2018-10-12 09:53:32 +02:00
parent b0ab2c5224
commit 13e06bf4fe
3 changed files with 161 additions and 135 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -67,6 +67,16 @@
layer.addListener(this._handleLayerUpdate.bind(this));
}
}
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]);
}
this._threads = this._createThreads(allComments);
}
GrDiffBuilder.GroupType = {
@@ -319,44 +329,51 @@
return button;
};
GrDiffBuilder.prototype._getCommentsForLine = function(comments, line,
opt_side) {
function byLineNum(lineNum) {
return function(c) {
return (c.line === lineNum) ||
(c.line === undefined && lineNum === GrDiffLine.FILE);
};
/**
* @param {!Array<Object>} threads
* @param {!GrDiffLine} line
* @param {!GrDiffBuilder.Side=} side The side (LEFT, RIGHT, BOTH) for which
* to return the threads (default: BOTH).
*/
GrDiffBuilder.prototype._filterThreadsForLine = function(
threads, line, side = GrDiffBuilder.Side.BOTH) {
function matchesLeftLine(thread) {
return thread.commentSide == GrDiffBuilder.Side.LEFT &&
thread.comments[0].line == line.beforeNumber;
}
const leftComments =
comments[GrDiffBuilder.Side.LEFT].filter(byLineNum(line.beforeNumber));
const rightComments =
comments[GrDiffBuilder.Side.RIGHT].filter(byLineNum(line.afterNumber));
leftComments.forEach(c => { c.__commentSide = 'left'; });
rightComments.forEach(c => { c.__commentSide = 'right'; });
let result;
switch (opt_side) {
case GrDiffBuilder.Side.LEFT:
result = leftComments;
break;
case GrDiffBuilder.Side.RIGHT:
result = rightComments;
break;
default:
result = leftComments.concat(rightComments);
break;
function matchesRightLine(thread) {
return thread.commentSide == GrDiffBuilder.Side.RIGHT &&
thread.comments[0].line == line.afterNumber;
}
function matchesFileComment(thread) {
return (side === GrDiffBuilder.Side.BOTH ||
thread.commentSide == side) &&
// line/range comments have 1-based line set, if line is falsy it's
// a file comment
!thread.comments[0].line;
}
return result;
// Select the appropriate matchers for the desired side and line
// If side is BOTH, we want both the left and right matcher.
const matchers = [];
if (side !== GrDiffBuilder.Side.RIGHT) {
matchers.push(matchesLeftLine);
}
if (side !== GrDiffBuilder.Side.LEFT) {
matchers.push(matchesRightLine);
}
if (line.afterNumber === GrDiffLine.FILE ||
line.beforeNumber === GrDiffLine.FILE) {
matchers.push(matchesFileComment);
}
return threads.filter(thread => matchers.find(matcher => matcher(thread)));
};
/**
* @param {Array<Object>} comments
* @param {string} patchForNewThreads
*/
GrDiffBuilder.prototype._getThreads = function(comments, patchForNewThreads) {
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; }
@@ -381,13 +398,7 @@
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,
patchNum: comment.patch_set,
rootId: comment.id || comment.__draftID,
};
if (comment.range) {
@@ -439,28 +450,30 @@
};
/**
* @param {GrDiffLine} line
* @param {string=} opt_side
* @param {!GrDiffLine} line
* @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, opt_side) {
const comments =
this._getCommentsForLine(this._comments, line, opt_side);
if (!comments || comments.length === 0) {
line, side = GrDiffBuilder.Side.BOTH) {
const threads =
this._filterThreadsForLine(this._threads, line, side);
if (!threads || threads.length === 0) {
return null;
}
const patchNum = this._determinePatchNumForNewThreads(
this._comments.meta.patchRange, line, opt_side);
const patchRange = this._comments.meta.patchRange;
const patchNumForNewThread = this._determinePatchNumForNewThreads(
patchRange, line, side);
const isOnParent = this._determineIsOnParent(
comments[0].side, this._comments.meta.patchRange, line, opt_side);
threads[0].side, patchRange, line, side);
const threadGroupEl = this._createThreadGroupFn(patchNum, isOnParent,
opt_side);
threadGroupEl.threads = this._getThreads(comments, patchNum);
if (opt_side) {
threadGroupEl.setAttribute('data-side', opt_side);
const threadGroupEl = this._createThreadGroupFn(
patchNumForNewThread, isOnParent, side);
threadGroupEl.threads = threads;
if (side) {
threadGroupEl.setAttribute('data-side', side);
}
return threadGroupEl;
};

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

@@ -57,6 +57,7 @@ limitations under the License.
<script>
suite('gr-diff-builder tests', () => {
let prefs;
let element;
let builder;
let createThreadGroupFn;
@@ -70,7 +71,7 @@ limitations under the License.
getLoggedIn() { return Promise.resolve(false); },
getProjectConfig() { return Promise.resolve({}); },
});
const prefs = {
prefs = {
line_length: 10,
show_tabs: true,
tab_size: 4,
@@ -84,8 +85,7 @@ limitations under the License.
teardown(() => { sandbox.restore(); });
test('_getThreads', () => {
const patchForNewThreads = 3;
test('_createThreads', () => {
const comments = [
{
id: 'sallys_confession',
@@ -108,7 +108,7 @@ limitations under the License.
},
];
let expectedThreadGroups = [
const expectedThreadGroups = [
{
start_datetime: '2015-12-23 15:00:20.396000000',
commentSide: 'left',
@@ -124,8 +124,8 @@ limitations under the License.
__commentSide: 'left',
in_reply_to: 'sallys_confession',
}],
patchNum: undefined,
rootId: 'sallys_confession',
patchNum: 3,
},
{
start_datetime: '2015-12-20 15:01:20.396000000',
@@ -139,17 +139,18 @@ limitations under the License.
updated: '2015-12-20 15:01:20.396000000',
},
],
patchNum: undefined,
rootId: 'new_draft',
patchNum: 3,
},
];
assert.deepEqual(
builder._getThreads(comments, patchForNewThreads),
builder._createThreads(comments),
expectedThreadGroups);
});
// Patch num should get inherited from comment rather
comments.push({
test('_createThreads inherits patchNum amd range', () => {
const comments = [{
id: 'betsys_confession',
message: 'i like you, jack',
updated: '2015-12-24 15:00:10.396000000',
@@ -159,28 +160,11 @@ limitations under the License.
end_line: 1,
end_character: 2,
},
patch_set: 5,
__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',
@@ -194,9 +178,10 @@ limitations under the License.
end_line: 1,
end_character: 2,
},
patch_set: 5,
__commentSide: 'left',
}],
patchNum: 3,
patchNum: 5,
rootId: 'betsys_confession',
range: {
start_line: 1,
@@ -205,25 +190,10 @@ limitations under the License.
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),
builder._createThreads(comments),
expectedThreadGroups);
});
@@ -241,7 +211,7 @@ limitations under the License.
__commentSide: 'left',
},
];
assert.equal(builder._getThreads(comments, '3').length, 2);
assert.equal(builder._createThreads(comments).length, 2);
});
test('_createElement classStr applies all classes', () => {
@@ -417,37 +387,78 @@ limitations under the License.
}
});
test('comments', () => {
test('_filterThreadsForLine with no threads', () => {
const line = new GrDiffLine(GrDiffLine.Type.BOTH);
line.beforeNumber = 3;
line.afterNumber = 5;
let comments = {left: [], right: []};
assert.deepEqual(builder._getCommentsForLine(comments, line), []);
assert.deepEqual(builder._getCommentsForLine(comments, line,
const threads = [];
assert.deepEqual(
builder._filterThreadsForLine(threads, line), []);
assert.deepEqual(builder._filterThreadsForLine(threads, line,
GrDiffBuilder.Side.LEFT), []);
assert.deepEqual(builder._getCommentsForLine(comments, line,
assert.deepEqual(builder._filterThreadsForLine(threads, line,
GrDiffBuilder.Side.RIGHT), []);
});
comments = {
left: [
{id: 'l3', line: 3},
{id: 'l5', line: 5},
],
right: [
{id: 'r3', line: 3},
{id: 'r5', line: 5},
],
test('_filterThreadsForLine for line comments', () => {
const line = new GrDiffLine(GrDiffLine.Type.BOTH);
line.beforeNumber = 3;
line.afterNumber = 5;
const l3 = {
comments: [{id: 'l3', line: 3}],
range: {end_line: 3},
commentSide: 'left',
};
assert.deepEqual(builder._getCommentsForLine(comments, line),
[{id: 'l3', line: 3, __commentSide: 'left'},
{id: 'r5', line: 5, __commentSide: 'right'}]);
assert.deepEqual(builder._getCommentsForLine(comments, line,
GrDiffBuilder.Side.LEFT), [{id: 'l3', line: 3,
__commentSide: 'left'}]);
assert.deepEqual(builder._getCommentsForLine(comments, line,
GrDiffBuilder.Side.RIGHT), [{id: 'r5', line: 5,
__commentSide: 'right'}]);
const l5 = {
comments: [{id: 'l5', line: 5}],
range: {end_line: 5},
commentSide: 'left',
};
const r3 = {
comments: [{id: 'r3', line: 3}],
range: {end_line: 3},
commentSide: 'right',
};
const r5 = {
comments: [{id: 'r5', line: 5}],
range: {end_line: 5},
commentSide: 'right',
};
const threads = [l3, l5, r3, r5];
assert.deepEqual(builder._filterThreadsForLine(threads, line),
[l3, r5]);
assert.deepEqual(builder._filterThreadsForLine(threads, line,
GrDiffBuilder.Side.LEFT), [l3]);
assert.deepEqual(builder._filterThreadsForLine(threads, line,
GrDiffBuilder.Side.RIGHT), [r5]);
});
test('_filterThreadsForLine for file comments', () => {
const line = new GrDiffLine(GrDiffLine.Type.BOTH);
line.beforeNumber = GrDiffLine.FILE;
line.afterNumber = GrDiffLine.FILE;
const l = {
comments: [{id: 'l', line: undefined}],
commentSide: 'left',
};
const r = {
comments: [{id: 'r', line: undefined}],
commentSide: 'right',
};
const threads = [l, r];
assert.deepEqual(builder._filterThreadsForLine(threads, line),
[l, r]);
assert.deepEqual(builder._filterThreadsForLine(threads, line,
GrDiffBuilder.Side.BOTH), [l, r]);
assert.deepEqual(builder._filterThreadsForLine(threads, line,
GrDiffBuilder.Side.LEFT), [l]);
assert.deepEqual(builder._filterThreadsForLine(threads, line,
GrDiffBuilder.Side.RIGHT), [r]);
});
test('comment thread group creation', () => {
@@ -458,19 +469,20 @@ limitations under the License.
const r5 = {id: 'r5', line: 5, updated: '2016-08-09 00:42:32.000000000',
__commentSide: 'right'};
builder._comments = {
meta: {
changeNum: '42',
patchRange: {
basePatchNum: 'PARENT',
patchNum: '3',
},
path: '/path/to/foo',
projectConfig: {foo: 'bar'},
},
left: [l3, l5],
right: [r5],
};
builder = new GrDiffBuilder(
{content: []}, {
meta: {
changeNum: '42',
patchRange: {
basePatchNum: 'PARENT',
patchNum: '3',
},
path: '/path/to/foo',
projectConfig: {foo: 'bar'},
},
left: [l3, l5],
right: [r5],
}, createThreadGroupFn, prefs);
function threadForComment(c, patchNum) {
return {
@@ -488,7 +500,7 @@ limitations under the License.
assert.equal(createThreadGroupFn.lastCall.args[1], isOnParent);
assert.deepEqual(
threadGroupEl.threads,
comments.map(c => threadForComment(c, patchNum)));
comments.map(c => threadForComment(c, undefined)));
}
let line = new GrDiffLine(GrDiffLine.Type.BOTH);

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

@@ -311,7 +311,8 @@ limitations under the License.
const mock = document.createElement('mock-diff-response');
element.$.diffBuilder._builder = element.$.diffBuilder._getDiffBuilder(
mock.diffResponse, {}, {tab_size: 2, line_length: 80});
mock.diffResponse, {left: [], right: []},
{tab_size: 2, line_length: 80});
// No thread groups.
assert.isNotOk(element._getThreadGroupForLine(contentEl));