opendev/gerrit
Code Issues Proposed changes

Add toggle to PolyGerrit diff view to switch between diff styles

Browse Source 
The gr-diff element was already capable of rendering diffs in either
side-by-side or unified style, but was configured by default to use
side-by-side. The GWT diff UI included an icon button toggle to switch
between these two styles above and to the right of the diff near the
"Diff View Preferences" button. This change introduces a selector in the
PolyGerrit UI to allow similar switching behavior.

Previously, the diff mode was encoded as a private property of the
gr-diff element. Now that the switcher is introduced at the same level
as the "Diff View Preferences" button (one level up from the gr-diff
element) it is changed to be a public property of the element so that it
can be changed through the parent view (in this case the gr-diff-view
element).

A dropdown is added to the gr-diff-view based in style on the
gr-patch-range-select element (which plays a similar role). This
dropdown has options for either of the two diff modes and controls the
diff display through a property on the view.

The view gets a new, computed property called _diffMode which encodes
which diff state should be displayed based on the changeViewState and
the user's preferences. The user's choice of diff view is persisted
across diff views, but is reverted to the preference when the change
view is visited.

A test is added to the gr-diff-view element.

Bug: Issue 3909
Change-Id: I00d93500f8d210394acc9c8f3398c9406ae74276
This commit is contained in:
Wyatt Allen
2016-05-05 16:53:38 -07:00
parent 6ed795da42
commit f93f1c24ee
7 changed files with 115 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -103,6 +103,9 @@ limitations under the License.
.prefsButton {
text-align: right;
}
#modeSelect {
margin-left: .5em;
}
@media screen and (max-width: 50em) {
.dash {
display: none;
@@ -168,11 +171,17 @@ limitations under the License.
patch-range="[[_patchRange]]"
available-patches="[[_computeAvailablePatches(_change.revisions)]]">
</gr-patch-range-select>
<gr-button link
class="prefsButton"
on-tap="_handlePrefsTap"
hidden$="[[_computePrefsButtonHidden(_prefs, _loggedIn)]]"
hidden>Diff View Preferences</gr-button>
<div>
<gr-button link
class="prefsButton"
on-tap="_handlePrefsTap"
hidden$="[[_computePrefsButtonHidden(_prefs, _loggedIn)]]"
hidden>Diff View Preferences</gr-button>
<select id="modeSelect" on-change="_handleModeChange">
<option value="SIDE_BY_SIDE">Side By Side</option>
<option value="UNIFIED_DIFF">Unified</option>
</select>
</div>
</div>
<gr-overlay id="prefsOverlay" with-backdrop>
<gr-diff-preferences
@@ -186,6 +195,7 @@ limitations under the License.
path="[[_path]]"
prefs="[[_prefs]]"
project-config="[[_projectConfig]]"
view-mode="[[_diffMode]]"
on-render="_handleDiffRender">
</gr-diff>
</div>

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

@@ -16,6 +16,11 @@
var COMMIT_MESSAGE_PATH = '/COMMIT_MSG';
var DiffViewMode = {
SIDE_BY_SIDE: 'SIDE_BY_SIDE',
UNIFIED: 'UNIFIED_DIFF',
};
Polymer({
is: 'gr-diff-view',
@@ -64,6 +69,11 @@
value: true,
},
_prefs: Object,
_userPrefs: Object,
_diffMode: {
type: String,
computed: '_getDiffViewMode(changeViewState.diffMode, _userPrefs)'
},
},
behaviors: [
@@ -74,6 +84,7 @@
'_getChangeDetail(_changeNum)',
'_getProjectConfig(_change.project)',
'_getFiles(_changeNum, _patchRange.patchNum)',
'_updateModeSelect(_diffMode)',
],
attached: function() {
@@ -92,6 +103,9 @@
},
detached: function() {
// Reset the diff mode to null so that it reverts to the user preference.
this.changeViewState.diffMode = null;
window.removeEventListener('resize', this._boundWindowResizeHandler);
},
@@ -124,6 +138,10 @@
return this.$.restAPI.getDiffPreferences();
},
_getPreferences: function() {
return this.$.restAPI.getPreferences();
},
_handleReviewedChange: function(e) {
this._setReviewed(Polymer.dom(e).rootTarget.checked);
},
@@ -242,6 +260,10 @@
this._prefs = prefs;
}.bind(this)));
promises.push(this._getPreferences().then(function(prefs) {
this._userPrefs = prefs;
}.bind(this)));
promises.push(this.$.diff.reload());
Promise.all(promises).then(function() {
@@ -360,5 +382,43 @@
e.stopPropagation();
this.$.prefsOverlay.close();
},
_handleModeChange: function(e) {
this.set('changeViewState.diffMode', this.$.modeSelect.value);
},
/**
* _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.changeViewState.diffMode) {
return this.changeViewState.diffMode;
} else if (this._userPrefs && this._userPrefs.diff_view) {
return this.changeViewState.diffMode = this._userPrefs.diff_view;
}
return DiffViewMode.SIDE_BY_SIDE;
},
/**
* Synchronize the mode select if it deviates from the selected mode state.
* This is mainly to keep it accurate when the page loads.
*/
_updateModeSelect: function() {
var mode = this._getDiffViewMode();
if (this.$.modeSelect.value !== mode) {
this.$.modeSelect.value = mode;
}
},
});
})();

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

@@ -356,5 +356,37 @@ limitations under the License.
done();
});
});
test('diff mode selector correctly toggles the diff', function() {
var select = element.$.modeSelect;
var diffDisplay = element.$.diff;
element._userPrefs = {diff_view: 'SIDE_BY_SIDE'};
// The mode selected in the view state reflects the selected option.
assert.equal(element._getDiffViewMode(), select.value);
// The mode selected in the view state reflects the view rednered in the
// diff.
assert.equal(select.value, diffDisplay.viewMode);
// We will simulate a user change of the selected mode.
var newMode = 'UNIFIED_DIFF';
// Listen to the change handler.
var eventStub = sinon.spy(element, '_handleModeChange');
// Set the actual value of the select, and simulate the change event.
select.value = newMode;
element.fire('change', {}, { node: select });
// Make sure the handler was called and the state is still coherent.
assert.isTrue(eventStub.called);
assert.equal(element._getDiffViewMode(), newMode);
assert.equal(element._getDiffViewMode(), select.value);
assert.equal(element._getDiffViewMode(), diffDisplay.viewMode);
eventStub.restore();
});
});
</script>

2
polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
View File

@@ -131,7 +131,7 @@ limitations under the License.
content: '\00BB';
}
</style>
<div class$="[[_computeContainerClass(_loggedIn, _viewMode)]]"
<div class$="[[_computeContainerClass(_loggedIn, viewMode)]]"
on-tap="_handleTap"
on-mousedown="_handleMouseDown"
on-copy="_handleCopy">

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

@@ -47,7 +47,7 @@
type: Boolean,
value: false,
},
_viewMode: {
viewMode: {
type: String,
value: DiffViewMode.SIDE_BY_SIDE,
},
@@ -69,7 +69,7 @@
},
observers: [
'_prefsChanged(prefs.*)',
'_prefsChanged(prefs.*, viewMode)',
],
attached: function() {
@@ -439,14 +439,14 @@
},
_getDiffBuilder: function(diff, comments, prefs) {
if (this._viewMode === DiffViewMode.SIDE_BY_SIDE) {
if (this.viewMode === DiffViewMode.SIDE_BY_SIDE) {
return new GrDiffBuilderSideBySide(diff, comments, prefs,
this.$.diffTable);
} else if (this._viewMode === DiffViewMode.UNIFIED) {
} else if (this.viewMode === DiffViewMode.UNIFIED) {
return new GrDiffBuilderUnified(diff, comments, prefs,
this.$.diffTable);
}
throw Error('Unsupported diff view mode: ' + this._viewMode);
throw Error('Unsupported diff view mode: ' + this.viewMode);
},
_projectConfigChanged: function(projectConfig) {

1
polygerrit-ui/app/elements/gr-app.js
View File

@@ -70,6 +70,7 @@
patchNum: null,
selectedFileIndex: 0,
showReplyDialog: false,
diffMode: null,
},
changeListView: {
query: null,

1
polygerrit-ui/app/test/index.html
View File

@@ -49,6 +49,7 @@ limitations under the License.
'../elements/diff/gr-diff-preferences/gr-diff-preferences_test.html',
'../elements/diff/gr-diff-view/gr-diff-view_test.html',
'../elements/diff/gr-patch-range-select/gr-patch-range-select_test.html',
'../elements/shared/gr-alert/gr-alert_test.html',
'../elements/shared/gr-account-label/gr-account-label_test.html',
'../elements/shared/gr-account-link/gr-account-link_test.html',
'../elements/shared/gr-alert/gr-alert_test.html',