Apply diff preferences immediately after clicking save

Bug: Issue 12707
Change-Id: I4937ba597ebeb2a232625092f871f8a16012e7c6

polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog.js

@@ -34,9 +34,20 @@ class GrDiffPreferencesDialog extends GestureEventListeners(
 
   static get properties() {
     return {
-    /** @type {?} */
+      /** @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,
     };
   }
@@ -62,6 +73,10 @@ class GrDiffPreferencesDialog extends GestureEventListeners(
   }
 
   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);
@@ -70,6 +85,7 @@ class GrDiffPreferencesDialog extends GestureEventListeners(
   }
 
   _handleSaveDiffPreferences() {
+    this.diffPrefs = this._editableDiffPrefs;
     this.$.diffPreferences.save().then(() => {
       this.dispatchEvent(new CustomEvent('reload-diff-preference', {
         composed: true, bubbles: false,

polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog_html.js

@@ -53,7 +53,7 @@ export const htmlTemplate = html`
     </div>
     <gr-diff-preferences
       id="diffPreferences"
-      diff-prefs="{{diffPrefs}}"
+      diff-prefs="{{_editableDiffPrefs}}"
       has-unsaved-changes="{{_diffPrefsChanged}}"
     ></gr-diff-preferences>
     <div class="diffActions">

polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog_test.js

@@ -0,0 +1,50 @@
+/**
+ * @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.
+ */
+
+import '../../../test/common-test-setup-karma.js';
+
+const basicFixture = fixtureFromElement('gr-diff-preferences-dialog');
+
+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);
+  });
+});

polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js

@@ -685,21 +685,26 @@ class GrDiff extends mixinBehaviors( [
   }
 
   _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);

polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js

@@ -702,6 +702,22 @@ suite('gr-diff tests', () => {
         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');
@@ -1117,4 +1133,21 @@ suite('gr-diff tests', () => {
     });
     element._renderDiffTable();
   });
+
+  test('_prefsEqual', () => {
+    element = basicFixture.instantiate();
+    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'}));
+  });
 });