Save diff mode preference when toggled
Introduce the gr-diff-mode-selector which componentizes the diff mode
buttons already used in the file list and adds them to the diff view.
With this new component, when authenticated users change their diff
preference using the new selector (or by the 'm' keyboard shortcut), the
new mode is saved to their preferences. Unauthenticated users see no
change in diff mode preference persistence.
The diff selector is now consistently labeled as "Diff view" rather than
as "Diff views".
Bug: Issue 8144
Change-Id: I4b30714deb9a466e707b3d4ae90c1d4c60222c64
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index 1662dbd..dd769e0 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -323,9 +323,9 @@
e.preventDefault();
if (this.viewState.diffMode === DiffViewMode.SIDE_BY_SIDE) {
- this.set('viewState.diffMode', DiffViewMode.UNIFIED);
+ this.$.fileListHeader.setDiffViewMode(DiffViewMode.UNIFIED);
} else {
- this.set('viewState.diffMode', DiffViewMode.SIDE_BY_SIDE);
+ this.$.fileListHeader.setDiffViewMode(DiffViewMode.SIDE_BY_SIDE);
}
},
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
index b958718..6734d27 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
@@ -260,17 +260,18 @@
test('m should toggle diff mode', () => {
sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false);
+ const setModeStub = sandbox.stub(element.$.fileListHeader,
+ 'setDiffViewMode');
const e = {preventDefault: () => {}};
flushAsynchronousOperations();
- // Initial state.
element.viewState.diffMode = 'SIDE_BY_SIDE';
-
element._handleMKey(e);
- assert.equal(element.viewState.diffMode, 'UNIFIED_DIFF');
+ assert.isTrue(setModeStub.calledWith('UNIFIED_DIFF'));
+ element.viewState.diffMode = 'UNIFIED_DIFF';
element._handleMKey(e);
- assert.equal(element.viewState.diffMode, 'SIDE_BY_SIDE');
+ assert.isTrue(setModeStub.calledWith('SIDE_BY_SIDE'));
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
index 8cd0329..6c3ef82 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
@@ -18,6 +18,7 @@
<link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
<link rel="import" href="../../../styles/shared-styles.html">
<link rel="import" href="../../core/gr-navigation/gr-navigation.html">
+<link rel="import" href="../../diff/gr-diff-mode-selector/gr-diff-mode-selector.html">
<link rel="import" href="../../diff/gr-patch-range-select/gr-patch-range-select.html">
<link rel="import" href="../../edit/gr-edit-controls/gr-edit-controls.html">
<link rel="import" href="../../shared/gr-editable-label/gr-editable-label.html">
@@ -99,9 +100,6 @@
.expanded #expandBtn {
display: none;
}
- gr-button.selected iron-icon {
- color: var(--color-link);
- }
gr-linked-chip {
--linked-chip-text-color: black;
}
@@ -232,21 +230,11 @@
</template>
<div class="fileViewActions">
<span class="separator"></span>
- <span>Diff Views:</span>
- <gr-button
- id="sideBySideBtn"
- link
- has-tooltip
- title="Side-by-side diff"
- class$="[[_computeSelectedClass(diffViewMode, _VIEW_MODES.SIDE_BY_SIDE)]]"
- on-tap="_handleSideBySideTap"><iron-icon icon="gr-icons:side-by-side"></iron-icon></gr-button>
- <gr-button
- id="unifiedBtn"
- link
- has-tooltip
- title="Unified dff"
- class$="[[_computeSelectedClass(diffViewMode, _VIEW_MODES.UNIFIED)]]"
- on-tap="_handleUnifiedTap"><iron-icon icon="gr-icons:unified"></iron-icon></gr-button>
+ <span>Diff view:</span>
+ <gr-diff-mode-selector
+ id="modeSelect"
+ mode="{{diffViewMode}}"
+ save-on-change="[[loggedIn]]"></gr-diff-mode-selector>
<span id="diffPrefsContainer"
class="hideOnEdit"
hidden$="[[_computePrefsButtonHidden(diffPrefs, loggedIn)]]"
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
index 9d32e75..f6b5ad5 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
@@ -56,15 +56,6 @@
type: Boolean,
computed: '_computeDescriptionReadOnly(loggedIn, change, account)',
},
- /** @type {?} */
- _VIEW_MODES: {
- type: Object,
- readOnly: true,
- value: {
- SIDE_BY_SIDE: 'SIDE_BY_SIDE',
- UNIFIED: 'UNIFIED_DIFF',
- },
- },
_revisionInfo: {
type: Object,
computed: '_getRevisionInfo(change)',
@@ -79,6 +70,10 @@
'_computePatchSetDescription(change, patchNum)',
],
+ setDiffViewMode(mode) {
+ this.$.modeSelect.setMode(mode);
+ },
+
_expandAllDiffs() {
this._expanded = true;
this.fire('expand-diffs');
@@ -89,10 +84,6 @@
this.fire('collapse-diffs');
},
- _computeSelectedClass(diffViewMode, buttonViewMode) {
- return buttonViewMode === diffViewMode ? 'selected' : '';
- },
-
_computeExpandedClass(filesExpanded) {
const classes = [];
if (filesExpanded === GrFileListConstants.FilesExpandedState.ALL) {
@@ -105,14 +96,6 @@
return classes.join(' ');
},
- _handleSideBySideTap() {
- this.diffViewMode = this._VIEW_MODES.SIDE_BY_SIDE;
- },
-
- _handleUnifiedTap() {
- this.diffViewMode = this._VIEW_MODES.UNIFIED;
- },
-
_computeDescriptionPlaceholder(readOnly) {
return (readOnly ? 'No' : 'Add') + ' patchset description';
},
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html
index 16cbfcb..a92d783 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html
@@ -194,19 +194,6 @@
});
});
- test('diff mode selector is set correctly', () => {
- const sideBySideBtn = element.$.sideBySideBtn;
- const unifiedBtn = element.$.unifiedBtn;
- element.diffViewMode = 'SIDE_BY_SIDE';
- flushAsynchronousOperations();
- assert.isTrue(sideBySideBtn.classList.contains('selected'));
- assert.isFalse(unifiedBtn.classList.contains('selected'));
- element.diffViewMode = 'UNIFIED_DIFF';
- flushAsynchronousOperations();
- assert.isFalse(sideBySideBtn.classList.contains('selected'));
- assert.isTrue(unifiedBtn.classList.contains('selected'));
- });
-
test('fileViewActions are properly hidden', () => {
const actions = element.$$('.fileViewActions');
assert.equal(getComputedStyle(actions).display, 'none');
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.html b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.html
new file mode 100644
index 0000000..1b140da
--- /dev/null
+++ b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.html
@@ -0,0 +1,50 @@
+<!--
+Copyright (C) 2018 The Android Open Source Project
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+-->
+
+<link rel="import" href="../../../bower_components/polymer/polymer.html">
+<link rel="import" href="../../../styles/shared-styles.html">
+<link rel="import" href="../../shared/gr-button/gr-button.html">
+<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
+
+<dom-module id="gr-diff-mode-selector">
+ <template>
+ <style include="shared-styles">
+ gr-button.selected iron-icon {
+ color: var(--color-link);
+ }
+ </style>
+ <gr-button
+ id="sideBySideBtn"
+ link
+ has-tooltip
+ class$="[[_computeSelectedClass(mode, _VIEW_MODES.SIDE_BY_SIDE)]]"
+ title="Side-by-side diff"
+ on-tap="_handleSideBySideTap"><!--
+ --><iron-icon icon="gr-icons:side-by-side"></iron-icon><!--
+ --></gr-button>
+ <gr-button
+ id="unifiedBtn"
+ link
+ has-tooltip
+ title="Unified diff"
+ class$="[[_computeSelectedClass(mode, _VIEW_MODES.UNIFIED)]]"
+ on-tap="_handleUnifiedTap"><!--
+ --><iron-icon icon="gr-icons:unified"></iron-icon><!--
+ --></gr-button>
+ <gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
+ </template>
+ <script src="gr-diff-mode-selector.js"></script>
+</dom-module>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.js b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.js
new file mode 100644
index 0000000..6c3a713
--- /dev/null
+++ b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.js
@@ -0,0 +1,68 @@
+// Copyright (C) 2018 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+(function() {
+ 'use strict';
+
+ Polymer({
+ is: 'gr-diff-mode-selector',
+
+ properties: {
+ mode: {
+ type: String,
+ notify: true,
+ },
+
+ /**
+ * If set to true, the user's preference will be updated every time a
+ * button is tapped. Don't set to true if there is no user.
+ */
+ saveOnChange: {
+ type: Boolean,
+ value: false,
+ },
+
+ /** @type {?} */
+ _VIEW_MODES: {
+ type: Object,
+ readOnly: true,
+ value: {
+ SIDE_BY_SIDE: 'SIDE_BY_SIDE',
+ UNIFIED: 'UNIFIED_DIFF',
+ },
+ },
+ },
+
+ /**
+ * Set the mode. If save on change is enabled also update the preference.
+ */
+ setMode(newMode) {
+ if (this.saveOnChange && this.mode && this.mode !== newMode) {
+ this.$.restAPI.savePreferences({diff_view: newMode});
+ }
+ this.mode = newMode;
+ },
+
+ _computeSelectedClass(diffViewMode, buttonViewMode) {
+ return buttonViewMode === diffViewMode ? 'selected' : '';
+ },
+
+ _handleSideBySideTap() {
+ this.setMode(this._VIEW_MODES.SIDE_BY_SIDE);
+ },
+
+ _handleUnifiedTap() {
+ this.setMode(this._VIEW_MODES.UNIFIED);
+ },
+ });
+})();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector_test.html b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector_test.html
new file mode 100644
index 0000000..c791091
--- /dev/null
+++ b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector_test.html
@@ -0,0 +1,84 @@
+<!DOCTYPE html>
+<!--
+Copyright (C) 2018 The Android Open Source Project
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+-->
+
+<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
+<title>gr-diff-mode-selector</title>
+
+<script src="../../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script>
+<script src="../../../bower_components/web-component-tester/browser.js"></script>
+<link rel="import" href="../../../test/common-test-setup.html"/>
+<script src="../../../bower_components/page/page.js"></script>
+<script src="../../../scripts/util.js"></script>
+
+<link rel="import" href="gr-diff-mode-selector.html">
+
+<script>void(0);</script>
+
+<test-fixture id="basic">
+ <template>
+ <gr-diff-mode-selector></gr-diff-mode-selector>
+ </template>
+</test-fixture>
+
+<script>
+ suite('gr-diff-mode-selector tests', () => {
+ let element;
+ let sandbox;
+
+ setup(() => {
+ sandbox = sinon.sandbox.create();
+ element = fixture('basic');
+ });
+
+ teardown(() => {
+ sandbox.restore();
+ });
+
+ test('_computeSelectedClass', () => {
+ assert.equal(
+ element._computeSelectedClass('SIDE_BY_SIDE', 'SIDE_BY_SIDE'),
+ 'selected');
+ assert.equal(
+ element._computeSelectedClass('SIDE_BY_SIDE', 'UNIFIED_DIFF'), '');
+ });
+
+ test('setMode', () => {
+ const saveStub = sandbox.stub(element.$.restAPI, 'savePreferences');
+
+ // Setting the mode initially does not save prefs.
+ element.saveOnChange = true;
+ element.setMode('SIDE_BY_SIDE');
+ assert.isFalse(saveStub.called);
+
+ // Setting the mode to itself does not save prefs.
+ element.setMode('SIDE_BY_SIDE');
+ assert.isFalse(saveStub.called);
+
+ // Setting the mode to something else does not save prefs if saveOnChange
+ // is false.
+ element.saveOnChange = false;
+ element.setMode('UNIFIED_DIFF');
+ assert.isFalse(saveStub.called);
+
+ // Setting the mode to something else does not save prefs if saveOnChange
+ // is false.
+ element.saveOnChange = true;
+ element.setMode('SIDE_BY_SIDE');
+ assert.isTrue(saveStub.calledOnce);
+ });
+ });
+</script>
diff --git 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
index 2ce29db..c5a188c 100644
--- 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
@@ -31,6 +31,7 @@
<link rel="import" href="../../shared/revision-info/revision-info.html">
<link rel="import" href="../gr-comment-api/gr-comment-api.html">
<link rel="import" href="../gr-diff-cursor/gr-diff-cursor.html">
+<link rel="import" href="../gr-diff-mode-selector/gr-diff-mode-selector.html">
<link rel="import" href="../gr-diff-preferences/gr-diff-preferences.html">
<link rel="import" href="../gr-diff/gr-diff.html">
<link rel="import" href="../gr-patch-range-select/gr-patch-range-select.html">
@@ -132,6 +133,14 @@
align-items: center;
display: flex;
}
+ .diffModeSelector {
+ align-items: center;
+ display: flex;
+ }
+ .diffModeSelector.hide,
+ .separator.hide {
+ display: none;
+ }
gr-dropdown-list {
--trigger-style: {
text-transform: none;
@@ -259,19 +268,17 @@
</span>
</div>
<div class="rightControls">
- <gr-select
- id="modeSelect"
- bind-value="{{changeViewState.diffMode}}"
- hidden$="[[_computeModeSelectHidden(_isImageDiff)]]">
- <select>
- <option value="SIDE_BY_SIDE">Side By Side</option>
- <option value="UNIFIED_DIFF">Unified</option>
- </select>
- </gr-select>
+ <div class$="diffModeSelector [[_computeModeSelectHideClass(_isImageDiff)]]">
+ Diff view:
+ <gr-diff-mode-selector
+ id="modeSelect"
+ save-on-change="[[_loggedIn]]"
+ mode="{{changeViewState.diffMode}}"></gr-diff-mode-selector>
+ </div>
<span id="diffPrefsContainer"
hidden$="[[_computePrefsButtonHidden(_prefs, _loggedIn)]]" hidden>
<span class="preferences desktop">
- <span class="separator" hidden$="[[_computeModeSelectHidden(_isImageDiff)]]"></span>
+ <span class$="separator [[_computeModeSelectHideClass(_isImageDiff)]]"></span>
<gr-button link
class="prefsButton"
on-tap="_handlePrefsTap">Preferences</gr-button>
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 f1cc83e..90a3913 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
@@ -437,9 +437,9 @@
e.preventDefault();
if (this._getDiffViewMode() === DiffViewMode.SIDE_BY_SIDE) {
- this.set('changeViewState.diffMode', DiffViewMode.UNIFIED);
+ this.$.modeSelect.setMode(DiffViewMode.UNIFIED);
} else {
- this.set('changeViewState.diffMode', DiffViewMode.SIDE_BY_SIDE);
+ this.$.modeSelect.setMode(DiffViewMode.SIDE_BY_SIDE);
}
},
@@ -814,15 +814,15 @@
if (this.changeViewState.diffMode) {
return this.changeViewState.diffMode;
} else if (this._userPrefs) {
- return this.changeViewState.diffMode =
- this._userPrefs.default_diff_view;
+ this.set('changeViewState.diffMode', this._userPrefs.default_diff_view);
+ return this._userPrefs.default_diff_view;
} else {
return 'SIDE_BY_SIDE';
}
},
- _computeModeSelectHidden() {
- return this._isImageDiff;
+ _computeModeSelectHideClass(isImageDiff) {
+ return isImageDiff ? 'hide' : '';
},
_onLineSelected(e, detail) {
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 ae98fe9..9776d4a 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
@@ -606,21 +606,21 @@
element._userPrefs = {default_diff_view: 'SIDE_BY_SIDE'};
// The mode selected in the view state reflects the selected option.
- assert.equal(element._getDiffViewMode(), select.nativeSelect.value);
+ assert.equal(element._getDiffViewMode(), select.mode);
// The mode selected in the view state reflects the view rednered in the
// diff.
- assert.equal(select.nativeSelect.value, diffDisplay.viewMode);
+ assert.equal(select.mode, diffDisplay.viewMode);
// We will simulate a user change of the selected mode.
const newMode = 'UNIFIED_DIFF';
- // Set the actual value of the select, and simulate the change event.
- select.nativeSelect.value = newMode;
- element.fire('change', {}, {node: select.nativeSelect});
+
+ // Set the mode, and simulate the change event.
+ element.set('changeViewState.diffMode', newMode);
// Make sure the handler was called and the state is still coherent.
assert.equal(element._getDiffViewMode(), newMode);
- assert.equal(element._getDiffViewMode(), select.nativeSelect.value);
+ assert.equal(element._getDiffViewMode(), select.mode);
assert.equal(element._getDiffViewMode(), diffDisplay.viewMode);
});
@@ -633,17 +633,16 @@
// Attach a new gr-diff-view so we can intercept the preferences fetch.
const view = document.createElement('gr-diff-view');
- const 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.nativeSelect.value, 'SIDE_BY_SIDE');
+ assert.equal(view._getDiffViewMode(), 'SIDE_BY_SIDE');
// Receive the overriding preference.
resolvePrefs({default_diff_view: 'UNIFIED'});
flushAsynchronousOperations();
- assert.equal(select.nativeSelect.value, 'SIDE_BY_SIDE');
+ assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
});
suite('_commitRange', () => {
diff --git a/polygerrit-ui/app/test/index.html b/polygerrit-ui/app/test/index.html
index 707f49f..bb61e05 100644
--- a/polygerrit-ui/app/test/index.html
+++ b/polygerrit-ui/app/test/index.html
@@ -95,6 +95,7 @@
'diff/gr-diff-cursor/gr-diff-cursor_test.html',
'diff/gr-diff-highlight/gr-annotation_test.html',
'diff/gr-diff-highlight/gr-diff-highlight_test.html',
+ 'diff/gr-diff-mode-selector/gr-diff-mode-selector_test.html',
'diff/gr-diff-preferences/gr-diff-preferences_test.html',
'diff/gr-diff-processor/gr-diff-processor_test.html',
'diff/gr-diff-selection/gr-diff-selection_test.html',
