From a4069e25b6588f5899d476978b73d2d441b2e7f5 Mon Sep 17 00:00:00 2001
From: Aaron Bohy <aab@odoo.com>
Date: Tue, 24 Oct 2023 10:12:56 +0200
Subject: [PATCH] [FIX] web: CodeEditor: do not notify `value` props updates

Before this commit, the CodeEditor component called the `onChange`
props function each time the ace library fired the `change` event.
This in particular happens the value of the editor is set
programmatically by a update of the `value` props. Worse, in that
case, the event is fired twice: once with the empty string, and
one with the new real value. This isn't what we want for the
CodeEditor API. We only want to notify the parent of updates done
by the user in the UI. This commit thus filters out the noisy
`change` events fired by ace.

This fixed an issue with the ResourceEditor of the website: select
a scss or js custom resource, click on Reset: the resource is
marked as dirty, because `onChange` is called when the CodeEditor
is updated with the new value.

closes odoo/odoo#139154

Related: odoo/enterprise#49286
Signed-off-by: Lucas Perais (lpe) <lpe@odoo.com>
---
 .../src/core/code_editor/code_editor.js       | 12 ++++++++--
 .../static/tests/core/code_editor_tests.js    | 23 ++++++++++++++++++-
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/addons/web/static/src/core/code_editor/code_editor.js b/addons/web/static/src/core/code_editor/code_editor.js
index 99440acbd20..aecaab91ec4 100644
--- a/addons/web/static/src/core/code_editor/code_editor.js
+++ b/addons/web/static/src/core/code_editor/code_editor.js
@@ -62,6 +62,12 @@ export class CodeEditor extends Component {
         onWillStart(async () => await loadBundle("web.ace_lib"));
 
         const sessions = {};
+        // The ace library triggers the "change" event even if the change is
+        // programmatic. Even worse, it triggers 2 "change" events in that case,
+        // one with the empty string, and one with the new value. We only want
+        // to notify the parent of changes done by the user, in the UI, so we
+        // use this flag to filter out noisy "change" events.
+        let ignoredAceChange = false;
         useEffect(
             (el) => {
                 if (!el) {
@@ -85,7 +91,7 @@ export class CodeEditor extends Component {
                 }
                 aceEditor.setValue(this.props.value);
                 session.on("change", () => {
-                    if (this.props.onChange) {
+                    if (this.props.onChange && !ignoredAceChange) {
                         this.props.onChange(this.aceEditor.getValue());
                     }
                 });
@@ -132,7 +138,9 @@ export class CodeEditor extends Component {
                 let session = sessions[sessionId];
                 if (session) {
                     if (session.getValue() !== value) {
+                        ignoredAceChange = true;
                         session.setValue(value);
+                        ignoredAceChange = false;
                     }
                 } else {
                     session = new window.ace.EditSession(value);
@@ -143,7 +151,7 @@ export class CodeEditor extends Component {
                         useSoftTabs: true,
                     });
                     session.on("change", () => {
-                        if (this.props.onChange) {
+                        if (this.props.onChange && !ignoredAceChange) {
                             this.props.onChange(this.aceEditor.getValue());
                         }
                     });
diff --git a/addons/web/static/tests/core/code_editor_tests.js b/addons/web/static/tests/core/code_editor_tests.js
index f11aa035b19..cd37b6e104d 100644
--- a/addons/web/static/tests/core/code_editor_tests.js
+++ b/addons/web/static/tests/core/code_editor_tests.js
@@ -128,6 +128,27 @@ QUnit.module("Web Components", (hooks) => {
         assert.verifySteps(["Some Text"], "Value properly given to onChange");
     });
 
+    QUnit.test("onChange props not called when value props is updated", async (assert) => {
+        class Parent extends Component {
+            static components = { CodeEditor };
+            static template = xml`<CodeEditor value="state.value" onChange.bind="onChange" />`;
+            state = useState({ value: "initial value" });
+            onChange(value) {
+                assert.step(value || "__emptystring__");
+            }
+        }
+
+        const parent = await mount(Parent, target, { env });
+        await nextTick();
+        assert.strictEqual(target.querySelector(".ace_line").textContent, "initial value");
+
+        parent.state.value = "new value";
+        await nextTick();
+        await nextTick();
+        assert.strictEqual(target.querySelector(".ace_line").textContent, "new value");
+        assert.verifySteps([]);
+    });
+
     QUnit.test("Default value correctly set and updates", async (assert) => {
         const textA = "<div>\n<p>A Paragraph</p>\n</div>";
         const textB = "<div>\n<p>An Other Paragraph</p>\n</div>";
@@ -180,7 +201,7 @@ QUnit.module("Web Components", (hooks) => {
             "When the props is updated the value is correctly changed in the dom"
         );
 
-        assert.verifySteps([textB, textC], "Changes properly given to onChange");
+        assert.verifySteps([textB], "Changes properly given to onChange");
     });
 
     QUnit.test("Mode props update imports the mode", async (assert) => {