Imperative named Slots
The idea is to instead of imperatively appending slotted comments to
threadGroupEls (which results in them being removed from the light DOM
of the gr-diff element and thus causing all kinds of tracking
difficulties), to slot them into a named slot in the appropriate line.
Change-Id: I17e99b236240baab581f8c266bed3d400a302408
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 5839141..0611a12 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
@@ -63,6 +63,12 @@
a.end_character === b.end_character;
}
+ /** @enum {string} */
+ Gerrit.DiffSide = {
+ LEFT: 'left',
+ RIGHT: 'right',
+ };
+
/**
* Wrapper around gr-diff.
*
@@ -196,11 +202,6 @@
type: Number,
computed: '_computeParentIndex(patchRange.*)',
},
-
- _threadEls: {
- type: Array,
- value: () => [],
- },
},
behaviors: [
@@ -344,7 +345,7 @@
* @return {!Array<!HTMLElement>}
*/
getThreadEls() {
- return this._threadEls;
+ return Polymer.dom(this.$.diff).querySelectorAll('.comment-thread');
},
/** @param {HTMLElement} el */
@@ -471,7 +472,6 @@
return isImageDiff(diff);
},
-
_commentsChanged(newComments) {
const allComments = [];
for (const side of [GrDiffBuilder.Side.LEFT, GrDiffBuilder.Side.RIGHT]) {
@@ -590,21 +590,20 @@
},
_attachThreadElement(threadEl) {
- this._threadEls.push(threadEl);
Polymer.dom(this.$.diff).appendChild(threadEl);
},
_clearThreads() {
- for (const threadEl of this._threadEls) {
+ for (const threadEl of this.getThreadEls()) {
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.slot = `${thread.commentSide}-${thread.lineNum}`;
threadEl.comments = thread.comments;
threadEl.commentSide = thread.commentSide;
threadEl.isOnParent = !!thread.isOnParent;
@@ -625,10 +624,6 @@
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);
};
@@ -660,12 +655,57 @@
return rangesEqual(threadRange, range);
}
- const filteredThreadEls = Gerrit.filterThreadElsForLocation(
- this._threadEls, line, commentSide).filter(matchesRange);
+ const filteredThreadEls = this._filterThreadElsForLocation(
+ this.getThreadEls(), line, commentSide).filter(matchesRange);
return filteredThreadEls.length ? filteredThreadEls[0] : null;
},
/**
+ * @param {!Array<!HTMLElement>} threadEls
+ * @param {!{beforeNumber: (number|string|undefined|null),
+ * afterNumber: (number|string|undefined|null)}}
+ * lineInfo
+ * @param {!Gerrit.DiffSide=} side The side (LEFT, RIGHT) for
+ * which to return the threads.
+ * @return {!Array<!HTMLElement>} The thread elements matching the given
+ * location.
+ */
+ _filterThreadElsForLocation(threadEls, lineInfo, side) {
+ function matchesLeftLine(threadEl) {
+ return threadEl.getAttribute('comment-side') ==
+ Gerrit.DiffSide.LEFT &&
+ threadEl.getAttribute('line-num') == lineInfo.beforeNumber;
+ }
+ function matchesRightLine(threadEl) {
+ return threadEl.getAttribute('comment-side') ==
+ Gerrit.DiffSide.RIGHT &&
+ threadEl.getAttribute('line-num') == lineInfo.afterNumber;
+ }
+ function matchesFileComment(threadEl) {
+ return threadEl.getAttribute('comment-side') == side &&
+ // line/range comments have 1-based line set, if line is falsy it's
+ // a file comment
+ !threadEl.getAttribute('line-num');
+ }
+
+ // Select the appropriate matchers for the desired side and line
+ // If side is BOTH, we want both the left and right matcher.
+ const matchers = [];
+ if (side !== Gerrit.DiffSide.RIGHT) {
+ matchers.push(matchesLeftLine);
+ }
+ if (side !== Gerrit.DiffSide.LEFT) {
+ matchers.push(matchesRightLine);
+ }
+ if (lineInfo.afterNumber === 'FILE' ||
+ lineInfo.beforeNumber === 'FILE') {
+ matchers.push(matchesFileComment);
+ }
+ return threadEls.filter(threadEl =>
+ matchers.some(matcher => matcher(threadEl)));
+ },
+
+ /**
* Take a diff that was loaded with a ignore-whitespace other than
* IGNORE_NONE, and convert delta chunks labeled as common into shared
* chunks.
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 5344256..53a60d7 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
@@ -769,10 +769,11 @@
});
});
- test('getThreadEls() returns _threadEls', () => {
- const returnValue = [document.createElement('b')];
- element._threadEls = returnValue;
- assert.equal(element.getThreadEls(), returnValue);
+ test('getThreadEls() returns .comment-threads', () => {
+ const threadEl = document.createElement('div');
+ threadEl.className = 'comment-thread';
+ Polymer.dom(element.$.diff).appendChild(threadEl);
+ assert.deepEqual(element.getThreadEls(), [threadEl]);
});
test('delegates addDraftAtLine(el)', () => {
@@ -1159,6 +1160,65 @@
assert.equal(threads[1].patchNum, 3);
});
+ test('_filterThreadElsForLocation with no threads', () => {
+ const line = {beforeNumber: 3, afterNumber: 5};
+
+ const threads = [];
+ assert.deepEqual(element._filterThreadElsForLocation(threads, line), []);
+ assert.deepEqual(element._filterThreadElsForLocation(threads, line,
+ Gerrit.DiffSide.LEFT), []);
+ assert.deepEqual(element._filterThreadElsForLocation(threads, line,
+ Gerrit.DiffSide.RIGHT), []);
+ });
+
+ test('_filterThreadElsForLocation for line comments', () => {
+ const line = {beforeNumber: 3, afterNumber: 5};
+
+ const l3 = document.createElement('div');
+ l3.setAttribute('line-num', 3);
+ l3.setAttribute('comment-side', 'left');
+
+ const l5 = document.createElement('div');
+ l5.setAttribute('line-num', 5);
+ l5.setAttribute('comment-side', 'left');
+
+ const r3 = document.createElement('div');
+ r3.setAttribute('line-num', 3);
+ r3.setAttribute('comment-side', 'right');
+
+ const r5 = document.createElement('div');
+ r5.setAttribute('line-num', 5);
+ r5.setAttribute('comment-side', 'right');
+
+ const threadEls = [l3, l5, r3, r5];
+ assert.deepEqual(element._filterThreadElsForLocation(threadEls, line),
+ [l3, r5]);
+ assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
+ Gerrit.DiffSide.LEFT), [l3]);
+ assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
+ Gerrit.DiffSide.RIGHT), [r5]);
+ });
+
+ test('_filterThreadElsForLocation for file comments', () => {
+ const line = {beforeNumber: 'FILE', afterNumber: 'FILE'};
+
+ const l = document.createElement('div');
+ l.setAttribute('comment-side', 'left');
+
+ const r = document.createElement('div');
+ r.setAttribute('comment-side', 'right');
+
+ const threadEls = [l, r];
+ assert.deepEqual(element._filterThreadElsForLocation(threadEls, line),
+ [l, r]);
+ assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
+ Gerrit.DiffSide.BOTH), [l, r]);
+ assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
+ Gerrit.DiffSide.LEFT), [l]);
+ assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
+ Gerrit.DiffSide.RIGHT), [r]);
+ });
+
suite('_translateChunksToIgnore', () => {
let content;
