From e515ff6c1283f7f9885686574dbb2b858d3ca197 Mon Sep 17 00:00:00 2001 From: Wyatt Allen Date: Fri, 19 Jan 2018 17:54:50 -0800 Subject: [PATCH] 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/gr-diff-builder/gr-diff-builder.html | 29 +++++++++++++++++-- .../gr-diff-builder/gr-diff-builder_test.html | 25 ++++++++++++++++ .../diff/gr-diff-view/gr-diff-view.js | 8 +++++ .../diff/gr-diff-view/gr-diff-view_test.html | 12 ++++++++ .../app/elements/diff/gr-diff/gr-diff.html | 3 +- .../app/elements/diff/gr-diff/gr-diff.js | 10 +++++++ 6 files changed, 84 insertions(+), 3 deletions(-) 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 3995c0d92b..708eb53f97 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 @@ limitations under the License. (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 @@ limitations under the License. revisionImage: Object, projectName: String, parentIndex: Number, + /** + * @type {Defs.LineOfInterest|null} + */ + lineOfInterest: Object, _builder: Object, _groups: Array, _layers: Array, @@ -149,7 +163,8 @@ limitations under the License. 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 @@ limitations under the License. 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 @@ limitations under the License. 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 e6f4f345d9..9f3bf0f561 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 @@ limitations under the License. `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 de9905aff9..94045b61bf 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 979f2532ed..9a55bbee6f 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 @@ limitations under the License. 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 532b92824d..dbb058a148 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 @@ limitations under the License. is-image-diff="[[isImageDiff]]" base-image="[[_baseImage]]" revision-image="[[_revisionImage]]" - parent-index="[[_parentIndex]]"> + parent-index="[[_parentIndex]]" + line-of-interest="[[lineOfInterest]]">