opendev/gerrit
Code Issues Proposed changes

Move diff view defaults to the rest api interface

Browse Source 
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
This commit is contained in:
Becky Siegel
2016-12-09 16:58:31 -08:00
parent f55bf18420
commit 76ec59681d
7 changed files with 125 additions and 103 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -265,7 +265,7 @@ limitations under the License.
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]]">

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

@@ -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;
},

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

@@ -553,12 +553,10 @@ limitations under the License.
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 @@ limitations under the License.
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();

36
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
View File

@@ -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() {

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

@@ -457,7 +457,7 @@ limitations under the License.
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 @@ limitations under the License.
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 @@ limitations under the License.
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>

21
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
View File

@@ -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,

73
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
View File

@@ -339,5 +339,78 @@ limitations under the License.
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>