Remove highlighted ranges when changing patchset
It turns out that changing the patchset does reuse the gr-diff element,
but does not remove the comment thread elements as it should. What was
missing was handling these comment thread removals and keep them in sync
with the rangesMap inside gr-ranged-comment-layer.
Issue: Bug 10060
Change-Id: Iff5be96a556c7a8081c0daae11249202585eb6c1
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 8b9d8066..fd460cc 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,24 +45,6 @@
return !!(diff.binary && (isA || isB));
}
- /**
- * 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.start_line === b.start_line &&
- a.start_character === b.start_character &&
- a.end_line === b.end_line &&
- a.end_character === b.end_character;
- }
-
/** @enum {string} */
Gerrit.DiffSide = {
LEFT: 'left',
@@ -652,7 +634,7 @@
function matchesRange(threadEl) {
const threadRange = /** @type {!Gerrit.Range} */(
JSON.parse(threadEl.getAttribute('range')));
- return rangesEqual(threadRange, range);
+ return Gerrit.rangesEqual(threadRange, range);
}
const filteredThreadEls = this._filterThreadElsForLocation(
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 996d484..b134d4e 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -55,6 +55,24 @@
* end_line: number, end_character: 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}
+ */
+ Gerrit.rangesEqual = function(a, b) {
+ if (!a && !b) { return true; }
+ if (!a || !b) { return false; }
+ return a.start_line === b.start_line &&
+ a.start_character === b.start_character &&
+ a.end_line === b.end_line &&
+ a.end_character === b.end_character;
+ };
+
function isThreadEl(node) {
return node.nodeType === Node.ELEMENT_NODE &&
node.classList.contains('comment-thread');
@@ -257,18 +275,14 @@
_observeNodes() {
this._nodeObserver = Polymer.dom(this).observeNodes(info => {
const addedThreadEls = info.addedNodes.filter(isThreadEl);
- // In principle 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.
- this._updateRanges(addedThreadEls);
- this._updateKeyLocations(addedThreadEls);
+ const removedThreadEls = info.removedNodes.filter(isThreadEl);
+ this._updateRanges(addedThreadEls, removedThreadEls);
+ this._updateKeyLocations(addedThreadEls, removedThreadEls);
this._redispatchHoverEvents(addedThreadEls);
});
},
- _updateRanges(addedThreadEls) {
+ _updateRanges(addedThreadEls, removedThreadEls) {
function commentRangeFromThreadEl(threadEl) {
const side = threadEl.getAttribute('comment-side');
const range = JSON.parse(threadEl.getAttribute('range'));
@@ -278,15 +292,30 @@
const addedCommentRanges = addedThreadEls
.map(commentRangeFromThreadEl)
.filter(({range}) => range);
+ const removedCommentRanges = removedThreadEls
+ .map(commentRangeFromThreadEl)
+ .filter(({range}) => range);
+ for (const removedCommentRange of removedCommentRanges) {
+ const i = this._commentRanges.findIndex(commentRange => {
+ return commentRange.side === removedCommentRange.side &&
+ Gerrit.rangesEqual(commentRange.range, removedCommentRange.range);
+ });
+ this.splice('_commentRanges', i, 1);
+ }
this.push('_commentRanges', ...addedCommentRanges);
},
- _updateKeyLocations(addedThreadEls) {
+ _updateKeyLocations(addedThreadEls, removedThreadEls) {
for (const threadEl of addedThreadEls) {
const commentSide = threadEl.getAttribute('comment-side');
const lineNum = threadEl.getAttribute('line-num') || GrDiffLine.FILE;
this._keyLocations[commentSide][lineNum] = true;
}
+ for (const threadEl of removedThreadEls) {
+ const commentSide = threadEl.getAttribute('comment-side');
+ const lineNum = threadEl.getAttribute('line-num') || GrDiffLine.FILE;
+ this._keyLocations[commentSide][lineNum] = false;
+ }
},
// Dispatch events that are handled by the gr-diff-highlight.
@@ -628,11 +657,11 @@
_handleRenderContent() {
this._incrementalNodeObserver = Polymer.dom(this).observeNodes(info => {
const addedThreadEls = info.addedNodes.filter(isThreadEl);
- // In principle 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.
+ // Removed nodes do not need to be handled because all this code does is
+ // adding a slot for the added thread elements, and the extra slots do
+ // not hurt. It's probably a bigger performance cost to remove them than
+ // to keep them around. Medium term we can even consider to add one slot
+ // for each line from the start.
for (const threadEl of addedThreadEls) {
const lineNumString = threadEl.getAttribute('line-num') || 'FILE';
const commentSide = threadEl.getAttribute('comment-side');
