From 5bd9054c1ac92e1ec024e0991666acd26f369bcc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9mi=20Rahir=20=28rar=29?= <rar@odoo.com>
Date: Wed, 1 Nov 2023 09:57:21 +0000
Subject: [PATCH] [REV] Charts: Do not rely on history for the chartRuntimes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Since [1], the commands "UNDO" and "REDO" no longer invalidate the chart
evaluation as it was supposedly handled by the plugin history. However,
the ChartEvaluation plugin would lazy create the chart runtime and
update the history outside of the dispatch context (the changes are
therefore not recorded).
After some consideration, we consider that the storage cost of the
objects `ChartRuntime` is greater than having to invalidate/recompute it
at each UNDO/REDO. We therefore revert the change.

[1] https://github.com/odoo/o-spreadsheet/pull/2408

closes odoo/o-spreadsheet#3150

Task: 3578417
X-original-commit: 80bd3b30ff189637172a4df29974729a3c7d0457
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
---
 src/plugins/ui_core_views/evaluation_chart.ts | 19 ++++++++-----------
 tests/figures/chart/chart_plugin.test.ts      | 17 +++++++++++++++++
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/src/plugins/ui_core_views/evaluation_chart.ts b/src/plugins/ui_core_views/evaluation_chart.ts
index dc4955f366..4d5d11591b 100644
--- a/src/plugins/ui_core_views/evaluation_chart.ts
+++ b/src/plugins/ui_core_views/evaluation_chart.ts
@@ -1,6 +1,6 @@
 import { BACKGROUND_CHART_COLOR } from "../../constants";
 import { chartRuntimeFactory, chartToImage } from "../../helpers/figures/charts";
-import { Color, ExcelWorkbookData, FigureData, Immutable, Range, UID } from "../../types";
+import { Color, ExcelWorkbookData, FigureData, Range, UID } from "../../types";
 import { ChartRuntime, ExcelChartDefinition } from "../../types/chart/chart";
 import {
   CoreViewCommand,
@@ -10,12 +10,12 @@ import {
 import { UIPlugin } from "../ui_plugin";
 
 interface EvaluationChartState {
-  readonly charts: Immutable<Record<UID, ChartRuntime | undefined>>;
+  charts: Record<UID, ChartRuntime | undefined>;
 }
 export class EvaluationChartPlugin extends UIPlugin<EvaluationChartState> {
   static getters = ["getChartRuntime", "getBackgroundOfSingleCellChart"] as const;
 
-  readonly charts: Immutable<Record<UID, ChartRuntime | undefined>> = {};
+  charts: Record<UID, ChartRuntime | undefined> = {};
 
   private createRuntimeChart = chartRuntimeFactory(this.getters);
 
@@ -26,10 +26,8 @@ export class EvaluationChartPlugin extends UIPlugin<EvaluationChartState> {
       cmd.type === "EVALUATE_CELLS" ||
       cmd.type === "UPDATE_CELL"
     ) {
-      if (cmd.type !== "UNDO" && cmd.type !== "REDO") {
-        for (const chartId in this.charts) {
-          this.history.update("charts", chartId, undefined);
-        }
+      for (const chartId in this.charts) {
+        this.charts[chartId] = undefined;
       }
     }
 
@@ -37,12 +35,12 @@ export class EvaluationChartPlugin extends UIPlugin<EvaluationChartState> {
       case "UPDATE_CHART":
       case "CREATE_CHART":
       case "DELETE_FIGURE":
-        this.history.update("charts", cmd.id, undefined);
+        this.charts[cmd.id] = undefined;
         break;
       case "DELETE_SHEET":
         for (let chartId in this.charts) {
           if (!this.getters.isChartDefined(chartId)) {
-            this.history.update("charts", chartId, undefined);
+            this.charts[chartId] = undefined;
           }
         }
         break;
@@ -55,8 +53,7 @@ export class EvaluationChartPlugin extends UIPlugin<EvaluationChartState> {
       if (!chart) {
         throw new Error(`No chart for the given id: ${figureId}`);
       }
-      const runtime = this.createRuntimeChart(chart) as Immutable<ChartRuntime>;
-      this.history.update("charts", figureId, runtime);
+      this.charts[figureId] = this.createRuntimeChart(chart);
     }
     return this.charts[figureId] as ChartRuntime;
   }
diff --git a/tests/figures/chart/chart_plugin.test.ts b/tests/figures/chart/chart_plugin.test.ts
index fe50b4eb2b..41e51e0177 100644
--- a/tests/figures/chart/chart_plugin.test.ts
+++ b/tests/figures/chart/chart_plugin.test.ts
@@ -2004,6 +2004,23 @@ describe("Chart evaluation", () => {
         .data![0]
     ).toBe("#REF");
   });
+
+  test("undo/redo invalidates the chart runtime", () => {
+    const chartId = "test";
+    setCellContent(model, "A1", "oui");
+    setCellContent(model, "A2", "non");
+    createChart(model, {}, chartId);
+
+    updateChart(model, chartId, { labelRange: "A1:A2" });
+    const chartRuntime1 = model.getters.getChartRuntime(chartId) as BarChartRuntime;
+    undo(model);
+    const chartRuntime2 = model.getters.getChartRuntime(chartId) as BarChartRuntime;
+    redo(model);
+    const chartRuntime3 = model.getters.getChartRuntime(chartId) as BarChartRuntime;
+    expect(chartRuntime1.chartJsConfig.data?.labels).toEqual(["oui", "non"]);
+    expect(chartRuntime2.chartJsConfig.data?.labels).toEqual([]);
+    expect(chartRuntime3.chartJsConfig.data?.labels).toEqual(["oui", "non"]);
+  });
 });
 
 describe("Cumulative Data line chart", () => {