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

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;
         },
 

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);

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',

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');

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]]">
             <table
                 id="diffTable"
                 class$="[[_diffTableClass]]"

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,