Do not collapse line numbers that are indicated in the URL
When users navigate to the diff view with a line number specified at the
end, depending on their context preference, the line might be in a
shared region that gets collapsed when the diff renders. With this
change, the location specified in the URL is prevented from being
collapsed by marking it as a "key" location.
Bug: Issue 5247
Change-Id: Ifd5827cd922b022cddb1601911a9ecea6a054f35
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 3995c0d..708eb53 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
@@ -50,6 +50,16 @@
(function() {
'use strict';
+ const Defs = {};
+
+ /**
+ * @typedef {{
+ * number: number,
+ * leftSide: {boolean}
+ * }}
+ */
+ Defs.LineOfInterest;
+
const DiffViewMode = {
SIDE_BY_SIDE: 'SIDE_BY_SIDE',
UNIFIED: 'UNIFIED_DIFF',
@@ -101,6 +111,10 @@
revisionImage: Object,
projectName: String,
parentIndex: Number,
+ /**
+ * @type {Defs.LineOfInterest|null}
+ */
+ lineOfInterest: Object,
_builder: Object,
_groups: Array,
_layers: Array,
@@ -149,7 +163,8 @@
this._builder = this._getDiffBuilder(this.diff, comments, prefs);
this.$.processor.context = prefs.context;
- this.$.processor.keyLocations = this._getCommentLocations(comments);
+ this.$.processor.keyLocations = this._getKeyLocations(comments,
+ this.lineOfInterest);
this._clearDiffContent();
this._builder.addColumns(this.diffElement, prefs.font_size);
@@ -315,7 +330,11 @@
this.diffElement.innerHTML = null;
},
- _getCommentLocations(comments) {
+ /**
+ * @param {!Object} comments
+ * @param {Defs.LineOfInterest|null} lineOfInterest
+ */
+ _getKeyLocations(comments, lineOfInterest) {
const result = {
left: {},
right: {},
@@ -329,6 +348,12 @@
result[side][c.line || GrDiffLine.FILE] = true;
}
}
+
+ if (lineOfInterest) {
+ const side = lineOfInterest.leftSide ? 'left' : 'right';
+ result[side][lineOfInterest.number] = true;
+ }
+
return result;
},
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 e6f4f345..9f3bf0f 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
@@ -371,6 +371,31 @@
`Fix in diff preferences`);
});
+ 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},
+ });
+
+ const lineOfInterest = {number: 789, leftSide: true};
+ assert.deepEqual(element._getKeyLocations(comments, lineOfInterest), {
+ left: {FILE: true, 123: true, 789: true},
+ right: {456: true},
+ });
+
+ delete lineOfInterest.leftSide;
+ assert.deepEqual(element._getKeyLocations(comments, lineOfInterest), {
+ left: {FILE: true, 123: true},
+ right: {456: true, 789: true},
+ });
+ });
+
suite('_isTotal', () => {
test('is total for add', () => {
const group = new GrDiffGroup(GrDiffGroup.Type.DELTA);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
index de9905a..94045b6 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -512,6 +512,7 @@
_paramsChanged(value) {
if (value.view !== Gerrit.Nav.View.DIFF) { return; }
+ this.$.diff.lineOfInterest = this._getLineOfInterest(this.params);
this._initCursor(this.params);
this._changeNum = value.changeNum;
@@ -610,6 +611,13 @@
this.$.cursor.initialLineNumber = params.lineNum;
},
+ _getLineOfInterest(params) {
+ // If there is a line number specified, pass it along to the diff so that
+ // it will not get collapsed.
+ if (!params.lineNum) { return null; }
+ return {number: params.lineNum, leftSide: params.leftSide};
+ },
+
_pathChanged(path) {
if (path) {
this.fire('title-change',
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
index 979f253..9a55bbe 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
@@ -646,6 +646,18 @@
assert.equal(element.$.cursor.side, 'right');
});
+ test('_getLineOfInterest', () => {
+ assert.isNull(element._getLineOfInterest({}));
+
+ let result = element._getLineOfInterest({lineNum: 12});
+ assert.equal(result.number, 12);
+ assert.isNotOk(result.leftSide);
+
+ result = element._getLineOfInterest({lineNum: 12, leftSide: true});
+ assert.equal(result.number, 12);
+ assert.isOk(result.leftSide);
+ });
+
test('_onLineSelected', () => {
const getUrlStub = sandbox.stub(Gerrit.Nav, 'getUrlForDiffById');
const replaceStateStub = sandbox.stub(history, 'replaceState');
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 532b928..dbb058a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -272,7 +272,8 @@
is-image-diff="[[isImageDiff]]"
base-image="[[_baseImage]]"
revision-image="[[_revisionImage]]"
- parent-index="[[_parentIndex]]">
+ parent-index="[[_parentIndex]]"
+ line-of-interest="[[lineOfInterest]]">
<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 75ed70c..06a2a27 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -95,6 +95,16 @@
value: DiffViewMode.SIDE_BY_SIDE,
observer: '_viewModeObserver',
},
+
+ /**
+ * Special line number which should not be collapsed into a shared region.
+ * @type {{
+ * number: number,
+ * leftSide: {boolean}
+ * }|null}
+ */
+ lineOfInterest: Object,
+
_loggedIn: {
type: Boolean,
value: false,
