[FIX] Chart Pannel: fixes updateChart process

Taks Description

In #1613, we tried to fix
an issue when updating a chart's title and then switching to
another chart in the chart pannel without confirming the title change
(ie without pressing enter key). This was done by introducing a
shouldUpdateChart flag that was false when we try to update the wrong
chart.

The fix was working back then but in a strange and kind of dangerous
way, leading us to another issue nowadays. The current issue is that,
when opening a chart pannel and then switching to another chart, we
cannot edit a property of the 2nd chart until we click another time
on it, as the shouldUpdateChart was set to false (and stay in this
state untill we reclick on the chart).

Related Task(s):

-Task-2961701 (old task with first attempt to fix)
-Task-3323875 (current task)

closes #2501

Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>

src/components/side_panel/chart/bar_chart/bar_chart_config_panel.ts

@@ -4,7 +4,7 @@ export class BarConfigPanel extends LineBarPieConfigPanel {
   static template = "o-spreadsheet-BarConfigPanel";
 
   onUpdateStacked(ev) {
-    this.props.updateChart({
+    this.props.updateChart(this.props.figureId, {
       stacked: ev.target.checked,
     });
   }

src/components/side_panel/chart/gauge_chart_panel/gauge_chart_config_panel.ts

@@ -7,7 +7,7 @@ import { ChartTerms } from "../../../translations_terms";
 interface Props {
   figureId: UID;
   definition: GaugeChartDefinition;
-  updateChart: (definition: Partial<GaugeChartDefinition>) => DispatchResult;
+  updateChart: (figureId: UID, definition: Partial<GaugeChartDefinition>) => DispatchResult;
 }
 
 interface PanelState {
@@ -42,7 +42,7 @@ export class GaugeChartConfigPanel extends Component<Props, SpreadsheetChildEnv>
   }
 
   updateDataRange() {
-    this.state.dataRangeDispatchResult = this.props.updateChart({
+    this.state.dataRangeDispatchResult = this.props.updateChart(this.props.figureId, {
       dataRange: this.dataRange,
     });
   }

src/components/side_panel/chart/gauge_chart_panel/gauge_chart_design_panel.ts

@@ -63,7 +63,7 @@ type GaugeMenu =
 interface Props {
   figureId: UID;
   definition: GaugeChartDefinition;
-  updateChart: (definition: Partial<GaugeChartDefinition>) => DispatchResult;
+  updateChart: (figureId: UID, definition: Partial<GaugeChartDefinition>) => DispatchResult;
 }
 
 interface PanelState {
@@ -97,13 +97,13 @@ export class GaugeChartDesignPanel extends Component<Props, SpreadsheetChildEnv>
 
   updateBackgroundColor(color: Color) {
     this.state.openedMenu = undefined;
-    this.props.updateChart({
+    this.props.updateChart(this.props.figureId, {
       background: color,
     });
   }
 
   updateTitle(ev) {
-    this.props.updateChart({
+    this.props.updateChart(this.props.figureId, {
       title: ev.target.value,
     });
   }
@@ -200,7 +200,7 @@ export class GaugeChartDesignPanel extends Component<Props, SpreadsheetChildEnv>
   }
 
   private updateSectionRule(sectionRule: SectionRule) {
-    this.state.sectionRuleDispatchResult = this.props.updateChart({
+    this.state.sectionRuleDispatchResult = this.props.updateChart(this.props.figureId, {
       sectionRule,
     });
   }

src/components/side_panel/chart/line_bar_pie_panel/config_panel.ts

@@ -10,6 +10,7 @@ interface Props {
   figureId: UID;
   definition: LineChartDefinition | BarChartDefinition | PieChartDefinition;
   updateChart: (
+    figureId: UID,
     definition: Partial<LineChartDefinition | BarChartDefinition | PieChartDefinition>
   ) => DispatchResult;
 }
@@ -55,7 +56,7 @@ export class LineBarPieConfigPanel extends Component<Props, SpreadsheetChildEnv>
   }
 
   onUpdateDataSetsHaveTitle(ev) {
-    this.props.updateChart({
+    this.props.updateChart(this.props.figureId, {
       dataSetsHaveTitle: ev.target.checked,
     });
   }
@@ -69,7 +70,7 @@ export class LineBarPieConfigPanel extends Component<Props, SpreadsheetChildEnv>
   }
 
   onDataSeriesConfirmed() {
-    this.state.datasetDispatchResult = this.props.updateChart({
+    this.state.datasetDispatchResult = this.props.updateChart(this.props.figureId, {
       dataSets: this.dataSeriesRanges,
     });
   }
@@ -83,7 +84,7 @@ export class LineBarPieConfigPanel extends Component<Props, SpreadsheetChildEnv>
   }
 
   onLabelRangeConfirmed() {
-    this.state.labelsDispatchResult = this.props.updateChart({
+    this.state.labelsDispatchResult = this.props.updateChart(this.props.figureId, {
       labelRange: this.labelRange,
     });
   }

src/components/side_panel/chart/line_bar_pie_panel/design_panel.ts

@@ -9,6 +9,7 @@ interface Props {
   figureId: UID;
   definition: LineChartDefinition | BarChartDefinition | PieChartDefinition;
   updateChart: (
+    figureId: UID,
     definition: Partial<LineChartDefinition | BarChartDefinition | PieChartDefinition>
   ) => CommandResult | CommandResult[];
 }
@@ -38,19 +39,19 @@ export class LineBarPieDesignPanel extends Component<Props, SpreadsheetChildEnv>
   }
 
   updateBackgroundColor(color: Color) {
-    this.props.updateChart({
+    this.props.updateChart(this.props.figureId, {
       background: color,
     });
   }
 
   updateTitle(ev) {
-    this.props.updateChart({
+    this.props.updateChart(this.props.figureId, {
       title: ev.target.value,
     });
   }
 
   updateSelect(attr: string, ev) {
-    this.props.updateChart({
+    this.props.updateChart(this.props.figureId, {
       [attr]: ev.target.value,
     });
   }

src/components/side_panel/chart/line_chart/line_chart_config_panel.ts

@@ -13,13 +13,13 @@ export class LineConfigPanel extends LineBarPieConfigPanel {
   }
 
   onUpdateLabelsAsText(ev) {
-    this.props.updateChart({
+    this.props.updateChart(this.props.figureId, {
       labelsAsText: ev.target.checked,
     });
   }
 
   onUpdateStacked(ev) {
-    this.props.updateChart({
+    this.props.updateChart(this.props.figureId, {
       stacked: ev.target.checked,
     });
   }

src/components/side_panel/chart/main_chart_panel/main_chart_panel.ts

@@ -45,23 +45,17 @@ interface Props {
 interface State {
   panel: "configuration" | "design";
   figureId: UID;
-  sheetId: UID;
 }
 
 export class ChartPanel extends Component<Props, SpreadsheetChildEnv> {
   static template = "o-spreadsheet-ChartPanel";
 
   private state!: State;
-  private shouldUpdateChart: boolean = true;
 
   get figureId(): UID {
     return this.state.figureId;
   }
 
-  get sheetId(): UID {
-    return this.state.sheetId;
-  }
-
   setup(): void {
     const selectedFigureId = this.env.model.getters.getSelectedFigureId();
     if (!selectedFigureId) {
@@ -70,17 +64,12 @@ export class ChartPanel extends Component<Props, SpreadsheetChildEnv> {
     this.state = useState({
       panel: "configuration",
       figureId: selectedFigureId,
-      sheetId: this.env.model.getters.getActiveSheetId(),
     });
 
     onWillUpdateProps(() => {
       const selectedFigureId = this.env.model.getters.getSelectedFigureId();
       if (selectedFigureId && selectedFigureId !== this.state.figureId) {
         this.state.figureId = selectedFigureId;
-        this.state.sheetId = this.env.model.getters.getActiveSheetId();
-        this.shouldUpdateChart = false;
-      } else {
-        this.shouldUpdateChart = true;
       }
       if (!this.env.model.getters.isChartDefined(this.figureId)) {
         this.props.onCloseSidePanel();
@@ -89,8 +78,8 @@ export class ChartPanel extends Component<Props, SpreadsheetChildEnv> {
     });
   }
 
-  updateChart<T extends ChartDefinition>(updateDefinition: Partial<T>) {
-    if (!this.shouldUpdateChart) {
+  updateChart<T extends ChartDefinition>(figureId: UID, updateDefinition: Partial<T>) {
+    if (figureId !== this.figureId) {
       return;
     }
     const definition: T = {
@@ -99,8 +88,8 @@ export class ChartPanel extends Component<Props, SpreadsheetChildEnv> {
     };
     return this.env.model.dispatch("UPDATE_CHART", {
       definition,
-      id: this.figureId,
-      sheetId: this.sheetId,
+      id: figureId,
+      sheetId: this.env.model.getters.getFigureSheetId(figureId)!,
     });
   }
 
@@ -113,7 +102,7 @@ export class ChartPanel extends Component<Props, SpreadsheetChildEnv> {
     this.env.model.dispatch("UPDATE_CHART", {
       definition,
       id: this.figureId,
-      sheetId: this.sheetId,
+      sheetId: this.env.model.getters.getFigureSheetId(this.figureId)!,
     });
   }
 

src/components/side_panel/chart/scorecard_chart_panel/scorecard_chart_config_panel.ts

@@ -7,7 +7,7 @@ import { ChartTerms } from "../../../translations_terms";
 interface Props {
   figureId: UID;
   definition: ScorecardChartDefinition;
-  updateChart: (definition: Partial<ScorecardChartDefinition>) => DispatchResult;
+  updateChart: (figureId: UID, definition: Partial<ScorecardChartDefinition>) => DispatchResult;
 }
 
 interface PanelState {
@@ -54,7 +54,7 @@ export class ScorecardChartConfigPanel extends Component<Props, SpreadsheetChild
   }
 
   updateKeyValueRange() {
-    this.state.keyValueDispatchResult = this.props.updateChart({
+    this.state.keyValueDispatchResult = this.props.updateChart(this.props.figureId, {
       keyValue: this.keyValue,
     });
   }
@@ -64,12 +64,12 @@ export class ScorecardChartConfigPanel extends Component<Props, SpreadsheetChild
   }
 
   updateBaselineRange() {
-    this.state.baselineDispatchResult = this.props.updateChart({
+    this.state.baselineDispatchResult = this.props.updateChart(this.props.figureId, {
       baseline: this.baseline,
     });
   }
 
   updateBaselineMode(ev) {
-    this.props.updateChart({ baselineMode: ev.target.value });
+    this.props.updateChart(this.props.figureId, { baselineMode: ev.target.value });
   }
 }

src/components/side_panel/chart/scorecard_chart_panel/scorecard_chart_design_panel.ts

@@ -8,7 +8,7 @@ type ColorPickerId = undefined | "backgroundColor" | "baselineColorUp" | "baseli
 interface Props {
   figureId: UID;
   definition: ScorecardChartDefinition;
-  updateChart: (definition: Partial<ScorecardChartDefinition>) => DispatchResult;
+  updateChart: (figureId: UID, definition: Partial<ScorecardChartDefinition>) => DispatchResult;
 }
 
 interface PanelState {
@@ -32,13 +32,13 @@ export class ScorecardChartDesignPanel extends Component<Props, SpreadsheetChild
   }
 
   updateTitle(ev) {
-    this.props.updateChart({
+    this.props.updateChart(this.props.figureId, {
       title: ev.target.value,
     });
   }
 
   updateBaselineDescr(ev) {
-    this.props.updateChart({ baselineDescr: ev.target.value });
+    this.props.updateChart(this.props.figureId, { baselineDescr: ev.target.value });
   }
 
   openColorPicker(colorPickerId: ColorPickerId) {
@@ -48,13 +48,13 @@ export class ScorecardChartDesignPanel extends Component<Props, SpreadsheetChild
   setColor(color: Color, colorPickerId: ColorPickerId) {
     switch (colorPickerId) {
       case "backgroundColor":
-        this.props.updateChart({ background: color });
+        this.props.updateChart(this.props.figureId, { background: color });
         break;
       case "baselineColorDown":
-        this.props.updateChart({ baselineColorDown: color });
+        this.props.updateChart(this.props.figureId, { baselineColorDown: color });
         break;
       case "baselineColorUp":
-        this.props.updateChart({ baselineColorUp: color });
+        this.props.updateChart(this.props.figureId, { baselineColorUp: color });
         break;
     }
     this.state.openedColorPicker = undefined;

tests/components/charts.test.ts

@@ -447,6 +447,43 @@ describe("figures", () => {
     expect(model.getters.getChartDefinition("2").title).toBe("old_title_2");
   });
 
+  test("selecting a chart then selecting another chart and editing property change the second chart", async () => {
+    createChart(
+      model,
+      {
+        dataSets: ["C1:C4"],
+        labelRange: "A2:A4",
+        type: "line",
+        title: "old_title_1",
+      },
+      "1"
+    );
+    await nextTick();
+    createChart(
+      model,
+      {
+        dataSets: ["C1:C4"],
+        labelRange: "A2:A4",
+        type: "line",
+        title: "old_title_2",
+      },
+      "2"
+    );
+    await nextTick();
+
+    const figures = fixture.querySelectorAll(".o-figure");
+    await simulateClick(figures[0] as HTMLElement);
+    await simulateClick(".o-chart-menu-item");
+    await simulateClick(".o-menu div[data-name='edit']");
+    await simulateClick(".o-panel .inactive");
+    await simulateClick(figures[1] as HTMLElement);
+    await simulateClick(".o-chart-title input");
+    setInputValueAndTrigger(".o-chart-title input", "new_title", "change");
+
+    expect(model.getters.getChartDefinition("1").title).toBe("old_title_1");
+    expect(model.getters.getChartDefinition("2").title).toBe("new_title");
+  });
+
   test.each(["basicChart", "scorecard"])(
     "can edit charts %s background",
     async (chartType: string) => {