Derive keyLocations from thread elements
This cuts one more dependency on Gerrit's comment model.
Change-Id: Ia0da0162f5bf97f350914cebda9be3b0a19a7680
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 d5f92f1..778c097 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -136,6 +136,21 @@
/** @type ?Defs.LineOfInterest */
lineOfInterest: Object,
+ /**
+ * The key locations based on the comments and line of interests,
+ * where lines should not be collapsed.
+ *
+ * @type {{left: Object<(string|number), number>,
+ * right: Object<(string|number), number>}}
+ */
+ _keyLocations: {
+ type: Object,
+ value: () => ({
+ left: {},
+ right: {},
+ }),
+ },
+
loading: {
type: Boolean,
value: false,
@@ -230,7 +245,7 @@
},
attached() {
- this._updateRangesWhenNodesChange();
+ this._observeNodes();
},
detached() {
@@ -238,25 +253,38 @@
this._unobserveNodes();
},
- _updateRangesWhenNodesChange() {
+ _observeNodes() {
+ this._nodeObserver = Polymer.dom(this).observeNodes(info => {
+ const addedThreadEls = info.addedNodes.filter(isThreadEl);
+ // 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.
+ this._updateRanges(addedThreadEls);
+ this._updateKeyLocations(addedThreadEls);
+ });
+ },
+
+ _updateRanges(addedThreadEls) {
function commentRangeFromThreadEl(threadEl) {
const side = threadEl.getAttribute('comment-side');
const range = JSON.parse(threadEl.getAttribute('range'));
return {side, range, hovering: false};
}
- this._nodeObserver = Polymer.dom(this).observeNodes(info => {
- const addedThreadEls = info.addedNodes.filter(isThreadEl);
- const addedCommentRanges = addedThreadEls
- .map(commentRangeFromThreadEl)
- .filter(({range}) => range);
- this.push('_commentRanges', ...addedCommentRanges);
- // 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.
- });
+ const addedCommentRanges = addedThreadEls
+ .map(commentRangeFromThreadEl)
+ .filter(({range}) => range);
+ this.push('_commentRanges', ...addedCommentRanges);
+ },
+
+ _updateKeyLocations(addedThreadEls) {
+ for (const threadEl of addedThreadEls) {
+ const commentSide = threadEl.getAttribute('comment-side');
+ const lineNum = threadEl.getAttribute('line-num') || GrDiffLine.FILE;
+ this._keyLocations[commentSide][lineNum] = true;
+ }
},
/** Cancel any remaining diff builder rendering work. */
@@ -645,9 +673,12 @@
}
this._showWarning = false;
- const keyLocations = this._getKeyLocations(this.comments,
- this.lineOfInterest);
- this.$.diffBuilder.render(keyLocations, this._getBypassPrefs());
+
+ if (this.lineOfInterest) {
+ const side = this.lineOfInterest.leftSide ? 'left' : 'right';
+ this._keyLocations[side][this.lineOfInterest.number] = true;
+ }
+ this.$.diffBuilder.render(this._keyLocations, this._getBypassPrefs());
},
_handleRenderContent() {
@@ -684,39 +715,6 @@
},
/**
- * Returns the key locations based on the comments and line of interests,
- * where lines should not be collapsed.
- *
- * @param {!Object} comments
- * @param {Defs.LineOfInterest|null} lineOfInterest
- *
- * @return {{left: Object<(string|number), boolean>,
- * right: Object<(string|number), boolean>}}
- */
- _getKeyLocations(comments, lineOfInterest) {
- const result = {
- left: {},
- right: {},
- };
- for (const side in comments) {
- if (side !== GrDiffBuilder.Side.LEFT &&
- side !== GrDiffBuilder.Side.RIGHT) {
- continue;
- }
- for (const c of comments[side]) {
- result[side][c.line || GrDiffLine.FILE] = true;
- }
- }
-
- if (lineOfInterest) {
- const side = lineOfInterest.leftSide ? 'left' : 'right';
- result[side][lineOfInterest.number] = true;
- }
-
- return result;
- },
-
- /**
* Get the preferences object including the safety bypass context (if any).
*/
_getBypassPrefs() {
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 c6ef806..0577d5a 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
@@ -1128,31 +1128,55 @@
});
});
- test('_getKeyLocations', () => {
- assert.deepEqual(element._getKeyLocations({left: [], right: []}, null),
- {left: {}, right: {}});
- const comments = {
- left: [{line: 123}, {}],
- right: [{line: 456}],
- };
- assert.deepEqual(element._getKeyLocations(comments, null), {
- left: {FILE: true, 123: true},
- right: {456: true},
+ suite('key locations', () => {
+ let renderStub;
+
+ setup(() => {
+ element = fixture('basic');
+ element.prefs = {};
+ renderStub = sandbox.stub(element.$.diffBuilder, 'render');
});
- const lineOfInterest = {number: 789, leftSide: true};
- assert.deepEqual(
- element._getKeyLocations(comments, lineOfInterest), {
- left: {FILE: true, 123: true, 789: true},
- right: {456: true},
- });
+ test('lineOfInterest is a key location', () => {
+ element.lineOfInterest = {number: 789, leftSide: true};
+ element._renderDiffTable();
+ assert.isTrue(renderStub.called);
+ assert.deepEqual(renderStub.lastCall.args[0], {
+ left: {789: true},
+ right: {},
+ });
+ });
- delete lineOfInterest.leftSide;
- assert.deepEqual(
- element._getKeyLocations(comments, lineOfInterest), {
- left: {FILE: true, 123: true},
- right: {456: true, 789: true},
- });
+ test('line comments are key locations', () => {
+ const threadEl = document.createElement('div');
+ threadEl.className = 'comment-thread';
+ threadEl.setAttribute('comment-side', 'right');
+ threadEl.setAttribute('line-num', 3);
+ Polymer.dom(element).appendChild(threadEl);
+ Polymer.dom.flush();
+
+ element._renderDiffTable();
+ assert.isTrue(renderStub.called);
+ assert.deepEqual(renderStub.lastCall.args[0], {
+ left: {},
+ right: {3: true},
+ });
+ });
+
+ test('file comments are key locations', () => {
+ const threadEl = document.createElement('div');
+ threadEl.className = 'comment-thread';
+ threadEl.setAttribute('comment-side', 'left');
+ Polymer.dom(element).appendChild(threadEl);
+ Polymer.dom.flush();
+
+ element._renderDiffTable();
+ assert.isTrue(renderStub.called);
+ assert.deepEqual(renderStub.lastCall.args[0], {
+ left: {FILE: true},
+ right: {},
+ });
+ });
});
});
