From 14cbcc07dd605dcbb4ea42b6f6b8d245b94c75a2 Mon Sep 17 00:00:00 2001 From: "Francois (fge)" Date: Wed, 15 Sep 2021 12:21:46 +0000 Subject: [PATCH] [FIX] web: concurrency issue in command palette Before this commit, if the user searched the command palette and pressed the enter key without waiting for the rendering, then the command executed was incorrect. The "inputPromise" promise used to know if the new commands were loaded correctly. This one did not wait for the resolution "setCommands" called by "onSearchInput". So we modified the "debouncePromise" utility to make sure that it waits for the resolution of the function given to it before resolving its promise. closes odoo/odoo#76613 X-original-commit: 53b905d6993b4e25fec574d92429f2ff91501dfa Signed-off-by: Aaron Bohy (aab) --- addons/web/static/src/core/utils/timing.js | 50 +++-------- .../src/webclient/commands/command_palette.js | 6 +- .../static/tests/core/utils/timing_tests.js | 83 +++++++++++++++++-- .../commands/command_palette_tests.js | 44 ++++++++++ 4 files changed, 135 insertions(+), 48 deletions(-) diff --git a/addons/web/static/src/core/utils/timing.js b/addons/web/static/src/core/utils/timing.js index 6c19b498ec4..08489d7e4fc 100644 --- a/addons/web/static/src/core/utils/timing.js +++ b/addons/web/static/src/core/utils/timing.js @@ -3,12 +3,12 @@ import { browser } from "../browser/browser"; /** - * Returns a function, that, as long as it continues to be invoked, will not - * be triggered. The function will be called after it stops being called for - * N milliseconds. If `immediate` is passed, trigger the function on the - * leading edge, instead of the trailing. - * - * Inspired by https://davidwalsh.name/javascript-debounce-function + * Creates and returns a new debounced version of the passed function (func) + * which will postpone its execution until after wait milliseconds + * have elapsed since the last time it was invoked. The debounced function + * will return a Promise that will be resolved when the function (func) + * has been fully executed. If `immediate` is passed, trigger the function + * on the leading edge, instead of the trailing. * @param {Function} func * @param {number} wait * @param {boolean} [immediate=false] @@ -20,46 +20,22 @@ export function debounce(func, wait, immediate = false) { return { [funcName](...args) { const context = this; + let resolve; + const prom = new Promise((r) => { + resolve = r; + }); function later() { if (!immediate) { - func.apply(context, args); + Promise.resolve(func.apply(context, args)).then(resolve); } } const callNow = immediate && !timeout; browser.clearTimeout(timeout); timeout = browser.setTimeout(later, wait); if (callNow) { - func.apply(context, args); + Promise.resolve(func.apply(context, args)).then(resolve); } - }, - }[funcName]; -} - -/** - * Returns a function which returns a Promise, that, as long as it continues - * to be invoked, will not be triggered. The function (func) will be called - * after it stops being called for N milliseconds and the Promise will be resolved. - * - * @param {Function} func - * @param {number} wait - * @returns {Function} - */ -export function debouncePromise(func, wait) { - let resolver; - const funcName = func.name ? func.name + " (promise)" : "promise"; - const obj = { - [funcName]: (...args) => { - func.apply(this, args); - resolver(); - }, - }; - const debouncedFunc = debounce(obj[funcName], wait); - return { - [funcName](...args) { - return new Promise((resolve) => { - resolver = resolve; - debouncedFunc.apply(this, args); - }); + return prom; }, }[funcName]; } diff --git a/addons/web/static/src/webclient/commands/command_palette.js b/addons/web/static/src/webclient/commands/command_palette.js index 6427f964811..8f46d5292c4 100644 --- a/addons/web/static/src/webclient/commands/command_palette.js +++ b/addons/web/static/src/webclient/commands/command_palette.js @@ -5,7 +5,7 @@ import { useHotkey } from "@web/core/hotkeys/hotkey_hook"; import { KeepLast } from "@web/core/utils/concurrency"; import { scrollTo } from "@web/core/utils/scrolling"; import { fuzzyLookup } from "@web/core/utils/search"; -import { debouncePromise } from "@web/core/utils/timing"; +import { debounce } from "@web/core/utils/timing"; import { _lt } from "@web/core/l10n/translation"; const { Component, hooks } = owl; @@ -69,7 +69,7 @@ export class CommandPalette extends Component { this.keepLast = new KeepLast(); this.DefaultCommandItem = DefaultCommandItem; this.activeElement = useService("ui").activeElement; - const onDebouncedSearchInput = debouncePromise.apply(this, [this.onSearchInput, 250]); + const onDebouncedSearchInput = debounce.apply(this, [this.onSearchInput, 200]); this.onDebouncedSearchInput = (...args) => { this.inputPromise = onDebouncedSearchInput.apply(this, args).catch(() => { this.inputPromise = null; @@ -258,7 +258,7 @@ export class CommandPalette extends Component { namespace = searchValue[0]; searchValue = searchValue.slice(1); } - this.setCommands(namespace, { + await this.setCommands(namespace, { searchValue, activeElement: this.activeElement, }); diff --git a/addons/web/static/tests/core/utils/timing_tests.js b/addons/web/static/tests/core/utils/timing_tests.js index 8a92406a179..510a9a445fa 100644 --- a/addons/web/static/tests/core/utils/timing_tests.js +++ b/addons/web/static/tests/core/utils/timing_tests.js @@ -1,19 +1,86 @@ /** @odoo-module **/ -import { debouncePromise } from "@web/core/utils/timing"; +import { browser } from "@web/core/browser/browser"; +import { debounce } from "@web/core/utils/timing"; +import { makeDeferred, patchWithCleanup } from "../../helpers/utils"; QUnit.module("utils", () => { QUnit.module("timing"); - QUnit.test("debouncePromise", async function (assert) { + QUnit.test("debounce on an async function", async function (assert) { + let callback; + patchWithCleanup(browser, { + setTimeout: (later) => { + callback = later; + }, + }); + const imSearchDef = makeDeferred(); const myFunc = () => { - assert.step("exec"); + assert.step("myFunc"); + return imSearchDef; }; - const myDebouncedFunc = debouncePromise(myFunc, 10); - let toAwait = myDebouncedFunc(); + const myDebouncedFunc = debounce(myFunc, 3000); + myDebouncedFunc().then(() => { + throw new Error("Should never be resolved"); + }); + myDebouncedFunc().then((x) => { + assert.step("resolved " + x); + }); assert.verifySteps([]); - toAwait = myDebouncedFunc(); - await toAwait; - assert.verifySteps(["exec"]); + callback(); + assert.verifySteps(["myFunc"]); + imSearchDef.resolve(42); + await Promise.resolve(); // wait for promise returned by myFunc + await Promise.resolve(); // wait for promise returned by debounce + + assert.verifySteps(["resolved 42"]); + }); + + QUnit.test("debounce on a sync function", async function (assert) { + let callback; + patchWithCleanup(browser, { + setTimeout: (later) => { + callback = later; + }, + }); + const myFunc = () => { + assert.step("myFunc"); + return 42; + }; + const myDebouncedFunc = debounce(myFunc, 3000); + myDebouncedFunc().then(() => { + throw new Error("Should never be resolved"); + }); + myDebouncedFunc().then((x) => { + assert.step("resolved " + x); + }); + assert.verifySteps([]); + callback(); + assert.verifySteps(["myFunc"]); + await Promise.resolve(); // wait for promise returned by myFunc + await Promise.resolve(); // wait for promise returned by debounce + + assert.verifySteps(["resolved 42"]); + }); + + QUnit.test("debounce with immediate", async function (assert) { + patchWithCleanup(browser, { + setTimeout: (later) => { + later(); + }, + }); + const myFunc = () => { + assert.step("myFunc"); + return 42; + }; + const myDebouncedFunc = debounce(myFunc, 3000, true); + myDebouncedFunc().then((x) => { + assert.step("resolved " + x); + }); + assert.verifySteps(["myFunc"]); + await Promise.resolve(); // wait for promise returned by myFunc + await Promise.resolve(); // wait for promise returned by debounce + + assert.verifySteps(["resolved 42"]); }); }); diff --git a/addons/web/static/tests/webclient/commands/command_palette_tests.js b/addons/web/static/tests/webclient/commands/command_palette_tests.js index 05b137cdaf4..56e14602834 100644 --- a/addons/web/static/tests/webclient/commands/command_palette_tests.js +++ b/addons/web/static/tests/webclient/commands/command_palette_tests.js @@ -1,5 +1,6 @@ /** @odoo-module **/ +import { makeDeferred } from "@mail/utils/deferred/deferred"; import { browser } from "@web/core/browser/browser"; import { dialogService } from "@web/core/dialog/dialog_service"; import { hotkeyService } from "@web/core/hotkeys/hotkey_service"; @@ -366,6 +367,49 @@ QUnit.test("multi provider with the same namespace", async (assert) => { ); }); +QUnit.test("check the concurrency during a research", async (assert) => { + testComponent = await mount(TestComponent, { env, target }); + const imSearchDef = makeDeferred(); + const provide = async (env, options) => { + if (options.searchValue) { + await imSearchDef; + } + return [ + { + name: "a", + action: () => { + assert.step("a"); + }, + }, + { + name: "b", + action: () => { + assert.step("b"); + }, + }, + ]; + }; + const providers = [{ namespace: "default", provide }]; + const config = { + providers, + }; + env.services.dialog.add(CommandPaletteDialog, { + config, + }); + await nextTick(); + assert.containsOnce(target, ".o_command_palette"); + assert.containsN(target, ".o_command", 2); + + await editSearchBar("b"); + triggerHotkey("enter"); + await nextTick(); + assert.verifySteps([]); + + imSearchDef.resolve(); + await nextTick(); + assert.verifySteps(["b"]); +}); + QUnit.test("open the command palette with a namespace already in the searchbar", async (assert) => { testComponent = await mount(TestComponent, { env, target }); const action = () => {};