opendev/gerrit
Code Issues Proposed changes

Move the actual comment element creation to host

Browse Source 
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
This commit is contained in:
Ole Rehmsen
2018-10-12 16:20:57 +02:00
parent b78deaf23a
commit 8c8ce2daba
18 changed files with 512 additions and 553 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

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

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

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

8
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
View File

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

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

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

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

@@ -16,9 +16,9 @@ limitations under the License.
-->
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../../core/gr-reporting/gr-reporting.html">
<link rel="import" href="../gr-diff-comment-thread/gr-diff-comment-thread.html">
<link rel="import" href="../gr-diff-comment-thread-group/gr-diff-comment-thread-group.html">
<link rel="import" href="../../shared/gr-js-api-interface/gr-js-api-interface.html">
<link rel="import" href="../gr-diff-processor/gr-diff-processor.html">
<link rel="import" href="../gr-diff-comment-thread/gr-diff-comment-thread.html">
<link rel="import" href="../gr-ranged-comment-layer/gr-ranged-comment-layer.html">
<link rel="import" href="../gr-syntax-layer/gr-syntax-layer.html">
@@ -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);

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

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

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

@@ -62,10 +62,6 @@ limitations under the License.
let builder;
let sandbox;
const LINE_FEED_HTML = '<span class="style-scope gr-diff br"></span>';
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<expectedThreadEls.length; i++) {
assert.equal(threadEls[i], expectedThreadEls[i]);
}
}
@@ -509,42 +347,42 @@ limitations under the License.
line.beforeNumber = 5;
line.afterNumber = 5;
let threadGroupEl = builder._commentThreadGroupForLine(line);
checkThreadGroupProps(threadGroupEl, '3', [l5, r5]);
checkThreadGroupProps(threadGroupEl, [l5, r5]);
threadGroupEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
checkThreadGroupProps(threadGroupEl, '3', [r5]);
checkThreadGroupProps(threadGroupEl, [r5]);
threadGroupEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
checkThreadGroupProps(threadGroupEl, '3', [l5]);
checkThreadGroupProps(threadGroupEl, [l5]);
builder._comments.meta.patchRange.basePatchNum = '1';
builder._patchRange.basePatchNum = '1';
threadGroupEl = builder._commentThreadGroupForLine(line);
checkThreadGroupProps(threadGroupEl, '3', [l5, r5]);
checkThreadGroupProps(threadGroupEl, [l5, r5]);
threadEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
checkThreadGroupProps(threadEl, '1', [l5]);
checkThreadGroupProps(threadEl, [l5]);
threadGroupEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
checkThreadGroupProps(threadGroupEl, '3', [r5]);
checkThreadGroupProps(threadGroupEl, [r5]);
builder._comments.meta.patchRange.basePatchNum = 'PARENT';
builder._patchRange.basePatchNum = 'PARENT';
line = new GrDiffLine(GrDiffLine.Type.REMOVE);
line.beforeNumber = 5;
line.afterNumber = 5;
threadGroupEl = builder._commentThreadGroupForLine(line);
checkThreadGroupProps(threadGroupEl, '3', [l5, r5]);
checkThreadGroupProps(threadGroupEl, [l5, r5]);
line = new GrDiffLine(GrDiffLine.Type.ADD);
line.beforeNumber = 3;
line.afterNumber = 5;
threadGroupEl = builder._commentThreadGroupForLine(line);
checkThreadGroupProps(threadGroupEl, '3', [l3, r5]);
checkThreadGroupProps(threadGroupEl, [l3, r5]);
});
@@ -578,16 +416,18 @@ limitations under the License.
});
const lineOfInterest = {number: 789, leftSide: true};
assert.deepEqual(element._getKeyLocations(comments, lineOfInterest), {
left: {FILE: true, 123: true, 789: true},
right: {456: true},
});
assert.deepEqual(
element._getKeyLocations(comments, lineOfInterest), {
left: {FILE: true, 123: true, 789: true},
right: {456: true},
});
delete lineOfInterest.leftSide;
assert.deepEqual(element._getKeyLocations(comments, lineOfInterest), {
left: {FILE: true, 123: true},
right: {456: true, 789: true},
});
assert.deepEqual(
element._getKeyLocations(comments, lineOfInterest), {
left: {FILE: true, 123: true},
right: {456: true, 789: true},
});
});
suite('_isTotal', () => {
@@ -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',

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

@@ -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;
}
</style>
<slot></slot>
<div>
<slot class="comment-thread"></slot>
</div>
</template>
<script src="gr-diff-comment-thread-group.js"></script>
</dom-module>

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

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

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

@@ -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);
});
});
</script>

6
polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
View File

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

5
polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
View File

@@ -19,6 +19,8 @@ limitations under the License.
<link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
<link rel="import" href="../../core/gr-reporting/gr-reporting.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../gr-diff-comment-thread/gr-diff-comment-thread.html">
<link rel="import" href="../gr-diff-comment-thread-group/gr-diff-comment-thread-group.html">
<link rel="import" href="../gr-diff/gr-diff.html">
<dom-module id="gr-diff-host">
@@ -46,8 +48,7 @@ limitations under the License.
base-image="[[_baseImage]]"
revision-image=[[_revisionImage]]
blame="[[_blame]]"
diff="[[_diff]]"
parent-index="[[_parentIndex]]"></gr-diff>
diff="[[_diff]]"></gr-diff>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-reporting id="reporting" category="diff"></gr-reporting>
</template>

198
polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
View File

@@ -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<!HTMLElement>}
*/
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<!Object>} comments
* @return {!Array<!Object>} 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

175
polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
View File

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

1
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
View File

@@ -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}}"

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

@@ -20,7 +20,6 @@ limitations under the License.
<link rel="import" href="../../../styles/shared-styles.html">
<link rel="import" href="../../shared/gr-button/gr-button.html">
<link rel="import" href="../gr-diff-builder/gr-diff-builder.html">
<link rel="import" href="../gr-diff-comment-thread/gr-diff-comment-thread.html">
<link rel="import" href="../gr-diff-highlight/gr-diff-highlight.html">
<link rel="import" href="../gr-diff-selection/gr-diff-selection.html">
<link rel="import" href="../gr-syntax-themes/gr-syntax-theme.html">
@@ -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]]">
<slot></slot>
<table
id="diffTable"
class$="[[_diffTableClass]]"

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

@@ -180,6 +180,9 @@
},
_diffLength: Number,
/** @type {?PolymerDomApi.ObserveHandle} */
_nodeObserver: Object,
},
behaviors: [
@@ -191,6 +194,11 @@
'comment-update': '_handleCommentUpdate',
'comment-save': '_handleCommentSave',
'create-range-comment': '_handleCreateRangeComment',
'render-content': '_handleRenderContent',
},
detached() {
this._unobserveNodes();
},
/** Cancel any remaining diff builder rendering work. */
@@ -230,17 +238,6 @@
{bubbles: true}));
},
/** @return {!Array<!HTMLElement>} */
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;
},

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

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