Merge "Move the key positions calculation up from gr-diff-builder to gr-diff"

2018-11-15 18:03:20 +00:00
parent 0c8c1c2771 ace16472a8
commit 516f4e1113
5 changed files with 95 additions and 97 deletions
--- 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
@@ -51,16 +51,6 @@ 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',
@@ -115,10 +105,6 @@ limitations under the License.
          parentIndex: Number,
          path: String,
          projectName: String,
-          /**
-           * @type {Defs.LineOfInterest|null}
-           */
-          lineOfInterest: Object,

          _builder: Object,
          _groups: Array,
@@ -161,7 +147,7 @@ limitations under the License.
          this._layers = layers;
        },

-        render(comments, prefs) {
+        render(keyLocations, prefs) {
          this.$.syntaxLayer.enabled = prefs.syntax_highlighting;
          this._showTabs = !!prefs.show_tabs;
          this._showTrailingWhitespace = !!prefs.show_whitespace_errors;
@@ -172,8 +158,7 @@ limitations under the License.
          this._builder = this._getDiffBuilder(this.diff, prefs);

          this.$.processor.context = prefs.context;
-          this.$.processor.keyLocations = this._getKeyLocations(comments,
-              this.lineOfInterest);
+          this.$.processor.keyLocations = keyLocations;

          this._clearDiffContent();
          this._builder.addColumns(this.diffElement, prefs.font_size);
@@ -332,33 +317,6 @@ limitations under the License.
          this.diffElement.innerHTML = null;
        },

-        /**
-         * @param {!Object} comments
-         * @param {Defs.LineOfInterest|null} lineOfInterest
-         */
-        _getKeyLocations(comments, lineOfInterest) {
-          const result = {
-            left: {},
-            right: {},
-          };
-          for (const side in comments) {
-            if (side !== GrDiffBuilder.Side.LEFT &&
-                side !== GrDiffBuilder.Side.RIGHT) {
-              continue;
-            }
-            for (const c of comments[side]) {
-              result[side][c.line || GrDiffLine.FILE] = true;
-            }
-          }
-
-          if (lineOfInterest) {
-            const side = lineOfInterest.leftSide ? 'left' : 'right';
-            result[side][lineOfInterest.number] = true;
-          }
-
-          return result;
-        },
-
        _groupsChanged(changeRecord) {
          if (!changeRecord) { return; }
          for (const splice of changeRecord.indexSplices) {
--- 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
@@ -396,33 +396,6 @@ 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);
@@ -852,7 +825,7 @@ limitations under the License.

    suite('rendering text, images and binary files', () => {
      let processStub;
-      let comments;
+      let keyLocations;
      let prefs;
      let content;

@@ -862,7 +835,7 @@ limitations under the License.
        processStub = sandbox.stub(element.$.processor, 'process')
            .returns(Promise.resolve());
        sandbox.stub(element, '_anyLineTooLong').returns(true);
-        comments = {left: [], right: [], meta: {patchRange: undefined}};
+        keyLocations = {left: {}, right: {}};
        prefs = {
          line_length: 10,
          show_tabs: true,
@@ -883,7 +856,7 @@ limitations under the License.

      test('text', () => {
        element.diff = {content};
-        return element.render(comments, prefs).then(() => {
+        return element.render(keyLocations, prefs).then(() => {
          assert.isTrue(processStub.calledOnce);
          assert.isFalse(processStub.lastCall.args[1]);
        });
@@ -892,7 +865,7 @@ limitations under the License.
      test('image', () => {
        element.diff = {content, binary: true};
        element.isImageDiff = true;
-        return element.render(comments, prefs).then(() => {
+        return element.render(keyLocations, prefs).then(() => {
          assert.isTrue(processStub.calledOnce);
          assert.isTrue(processStub.lastCall.args[1]);
        });
@@ -900,7 +873,7 @@ limitations under the License.

      test('binary', () => {
        element.diff = {content, binary: true};
-        return element.render(comments, prefs).then(() => {
+        return element.render(keyLocations, prefs).then(() => {
          assert.isTrue(processStub.calledOnce);
          assert.isTrue(processStub.lastCall.args[1]);
        });
@@ -910,7 +883,7 @@ limitations under the License.
    suite('rendering', () => {
      let content;
      let outputEl;
-      let comments;
+      let keyLocations;

      setup(done => {
        const prefs = {
@@ -938,7 +911,7 @@ limitations under the License.
        });
        element = fixture('basic');
        outputEl = element.queryEffectiveChildren('#diffTable');
-        comments = {left: [], right: [], meta: {patchRange: undefined}};
+        keyLocations = {left: {}, right: {}};
        sandbox.stub(element, '_getDiffBuilder', () => {
          const builder = new GrDiffBuilder({content}, [], prefs, outputEl);
          sandbox.stub(builder, 'addColumns');
@@ -952,7 +925,7 @@ limitations under the License.
          return builder;
        });
        element.diff = {content};
-        element.render(comments, prefs).then(done);
+        element.render(keyLocations, prefs).then(done);
      });

      test('reporting', done => {
@@ -977,7 +950,7 @@ limitations under the License.
      });

      test('addColumns is called', done => {
-        element.render(comments, {}).then(done);
+        element.render(keyLocations, {}).then(done);
        assert.isTrue(element._builder.addColumns.called);
      });

@@ -1001,7 +974,7 @@ limitations under the License.

      test('render-start and render are fired', done => {
        const dispatchEventStub = sandbox.stub(element, 'dispatchEvent');
-        element.render(comments, {}).then(() => {
+        element.render(keyLocations, {}).then(() => {
          const firedEventTypes = dispatchEventStub.getCalls()
              .map(c => { return c.args[0].type; });
          assert.include(firedEventTypes, 'render-start');
@@ -1029,7 +1002,7 @@ limitations under the License.
          context: -1,
          syntax_highlighting: true,
        };
-        element.render(comments, prefs);
+        element.render(keyLocations, prefs);
      });

      test('cancel', () => {
@@ -1046,7 +1019,7 @@ limitations under the License.
      let builder;
      let diff;
      let prefs;
-      let comments;
+      let keyLocations;

      setup(done => {
        element = fixture('mock-diff');
@@ -1058,9 +1031,9 @@ limitations under the License.
          show_tabs: true,
          tab_size: 4,
        };
-        comments = {left: [], right: [], meta: {patchRange: undefined}};
+        keyLocations = {left: {}, right: {}};

-        element.render(comments, prefs).then(() => {
+        element.render(keyLocations, prefs).then(() => {
          builder = element._builder;
          done();
        });
@@ -1170,7 +1143,7 @@ limitations under the License.
      test('_getNextContentOnSide unified left', done => {
        // Re-render as unified:
        element.viewMode = 'UNIFIED_DIFF';
-        element.render(comments, prefs).then(() => {
+        element.render(keyLocations, prefs).then(() => {
          builder = element._builder;

          const startElem = builder.getContentByLine(5, 'left',
@@ -1190,7 +1163,7 @@ limitations under the License.
      test('_getNextContentOnSide unified right', done => {
        // Re-render as unified:
        element.viewMode = 'UNIFIED_DIFF';
-        element.render(comments, prefs).then(() => {
+        element.render(keyLocations, prefs).then(() => {
          builder = element._builder;

          const startElem = builder.getContentByLine(5, 'right',
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -293,8 +293,7 @@ limitations under the License.
              line-wrapping="[[lineWrapping]]"
              is-image-diff="[[isImageDiff]]"
              base-image="[[baseImage]]"
-              revision-image="[[revisionImage]]"
-              line-of-interest="[[lineOfInterest]]">
+              revision-image="[[revisionImage]]">
            <slot></slot>
            <table
                id="diffTable"
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -35,6 +35,18 @@
    RIGHT: 'right',
  };

+  const Defs = {};
+
+  /**
+   * Special line number which should not be collapsed into a shared region.
+   *
+   * @typedef {{
+   *  number: number,
+   *  leftSide: boolean
+   * }}
+   */
+  Defs.LineOfInterest;
+
  const LARGE_DIFF_THRESHOLD_LINES = 10000;
  const FULL_CONTEXT = -1;
  const LIMITED_CONTEXT = 10;
@@ -121,13 +133,7 @@
        observer: '_viewModeObserver',
      },

-      /**
-       * Special line number which should not be collapsed into a shared region.
-       * @type {{
-       *  number: number,
-       *  leftSide: {boolean}
-       * }|null}
-       */
+       /** @type ?Defs.LineOfInterest */
      lineOfInterest: Object,

      loading: {
@@ -639,7 +645,9 @@
      }

      this._showWarning = false;
-      this.$.diffBuilder.render(this.comments, this._getBypassPrefs());
+      const keyLocations = this._getKeyLocations(this.comments,
+          this.lineOfInterest);
+      this.$.diffBuilder.render(keyLocations, this._getBypassPrefs());
    },

    _handleRenderContent() {
@@ -675,6 +683,39 @@
      }
    },

+    /**
+     * Returns the key locations based on the comments and line of interests,
+     * where lines should not be collapsed.
+     *
+     * @param {!Object} comments
+     * @param {Defs.LineOfInterest|null} lineOfInterest
+     *
+     * @return {{left: Object<(string|number), boolean>,
+     *     right: Object<(string|number), boolean>}}
+     */
+    _getKeyLocations(comments, lineOfInterest) {
+      const result = {
+        left: {},
+        right: {},
+      };
+      for (const side in comments) {
+        if (side !== GrDiffBuilder.Side.LEFT &&
+            side !== GrDiffBuilder.Side.RIGHT) {
+          continue;
+        }
+        for (const c of comments[side]) {
+          result[side][c.line || GrDiffLine.FILE] = true;
+        }
+      }
+
+      if (lineOfInterest) {
+        const side = lineOfInterest.leftSide ? 'left' : 'right';
+        result[side][lineOfInterest.number] = true;
+      }
+
+      return result;
+    },
+
    /**
     * Get the preferences object including the safety bypass context (if any).
     */
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -1127,6 +1127,33 @@ limitations under the License.
        assert.equal(element._computeNewlineWarningClass('foo', false), shown);
      });
    });
+
+    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},
+          });
+    });
  });

  a11ySuite('basic');