Move diff view defaults to the rest api interface
Previously, there was logic regarding diff view defaults in both the
diff view and also the file list views. When the unified view became
the mobile default for the diff view, the file list was forgotten, and
if a user visited a change view first and then a diff view (without
refreshing the page) they wouldn't get defaulted to unified on mobile.
This change fixes the issue and moves the logic for which view type to
display to the rest interface, so that it doesn't have to be implemented
in multiple places.
Bug: Issue 5119
Change-Id: I95bfe1540cc9439bd6d3e3e39d13a5e32962b7fa
diff --git 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
index 6425acb..3577261 100644
--- 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
@@ -265,7 +265,7 @@
path="[[file.__path]]"
prefs="[[_diffPrefs]]"
project-config="[[projectConfig]]"
- view-mode="[[_diffMode]]"></gr-diff>
+ view-mode="[[diffViewMode]]"></gr-diff>
</template>
<div class="row totalChanges">
<div class="total-stats" hidden$="[[_hideChangeTotals]]">
diff --git 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
index 22a07b0..1980af0 100644
--- 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
@@ -16,11 +16,6 @@
var COMMIT_MESSAGE_PATH = '/COMMIT_MSG';
- var DiffViewMode = {
- SIDE_BY_SIDE: 'SIDE_BY_SIDE',
- UNIFIED: 'UNIFIED_DIFF',
- };
-
Polymer({
is: 'gr-file-list',
@@ -87,10 +82,6 @@
type: Array,
computed: '_computeFilesShown(_numFilesShown, _files.*)',
},
- _diffMode: {
- type: String,
- computed: '_getDiffViewMode(diffViewMode, _userPrefs)',
- },
// Caps the number of files that can be shown and have the 'show diffs' /
// 'hide diffs' buttons still be functional.
_maxFilesForBulkActions: {
@@ -148,16 +139,10 @@
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);
+ if (!this.diffViewMode) {
+ this.set('diffViewMode', prefs.default_diff_view);
}
}.bind(this)));
},
@@ -595,29 +580,6 @@
this._diffAgainst = patchRange.basePatchNum;
},
- /**
- * _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;
- },
-
_fileListActionsVisible: function(numFilesShown, maxFilesForBulkActions) {
return numFilesShown <= maxFilesForBulkActions;
},
diff --git 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
index 5da2995..6667f80 100644
--- 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
@@ -553,12 +553,10 @@
element.$.fileCursor.setCursorAtIndex(0);
flushAsynchronousOperations();
var diffDisplay = element.diffs[0];
- element._userPrefs = {diff_view: 'SIDE_BY_SIDE'};
- assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
+ element._userPrefs = {default_diff_view: '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');
});
@@ -579,7 +577,7 @@
assert.equal(select.value, 'SIDE_BY_SIDE');
// Receive the overriding preference.
- resolvePrefs({diff_view: 'UNIFIED'});
+ resolvePrefs({default_diff_view: 'UNIFIED'});
flushAsynchronousOperations();
assert.equal(select.value, 'SIDE_BY_SIDE');
document.getElementById('blank').restore();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
index f8f646a..b776226 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -16,13 +16,6 @@
var COMMIT_MESSAGE_PATH = '/COMMIT_MSG';
- var MAX_UNIFIED_DEFAULT_WINDOW_WIDTH_PX = 900;
-
- var DiffViewMode = {
- SIDE_BY_SIDE: 'SIDE_BY_SIDE',
- UNIFIED: 'UNIFIED_DIFF',
- };
-
var DiffSides = {
LEFT: 'left',
RIGHT: 'right',
@@ -125,16 +118,9 @@
}.bind(this));
if (this.changeViewState.diffMode === null) {
// If screen size is small, always default to unified view.
- if (this._getWindowWidth() < MAX_UNIFIED_DEFAULT_WINDOW_WIDTH_PX) {
- this.set('changeViewState.diffMode', DiffViewMode.UNIFIED);
- } else {
- // Initialize with user's diff mode preference. Default to
- // SIDE_BY_SIDE in the meantime.
- this.set('changeViewState.diffMode', DiffViewMode.SIDE_BY_SIDE);
- this.$.restAPI.getPreferences().then(function(prefs) {
- this.set('changeViewState.diffMode', prefs.diff_view);
- }.bind(this));
- }
+ this.$.restAPI.getPreferences().then(function(prefs) {
+ this.set('changeViewState.diffMode', prefs.default_diff_view);
+ }.bind(this));
}
if (this._path) {
@@ -572,9 +558,10 @@
* 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.
+ * preferences unless they have manually chosen the alternative view or they
+ * are on a mobile device. 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.
*
@@ -583,11 +570,12 @@
_getDiffViewMode: function() {
if (this.changeViewState.diffMode) {
return this.changeViewState.diffMode;
- } else if (this._userPrefs && this._userPrefs.diff_view) {
- return this.changeViewState.diffMode = this._userPrefs.diff_view;
+ } else if (this._userPrefs) {
+ return this.changeViewState.diffMode =
+ this._userPrefs.default_diff_view;
+ } else {
+ return 'SIDE_BY_SIDE';
}
-
- return DiffViewMode.SIDE_BY_SIDE;
},
_computeModeSelectHidden: function() {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
index c6ccb1b..c1ac981 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
@@ -457,7 +457,7 @@
test('diff mode selector correctly toggles the diff', function() {
var select = element.$.modeSelect;
var diffDisplay = element.$.diff;
- element._userPrefs = {diff_view: 'SIDE_BY_SIDE'};
+ element._userPrefs = {default_diff_view: 'SIDE_BY_SIDE'};
// The mode selected in the view state reflects the selected option.
assert.equal(element._getDiffViewMode(), select.value);
@@ -496,42 +496,11 @@
assert.equal(select.value, 'SIDE_BY_SIDE');
// Receive the overriding preference.
- resolvePrefs({diff_view: 'UNIFIED'});
+ resolvePrefs({default_diff_view: 'UNIFIED'});
flushAsynchronousOperations();
assert.equal(select.value, 'SIDE_BY_SIDE');
});
- test('unified view is always default on small screens', function() {
- var resolvePrefs;
- var prefsPromise = new Promise(function(resolve) {
- resolvePrefs = resolve;
- });
-
- var getPreferencesStub = sandbox.stub(element.$.restAPI, 'getPreferences',
- function() { return prefsPromise; });
-
- // Attach a new gr-diff-view so we can intercept the preferences fetch.
- var view = document.createElement('gr-diff-view');
-
- view.changeViewState = {diffMode: null};
-
- sandbox.stub(view, '_getWindowWidth', function() { return 800; });
-
- 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, 'UNIFIED_DIFF');
-
- // Receive the overriding preference.
- resolvePrefs({diff_view: 'SIDE_BY_SIDE'});
- flushAsynchronousOperations();
-
- // On small screens, unified should override user perferences
- assert.equal(select.value, 'UNIFIED_DIFF');
- });
-
test('_loadHash', function() {
assert.isNotOk(element.$.cursor.initialLineNumber);
@@ -598,5 +567,18 @@
assert.equal(element._getDiffURL(changeNum, patchRange, path),
'/c/123/123..456/c%252B%252B/cpp.cpp');
});
+
+ test('_getDiffViewMode', function() {
+ // No user prefs or change view state set.
+ assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
+
+ // User prefs but no change view state set.
+ element._userPrefs = {default_diff_view: 'UNIFIED_DIFF'};
+ assert.equal(element._getDiffViewMode(), 'UNIFIED_DIFF');
+
+ // User prefs and change view state set.
+ element.changeViewState = {diffMode: 'SIDE_BY_SIDE'};
+ assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
+ });
});
</script>
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index 48682e9..8cfa3ef 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -14,7 +14,12 @@
(function() {
'use strict';
+ var DiffViewMode = {
+ SIDE_BY_SIDE: 'SIDE_BY_SIDE',
+ UNIFIED: 'UNIFIED_DIFF',
+ };
var JSON_PREFIX = ')]}\'';
+ var MAX_UNIFIED_DEFAULT_WINDOW_WIDTH_PX = 900;
var PARENT_PATCH_NUM = 'PARENT';
// Must be kept in sync with the ListChangesOption enum and protobuf.
@@ -295,11 +300,21 @@
getPreferences: function() {
return this.getLoggedIn().then(function(loggedIn) {
if (loggedIn) {
- return this._fetchSharedCacheURL('/accounts/self/preferences');
+ return this._fetchSharedCacheURL('/accounts/self/preferences').then(
+ function(res) {
+ if (this._isNarrowScreen()) {
+ res.default_diff_view = DiffViewMode.UNIFIED;
+ } else {
+ res.default_diff_view = res.diff_view;
+ }
+ return Promise.resolve(res);
+ }.bind(this));
}
return Promise.resolve({
changes_per_page: 25,
+ default_diff_view: this._isNarrowScreen() ?
+ DiffViewMode.UNIFIED : DiffViewMode.SIDE_BY_SIDE,
diff_view: 'SIDE_BY_SIDE',
});
}.bind(this));
@@ -344,6 +359,10 @@
return this._sharedFetchPromises[url];
},
+ _isNarrowScreen: function() {
+ return window.innerWidth < MAX_UNIFIED_DEFAULT_WINDOW_WIDTH_PX;
+ },
+
getChanges: function(changesPerPage, opt_query, opt_offset) {
var options = this._listChangesOptionsToHex(
ListChangesOption.LABELS,
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
index d07968f..27d5d32 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
@@ -339,5 +339,78 @@
assert.isTrue(sendStub.called);
assert.notOk(element._cache[cacheKey]);
});
+
+ var preferenceSetup = function(testJSON, loggedIn, smallScreen) {
+ sandbox.stub(element, 'getLoggedIn', function() {
+ return Promise.resolve(loggedIn);
+ });
+ sandbox.stub(element, '_isNarrowScreen', function() {
+ return smallScreen;
+ });
+ sandbox.stub(element, '_fetchSharedCacheURL', function() {
+ return Promise.resolve(testJSON);
+ });
+ };
+
+ test('getPreferences returns correctly on small screens logged in',
+ function(done) {
+
+ var testJSON = {diff_view: 'SIDE_BY_SIDE'};
+ var loggedIn = true;
+ var smallScreen = true;
+
+ preferenceSetup(testJSON, loggedIn, smallScreen);
+
+ element.getPreferences().then(function(obj) {
+ assert.equal(obj.default_diff_view, 'UNIFIED_DIFF');
+ assert.equal(obj.diff_view, 'SIDE_BY_SIDE');
+ done();
+ });
+ });
+
+ test('getPreferences returns correctly on small screens not logged in',
+ function(done) {
+
+ var testJSON = {diff_view: 'SIDE_BY_SIDE'};
+ var loggedIn = false;
+ var smallScreen = true;
+
+ preferenceSetup(testJSON, loggedIn, smallScreen);
+ element.getPreferences().then(function(obj) {
+ assert.equal(obj.default_diff_view, 'UNIFIED_DIFF');
+ assert.equal(obj.diff_view, 'SIDE_BY_SIDE');
+ done();
+ });
+ });
+
+ test('getPreferences returns correctly on larger screens logged in',
+ function(done) {
+ var testJSON = {diff_view: 'UNIFIED_DIFF'};
+ var loggedIn = true;
+ var smallScreen = false;
+
+ preferenceSetup(testJSON, loggedIn, smallScreen);
+
+ element.getPreferences().then(function(obj) {
+ assert.equal(obj.default_diff_view, 'UNIFIED_DIFF');
+ assert.equal(obj.diff_view, 'UNIFIED_DIFF');
+ done();
+ });
+ });
+
+ test('getPreferences returns correctly on larger screens not logged in',
+ function(done) {
+ var testJSON = {diff_view: 'UNIFIED_DIFF'};
+ var loggedIn = false;
+ var smallScreen = false;
+
+ preferenceSetup(testJSON, loggedIn, smallScreen);
+
+ element.getPreferences().then(function(obj) {
+ assert.equal(obj.default_diff_view, 'SIDE_BY_SIDE');
+ assert.equal(obj.diff_view, 'SIDE_BY_SIDE');
+ done();
+ });
+ });
});
</script>
