Fix data binding in gr-file-list, introduced in 86117

In https://gerrit-review.googlesource.com/c/86117/ the data binding between the _files array and the dom-repeat in gr-file-list was not properly enforced. This change fixes that, and also properly references a static file-list property. In addition, tests were implemented to ensure this regression is caught in the future. Bug: Issue 4575 Change-Id: I32a5645ec47d953ee6a7a71f4b14fc8c9483df3d
2016-09-27 10:51:27 -07:00
parent 1b944dffd8
commit bf457dd412
5 changed files with 114 additions and 17 deletions
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -19,6 +19,7 @@ limitations under the License.
 <link rel="import" href="../../diff/gr-diff/gr-diff.html">
 <link rel="import" href="../../diff/gr-diff-cursor/gr-diff-cursor.html">
 <link rel="import" href="../../shared/gr-button/gr-button.html">
+<link rel="import" href="../../shared/gr-linked-text/gr-linked-text.html">
 <link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">

 <dom-module id="gr-file-list">
@@ -103,6 +104,9 @@ limitations under the License.
      .show-hide {
        margin-left: .4em;
      }
+      .fileListButton {
+        margin: .5em;
+      }
      input.show-hide {
        display: none;
      }
@@ -156,7 +160,10 @@ limitations under the License.
        </label>
      </div>
    </header>
-    <template is="dom-repeat" items="[[_files]]" as="file" initial-count="25" target-framerate="30">
+    <template is="dom-repeat"
+        items="[[_shownFiles]]"
+        as="file"
+        initial-count="[[_fileListIncrement]]">
      <div class="row" selected$="[[_computeFileSelected(index, selectedIndex)]]">
        <div class="reviewed" hidden$="[[!_loggedIn]]" hidden>
          <input type="checkbox" checked$="[[_computeReviewed(file, _reviewed)]]"
@@ -203,6 +210,20 @@ limitations under the License.
          project-config="[[projectConfig]]"
          view-mode="[[_userPrefs.diff_view]]"></gr-diff>
    </template>
+    <gr-button
+        class="fileListButton"
+        id="incrementButton"
+        hidden$="[[_computeFileListButtonHidden(_numFilesShown, _files)]]"
+        link on-tap="_incrementNumFilesShown">
+      [[_computeIncrementText(_numFilesShown, _files)]]
+    </gr-button>
+    <gr-button
+        class="fileListButton"
+        id="showAllButton"
+        hidden$="[[_computeFileListButtonHidden(_numFilesShown, _files)]]"
+        link on-tap="_showAllFiles">
+      [[_computeShowAllText(_files)]]
+    </gr-button>
    <gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
    <gr-storage id="storage"></gr-storage>
    <gr-diff-cursor id="cursor"></gr-diff-cursor>
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -40,6 +40,7 @@
      _files: {
        type: Array,
        observer: '_filesChanged',
+        value: function() { return []; },
      },
      _loggedIn: {
        type: Boolean,
@@ -53,6 +54,19 @@
      _userPrefs: Object,
      _localPrefs: Object,
      _showInlineDiffs: Boolean,
+      _numFilesShown: {
+        type: Number,
+        value: 75,
+      },
+      _fileListIncrement: {
+        type: Number,
+        readOnly: true,
+        value: 75,
+      },
+      _shownFiles: {
+        type: Array,
+        computed: '_computeFilesShown(_numFilesShown, _files.*)',
+      },
    },

    behaviors: [
@@ -63,9 +77,7 @@
      if (!this.changeNum || !this.patchRange.patchNum) {
        return Promise.resolve();
      }
-
      this._collapseAllDiffs();
-
      var promises = [];
      var _this = this;

@@ -139,10 +151,18 @@
      }
    },

+    /**
+     * Until upgrading to Polymer 2.0, manual management of reflection between
+     * _shownFiles and _files is necessary. Performance of linkPaths is very
+     * poor.
+     */
    _expandAllDiffs: function(e) {
      this._showInlineDiffs = true;
-      for (var index in this._files) {
-        this.set(['_files', index, '__expanded'], true);
+      for (var i = 0; i < this._files.length; i++) {
+        if (i < this._shownFiles.length) {
+          this.set(['_shownFiles', i, '__expanded'], true);
+        }
+        this.set(['_files', i, '__expanded'], true);
      }
      if (e && e.target) {
        e.target.blur();
@@ -151,8 +171,11 @@

    _collapseAllDiffs: function(e) {
      this._showInlineDiffs = false;
-      for (var index in this._files) {
-        this.set(['_files', index, '__expanded'], false);
+      for (var i = 0; i < this._files.length; i++) {
+        if (i < this._shownFiles.length) {
+          this.set(['_shownFiles', i, '__expanded'], false);
+        }
+        this.set(['_files', i, '__expanded'], false);
      }
      this.$.cursor.handleDiffUpdate();
      if (e && e.target) {
@@ -247,6 +270,10 @@
          } else if (this.selectedIndex !== undefined) {
            e.preventDefault();
            var expanded = this._files[this.selectedIndex].__expanded;
+            // Until Polymer 2.0, manual management of reflection between _files
+            // and _shownFiles is necessary.
+            this.set(['_shownFiles', this.selectedIndex, '__expanded'],
+                !expanded);
            this.set(['_files', this.selectedIndex, '__expanded'], !expanded);
          }
          break;
@@ -257,7 +284,7 @@
            this.$.cursor.moveDown();
          } else {
            this.selectedIndex =
-                Math.min(this._files.length - 1, this.selectedIndex + 1);
+                Math.min(this._numFilesShown, this.selectedIndex + 1);
            this._scrollToSelectedFile();
          }
          break;
@@ -422,6 +449,10 @@
      return !expanded;
    },

+    _computeFilesShown: function(numFilesShown, files) {
+      return files.base.slice(0, numFilesShown);
+    },
+
    _filesChanged: function() {
      this.async(function() {
        var diffElements = Polymer.dom(this.root).querySelectorAll('gr-diff');
@@ -431,5 +462,29 @@
            ['diffs', 0, this.$.cursor.diffs.length].concat(diffElements));
      }.bind(this), 1);
    },
+
+    _incrementNumFilesShown: function() {
+      this._numFilesShown += this._fileListIncrement;
+    },
+
+    _computeFileListButtonHidden: function(numFilesShown, files) {
+      return numFilesShown >= files.length;
+    },
+
+    _computeIncrementText: function(numFilesShown, files) {
+      if (!files) { return ''; }
+      var text =
+          Math.min(this._fileListIncrement, files.length - numFilesShown);
+      return 'Show ' + text + ' more';
+    },
+
+    _computeShowAllText: function(files) {
+      if (!files) { return ''; }
+      return 'Show all ' + files.length + ' files';
+    },
+
+    _showAllFiles: function() {
+      this._numFilesShown = this._files.length;
+    },
  });
 })();
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
@@ -106,11 +106,6 @@ limitations under the License.
      });

      test('keyboard shortcuts', function() {
-        var toggleInlineDiffsStub = sinon.stub(element, '_toggleInlineDiffs');
-        MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift');  // 'I'
-        assert.isTrue(toggleInlineDiffsStub.calledOnce);
-        toggleInlineDiffsStub.restore();
-
        flushAsynchronousOperations();
        var elementItems = Polymer.dom(element.root).querySelectorAll(
            '.row:not(.header)');
@@ -145,12 +140,26 @@ limitations under the License.
      test('i key shows/hides selected inline diff', function() {
        element.selectedIndex = 0;
        MockInteractions.pressAndReleaseKeyOn(element, 73);  // 'I'
-        assert.isTrue(element._files[0].__expanded);
+        flushAsynchronousOperations();
+        assert.isFalse(element.diffs[0].hasAttribute('hidden'));
        MockInteractions.pressAndReleaseKeyOn(element, 73);  // 'I'
-        assert.isFalse(element._files[0].__expanded);
+        flushAsynchronousOperations();
+        assert.isTrue(element.diffs[0].hasAttribute('hidden'));
        element.selectedIndex = 1;
        MockInteractions.pressAndReleaseKeyOn(element, 73);  // 'I'
-        assert.isTrue(element._files[1].__expanded);
+        flushAsynchronousOperations();
+        assert.isFalse(element.diffs[1].hasAttribute('hidden'));
+
+        MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift');  // 'I'
+        flushAsynchronousOperations();
+        for (var index in element.diffs) {
+          assert.isFalse(element.diffs[index].hasAttribute('hidden'));
+        }
+        MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift');  // 'I'
+        flushAsynchronousOperations();
+        for (var index in element.diffs) {
+          assert.isTrue(element.diffs[index].hasAttribute('hidden'));
+        }
      });
    });

--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
@@ -149,7 +149,11 @@ limitations under the License.
          </gr-button>
          <iron-dropdown id="dropdown" vertical-align="top" vertical-offset="25">
            <div class="dropdown-content">
-              <template is="dom-repeat" items="[[_fileList]]" as="path">
+              <template
+                  is="dom-repeat"
+                  items="[[_fileList]]"
+                  as="path"
+                  initial-count="75">
                <a href$="[[_computeDiffURL(_changeNum, _patchRange.*, path)]]"
                   selected$="[[_computeFileSelected(path, _path)]]"
                   data-key-nav$="[[_computeKeyNav(path, _path, _fileList)]]"
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -88,6 +88,14 @@
      this._getLoggedIn().then(function(loggedIn) {
        this._loggedIn = loggedIn;
      }.bind(this));
+
+    },
+
+    ready: function() {
+      // Reload only if the diff is not hidden and params are supplied.
+      if (this.changeNum && this.patchRange && this.path && !this.hidden) {
+        this.reload();
+      }
    },

    _handleShowDiff: function(hidden) {