Merge "gr-diff-builder: Changing single-directional context expansion to bi-directional ('+10' button)"

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

@@ -83,11 +83,16 @@
   };
 
   GrDiffBuilder.ContextButtonType = {
-    ABOVE: 'above',
+    STEP: 'step',
-    BELOW: 'below',
     ALL: 'all',
   };
 
+  const ContextPlacement = {
+    LAST: 'last',
+    FIRST: 'first',
+    MIDDLE: 'middle',
+  };
+
   const PARTIAL_CONTEXT_AMOUNT = 10;
 
   /**
@@ -231,40 +236,53 @@
         group => { return group.element; });
   };
 
+  /**
+   * @param {!Array<!Object>} contextGroups (!Array<!GrDiffGroup>)
+   */
+  GrDiffBuilder.prototype._getContextControlPlacementFor = function(
+      contextGroups) {
+    const firstContextGroup = contextGroups[0];
+    const lastContextGroup = contextGroups[contextGroups.length - 1];
+    const firstLines = firstContextGroup.lines;
+    const lastLines = lastContextGroup.lines;
+    const firstContextInFile = firstLines.length && firstLines[0].firstInFile;
+    const lastContextInFile = lastLines.length &&
+        lastLines[lastLines.length - 1].lastInFile;
+    if (firstContextInFile && !lastContextInFile) {
+      return ContextPlacement.FIRST;
+    } else if (lastContextInFile && !firstContextInFile) {
+      return ContextPlacement.LAST;
+    }
+    return ContextPlacement.MIDDLE;
+  };
+
   GrDiffBuilder.prototype._createContextControl = function(section, line) {
-    if (!line.contextGroups) return null;
+    const contextGroups = line.contextGroups;
-
+    if (!contextGroups) return null;
-    const numLines =
+    const numLines = contextGroups[contextGroups.length - 1].lineRange.left.end
-        line.contextGroups[line.contextGroups.length - 1].lineRange.left.end -
+                    - contextGroups[0].lineRange.left.start + 1;
-        line.contextGroups[0].lineRange.left.start + 1;
-
     if (numLines === 0) return null;
 
+    const contextPlacement = this._getContextControlPlacementFor(contextGroups);
+    const isStepBidirectional = (contextPlacement === ContextPlacement.MIDDLE);
+    const minimalForStepExpansion = isStepBidirectional ?
+        PARTIAL_CONTEXT_AMOUNT * 2 : PARTIAL_CONTEXT_AMOUNT;
+    const showStepExpansion = numLines > minimalForStepExpansion;
+
     const td = this._createElement('td');
-    const showPartialLinks = numLines > PARTIAL_CONTEXT_AMOUNT;
+    if (showStepExpansion) {
-
-    if (showPartialLinks) {
       td.appendChild(this._createContextButton(
-          GrDiffBuilder.ContextButtonType.ABOVE, section, line, numLines));
+          GrDiffBuilder.ContextButtonType.STEP, section, line,
-      td.appendChild(document.createTextNode(' - '));
+          numLines, contextPlacement));
     }
-
     td.appendChild(this._createContextButton(
         GrDiffBuilder.ContextButtonType.ALL, section, line, numLines));
 
-    if (showPartialLinks) {
-      td.appendChild(document.createTextNode(' - '));
-      td.appendChild(this._createContextButton(
-          GrDiffBuilder.ContextButtonType.BELOW, section, line, numLines));
-    }
-
     return td;
   };
 
   GrDiffBuilder.prototype._createContextButton = function(type, section, line,
-      numLines) {
+      numLines, contextPlacement) {
-    const context = PARTIAL_CONTEXT_AMOUNT;
-
     const button = this._createElement('gr-button', 'showContext');
     button.setAttribute('link', true);
     button.setAttribute('no-uppercase', true);
@@ -276,14 +294,14 @@
       text = 'Show ' + numLines + ' common line';
       if (numLines > 1) { text += 's'; }
       groups.push(...line.contextGroups);
-    } else if (type === GrDiffBuilder.ContextButtonType.ABOVE) {
+    } else if (type === GrDiffBuilder.ContextButtonType.STEP) {
-      text = '+' + context + '↑';
+      const linesToShowAbove = contextPlacement === ContextPlacement.FIRST ?
-      groups = GrDiffGroup.hideInContextControl(line.contextGroups,
+          0 : PARTIAL_CONTEXT_AMOUNT;
-          context, numLines);
+      const linesToShowBelow = contextPlacement === ContextPlacement.LAST ?
-    } else if (type === GrDiffBuilder.ContextButtonType.BELOW) {
+          0 : PARTIAL_CONTEXT_AMOUNT;
-      text = '+' + context + '↓';
+      text = '+' + PARTIAL_CONTEXT_AMOUNT + ' Lines';
-      groups = GrDiffGroup.hideInContextControl(line.contextGroups,
+      groups = GrDiffGroup.hideInContextControl(
-          0, numLines - context);
+          line.contextGroups, linesToShowAbove, numLines - linesToShowBelow);
     }
 
     Polymer.dom(button).textContent = text;

polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html

@@ -89,44 +89,92 @@ limitations under the License.
       assert.isTrue(node.classList.contains('classes'));
     });
 
-    test('context control buttons', () => {
+    function assertContextControl(buttons, expectedButtons) {
-      // Create 10 lines.
+      const actualButtonLabels = buttons.map(
+          b => Polymer.dom(b).textContent);
+      assert.deepEqual(actualButtonLabels, expectedButtons);
+    }
+
+    function createContextControl({numLines,
+                    firstInFile, lastInFile,
+                    expectStepExpansion}) {
       const lines = [];
-      for (let i = 0; i < 10; i++) {
+      for (let i = 0; i < numLines; i++) {
         const line = new GrDiffLine(GrDiffLine.Type.BOTH);
         line.beforeNumber = i + 1;
         line.afterNumber = i + 1;
         line.text = 'lorem upsum';
         lines.push(line);
       }
-
+      lines[0].firstInFile = !!firstInFile;
+      lines[lines.length - 1].lastInFile = !!lastInFile;
       const contextLine = {
         contextGroups: [new GrDiffGroup(GrDiffGroup.Type.BOTH, lines)],
       };
-
       const section = {};
+      const td = builder._createContextControl(section, contextLine);
+      return [...td.querySelectorAll('gr-button.showContext')];
+    }
+
+    function getGroupsAfterExpanding(button) {
+      let newGroups;
+      button.addEventListener('tap', e => { newGroups = e.detail.groups; });
+      MockInteractions.tap(button);
+      return newGroups;
+    }
+
+    test('context control buttons in the middle of the file', () => {
+      // Does not include +10 buttons when there are fewer than 21 lines.
+      let buttons = createContextControl({numLines: 20});
+      assertContextControl(buttons, ['Show 20 common lines']);
+
+      // Includes +10 buttons when there are at least 21 lines.
+      buttons = createContextControl({numLines: 21});
+      assertContextControl(buttons, ['+10 Lines', 'Show 21 common lines']);
+
+      // When clicked with 22 Lines expands in both directions with remaining context in the middle.
+
+      buttons = createContextControl({numLines: 22});
+      assertContextControl(buttons, ['+10 Lines', 'Show 22 common lines']);
+      const newGroupTypes = getGroupsAfterExpanding(buttons[0]).map(g => g.type);
+      assert.deepEqual(newGroupTypes,
+          [GrDiffLine.Type.BOTH, GrDiffLine.Type.CONTEXT_CONTROL,
+            GrDiffLine.Type.BOTH]);
+    });
+  
+    test('context control buttons in the beginning of the file', () => {
       // Does not include +10 buttons when there are fewer than 11 lines.
-      let td = builder._createContextControl(section, contextLine);
+      let buttons = createContextControl({numLines: 10, firstInFile: true});
-      let buttons = td.querySelectorAll('gr-button.showContext');
+      assertContextControl(buttons, ['Show 10 common lines']);
 
-      assert.equal(buttons.length, 1);
+      // Includes +10 button when there are at least 11 lines.
-      assert.equal(Polymer.dom(buttons[0]).textContent, 'Show 10 common lines');
+      buttons = createContextControl({numLines: 11, firstInFile: true});
+      assertContextControl(buttons, ['+10 Lines', 'Show 11 common lines']);
 
-      // Add another line.
+      // When clicked with 12 Lines expands only up and remaining context in the beginning of the file.
-      const line = new GrDiffLine(GrDiffLine.Type.BOTH);
+      buttons = createContextControl({numLines: 12, firstInFile: true});
-      line.text = 'lorem upsum';
+      assertContextControl(buttons, ['+10 Lines', 'Show 12 common lines']);
-      line.beforeNumber = 11;
+      const newGroupTypes = getGroupsAfterExpanding(buttons[0]).map(g => g.type);
-      line.afterNumber = 11;
+      assert.deepEqual(newGroupTypes,
-      contextLine.contextGroups[0].addLine(line);
+          [GrDiffLine.Type.CONTEXT_CONTROL, GrDiffLine.Type.BOTH]);
+    });
 
-      // Includes +10 buttons when there are at least 11 lines.
+    test('context control buttons in the end of the file', () => {
-      td = builder._createContextControl(section, contextLine);
+      // Does not include +10 buttons when there are fewer than 11 lines.
-      buttons = td.querySelectorAll('gr-button.showContext');
+      let buttons = createContextControl({numLines: 10, lastInFile: true});
+      assertContextControl(buttons, ['Show 10 common lines']);
 
-      assert.equal(buttons.length, 3);
+      // Includes +10 button when there are at least 11 lines.
-      assert.equal(Polymer.dom(buttons[0]).textContent, '+10↑');
+      buttons = createContextControl({numLines: 11, lastInFile: true});
-      assert.equal(Polymer.dom(buttons[1]).textContent, 'Show 11 common lines');
+      assertContextControl(buttons, ['+10 Lines', 'Show 11 common lines']);
-      assert.equal(Polymer.dom(buttons[2]).textContent, '+10↓');
+
+      // When clicked with 12 Lines expands only up and remaining context in the beginning of the file.
+      buttons = createContextControl({numLines: 12, lastInFile: true});
+      assertContextControl(buttons, ['+10 Lines', 'Show 12 common lines']);
+      // When clicked with 12 Lines expands only down and remaining context in the end of the file.
+      const newGroupTypes = getGroupsAfterExpanding(buttons[0]).map(g => g.type);
+      assert.deepEqual(newGroupTypes, [GrDiffLine.Type.BOTH,
+        GrDiffLine.Type.CONTEXT_CONTROL]);
     });
 
     test('newlines 1', () => {

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

@@ -66,7 +66,6 @@
     ADDED: 'edit_b',
     REMOVED: 'edit_a',
   };
-
   /**
    * The maximum size for an addition or removal chunk before it is broken down
    * into a series of chunks that are this size at most.
@@ -269,7 +268,8 @@
             right: this._linesRight(chunk).length,
           },
           groups: [this._chunkToGroup(
-              chunk, state.lineNums.left + 1, state.lineNums.right + 1)],
+              chunk, state.lineNums.left + 1, state.lineNums.right + 1,
+              /* firstInFile */ false, /* lastInFile */ false)],
           newChunkIndex: state.chunkIndex + 1,
         };
       }
@@ -321,10 +321,13 @@
       const lineCount = collapsibleChunks.reduce(
           (sum, chunk) => sum + this._commonChunkLength(chunk), 0);
 
+      const firstChunk = state.chunkIndex === 0;
+      const lastChunk = state.chunkIndex === chunks.length - 1;
       let groups = this._chunksToGroups(
           collapsibleChunks,
           state.lineNums.left + 1,
-          state.lineNums.right + 1);
+          state.lineNums.right + 1,
+          firstChunk, lastChunk);
 
       if (this.context !== WHOLE_FILE) {
         const hiddenStart = state.chunkIndex === 0 ? 0 : this.context;
@@ -357,11 +360,17 @@
      * @param {!Array<!Object>} chunks
      * @param {number} offsetLeft
      * @param {number} offsetRight
+     * @param {boolean} firstProcessed
+     * @param {boolean} lastProcessed
      * @return {!Array<!Object>} (GrDiffGroup)
      */
-    _chunksToGroups(chunks, offsetLeft, offsetRight) {
+    _chunksToGroups(chunks, offsetLeft, offsetRight, firstProcessed,
-      return chunks.map(chunk => {
+        lastProcessed) {
-        const group = this._chunkToGroup(chunk, offsetLeft, offsetRight);
+      return chunks.map((chunk, index) => {
+        const firstInFile = firstProcessed && index === 0;
+        const lastInFile = lastProcessed && index === chunks.length - 1;
+        const group = this._chunkToGroup(chunk, offsetLeft,
+            offsetRight, firstInFile, lastInFile);
         const chunkLength = this._commonChunkLength(chunk);
         offsetLeft += chunkLength;
         offsetRight += chunkLength;
@@ -375,9 +384,10 @@
      * @param {number} offsetRight
      * @return {!Object} (GrDiffGroup)
      */
-    _chunkToGroup(chunk, offsetLeft, offsetRight) {
+    _chunkToGroup(chunk, offsetLeft, offsetRight, firstInFile, lastInFile) {
       const type = chunk.ab ? GrDiffGroup.Type.BOTH : GrDiffGroup.Type.DELTA;
-      const lines = this._linesFromChunk(chunk, offsetLeft, offsetRight);
+      const lines = this._linesFromChunk(chunk, offsetLeft, offsetRight,
+          firstInFile, lastInFile);
       const group = new GrDiffGroup(type, lines);
       group.keyLocation = chunk.keyLocation;
       group.dueToRebase = chunk.due_to_rebase;
@@ -385,12 +395,12 @@
       return group;
     },
 
-    _linesFromChunk(chunk, offsetLeft, offsetRight) {
+    _linesFromChunk(chunk, offsetLeft, offsetRight, firstInFile, lastInFile) {
-      if (chunk.ab) {
-        return chunk.ab.map((row, i) => this._lineFromRow(
-            GrDiffLine.Type.BOTH, offsetLeft, offsetRight, row, i));
-      }
       let lines = [];
+      if (chunk.ab) {
+        lines = chunk.ab.map((row, i) => this._lineFromRow(
+            GrDiffLine.Type.BOTH, offsetLeft, offsetRight, row, i));
+      } else {
         if (chunk.a) {
           // Avoiding a.push(...b) because that causes callstack overflows for
           // large b, which can occur when large files are added removed.
@@ -405,6 +415,11 @@
               GrDiffLine.Type.ADD, chunk.b, offsetRight,
               chunk[DiffHighlights.ADDED]));
         }
+      }
+      if (lines.length > 0) {
+        lines[0].firstInFile = firstInFile;
+        lines[lines.length - 1].lastInFile = lastInFile;
+      }
       return lines;
     },
 

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

@@ -44,6 +44,12 @@
     this.contextGroups = null;
 
     this.text = '';
+
+    /** @type {boolean} */
+    this.firstInFile = false;
+
+    /** @type {boolean} */
+    this.lastInFile = false;
   }
 
   GrDiffLine.Type = {

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

@@ -187,6 +187,8 @@ limitations under the License.
       }
       .contextControl gr-button {
         display: inline-block;
+        margin-left: 1em;
+        margin-right: 1em;
         text-decoration: none;
         --gr-button: {
           color: var(--diff-context-control-color);