Do not hide key locations

Probably a regression from properly supporting whitespace only changes.

Change-Id: I6cdeef3bb5c617c673222402643c3e2f98c101a3

polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js

@@ -146,7 +146,7 @@
             };
 
             content = this._splitLargeChunks(content);
-            content = this._splitUnchangedChunksWithComments(content);
+            content = this._splitUnchangedChunksWithKeyLocations(content);
 
             let currentBatch = 0;
             const nextStep = () => {
@@ -200,15 +200,15 @@
     },
 
     /**
-     * Process the next uncommon section, or the next common sections.
+     * Process the next uncollapsible section, or the next collapsible sections.
      *
      * @param {!Object} state
      * @param {!Array<!Object>} sections
      */
     _processNext(state, sections) {
-      const firstUncommonSectionIndex = this._firstUncommonSectionIndex(
+      const firstUncollapsibleSectionIndex =
-          sections, state.sectionIndex);
+          this._firstUncollapsibleSectionIndex(sections, state.sectionIndex);
-      if (firstUncommonSectionIndex === state.sectionIndex) {
+      if (firstUncollapsibleSectionIndex === state.sectionIndex) {
         const section = sections[state.sectionIndex];
         return {
           lineDelta: {
@@ -221,25 +221,25 @@
         };
       }
 
-      return this._processCommonSections(
+      return this._processCollapsibleSections(
-          state, sections, firstUncommonSectionIndex);
+          state, sections, firstUncollapsibleSectionIndex);
     },
 
-    _firstUncommonSectionIndex(sections, offset) {
+    _firstUncollapsibleSectionIndex(sections, offset) {
       let sectionIndex = offset;
       while (sectionIndex < sections.length &&
-          this._isCommonSection(sections[sectionIndex])) {
+          this._isCollapsibleSection(sections[sectionIndex])) {
         sectionIndex++;
       }
       return sectionIndex;
     },
 
-    _isCommonSection(section) {
+    _isCollapsibleSection(section) {
-      return section.ab || section.common;
+      return (section.ab || section.common) && !section.keyLocation;
     },
 
     /**
-     * Process a stretch of common sections.
+     * Process a stretch of collapsible sections.
      *
      * Outputs up to three groups:
      *  1) Visible context before the hidden common code, unless it's the
@@ -250,22 +250,25 @@
      *
      * @param {!Object} state
      * @param {!Array<Object>} sections
-     * @param {number} firstUncommonSectionIndex
+     * @param {number} firstUncollapsibleSectionIndex
      * @return {!Object}
      */
-    _processCommonSections(state, sections, firstUncommonSectionIndex) {
+    _processCollapsibleSections(
-      const commonSections = sections.slice(
+        state, sections, firstUncollapsibleSectionIndex) {
-          state.sectionIndex, firstUncommonSectionIndex);
+      const collapsibleSections = sections.slice(
-      const lineCount = commonSections.reduce(
+          state.sectionIndex, firstUncollapsibleSectionIndex);
+      const lineCount = collapsibleSections.reduce(
           (sum, section) => sum + this._commonSectionLength(section), 0);
 
       let groups = this._sectionsToGroups(
-          commonSections, state.lineNums.left + 1, state.lineNums.right + 1);
+          collapsibleSections,
+          state.lineNums.left + 1,
+          state.lineNums.right + 1);
 
       if (this.context !== WHOLE_FILE) {
         const hiddenStart = state.sectionIndex === 0 ? 0 : this.context;
         const hiddenEnd = lineCount - (
-            firstUncommonSectionIndex === sections.length ?
+            firstUncollapsibleSectionIndex === sections.length ?
             0 : this.context);
         groups = GrDiffGroup.hideInContextControl(
             groups, hiddenStart, hiddenEnd);
@@ -277,14 +280,15 @@
           right: lineCount,
         },
         groups,
-        newSectionIndex: firstUncommonSectionIndex,
+        newSectionIndex: firstUncollapsibleSectionIndex,
       };
     },
 
     _commonSectionLength(section) {
       console.assert(section.ab || section.common);
       console.assert(
-          !section.a || (section.b && section.a.length === section.b.length));
+          !section.a || (section.b && section.a.length === section.b.length),
+          `common section needs same number of a and b lines: `, section);
       return (section.ab || section.a).length;
     },
 
@@ -302,6 +306,7 @@
       const type = section.ab ? GrDiffGroup.Type.BOTH : GrDiffGroup.Type.DELTA;
       const lines = this._linesFromSection(section, offsetLeft, offsetRight);
       const group = new GrDiffGroup(type, lines);
+      group.keyLocation = section.keyLocation;
       group.dueToRebase = section.due_to_rebase;
       group.ignoredWhitespaceOnly = section.common;
       return group;
@@ -414,13 +419,13 @@
     },
 
     /**
-     * In order to show comments out of the bounds of the selected context,
+     * In order to show key locations, such as comments, out of the bounds of
-     * treat them as separate chunks within the model so that the content (and
+     * the selected context, treat them as separate chunks within the model so
-     * context surrounding it) renders correctly.
+     * that the content (and context surrounding it) renders correctly.
      * @param {!Array<!Object>} chunks DiffContents as returned from server.
      * @return {!Array<!Object>} Finer grained DiffContents.
      */
-    _splitUnchangedChunksWithComments(chunks) {
+    _splitUnchangedChunksWithKeyLocations(chunks) {
       const result = [];
       let leftLineNum = 1;
       let rightLineNum = 1;
@@ -450,18 +455,24 @@
 
         if (chunk.ab) {
           result.push(...this._splitAtChunkEnds(chunk.ab, chunkEnds)
-              .map(lines => Object.assign({}, chunk, {ab: lines})));
+              .map(({lines, keyLocation}) =>
+                  Object.assign({}, chunk, {ab: lines, keyLocation})));
         } else if (chunk.common) {
           const aChunks = this._splitAtChunkEnds(chunk.a, chunkEnds);
           const bChunks = this._splitAtChunkEnds(chunk.b, chunkEnds);
-          result.push(...aChunks.map((lines, i) =>
+          result.push(...aChunks.map(({lines, keyLocation}, i) =>
-              Object.assign({}, chunk, {a: lines, b: bChunks[i]})));
+            Object.assign(
+                {}, chunk, {a: lines, b: bChunks[i].lines, keyLocation})));
         }
       }
 
       return result;
     },
 
+    /**
+     * @return {!Array<{offset: number, keyLocation: boolean}>} Offsets of the
+     *   new chunk ends, including whether it's a key location.
+     */
     _findChunkEndsAtKeyLocations(numLines, leftOffset, rightOffset) {
       const result = [];
       let lastChunkEnd = 0;
@@ -473,17 +484,17 @@
           // this non-collapse line, then add them as a chunk and start a new
           // one.
           if (i > lastChunkEnd) {
-            result.push(i);
+            result.push({offset: i, keyLocation: false});
             lastChunkEnd = i;
           }
 
           // Add the non-collapse line as its own chunk.
-          result.push(i + 1);
+          result.push({offset: i + 1, keyLocation: true});
         }
       }
 
       if (numLines > lastChunkEnd) {
-        result.push(numLines);
+        result.push({offset: numLines, keyLocation: false});
       }
 
       return result;
@@ -491,10 +502,11 @@
 
     _splitAtChunkEnds(lines, chunkEnds) {
       const result = [];
-      let lastChunkEnd = 0;
+      let lastChunkEndOffset = 0;
-      for (const chunkEnd of chunkEnds) {
+      for (const {offset, keyLocation} of chunkEnds) {
-        result.push(lines.slice(lastChunkEnd, chunkEnd));
+        result.push(
-        lastChunkEnd = chunkEnd;
+            {lines: lines.slice(lastChunkEndOffset, offset), keyLocation});
+        lastChunkEndOffset = offset;
       }
       return result;
     },

polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.html

@@ -460,10 +460,11 @@ limitations under the License.
           },
         ];
         const result =
-            element._splitUnchangedChunksWithComments(content);
+            element._splitUnchangedChunksWithKeyLocations(content);
         assert.deepEqual(result, [
           {
             ab: ['Copyright (C) 2015 The Android Open Source Project'],
+            keyLocation: true,
           },
           {
             ab: [
@@ -477,10 +478,12 @@ limitations under the License.
               '',
               'Unless required by applicable law or agreed to in writing, ',
             ],
+            keyLocation: false,
           },
           {
             ab: [
               'software distributed under the License is distributed on an '],
+            keyLocation: true,
           },
           {
             ab: [
@@ -489,6 +492,7 @@ limitations under the License.
               'language governing permissions and limitations under the ' +
                   'License.',
             ],
+            keyLocation: false,
           },
         ]);
       });
@@ -505,14 +509,16 @@ limitations under the License.
         assert.deepEqual(result[1].ab, content[0].ab.slice(120));
       });
 
-      test('does not break-down shared chunks w/ context', () => {
+      test('does not break-down common chunks w/ context', () => {
         const content = [{
           ab: _.times(75, () => { return `${Math.random()}`; }),
         }];
         element.context = 4;
         const result =
-            element._splitUnchangedChunksWithComments(content);
+            element._splitUnchangedChunksWithKeyLocations(content);
-        assert.deepEqual(result, content);
+        assert.equal(result.length, 1);
+        assert.deepEqual(result[0].ab, content[0].ab);
+        assert.isFalse(result[0].keyLocation);
       });
 
       test('intraline normalization', () => {
@@ -764,6 +770,32 @@ limitations under the License.
           assert.equal(result.groups.length, 1, 'Results in one group');
           assert.equal(result.groups[0].lines.length, rows.length);
         });
+
+        test('with key location', () => {
+          element.context = 10;
+          const state = {
+            lineNums: {left: 10, right: 100},
+            sectionIndex: 0,
+          };
+          const sections = [
+            {ab: rows},
+            {ab: ['foo'], keyLocation: true},
+            {ab: rows},
+          ];
+          const result = element._processNext(state, sections);
+
+          assert.equal(result.groups.length, 2);
+
+          // The first section has a single context-control line, the second
+          // section is the context before the key location not processed in
+          // this step
+          assert.equal(result.groups[0].lines.length, 1);
+          assert.equal(result.groups[1].lines.length, element.context);
+
+          // The collapsed group has the hidden lines as its context group.
+          assert.equal(result.groups[0].lines[0].contextGroups[0].lines.length,
+              rows.length - element.context);
+        });
       });
 
       suite('gr-diff-processor helpers', () => {

polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.js

@@ -40,6 +40,12 @@
      */
     this.ignoredWhitespaceOnly = false;
 
+    /**
+     * True means it should not be collapsed (because it was in the URL, or
+     * there is a comment on that line)
+     */
+    this.keyLocation = false;
+
     /** @type {?HTMLElement} */
     this.element = null;