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-builder/gr-diff-builder-binary.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-binary.js
index d2731a2..6f5a8d3 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,8 @@
// Prevent redefinition.
if (window.GrDiffBuilderBinary) { return; }
- function GrDiffBuilderBinary(diff, commentThreadEls, prefs,
- outputEl) {
- GrDiffBuilder.call(this, diff, commentThreadEls, prefs,
- outputEl);
+ function GrDiffBuilderBinary(diff, prefs, outputEl) {
+ GrDiffBuilder.call(this, diff, 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 f05f4f0..bf543e5 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,8 @@
const IMAGE_MIME_PATTERN = /^image\/(bmp|gif|jpeg|jpg|png|tiff|webp)$/;
- function GrDiffBuilderImage(diff, commentThreadEls, prefs,
- outputEl, baseImage, revisionImage) {
- GrDiffBuilderSideBySide.call(this, diff, commentThreadEls,
- prefs, outputEl, []);
+ function GrDiffBuilderImage(diff, prefs, outputEl, baseImage, revisionImage) {
+ GrDiffBuilderSideBySide.call(this, diff, 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 81cbabb..2cf9782 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,8 @@
// Prevent redefinition.
if (window.GrDiffBuilderSideBySide) { return; }
- function GrDiffBuilderSideBySide(diff, commentThreadEls,
- prefs, outputEl, layers) {
- GrDiffBuilder.call(this, diff, commentThreadEls, prefs,
- outputEl, layers);
+ function GrDiffBuilderSideBySide(diff, prefs, outputEl, layers) {
+ GrDiffBuilder.call(this, diff, prefs, outputEl, layers);
}
GrDiffBuilderSideBySide.prototype = Object.create(GrDiffBuilder.prototype);
GrDiffBuilderSideBySide.prototype.constructor = GrDiffBuilderSideBySide;
@@ -99,10 +97,6 @@
row.appendChild(action);
} else {
const textEl = this._createTextEl(line, side);
- const threadGroupEl = this._commentThreadGroupForLine(line, side);
- if (threadGroupEl) {
- textEl.appendChild(threadGroupEl);
- }
row.appendChild(textEl);
}
};
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 2dcdee4..6020e19 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,8 @@
// Prevent redefinition.
if (window.GrDiffBuilderUnified) { return; }
- function GrDiffBuilderUnified(diff, commentThreadEls, prefs,
- outputEl, layers) {
- GrDiffBuilder.call(this, diff, commentThreadEls, prefs,
- outputEl, layers);
+ function GrDiffBuilderUnified(diff, prefs, outputEl, layers) {
+ GrDiffBuilder.call(this, diff, prefs, outputEl, layers);
}
GrDiffBuilderUnified.prototype = Object.create(GrDiffBuilder.prototype);
GrDiffBuilderUnified.prototype.constructor = GrDiffBuilderUnified;
@@ -88,10 +86,6 @@
row.appendChild(action);
} else {
const textEl = this._createTextEl(line);
- const threadGroupEl = this._commentThreadGroupForLine(line);
- if (threadGroupEl) {
- textEl.appendChild(threadGroupEl);
- }
row.appendChild(textEl);
}
return row;
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 6aea35b..c19d23f 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
@@ -121,10 +121,6 @@
return this.queryEffectiveChildren('#diffTable');
},
- get _commentThreadElements() {
- return this.queryAllEffectiveChildren('.comment-thread');
- },
-
observers: [
'_groupsChanged(_groups.splices)',
],
@@ -292,20 +288,16 @@
let builder = null;
if (this.isImageDiff) {
- builder = new GrDiffBuilderImage(diff,
- this._commentThreadElements, prefs, this.diffElement,
+ builder = new GrDiffBuilderImage(diff, prefs, this.diffElement,
this.baseImage, this.revisionImage);
} else if (diff.binary) {
// If the diff is binary, but not an image.
- return new GrDiffBuilderBinary(diff,
- this._commentThreadElements, prefs, this.diffElement);
+ return new GrDiffBuilderBinary(diff, prefs, this.diffElement);
} else if (this.viewMode === DiffViewMode.SIDE_BY_SIDE) {
- builder = new GrDiffBuilderSideBySide(diff,
- this._commentThreadElements, prefs, this.diffElement,
+ builder = new GrDiffBuilderSideBySide(diff, prefs, this.diffElement,
this._layers);
} else if (this.viewMode === DiffViewMode.UNIFIED) {
- builder = new GrDiffBuilderUnified(diff,
- this._commentThreadElements, prefs, this.diffElement,
+ builder = new GrDiffBuilderUnified(diff, prefs, this.diffElement,
this._layers);
}
if (!builder) {
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 d428f68..e892605 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
@@ -20,60 +20,6 @@
// Prevent redefinition.
if (window.GrDiffBuilder) { return; }
- /** @enum {string} */
- Gerrit.DiffSide = {
- LEFT: 'left',
- RIGHT: 'right',
- BOTH: 'both',
- };
-
- /**
- * @param {!Array<!HTMLElement>} threadEls
- * @param {!{beforeNumber: (number|string|undefined),
- * afterNumber: (number|string|undefined)}}
- * lineInfo
- * @param {!Gerrit.DiffSide=} side The side (LEFT, RIGHT, BOTH) for
- * which to return the threads (default: BOTH).
- * @return {!Array<!HTMLElement>} The thread elements matching the given
- * location.
- */
- Gerrit.filterThreadElsForLocation = function(
- threadEls, lineInfo, side = Gerrit.DiffSide.BOTH) {
- 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 (side === Gerrit.DiffSide.BOTH ||
- 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)));
- };
-
/**
* In JS, unicode code points above 0xFFFF occupy two elements of a string.
* For example '𐀏'.length is 2. An occurence of such a code point is called a
@@ -96,8 +42,7 @@
*/
const REGEX_TAB_OR_SURROGATE_PAIR = /\t|[\uD800-\uDBFF][\uDC00-\uDFFF]/;
- function GrDiffBuilder(diff, commentThreadEls, prefs,
- outputEl, layers) {
+ function GrDiffBuilder(diff, prefs, outputEl, layers) {
this._diff = diff;
this._prefs = prefs;
this._outputEl = outputEl;
@@ -119,8 +64,6 @@
layer.addListener(this._handleLayerUpdate.bind(this));
}
}
-
- this._threadEls = commentThreadEls;
}
GrDiffBuilder.GroupType = {
@@ -373,31 +316,6 @@
return button;
};
- /**
- * @param {!GrDiffLine} line
- * @param {!GrDiffBuilder.Side=} side The side (LEFT, RIGHT, BOTH) for which
- * to return the thread group (default: BOTH).
- * @return {!Object}
- */
- GrDiffBuilder.prototype._commentThreadGroupForLine = function(
- line, commentSide = GrDiffBuilder.Side.BOTH) {
- const threadElsForGroup =
- Gerrit.filterThreadElsForLocation(this._threadEls, line, commentSide);
- if (!threadElsForGroup || threadElsForGroup.length === 0) {
- return null;
- }
-
- const threadGroupEl = document.createElement('div');
- threadGroupEl.className = 'thread-group';
- for (const threadEl of threadElsForGroup) {
- Polymer.dom(threadGroupEl).appendChild(threadEl);
- }
- if (commentSide) {
- threadGroupEl.setAttribute('data-side', commentSide);
- }
- return threadGroupEl;
- };
-
GrDiffBuilder.prototype._createLineEl = function(
line, number, type, opt_class) {
const td = this._createElement('td');
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 c7a1140..1b0ba04 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
@@ -75,70 +75,11 @@
show_tabs: true,
tab_size: 4,
};
- builder = new GrDiffBuilder({content: []}, [], prefs);
+ builder = new GrDiffBuilder({content: []}, prefs);
});
teardown(() => { sandbox.restore(); });
- test('filterThreadElsForLocation with no threads', () => {
- const line = {beforeNumber: 3, afterNumber: 5};
-
- const threads = [];
- assert.deepEqual(Gerrit.filterThreadElsForLocation(threads, line), []);
- assert.deepEqual(Gerrit.filterThreadElsForLocation(threads, line,
- Gerrit.DiffSide.LEFT), []);
- assert.deepEqual(Gerrit.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(Gerrit.filterThreadElsForLocation(threadEls, line),
- [l3, r5]);
- assert.deepEqual(Gerrit.filterThreadElsForLocation(threadEls, line,
- Gerrit.DiffSide.LEFT), [l3]);
- assert.deepEqual(Gerrit.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(Gerrit.filterThreadElsForLocation(threadEls, line),
- [l, r]);
- assert.deepEqual(Gerrit.filterThreadElsForLocation(threadEls, line,
- Gerrit.DiffSide.BOTH), [l, r]);
- assert.deepEqual(Gerrit.filterThreadElsForLocation(threadEls, line,
- Gerrit.DiffSide.LEFT), [l]);
- assert.deepEqual(Gerrit.filterThreadElsForLocation(threadEls, line,
- Gerrit.DiffSide.RIGHT), [r]);
- });
-
test('_createElement classStr applies all classes', () => {
const node = builder._createElement('div', 'test classes');
assert.isTrue(node.classList.contains('gr-diff'));
@@ -312,73 +253,6 @@
}
});
- test('comment thread group creation', () => {
- 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: []}, [l3, l5, r5], prefs);
-
- function checkThreadGroupProps(threadGroupEl,
- expectedThreadEls) {
- const threadEls = Polymer.dom(threadGroupEl).queryDistributedElements(
- '.comment-thread');
- assert.equal(threadEls.length, expectedThreadEls.length);
- for (let i=0; i<expectedThreadEls.length; i++) {
- assert.equal(threadEls[i], expectedThreadEls[i]);
- }
- }
-
- let line = new GrDiffLine(GrDiffLine.Type.BOTH);
- line.beforeNumber = 5;
- line.afterNumber = 5;
- let threadGroupEl = builder._commentThreadGroupForLine(line);
- checkThreadGroupProps(threadGroupEl, [l5, r5]);
-
- threadGroupEl =
- builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
- checkThreadGroupProps(threadGroupEl, [r5]);
-
- threadGroupEl =
- builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
- checkThreadGroupProps(threadGroupEl, [l5]);
-
- threadGroupEl = builder._commentThreadGroupForLine(line);
- checkThreadGroupProps(threadGroupEl, [l5, r5]);
-
- threadEl =
- builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
- checkThreadGroupProps(threadEl, [l5]);
-
- threadGroupEl =
- builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
- checkThreadGroupProps(threadGroupEl, [r5]);
-
- line = new GrDiffLine(GrDiffLine.Type.REMOVE);
- line.beforeNumber = 5;
- line.afterNumber = 5;
- threadGroupEl = builder._commentThreadGroupForLine(line);
- checkThreadGroupProps(threadGroupEl, [l5, r5]);
-
- line = new GrDiffLine(GrDiffLine.Type.ADD);
- line.beforeNumber = 3;
- line.afterNumber = 5;
- threadGroupEl = builder._commentThreadGroupForLine(line);
- checkThreadGroupProps(threadGroupEl, [l3, r5]);
- });
-
-
test('_handlePreferenceError called with invalid preference', () => {
sandbox.stub(element, '_handlePreferenceError');
const prefs = {tab_size: 0};
@@ -913,7 +787,7 @@
outputEl = element.queryEffectiveChildren('#diffTable');
keyLocations = {left: {}, right: {}};
sandbox.stub(element, '_getDiffBuilder', () => {
- const builder = new GrDiffBuilder({content}, [], prefs, outputEl);
+ const builder = new GrDiffBuilder({content}, prefs, outputEl);
sandbox.stub(builder, 'addColumns');
builder.buildSectionElement = function(group) {
const section = document.createElement('stub');
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
index 75a2b15..1646583 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
@@ -74,8 +74,16 @@
this.debounce('selectionChange', this._handleSelection, 200);
},
+ _getThreadEl(e) {
+ const path = Polymer.dom(e).path || [];
+ for (const pathEl of path) {
+ if (pathEl.classList.contains('comment-thread')) return pathEl;
+ }
+ return null;
+ },
+
_handleCommentThreadMouseenter(e) {
- const threadEl = Polymer.dom(e).localTarget;
+ const threadEl = this._getThreadEl(e);
const index = this._indexForThreadEl(threadEl);
if (index !== undefined) {
@@ -84,7 +92,7 @@
},
_handleCommentThreadMouseleave(e) {
- const threadEl = Polymer.dom(e).localTarget;
+ const threadEl = this._getThreadEl(e);
const index = this._indexForThreadEl(threadEl);
if (index !== undefined) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
index 8c30afa..6f6cbc4 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
@@ -199,6 +199,7 @@
test('comment-thread-mouseenter from line comments is ignored', () => {
const threadEl = document.createElement('div');
+ threadEl.className = 'comment-thread';
threadEl.setAttribute('comment-side', 'right');
threadEl.setAttribute('line-num', 3);
element.appendChild(threadEl);
@@ -212,6 +213,7 @@
test('comment-thread-mouseenter from ranged comment causes set', () => {
const threadEl = document.createElement('div');
+ threadEl.className = 'comment-thread';
threadEl.setAttribute('comment-side', 'right');
threadEl.setAttribute('line-num', 3);
threadEl.setAttribute('range', JSON.stringify({
@@ -239,6 +241,7 @@
test('comment-thread-mouseleave from line comments is ignored', () => {
const threadEl = document.createElement('div');
+ threadEl.className = 'comment-thread';
threadEl.setAttribute('comment-side', 'right');
threadEl.setAttribute('line-num', 3);
element.appendChild(threadEl);
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;
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 e587953..4ccdf96 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -294,7 +294,6 @@
is-image-diff="[[isImageDiff]]"
base-image="[[baseImage]]"
revision-image="[[revisionImage]]">
- <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 8b5c6d6..996d484 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -60,6 +60,20 @@
node.classList.contains('comment-thread');
}
+ /**
+ * Turn a slot element into the corresponding content element.
+ * Slots are only fully supported in Polymer 2 - in Polymer 1, they are
+ * replaced with content elements during template parsing. This conversion is
+ * not applied for imperatively created slot elements, so this method
+ * implements the same behavior as the template parsing for imperative slots.
+ */
+ Gerrit.slotToContent = function(slot) {
+ const content = document.createElement('content');
+ content.name = slot.name;
+ content.setAttribute('select', `[slot='${slot.name}']`);
+ return content;
+ };
+
Polymer({
is: 'gr-diff',
@@ -455,14 +469,16 @@
* Gets or creates a comment thread group for a specific line and side on a
* diff.
* @param {!Object} contentEl
+ * @param {!Gerrit.DiffSide} commentSide
* @return {!Node}
*/
- _getOrCreateThreadGroup(contentEl) {
+ _getOrCreateThreadGroup(contentEl, commentSide) {
// Check if thread group exists.
let threadGroupEl = this._getThreadGroupForLine(contentEl);
if (!threadGroupEl) {
threadGroupEl = document.createElement('div');
threadGroupEl.className = 'thread-group';
+ threadGroupEl.setAttribute('data-side', commentSide);
contentEl.appendChild(threadGroupEl);
}
return threadGroupEl;
@@ -624,8 +640,17 @@
lineNumString, commentSide);
const contentText = this.$.diffBuilder.getContentByLineEl(lineEl);
const contentEl = contentText.parentElement;
- const threadGroupEl = this._getOrCreateThreadGroup(contentEl);
- Polymer.dom(threadGroupEl).appendChild(threadEl);
+ const threadGroupEl = this._getOrCreateThreadGroup(
+ contentEl, commentSide);
+ // Create a slot for the thread and attach it to the thread group.
+ // The Polyfill has some bugs and this only works if the slot is
+ // attached to the group after the group is attached to the DOM.
+ // The thread group may already have a slot with the right name, but
+ // that is okay because the first matching slot is used and the rest
+ // are ignored.
+ const slot = document.createElement('slot');
+ slot.name = threadEl.slot;
+ Polymer.dom(threadGroupEl).appendChild(Gerrit.slotToContent(slot));
}
});
},
