Skip to content

Commit

Permalink
[FIX] find & replace: remove debounce from store
Browse files Browse the repository at this point in the history
The method `updateSearchContent` was debounced inside the f&r store.
This does not work as expected, as the re-rendering of the panel was
done when calling the `updateSearchContent` method, and not when the
debounced function was called and the store state was updated.

I'm not sure why it still worked 50% of the time.

closes #4847

Task: 4102172
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
  • Loading branch information
hokolomopo committed Aug 22, 2024
1 parent ea88347 commit e94eb2d
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 32 deletions.
15 changes: 11 additions & 4 deletions src/components/side_panel/find_and_replace/find_and_replace.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { Component, onMounted, useRef, useState } from "@odoo/owl";
import { zoneToXc } from "../../../helpers";
import { Component, onMounted, onWillUnmount, useRef, useState } from "@odoo/owl";
import { debounce, zoneToXc } from "../../../helpers";
import { Store, useLocalStore } from "../../../store_engine";
import { _t } from "../../../translation";
import { SpreadsheetChildEnv } from "../../../types/index";
import { DebouncedFunction, SpreadsheetChildEnv } from "../../../types/index";
import { css } from "../../helpers/css";
import { SelectionInput } from "../../selection_input/selection_input";
import { Checkbox } from "../components/checkbox/checkbox";
Expand Down Expand Up @@ -50,6 +50,7 @@ export class FindAndReplacePanel extends Component<Props, SpreadsheetChildEnv> {
private searchInput = useRef("searchInput");
private store!: Store<FindAndReplaceStore>;
private state!: { dataRange: string };
private updateSearchContent!: DebouncedFunction<(value: string) => void>;

get hasSearchResult() {
return this.store.selectedMatchIndex !== null;
Expand Down Expand Up @@ -87,14 +88,16 @@ export class FindAndReplacePanel extends Component<Props, SpreadsheetChildEnv> {
this.store = useLocalStore(FindAndReplaceStore);
this.state = useState({ dataRange: "" });
onMounted(() => this.searchInput.el?.focus());
onWillUnmount(() => this.updateSearchContent.stopDebounce());
this.updateSearchContent = debounce(this.store.updateSearchContent, 200);
}

onFocusSearch() {
this.updateDataRange();
}

onSearchInput(ev: InputEvent) {
this.store.updateSearchContent((ev.target as HTMLInputElement)?.value || "");
this.updateSearchContent((ev.target as HTMLInputElement).value);
}

onKeydownSearch(ev: KeyboardEvent) {
Expand Down Expand Up @@ -144,4 +147,8 @@ export class FindAndReplacePanel extends Component<Props, SpreadsheetChildEnv> {
);
this.store.updateSearchOptions({ specificRange });
}

get pendingSearch() {
return this.updateSearchContent.isDebouncePending();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
/
<t t-esc="store.searchMatches.length"/>
</div>
<div t-elif="!store.pendingSearch and store.toSearch !== ''" class="o-input-count">
<div t-elif="!this.pendingSearch and store.toSearch !== ''" class="o-input-count">
0 / 0
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { debounce, getSearchRegex, isInside, positionToZone } from "../../../helpers";
import { getSearchRegex, isInside, positionToZone } from "../../../helpers";
import { HighlightProvider, HighlightStore } from "../../../stores/highlight_store";
import { CellPosition, Color, Command, Highlight } from "../../../types";

Expand Down Expand Up @@ -45,7 +45,6 @@ export class FindAndReplaceStore extends SpreadsheetStore implements HighlightPr
specificRange: undefined,
};

updateSearchContent = debounce(this._updateSearchContent.bind(this), 200);
constructor(get: Get) {
super(get);
this.initialShowFormulaState = this.model.getters.shouldShowFormulas();
Expand All @@ -55,7 +54,6 @@ export class FindAndReplaceStore extends SpreadsheetStore implements HighlightPr
highlightStore.register(this);
this.onDispose(() => {
this.model.dispatch("SET_FORMULA_VISIBILITY", { show: this.initialShowFormulaState });
this.updateSearchContent.stopDebounce();
highlightStore.unRegister(this);
});
}
Expand All @@ -71,7 +69,7 @@ export class FindAndReplaceStore extends SpreadsheetStore implements HighlightPr
}
}

private _updateSearchContent(toSearch: string) {
updateSearchContent(toSearch: string) {
this._updateSearch(toSearch, this.searchOptions);
}

Expand All @@ -92,10 +90,6 @@ export class FindAndReplaceStore extends SpreadsheetStore implements HighlightPr
this.selectNextCell(Direction.next);
}

get pendingSearch() {
return this.updateSearchContent.isDebouncePending();
}

handle(cmd: Command) {
switch (cmd.type) {
case "SET_FORMULA_VISIBILITY":
Expand Down
19 changes: 0 additions & 19 deletions tests/find_and_replace/find_and_replace_store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,6 @@ import {
} from "../test_helpers/getters_helpers";
import { makeStore } from "../test_helpers/stores";

// Disable debounce for tests
jest.mock("../../src/helpers/misc.ts", () => {
return {
...jest.requireActual("../../src/helpers/misc.ts"),
...jest.requireActual("../__mocks__/mock_misc_helpers.ts"),
};
});

let model: Model;
let store: FindAndReplaceStore;

Expand Down Expand Up @@ -314,17 +306,6 @@ describe("basic search", () => {
expect(store.selectedMatchIndex).toStrictEqual(0);
});

test("Store update search content method is debounced and debounce timeout is cleared on dispose", () => {
const debouncedSearch = store.updateSearchContent;
expect(typeof debouncedSearch).toBe("function");
expect(debouncedSearch.isDebouncePending).toBeTruthy();
expect(debouncedSearch.stopDebounce).toBeTruthy();

const spyStopDebounce = jest.spyOn(debouncedSearch, "stopDebounce");
store.dispose();
expect(spyStopDebounce).toHaveBeenCalled();
});

test("Switching sheet properly recomputes search results and shows them in the viewport", () => {
setCellContent(model, "A2", "Hello");
setCellContent(model, "A3", "Hello");
Expand Down
11 changes: 11 additions & 0 deletions tests/find_and_replace/find_replace_side_panel_component.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,17 @@ describe("find and replace sidePanel component", () => {
expect(getMatchesCount()).toBeUndefined();
});

test("Search is debounced", async () => {
setCellContent(model, "A1", "ok");
setInputValueAndTrigger(selectors.inputSearch, "o");
await nextTick();
expect(getMatchesCount()).toBeUndefined();

jest.runOnlyPendingTimers();
await nextTick();
expect(getMatchesCount()).toMatchObject({ allSheets: 1, currentSheet: 1 });
});

test("clicking on specific range and hitting confirm will search in the range", async () => {
setCellContent(model, "A1", "1");
inputSearchValue("1");
Expand Down

0 comments on commit e94eb2d

Please sign in to comment.