Merge "Add dropdown for selecting diff view mode to file list"

polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html

@@ -302,7 +302,8 @@ limitations under the License.
             drafts="[[_diffDrafts]]"
             revisions="[[_change.revisions]]"
             projectConfig="[[_projectConfig]]"
-            selected-index="{{viewState.selectedFileIndex}}"></gr-file-list>
+            selected-index="{{viewState.selectedFileIndex}}"
+            diff-view-mode="{{viewState.diffMode}}"></gr-file-list>
       </section>
       <gr-messages-list id="messageList"
           change-num="[[_changeNum]]"

polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html

@@ -147,9 +147,18 @@ limitations under the License.
         /
         <gr-button link on-tap="_collapseAllDiffs">Hide diffs</gr-button>
         /
+        <select
+            id="modeSelect"
+            is="gr-select"
+            bind-value="{{diffViewMode}}"
+            on-change="_handleDropdownChange">
+          <option value="SIDE_BY_SIDE">Side By Side</option>
+          <option value="UNIFIED_DIFF">Unified</option>
+        </select>
+        /
         <label>
           Diff against
-          <select on-change="_handlePatchChange">
+          <select id="patchChange" on-change="_handlePatchChange">
             <option value="PARENT">Base</option>
             <template is="dom-repeat" items="[[_computePatchSets(revisions, patchRange.*)]]" as="patchNum">
               <option
@@ -210,7 +219,7 @@ limitations under the License.
           path="[[file.__path]]"
           prefs="[[_diffPrefs]]"
           project-config="[[projectConfig]]"
-          view-mode="[[_userPrefs.diff_view]]"></gr-diff>
+          view-mode="[[_diffMode]]"></gr-diff>
     </template>
     <gr-button
         class="fileListButton"

polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js

@@ -16,6 +16,11 @@
 
   var COMMIT_MESSAGE_PATH = '/COMMIT_MSG';
 
+  var DiffViewMode = {
+    SIDE_BY_SIDE: 'SIDE_BY_SIDE',
+    UNIFIED: 'UNIFIED_DIFF',
+  };
+
   Polymer({
     is: 'gr-file-list',
 
@@ -36,7 +41,10 @@
         value: function() { return document.body; },
       },
       change: Object,
-
+      diffViewMode: {
+        type: String,
+        notify: true,
+      },
       _files: {
         type: Array,
         observer: '_filesChanged',
@@ -67,6 +75,10 @@
         type: Array,
         computed: '_computeFilesShown(_numFilesShown, _files.*)',
       },
+      _diffMode: {
+        type: String,
+        computed: '_getDiffViewMode(diffViewMode, _userPrefs)',
+      },
     },
 
     behaviors: [
@@ -100,8 +112,17 @@
         this._diffPrefs = prefs;
       }.bind(this)));
 
+      // Initialize with user's diff mode preference. Default to
+      // SIDE_BY_SIDE in the meantime.
+      var setDiffViewMode = this.diffViewMode === null;
+      if (setDiffViewMode) {
+        this.set('diffViewMode', DiffViewMode.SIDE_BY_SIDE);
+      }
       promises.push(this._getPreferences().then(function(prefs) {
         this._userPrefs = prefs;
+        if (setDiffViewMode) {
+          this.set('diffViewMode', prefs.diff_view);
+        }
       }.bind(this)));
     },
 
@@ -474,5 +495,32 @@
     _showAllFiles: function() {
       this._numFilesShown = this._files.length;
     },
+
+    /**
+     * _getDiffViewMode: Get the diff view (side-by-side or unified) based on
+     * the current state.
+     *
+     * The expected behavior is to use the mode specified in the user's
+     * preferences unless they have manually chosen the alternative view. If the
+     * user navigates up to the change view, it should clear this choice and
+     * revert to the preference the next time a diff is viewed.
+     *
+     * Use side-by-side if the user is not logged in.
+     *
+     * @return {String}
+     */
+    _getDiffViewMode: function() {
+      if (this.diffViewMode) {
+        return this.diffViewMode;
+      } else if (this._userPrefs && this._userPrefs.diff_view) {
+        return this.diffViewMode = this._userPrefs.diff_view;
+      }
+
+      return DiffViewMode.SIDE_BY_SIDE;
+    },
+
+    _handleDropdownChange: function(e) {
+      e.target.blur();
+    },
   });
 })();

polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html

@@ -32,20 +32,36 @@ limitations under the License.
   </template>
 </test-fixture>
 
+<test-fixture id="blank">
+  <template>
+    <div></div>
+  </template>
+</test-fixture>
+
 <script>
   suite('gr-file-list tests', function() {
     var element;
+    var sandbox;
 
     setup(function() {
+      sandbox = sinon.sandbox.create();
       stub('gr-rest-api-interface', {
         getLoggedIn: function() { return Promise.resolve(true); },
         getPreferences: function() { return Promise.resolve({}); },
+        fetchJSON: function() { return Promise.resolve({}); },
+      });
+      stub('gr-date-formatter', {
+        _loadTimeFormat: function() { return Promise.resolve(''); }
       });
       element = fixture('basic');
     });
 
+    teardown(function() {
+      sandbox.restore();
+    });
+
     test('get file list', function(done) {
-      var getChangeFilesStub = sinon.stub(element.$.restAPI, 'getChangeFiles',
+      var getChangeFilesStub = sandbox.stub(element.$.restAPI, 'getChangeFiles',
           function() {
             return Promise.resolve({
               '/COMMIT_MSG': {lines_inserted: 9},
@@ -97,12 +113,15 @@ limitations under the License.
       });
 
       test('toggle left diff via shortcut', function() {
-        var toggleLeftDiffStub = sinon.stub();
+        var toggleLeftDiffStub = sandbox.stub();
-        sinon.stub(element, 'diffs', {get: function() {
+        // Property getter cannot be stubbed w/ sandbox due to a bug in Sinon.
+        // https://github.com/sinonjs/sinon/issues/781
+        var diffsStub = sinon.stub(element, 'diffs', {get: function() {
           return [{toggleLeftDiff: toggleLeftDiffStub}];
         }});
         MockInteractions.pressAndReleaseKeyOn(element, 65, 'shift');  // 'A'
         assert.isTrue(toggleLeftDiffStub.calledOnce);
+        diffsStub.restore();
       });
 
       test('keyboard shortcuts', function() {
@@ -117,7 +136,7 @@ limitations under the License.
         assert.equal(element.selectedIndex, 1);
         MockInteractions.pressAndReleaseKeyOn(element, 74);  // 'J'
 
-        var showStub = sinon.stub(page, 'show');
+        var showStub = sandbox.stub(page, 'show');
         assert.equal(element.selectedIndex, 2);
         MockInteractions.pressAndReleaseKeyOn(element, 13);  // 'ENTER'
         assert(showStub.lastCall.calledWith('/c/42/2/myfile.txt'),
@@ -241,7 +260,7 @@ limitations under the License.
       assert.isFalse(fileAdded.checked);
       assert.isTrue(myFile.checked);
 
-      var saveStub = sinon.stub(element, '_saveReviewedState',
+      var saveStub = sandbox.stub(element, '_saveReviewedState',
           function() { return Promise.resolve(); });
 
       MockInteractions.tap(commitMsg);
@@ -272,7 +291,7 @@ limitations under the License.
     });
 
     test('diff against dropdown', function(done) {
-      var showStub = sinon.stub(page, 'show');
+      var showStub = sandbox.stub(page, 'show');
       element.changeNum = '42';
       element.patchRange = {
         basePatchNum: 'PARENT',
@@ -284,7 +303,7 @@ limitations under the License.
         rev3: {_number: 3},
       };
       flush(function() {
-        var selectEl = element.$$('select');
+        var selectEl = element.$.patchChange;
         assert.equal(selectEl.value, 'PARENT');
         assert.isTrue(element.$$('option[value="3"]').hasAttribute('disabled'));
         selectEl.addEventListener('change', function() {
@@ -313,7 +332,7 @@ limitations under the License.
       var fileRows =
           Polymer.dom(element.root).querySelectorAll('.row:not(.header)');
       // Prevent diff from making API call.
-      var diffStub = sinon.stub(element.diffs[0], 'reload');
+      var diffStub = sandbox.stub(element.diffs[0], 'reload');
       var showHideCheck = fileRows[0].querySelector(
           'input.show-hide[type="checkbox"]');
       assert.isTrue(showHideCheck.checked);
@@ -339,5 +358,50 @@ limitations under the License.
           element.$$('a').getAttribute('href'),
           '/c/42/2/foo+bar/my%252Bfile.txt%2525');
     });
+
+    test('diff mode correctly toggles the diffs', function() {
+      element._files = [
+        {__path: 'myfile.txt', __expanded: false},
+      ];
+      element.changeNum = '42';
+      element.patchRange = {
+        basePatchNum: 'PARENT',
+        patchNum: '2',
+      };
+      element.selectedIndex = 0;
+      flushAsynchronousOperations();
+      var select = element.$.modeSelect;
+      var diffDisplay = element.diffs[0];
+      element._userPrefs = {diff_view: 'SIDE_BY_SIDE'};
+      assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
+      assert.equal(element.diffViewMode, 'SIDE_BY_SIDE');
+      assert.equal(diffDisplay.viewMode, 'SIDE_BY_SIDE');
+      element.set('diffViewMode', 'UNIFIED_DIFF');
+      assert.equal(element._getDiffViewMode(), 'UNIFIED_DIFF');
+      assert.equal(diffDisplay.viewMode, 'UNIFIED_DIFF');
+    });
+
+    test('diff mode selector initializes from preferences', function() {
+      var resolvePrefs;
+      var prefsPromise = new Promise(function(resolve) {
+        resolvePrefs = resolve;
+      });
+      sandbox.stub(element, '_getPreferences').returns(prefsPromise);
+
+      // Attach a new gr-file-list so we can intercept the preferences fetch.
+      var view = document.createElement('gr-file-list');
+      var select = view.$.modeSelect;
+      fixture('blank').appendChild(view);
+      flushAsynchronousOperations();
+
+      // At this point the diff mode doesn't yet have the user's preference.
+      assert.equal(select.value, 'SIDE_BY_SIDE');
+
+      // Receive the overriding preference.
+      resolvePrefs({diff_view: 'UNIFIED'});
+      flushAsynchronousOperations();
+      assert.equal(select.value, 'SIDE_BY_SIDE');
+      document.getElementById('blank').restore();
+    });
   });
 </script>

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

@@ -104,7 +104,6 @@
           this._setReviewed(true);
         }
       }.bind(this));
-
       if (this.changeViewState.diffMode === null) {
         // Initialize with user's diff mode preference. Default to
         // SIDE_BY_SIDE in the meantime.

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

@@ -457,7 +457,6 @@ limitations under the License.
 
       // We will simulate a user change of the selected mode.
       var newMode = 'UNIFIED_DIFF';
-
       // Set the actual value of the select, and simulate the change event.
       select.value = newMode;
       element.fire('change', {}, {node: select});