Merge "Correct javadoc @link to avoid warning in bazel build //..."
diff --git a/java/com/google/gerrit/server/project/CreateProjectArgs.java b/java/com/google/gerrit/server/project/CreateProjectArgs.java
index a68bd84..df31c19 100644
--- a/java/com/google/gerrit/server/project/CreateProjectArgs.java
+++ b/java/com/google/gerrit/server/project/CreateProjectArgs.java
@@ -49,6 +49,7 @@
enableSignedPush = InheritableBoolean.INHERIT;
requireSignedPush = InheritableBoolean.INHERIT;
submitType = SubmitType.MERGE_IF_NECESSARY;
+ rejectEmptyCommit = InheritableBoolean.INHERIT;
}
public Project.NameKey getProject() {
diff --git a/java/com/google/gerrit/server/restapi/project/CreateProject.java b/java/com/google/gerrit/server/restapi/project/CreateProject.java
index 7773914..60a24d8 100644
--- a/java/com/google/gerrit/server/restapi/project/CreateProject.java
+++ b/java/com/google/gerrit/server/restapi/project/CreateProject.java
@@ -109,7 +109,7 @@
private final MetaDataUpdate.User metaDataUpdateFactory;
private final GitReferenceUpdated referenceUpdated;
private final RepositoryConfig repositoryCfg;
- private final PersonIdent serverIdent;
+ private final Provider<PersonIdent> serverIdent;
private final Provider<IdentifiedUser> identifiedUser;
private final Provider<PutConfig> putConfig;
private final AllProjectsName allProjects;
@@ -131,7 +131,7 @@
MetaDataUpdate.User metaDataUpdateFactory,
GitReferenceUpdated referenceUpdated,
RepositoryConfig repositoryCfg,
- @GerritPersonIdent PersonIdent serverIdent,
+ @GerritPersonIdent Provider<PersonIdent> serverIdent,
Provider<IdentifiedUser> identifiedUser,
Provider<PutConfig> putConfig,
AllProjectsName allProjects,
@@ -357,7 +357,7 @@
CommitBuilder cb = new CommitBuilder();
cb.setTreeId(oi.insert(Constants.OBJ_TREE, new byte[] {}));
cb.setAuthor(metaDataUpdateFactory.getUserPersonIdent());
- cb.setCommitter(serverIdent);
+ cb.setCommitter(serverIdent.get());
cb.setMessage("Initial empty repository\n");
ObjectId id = oi.insert(cb);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-binary.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-binary.js
index 11a36b3..a9242be 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-binary.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-binary.js
@@ -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);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-image.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-image.js
index dce66e1..c52a504 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-image.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-image.js
@@ -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;
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
index 90688fc..da085c2 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
@@ -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;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js
index 1c9c1b2..0657ee4 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js
@@ -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;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
index 91cd0cf..33e2a19 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
@@ -16,9 +16,9 @@
-->
<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 @@
return this.queryEffectiveChildren('#diffTable');
},
+ get _commentThreadElements() {
+ return this.queryAllEffectiveChildren('.comment-thread');
+ },
+
observers: [
'_groupsChanged(_groups.splices)',
],
@@ -166,7 +170,8 @@
// 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 @@
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 @@
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);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
index 7b568d1..6ea48ac 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -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 = {
@@ -397,87 +375,26 @@
};
/**
- * @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;
}
- const threadGroupEl =
- document.createElement('gr-diff-comment-thread-group');
+ const threadGroupEl = document.createElement('div');
+ threadGroupEl.className = 'thread-group';
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;
};
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
index bd65703..a855833 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -62,10 +62,6 @@
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 @@
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 @@
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 @@
});
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 @@
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 @@
});
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 @@
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 @@
suite('rendering', () => {
let content;
let outputEl;
+ let comments;
setup(done => {
const prefs = {
@@ -1104,10 +945,10 @@
});
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 @@
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 @@
});
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 @@
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 @@
context: -1,
syntax_highlighting: true,
};
- element.render({left: [], right: []}, prefs);
+ element.render(comments, prefs);
});
test('cancel', () => {
@@ -1213,6 +1054,7 @@
let builder;
let diff;
let prefs;
+ let comments;
setup(done => {
element = fixture('mock-diff');
@@ -1224,12 +1066,12 @@
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 @@
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 @@
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',
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
deleted file mode 100644
index 9ecc2b1..0000000
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
+++ /dev/null
@@ -1,37 +0,0 @@
-<!--
-@license
-Copyright (C) 2017 The Android Open Source Project
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
--->
-
-<link rel="import" href="../../../bower_components/polymer/polymer.html">
-<link rel="import" href="../gr-diff-comment-thread/gr-diff-comment-thread.html">
-<link rel="import" href="../../../styles/shared-styles.html">
-
-<dom-module id="gr-diff-comment-thread-group">
- <template>
- <style include="shared-styles">
- :host {
- display: block;
- max-width: var(--content-width, 80ch);
- white-space: normal;
- }
- gr-diff-comment-thread + gr-diff-comment-thread {
- margin-top: .2em;
- }
- </style>
- <slot></slot>
- </template>
- <script src="gr-diff-comment-thread-group.js"></script>
-</dom-module>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
deleted file mode 100644
index 9eb3fa2..0000000
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
+++ /dev/null
@@ -1,118 +0,0 @@
-/**
- * @license
- * Copyright (C) 2017 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-(function() {
- 'use strict';
-
- window.Gerrit = window.Gerrit || {};
-
- // This method will eventually move to gr-diff-host (so that gr-diff and it's
- // descendants, including gr-diff-comment-thread-group, do not depend on a
- // specific comment thread implementation, but can instead be used with other
- // comment widgets). I cannot move it there yet, because it is still called
- // from this file, and this file cannot depend on gr-diff-host. I decided to
- // make it a function on the global Gerrit namespace, so that
- // 1) I can move the call-side in the next change without moving this code,
- // and thereby reduce the number of moving parts per commit.
- // 2) To already now cut the ties to the this object - if it was an element
- // method, I would probably want to use isOnParent etc. from `this`, and
- // thus be required to change the code when I move it to gr-diff host.
- // Making it a free function first requires me to catch any references to
- // `this` and instead pass those in as parameter, which then allows me
- // to move it later without any other changes, which makes the diff
- // easier to read.
- /**
- * @param {Object} thread
- * @param {number} parentIndex
- * @param {number} changeNum
- * @param {string} path
- * @param {string} projectName
- */
- window.Gerrit.createThreadElement = function(
- thread, parentIndex, changeNum, path, projectName) {
- const threadEl = document.createElement('gr-diff-comment-thread');
- threadEl.comments = thread.comments;
- threadEl.commentSide = thread.commentSide;
- threadEl.isOnParent = thread.isOnParent;
- threadEl.parentIndex = parentIndex;
- threadEl.changeNum = changeNum;
- threadEl.patchNum = thread.patchNum;
- threadEl.lineNum = thread.lineNum;
- const rootIdChangedListener = changeEvent => {
- thread.rootId = changeEvent.detail.value;
- };
- threadEl.addEventListener('root-id-changed', rootIdChangedListener);
- threadEl.path = path;
- threadEl.projectName = projectName;
- threadEl.range = thread.range;
- const threadDiscardListener = e => {
- const threadEl = /** @type {!Node} */ (e.currentTarget);
- const parent = Polymer.dom(threadEl).parentNode;
- threadEl.removeEventListener('root-id-changed', rootIdChangedListener);
- threadEl.removeEventListener('thread-discard', threadDiscardListener);
- Polymer.dom(parent).removeChild(threadEl);
- };
- threadEl.addEventListener('thread-discard', threadDiscardListener);
- return threadEl;
- };
-
- Polymer({
- is: 'gr-diff-comment-thread-group',
-
- 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;
- },
- });
-})();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html
deleted file mode 100644
index 90f50a8..0000000
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html
+++ /dev/null
@@ -1,165 +0,0 @@
-<!DOCTYPE html>
-<!--
-@license
-Copyright (C) 2017 The Android Open Source Project
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
--->
-
-<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
-<title>gr-diff-comment-thread-group</title>
-
-<script src="../../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script>
-<script src="../../../bower_components/web-component-tester/browser.js"></script>
-<link rel="import" href="../../../test/common-test-setup.html"/>
-<script src="../../../scripts/util.js"></script>
-
-<link rel="import" href="gr-diff-comment-thread-group.html">
-
-<script>void(0);</script>
-
-<test-fixture id="basic">
- <template>
- <gr-diff-comment-thread-group></gr-diff-comment-thread-group>
- </template>
-</test-fixture>
-
-<script>
- suite('gr-diff-comment-thread-group tests', () => {
- let element;
- let sandbox;
-
- setup(() => {
- sandbox = sinon.sandbox.create();
- stub('gr-rest-api-interface', {
- getLoggedIn() { return Promise.resolve(false); },
- });
- element = fixture('basic');
- });
-
- teardown(() => {
- 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');
- Polymer.dom(element).appendChild(threadEl);
- }
-
- element.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)));
- });
- });
-</script>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
index 9668a54..f111378 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
@@ -60,7 +60,11 @@
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();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
index a5f5fd9..d335e7a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
@@ -19,6 +19,7 @@
<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/gr-diff.html">
<dom-module id="gr-diff-host">
@@ -46,8 +47,7 @@
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>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
index 68b4616..5e4a3fd 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
@@ -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,117 @@
/** @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 = document.createElement('gr-diff-comment-thread');
+ threadEl.className = 'comment-thread';
+ threadEl.comments = thread.comments;
+ threadEl.commentSide = thread.commentSide;
+ threadEl.isOnParent = !!thread.isOnParent;
+ threadEl.parentIndex = this._parentIndex;
+ threadEl.changeNum = this.changeNum;
+ threadEl.patchNum = thread.patchNum;
+ threadEl.lineNum = thread.lineNum;
+ const rootIdChangedListener = changeEvent => {
+ thread.rootId = changeEvent.detail.value;
+ };
+ threadEl.addEventListener('root-id-changed', rootIdChangedListener);
+ threadEl.path = this.path;
+ threadEl.projectName = this.projectName;
+ threadEl.range = thread.range;
+ const threadDiscardListener = e => {
+ const threadEl = /** @type {!Node} */ (e.currentTarget);
+
+ const parent = Polymer.dom(threadEl).parentNode;
+ Polymer.dom(parent).removeChild(threadEl);
+
+ const i = this._threadEls.findIndex(
+ threadEl => threadEl.rootId == e.detail.rootId);
+ this._threadEls.splice(i, 1);
+
+ threadEl.removeEventListener('root-id-changed', rootIdChangedListener);
+ threadEl.removeEventListener('thread-discard', threadDiscardListener);
+ };
+ threadEl.addEventListener('thread-discard', threadDiscardListener);
+ 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
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
index 7117574..c7ee1c2 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
@@ -46,12 +46,41 @@
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(() => {
sandbox.restore();
});
+ test('thread-discard handling', () => {
+ const threads = [
+ {comments: [{id: 4711}]},
+ {comments: [{id: 42}]},
+ ];
+ element._parentIndex = 1;
+ element.changeNum = '2';
+ element.path = 'some/path';
+ element.projectName = 'Some project';
+ const threadEls = threads.map(
+ thread => element._createThreadElement(thread));
+ 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);
+ }
+
+ threadEls[0].dispatchEvent(
+ new CustomEvent('thread-discard', {detail: {rootId: 4711}}));
+ const attachedThreads = element.queryAllEffectiveChildren(
+ 'gr-diff-comment-thread');
+ assert.equal(attachedThreads.length, 1);
+ assert.equal(attachedThreads[0].rootId, 42);
+ });
+
test('reload() cancels before network resolves', () => {
const cancelStub = sandbox.stub(element.$.diff, 'cancel');
@@ -182,7 +211,11 @@
});
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 +589,10 @@
});
});
- 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 +801,160 @@
});
});
+ 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 +973,9 @@
};
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);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
index 8acbd5d..b3210cc 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
@@ -332,7 +332,6 @@
patch-range="[[_patchRange]]"
path="[[_path]]"
prefs="[[_prefs]]"
- project-config="[[_projectConfig]]"
project-name="[[_change.project]]"
view-mode="[[_diffMode]]"
is-blame-loaded="{{_isBlameLoaded}}"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
index a639d51..8f327aa 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -20,7 +20,6 @@
<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">
@@ -36,6 +35,11 @@
:host(.no-left) .sideBySide ::content .right:not([data-value]) + td {
display: none;
}
+ ::slotted(*) .thread-group {
+ display: block;
+ max-width: var(--content-width, 80ch);
+ white-space: normal;
+ }
.diffContainer {
display: flex;
font-family: var(--monospace-font-family);
@@ -290,9 +294,8 @@
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]]"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index 7e6e9bc..76a62b8 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -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,42 +351,40 @@
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,
},
}));
},
_getThreadGroupForLine(contentEl) {
- return contentEl.querySelector('gr-diff-comment-thread-group');
+ return contentEl.querySelector('.thread-group');
},
/**
* 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.
let threadGroupEl = this._getThreadGroupForLine(contentEl);
if (!threadGroupEl) {
- threadGroupEl = document.createElement('gr-diff-comment-thread-group');
+ threadGroupEl = document.createElement('div');
+ threadGroupEl.className = 'thread-group';
contentEl.appendChild(threadGroupEl);
}
return threadGroupEl;
},
-
/**
* The value to be used for the patch number of new comments created at the
* given line and content elements.
@@ -578,6 +573,7 @@
},
_renderDiffTable() {
+ this._unobserveNodes();
if (!this.prefs) {
this.dispatchEvent(new CustomEvent('render', {bubbles: true}));
return;
@@ -594,6 +590,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 +629,7 @@
},
clearDiffContent() {
+ this._unobserveNodes();
this.$.diffTable.innerHTML = null;
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index 193e7eb..4befd2f 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -315,8 +315,7 @@
// The new thread group can be fetched.
assert.isOk(element._getThreadGroupForLine(contentEl));
- assert.equal(contentEl.querySelectorAll(
- 'gr-diff-comment-thread-group').length, 1);
+ assert.equal(contentEl.querySelectorAll('.thread-group').length, 1);
});
suite('image diffs', () => {
@@ -335,7 +334,11 @@
};
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 +667,7 @@
element.comments = {
left: [],
right: [],
+ meta: {patchRange: undefined},
};
element.prefs = {
context: 10,
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.js b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.js
index dcb428f..3d9d36b 100644
--- a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.js
+++ b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.js
@@ -229,7 +229,7 @@
if (typeof link.url === 'undefined') {
return '';
}
- if (link.target) {
+ if (link.target || !link.url.startsWith('/')) {
return link.url;
}
return this._computeRelativeURL(link.url);
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_test.html b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_test.html
index 456f235..7bb4dce 100644
--- a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_test.html
@@ -73,6 +73,12 @@
test('link URLs', () => {
assert.equal(
+ element._computeLinkURL({url: 'http://example.com/test'}),
+ 'http://example.com/test');
+ assert.equal(
+ element._computeLinkURL({url: 'https://example.com/test'}),
+ 'https://example.com/test');
+ assert.equal(
element._computeLinkURL({url: '/test'}),
'//' + window.location.host + '/test');
assert.equal(
diff --git a/polygerrit-ui/app/test/index.html b/polygerrit-ui/app/test/index.html
index 5b9ae15..fa44a74 100644
--- a/polygerrit-ui/app/test/index.html
+++ b/polygerrit-ui/app/test/index.html
@@ -103,7 +103,6 @@
'core/gr-smart-search/gr-smart-search_test.html',
'diff/gr-comment-api/gr-comment-api_test.html',
'diff/gr-diff-builder/gr-diff-builder_test.html',
- 'diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html',
'diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html',
'diff/gr-diff-comment/gr-diff-comment_test.html',
'diff/gr-diff-cursor/gr-diff-cursor_test.html',
