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,