Merge changes Ibbbb3e2b,Ia38e0706,I0be589fb
* changes:
Split thread group creation out of thread creation
Remove range property from thread group
Remove unused code from thread group element
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
index ae45c93..23d0a58 100644
--- 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
@@ -24,7 +24,6 @@
changeNum: String,
projectName: String,
patchForNewThreads: String,
- range: Object,
isOnParent: {
type: Boolean,
value: false,
@@ -105,29 +104,5 @@
a.endLine === b.endLine &&
a.endChar === b.endChar;
},
-
- _sortByDate(threadGroups) {
- if (!threadGroups.length) { return; }
- return threadGroups.sort((a, b) => {
- // If a comment is a draft, it doesn't have a start_datetime yet.
- // Assume it is newer than the comment it is being compared to.
- if (!a.start_datetime) {
- return 1;
- }
- if (!b.start_datetime) {
- return -1;
- }
- return util.parseDate(a.start_datetime) -
- util.parseDate(b.start_datetime);
- });
- },
-
- _calculateLocationRange(range, comment) {
- return 'range-' + range.start_line + '-' +
- range.start_character + '-' +
- range.end_line + '-' +
- range.end_character + '-' +
- comment.__commentSide;
- },
});
})();
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
index 1fb8136..8897ea9 100644
--- 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
@@ -122,76 +122,6 @@
assert.deepEqual(element.getThread('left', range).comments.length, 1);
});
- test('_sortByDate', () => {
- let threadGroups = [
- {
- start_datetime: '2015-12-23 15:00:20.396000000',
- comments: [],
- locationRange: 'line',
- },
- {
- start_datetime: '2015-12-22 15:00:10.396000000',
- comments: [],
- locationRange: 'range-1-1-1-2',
- },
- ];
-
- let expectedResult = [
- {
- start_datetime: '2015-12-22 15:00:10.396000000',
- comments: [],
- locationRange: 'range-1-1-1-2',
- }, {
- start_datetime: '2015-12-23 15:00:20.396000000',
- comments: [],
- locationRange: 'line',
- },
- ];
-
- assert.deepEqual(element._sortByDate(threadGroups), expectedResult);
-
- // When a comment doesn't have a date, the one without the date should be
- // last.
- threadGroups = [
- {
- start_datetime: '2015-12-23 15:00:20.396000000',
- comments: [],
- locationRange: 'line',
- },
- {
- comments: [],
- locationRange: 'range-1-1-1-2',
- },
- ];
-
- expectedResult = [
- {
- start_datetime: '2015-12-23 15:00:20.396000000',
- comments: [],
- locationRange: 'line',
- },
- {
- comments: [],
- locationRange: 'range-1-1-1-2',
- },
- ];
-
- assert.deepEqual(element._sortByDate(threadGroups), expectedResult);
- });
-
- test('_calculateLocationRange', () => {
- const comment = {__commentSide: 'left'};
- const range = {
- start_line: 1,
- start_character: 2,
- end_line: 3,
- end_character: 4,
- };
- assert.equal(
- element._calculateLocationRange(range, comment),
- 'range-1-2-3-4-left');
- });
-
test('addNewThread', () => {
const locationRange = 'range-1-2-3-4';
element._threads = [{locationRange: 'line'}];
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 3ae8806..003e024 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -372,41 +372,26 @@
const patchNum = this._getPatchNumByLineAndContent(lineEl, contentEl);
const isOnParent =
this._getIsParentCommentByLineAndContent(lineEl, contentEl);
- const threadEl = this._getOrCreateThread(contentEl, patchNum,
- side, isOnParent, opt_range);
+ const threadGroupEl = this._getOrCreateThreadGroup(contentEl, patchNum,
+ side, isOnParent);
+ const threadEl = this._getOrCreateThread(threadGroupEl, side, opt_range);
threadEl.addOrEditDraft(opt_lineNum, opt_range);
},
- /**
- * Fetch the thread group at the given range, or the range-less thread
- * on the line if no range is provided.
- *
- * @param {!Object} threadGroupEl
- * @param {string} commentSide
- * @param {!Object=} opt_range
- * @return {!Object}
- */
- _getThread(threadGroupEl, commentSide, opt_range) {
- return threadGroupEl.getThread(commentSide, opt_range);
- },
-
_getThreadGroupForLine(contentEl) {
return contentEl.querySelector('gr-diff-comment-thread-group');
},
/**
- * Gets or creates a comment thread for a specific spot on a diff.
- * May include a range, if the comment is a range comment.
- *
+ * Gets or creates a comment thread group for a specific line and side on a
+ * diff.
* @param {!Object} contentEl
* @param {number} patchNum
* @param {string} commentSide
* @param {boolean} isOnParent
- * @param {!Object=} opt_range
* @return {!Object}
*/
- _getOrCreateThread(contentEl, patchNum, commentSide,
- isOnParent, opt_range) {
+ _getOrCreateThreadGroup(contentEl, patchNum, commentSide, isOnParent) {
// Check if thread group exists.
let threadGroupEl = this._getThreadGroupForLine(contentEl);
if (!threadGroupEl) {
@@ -414,13 +399,25 @@
commentSide);
contentEl.appendChild(threadGroupEl);
}
+ return threadGroupEl;
+ },
- let threadEl = this._getThread(threadGroupEl, commentSide, opt_range);
+ /**
+ * Gets or creates a comment thread from a specific thread group.
+ * May include a range, if the comment is a range comment.
+ *
+ * @param {!Object} threadGroupEl
+ * @param {string} commentSide
+ * @param {!Object=} range
+ * @return {!Object}
+ */
+ _getOrCreateThread(threadGroupEl, commentSide, range=undefined) {
+ let threadEl = threadGroupEl.getThread(commentSide, range);
if (!threadEl) {
- threadGroupEl.addNewThread(commentSide, opt_range);
+ threadGroupEl.addNewThread(commentSide, range);
Polymer.dom.flush();
- threadEl = this._getThread(threadGroupEl, commentSide, opt_range);
+ threadEl = threadGroupEl.getThread(commentSide, range);
}
return threadEl;
},
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 43f90bf..aa28713 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
@@ -318,8 +318,12 @@
assert.isNotOk(element._getThreadGroupForLine(contentEl));
// A thread group gets created.
- assert.isOk(element._getOrCreateThread(contentEl,
- patchNum, commentSide, side));
+ const threadGroupEl = element._getOrCreateThreadGroup(contentEl,
+ patchNum, commentSide, side);
+ assert.isOk(threadGroupEl);
+
+ assert.isOk(element._getOrCreateThread(threadGroupEl,
+ commentSide, side));
// Try to fetch a thread with a different range.
range = {
@@ -330,7 +334,7 @@
};
assert.isOk(element._getOrCreateThread(
- contentEl, patchNum, commentSide, side, range));
+ threadGroupEl, commentSide, range));
// The new thread group can be fetched.
assert.isOk(element._getThreadGroupForLine(contentEl));
