Merge "Apply diff preferences immediately after clicking save" into stable-2.16
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog.html b/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog.html
index ae53f76..b850f2c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog.html
@@ -57,7 +57,7 @@
<div class$="diffHeader [[_computeHeaderClass(_diffPrefsChanged)]]">Diff Preferences</div>
<gr-diff-preferences
id="diffPreferences"
- diff-prefs="{{diffPrefs}}"
+ diff-prefs="{{_editableDiffPrefs}}"
has-unsaved-changes="{{_diffPrefsChanged}}"></gr-diff-preferences>
<div class="diffActions">
<gr-button
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog.js b/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog.js
index b50ef69..f2a6363 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog.js
@@ -24,6 +24,17 @@
/** @type {?} */
diffPrefs: Object,
+ /**
+ * _editableDiffPrefs is a clone of diffPrefs.
+ * All changes in the dialog are applied to this object
+ * immediately, when a value in an editor is changed.
+ * The "Save" button replaces the "diffPrefs" object with
+ * the value of _editableDiffPrefs.
+ *
+ * @type {?}
+ */
+ _editableDiffPrefs: Object,
+
_diffPrefsChanged: Boolean,
},
@@ -48,6 +59,10 @@
},
open() {
+ // JSON.parse(JSON.stringify(...)) makes a deep clone of diffPrefs.
+ // It is known, that diffPrefs is obtained from an RestAPI call and
+ // it is safe to clone the object this way.
+ this._editableDiffPrefs = JSON.parse(JSON.stringify(this.diffPrefs));
this.$.diffPrefsOverlay.open().then(() => {
const focusStops = this.getFocusStops();
this.$.diffPrefsOverlay.setFocusStops(focusStops);
@@ -56,6 +71,7 @@
},
_handleSaveDiffPreferences() {
+ this.diffPrefs = this._editableDiffPrefs;
this.$.diffPreferences.save().then(() => {
this.fire('reload-diff-preference', null, {bubbles: false});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog_test.html b/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog_test.html
new file mode 100644
index 0000000..156ec69
--- /dev/null
+++ b/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog_test.html
@@ -0,0 +1,63 @@
+<!DOCTYPE html>
+<!--
+@license
+Copyright (C) 2020 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-preferences-dialog</title>
+
+<script src="../../../bower_components/web-component-tester/browser.js"></script>
+<link rel="import" href="../../../test/common-test-setup.html"/>
+
+<script>void(0);</script>
+
+<test-fixture id="basic">
+ <template>
+ <gr-diff-preferences-dialog></gr-diff-preferences-dialog>
+ </template>
+</test-fixture>
+
+<script>
+ suite('gr-diff-preferences-dialog', () => {
+ let element;
+ setup(() => {
+ element = basicFixture.instantiate();
+ });
+ test('changes applies only on save', async () => {
+ const originalDiffPrefs = {
+ line_wrapping: true,
+ };
+ element.diffPrefs = originalDiffPrefs;
+
+ element.open();
+ await flush();
+ assert.isTrue(element.$.diffPreferences.$.lineWrappingInput.checked);
+
+ MockInteractions.tap(element.$.diffPreferences.$.lineWrappingInput);
+ await flush();
+ assert.isFalse(element.$.diffPreferences.$.lineWrappingInput.checked);
+ assert.isTrue(element._diffPrefsChanged);
+ assert.isTrue(element.diffPrefs.line_wrapping);
+ assert.isTrue(originalDiffPrefs.line_wrapping);
+
+ MockInteractions.tap(element.$.saveButton);
+ await flush();
+ // Original prefs must remains unchanged, dialog must expose a new object
+ assert.isTrue(originalDiffPrefs.line_wrapping);
+ assert.isFalse(element.diffPrefs.line_wrapping);
+ });
+ });
+</script>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index 7a670d3..da7dea1 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -576,21 +576,26 @@
},
_prefsObserver(newPrefs, oldPrefs) {
- // Scan the preference objects one level deep to see if they differ.
- let differ = !oldPrefs;
- if (newPrefs && oldPrefs) {
- for (const key in newPrefs) {
- if (newPrefs[key] !== oldPrefs[key]) {
- differ = true;
- }
- }
- }
-
- if (differ) {
+ if (!this._prefsEqual(newPrefs, oldPrefs)) {
this._prefsChanged(newPrefs);
}
},
+ _prefsEqual(prefs1, prefs2) {
+ if (prefs1 === prefs2) {
+ return true;
+ }
+ if (!prefs1 || !prefs2) {
+ return false;
+ }
+ // Scan the preference objects one level deep to see if they differ.
+ const keys1 = Object.keys(prefs1);
+ const keys2 = Object.keys(prefs2);
+ return keys1.length === keys2.length &&
+ keys1.every(key => prefs1[key] === prefs2[key]) &&
+ keys2.every(key => prefs1[key] === prefs2[key]);
+ },
+
_pathObserver() {
// Call _prefsChanged(), because line-limit style value depends on path.
this._prefsChanged(this.prefs);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index ec71563..5e20499 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -40,6 +40,8 @@
let element;
let sandbox;
+ const MINIMAL_PREFS = {tab_size: 2, line_length: 80};
+
setup(() => {
sandbox = sinon.sandbox.create();
});
@@ -311,7 +313,7 @@
const mock = document.createElement('mock-diff-response');
element.$.diffBuilder._builder = element.$.diffBuilder._getDiffBuilder(
- mock.diffResponse, {}, {tab_size: 2, line_length: 80});
+ mock.diffResponse, {}, MINIMAL_PREFS);
// No thread groups.
assert.isNotOk(element._getThreadGroupForLine(contentEl));
@@ -819,6 +821,22 @@
assert.isTrue(element._renderDiffTable.called);
});
+ test('adding/removing property in preferences re-renders diff', () => {
+ const stub = sandbox.stub(element, '_renderDiffTable');
+ const newPrefs1 = Object.assign({}, MINIMAL_PREFS,
+ {line_wrapping: true});
+ element.prefs = newPrefs1;
+ element.flushDebouncer('renderDiffTable');
+ assert.isTrue(element._renderDiffTable.called);
+ stub.reset();
+
+ const newPrefs2 = Object.assign({}, newPrefs1);
+ delete newPrefs2.line_wrapping;
+ element.prefs = newPrefs2;
+ element.flushDebouncer('renderDiffTable');
+ assert.isTrue(element._renderDiffTable.called);
+ });
+
test('change in preferences does not re-renders diff with ' +
'noRenderOnPrefsChange', () => {
sandbox.stub(element, '_renderDiffTable');
@@ -1147,6 +1165,24 @@
assert.equal(element._computeNewlineWarningClass(null, false), hidden);
assert.equal(element._computeNewlineWarningClass('foo', false), shown);
});
+
+ test('_prefsEqual', () => {
+ element = fixture('basic');
+ assert.isTrue(element._prefsEqual(null, null));
+ assert.isTrue(element._prefsEqual({}, {}));
+ assert.isTrue(element._prefsEqual({x: 1}, {x: 1}));
+ assert.isTrue(
+ element._prefsEqual({x: 1, abc: 'def'}, {x: 1, abc: 'def'}));
+ const somePref = {abc: 'def', p: true};
+ assert.isTrue(element._prefsEqual(somePref, somePref));
+
+ assert.isFalse(element._prefsEqual({}, null));
+ assert.isFalse(element._prefsEqual(null, {}));
+ assert.isFalse(element._prefsEqual({x: 1}, {x: 2}));
+ assert.isFalse(element._prefsEqual({x: 1, y: 'abc'}, {x: 1, y: 'abcd'}));
+ assert.isFalse(element._prefsEqual({x: 1, y: 'abc'}, {x: 1}));
+ assert.isFalse(element._prefsEqual({x: 1}, {x: 1, y: 'abc'}));
+ });
});
});
