Skip to content

Commit

Permalink
[IMP] CellPlugin: Avoid useless UPDATE_CELL
Browse files Browse the repository at this point in the history
The CellPlugin handler for "DELETE_CONTENT" will dispatch an UPDATE_CELL
to empty its content regardless of the cell. On a big zone this can end
up being quite costly as we pass for no reason in all the plugins and
stores to apply 0 changes, it only adds some overhead.

the allowDispatch of UPDATE_CELL handles this case but in this case it's
a subcommand and the allowDispatch is bypassed.

Benchmark
=========

Measure of the execution time of `model.Dispatch` on a 26x100 cells
sheet with **styled** cells.

cells with content (formula or text)
- before: 120~140ms
- after: 120~140ms

cells without content
- before 120~140ms
- 4~9ms

closes #5283

Task: 4367772
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
  • Loading branch information
rrahir committed Nov 29, 2024
1 parent 875c901 commit 28722d2
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 3 deletions.
3 changes: 1 addition & 2 deletions src/plugins/core/cell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ export class CellPlugin extends CorePlugin<CoreState> implements CoreState {
for (let col = zone.left; col <= zone.right; col++) {
for (let row = zone.top; row <= zone.bottom; row++) {
const cell = this.getters.getCell({ sheetId, col, row });
if (cell) {
if (cell?.isFormula || cell?.content) {
this.dispatch("UPDATE_CELL", {
sheetId: sheetId,
content: "",
Expand Down Expand Up @@ -200,7 +200,6 @@ export class CellPlugin extends CorePlugin<CoreState> implements CoreState {
for (let zone of recomputeZones(zones)) {
for (let col = zone.left; col <= zone.right; col++) {
for (let row = zone.top; row <= zone.bottom; row++) {
// commandHelpers.updateCell(sheetId, col, row, { style: undefined});
this.dispatch("UPDATE_CELL", {
sheetId,
col,
Expand Down
23 changes: 22 additions & 1 deletion tests/cells/cell_plugin.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { Model } from "../../src";
import { CoreCommand, CorePlugin, Model } from "../../src";
import { LINK_COLOR } from "../../src/constants";
import { buildSheetLink, toZone } from "../../src/helpers";
import { urlRepresentation } from "../../src/helpers/links";
import { corePluginRegistry } from "../../src/plugins";
import { CellValueType, CommandResult } from "../../src/types";
import {
addColumns,
Expand All @@ -11,6 +12,7 @@ import {
copy,
createSheet,
deleteColumns,
deleteContent,
deleteRows,
deleteSheet,
paste,
Expand All @@ -27,6 +29,7 @@ import {
getEvaluatedCell,
getStyle,
} from "../test_helpers/getters_helpers";
import { addTestPlugin } from "../test_helpers/helpers";

describe("getCellText", () => {
test("Update cell with a format is correctly set", () => {
Expand Down Expand Up @@ -643,4 +646,22 @@ describe("Cell dependencies and tokens are updated", () => {
},
});
});

test("Do not dispatch UPDATE_CELL subcommands if the content is empty", () => {
let counter = 0;
class SubCommandCounterRange extends CorePlugin {
static getters = [];
handle(command: CoreCommand) {
if (command.type === "UPDATE_CELL") {
counter++;
}
}
}
addTestPlugin(corePluginRegistry, SubCommandCounterRange);
const model = new Model();
setStyle(model, "A1", { bold: true });
counter = 0;
deleteContent(model, ["A1"]);
expect(counter).toBe(0);
});
});

0 comments on commit 28722d2

Please sign in to comment.