[IMP] model: improve figure export data

Currently, charts/images/carousels are exported inside `figure.data`
in the JSON. That's not ideal, `figure.data` then needs to be typed as
`any`, and the charts plugin need to be aware of carousels for its
import.

This commit changes the export, now each plugin has its own key in the
data, so it can be typed correctly and they are not interlinked.

Task: 5002886

Author: hokolomopo

src/migrations/data.ts

@@ -272,15 +272,19 @@ function fixChartDefinitions(data: Partial<WorkbookData>, initialMessages: State
   const map = {};
   for (const sheet of data.sheets || []) {
     sheet.figures?.forEach((figure) => {
-      if (figure.tag === "chart") {
-        // chart definition
-        if (data.version && compareVersions(String(data.version), "18.5.1") <= 0) {
-          map[figure.data.chartId] = figure.data;
-        } else {
-          map[figure.id] = figure.data;
-        }
+      if (
+        figure.tag === "chart" &&
+        data.version &&
+        compareVersions(String(data.version), "18.5.1") > 0
+      ) {
+        map[figure.id] = (figure as any).data;
       }
     });
+    for (const chartId in sheet.charts || {}) {
+      if (data.version && compareVersions(String(data.version), "18.5.1") <= 0) {
+        map[chartId] = sheet.charts[chartId].chart;
+      }
+    }
   }
   for (const message of initialMessages) {
     if (message.type === "REMOTE_REVISION") {
@@ -415,6 +419,9 @@ export function createEmptySheet(sheetId: UID, name: string): SheetData {
     conditionalFormats: [],
     dataValidationRules: [],
     figures: [],
+    charts: {},
+    carousels: {},
+    images: {},
     tables: [],
     isVisible: true,
   };
@@ -438,10 +445,27 @@ export function createEmptyWorkbookData(sheetName = "Sheet1"): WorkbookData {
 
 export function createEmptyExcelSheet(sheetId: UID, name: string): ExcelSheetData {
   return {
-    ...(createEmptySheet(sheetId, name) as Omit<ExcelSheetData, "charts">),
-    charts: [],
-    images: [],
+    id: sheetId,
+    name,
+    colNumber: 26,
+    rowNumber: 100,
+    cells: {},
+    styles: {},
+    formats: {},
+    borders: {},
+    cols: {},
+    rows: {},
+    merges: [],
+    conditionalFormats: [],
+    dataValidationRules: [],
+    figures: [],
+    charts: {},
+    carousels: {},
+    tables: [],
+    isVisible: true,
+    images: {},
     cellValues: {},
+    formulaSpillRanges: {},
   };
 }
 

src/migrations/migration_steps.ts

@@ -372,7 +372,7 @@ migrationStepRegistry
   })
   .add("18.0.4", {
     // "Add operator in gauge inflection points",
-    migrate(data: WorkbookData): any {
+    migrate(data: any): any {
       for (const sheet of data.sheets || []) {
         for (const figure of sheet.figures || []) {
           if (figure.tag !== "chart" || figure.data.type !== "gauge") {
@@ -503,7 +503,7 @@ migrationStepRegistry
     },
   })
   .add("18.4.2", {
-    migrate(data: WorkbookData): any {
+    migrate(data: any): any {
       for (const sheet of data.sheets || []) {
         for (const figure of sheet.figures || []) {
           if (figure.tag !== "chart" || figure.data.type !== "scorecard") {
@@ -538,11 +538,24 @@ migrationStepRegistry
     },
   })
   .add("18.5.1", {
-    migrate(data: WorkbookData): any {
+    migrate(data: any): WorkbookData {
       for (const sheet of data.sheets || []) {
+        if (!sheet.charts) {
+          sheet.charts = {};
+        }
+        if (!sheet.images) {
+          sheet.images = {};
+        }
+
         for (const figure of sheet.figures || []) {
           if (figure.tag === "chart") {
-            figure.data.chartId = figure.id;
+            const chart = figure.data;
+            delete figure.data;
+            sheet.charts[figure.id] = { figureId: figure.id, chart };
+          } else if (figure.tag === "image") {
+            const image = figure.data;
+            delete figure.data;
+            sheet.images[figure.id] = { figureId: figure.id, image };
           }
         }
       }

src/plugins/core/carousel.ts

@@ -122,19 +122,19 @@ export class CarouselPlugin extends CorePlugin<CarouselState> implements Carouse
 
   import(data: WorkbookData) {
     for (const sheet of data.sheets) {
-      const carousels = (sheet.figures || []).filter((figure) => figure.tag === "carousel");
-      for (const carousel of carousels) {
-        this.history.update("carousels", sheet.id, carousel.id, { items: carousel.data.items });
+      for (const carouselId in sheet.carousels || {}) {
+        const { carousel } = sheet.carousels[carouselId];
+        this.history.update("carousels", sheet.id, carouselId, carousel);
       }
     }
   }
 
   export(data: WorkbookData) {
     for (const sheet of data.sheets) {
-      const carousels = sheet.figures.filter((figure) => figure.tag === "carousel");
-      for (const carousel of carousels) {
-        if (this.carousels[sheet.id]?.[carousel.id]) {
-          carousel.data = { ...carousel.data, ...this.carousels[sheet.id]?.[carousel.id] };
+      for (const carouselId in this.carousels[sheet.id] || {}) {
+        const carousel = this.carousels[sheet.id]?.[carouselId];
+        if (carousel) {
+          sheet.carousels[carouselId] = { figureId: carouselId, carousel };
         }
       }
     }

src/plugins/core/chart.ts

@@ -12,7 +12,6 @@ import {
   CreateChartCommand,
   DeleteChartCommand,
   DOMDimension,
-  FigureData,
   HeaderIndex,
   PixelPosition,
   UID,
@@ -212,59 +211,26 @@ export class ChartPlugin extends CorePlugin<ChartState> implements ChartState {
 
   import(data: WorkbookData) {
     for (const sheet of data.sheets) {
-      if (sheet.figures) {
-        for (const figure of sheet.figures) {
-          // TODO:
-          // figure data should be external IMO => chart should be in sheet.chart
-          // instead of in figure.data
-          if (figure.tag === "chart") {
-            const chartId = figure.data.chartId;
-            const chart = this.createChart(figure.id, figure.data, sheet.id);
-            this.charts[chartId] = { chart, figureId: figure.id };
-          } else if (figure.tag === "carousel") {
-            for (const chartId in figure.data.chartDefinitions || {}) {
-              const chartDefinition = figure.data.chartDefinitions[chartId];
-              const chart = this.createChart(figure.id, chartDefinition, sheet.id);
-              this.charts[chartId] = { chart, figureId: figure.id };
-            }
-          }
-        }
+      for (const chartId in sheet.charts) {
+        const { figureId, chart: chartData } = sheet.charts[chartId];
+        const chart = this.createChart(figureId, chartData, sheet.id);
+        this.charts[chartId] = { chart, figureId };
       }
     }
   }
 
   export(data: WorkbookData) {
     if (data.sheets) {
       for (const sheet of data.sheets) {
-        // TODO This code is false, if two plugins want to insert figures on the sheet, it will crash !
-        const sheetFigures = this.getters.getFigures(sheet.id);
-        const figures: FigureData<any>[] = [];
-        for (const sheetFigure of sheetFigures) {
-          const figure = sheetFigure as FigureData<any>;
-          const chartId = Object.keys(this.charts).find(
-            (chartId) => this.charts[chartId]?.figureId === sheetFigure.id
-          );
-          if (figure && figure.tag === "chart" && chartId) {
-            const data = this.charts[chartId]?.chart.getDefinition();
-            if (data) {
-              figure.data = { ...data, chartId };
-              figures.push(figure);
-            }
-          } else if (figure && figure.tag === "carousel") {
-            const chartIds = Object.keys(this.charts).filter(
-              (chartId) => this.charts[chartId]?.figureId === sheetFigure.id
-            );
-            const data = {};
-            for (const chartId of chartIds) {
-              data[chartId] = this.charts[chartId]?.chart.getDefinition();
-            }
-            figure.data = { chartDefinitions: data };
-            figures.push(figure);
-          } else {
-            figures.push(figure);
+        for (const chartId of this.getChartIds(sheet.id)) {
+          const figureChart = this.charts[chartId];
+          if (figureChart) {
+            sheet.charts[chartId] = {
+              figureId: figureChart.figureId,
+              chart: figureChart.chart.getDefinition(),
+            };
           }
         }
-        sheet.figures = figures;
       }
     }
   }

src/plugins/core/figures.ts

@@ -364,8 +364,7 @@ export class FigurePlugin extends CorePlugin<FigureState> implements FigureState
   export(data: WorkbookData) {
     for (const sheet of data.sheets) {
       for (const figure of this.getFigures(sheet.id)) {
-        const data = undefined;
-        sheet.figures.push({ ...figure, data });
+        sheet.figures.push({ ...figure });
       }
     }
   }

src/plugins/core/image.ts

@@ -7,7 +7,6 @@ import {
   CoreCommand,
   DOMDimension,
   ExcelWorkbookData,
-  FigureData,
   FigureSize,
   HeaderIndex,
   PixelPosition,
@@ -141,43 +140,27 @@ export class ImagePlugin extends CorePlugin<ImageState> implements ImageState {
 
   import(data: WorkbookData) {
     for (const sheet of data.sheets) {
-      const images = (sheet.figures || []).filter((figure) => figure.tag === "image");
-      for (const image of images) {
-        this.history.update("images", sheet.id, image.id, image.data);
-        this.syncedImages.add(image.data.path);
+      for (const imageId in sheet.images || {}) {
+        const { figureId, image } = sheet.images[imageId];
+        this.history.update("images", sheet.id, figureId, image);
+        this.syncedImages.add(image.path);
       }
     }
   }
 
   export(data: WorkbookData) {
     for (const sheet of data.sheets) {
-      const images = sheet.figures.filter((figure) => figure.tag === "image");
-      for (const image of images) {
-        image.data = this.images[sheet.id]?.[image.id];
+      for (const imageId in this.images[sheet.id] || {}) {
+        const image = this.images[sheet.id]?.[imageId];
+        if (image) {
+          sheet.images[imageId] = { figureId: imageId, image };
+        }
       }
     }
   }
 
   exportForExcel(data: ExcelWorkbookData) {
-    for (const sheet of data.sheets) {
-      if (!sheet.images) {
-        sheet.images = [];
-      }
-      const figures = this.getters.getFigures(sheet.id);
-      const images: FigureData<Image>[] = [];
-      for (const figure of figures) {
-        if (figure?.tag === "image") {
-          const image = this.getImage(figure.id);
-          if (image) {
-            images.push({
-              ...figure,
-              data: deepCopy(image),
-            });
-          }
-        }
-      }
-      sheet.images = [...sheet.images, ...images];
-    }
+    return this.export(data);
   }
 
   private getAllImages(): Image[] {

src/plugins/core/sheet.ts

@@ -17,6 +17,7 @@ import {
   toCartesian,
 } from "../../helpers/index";
 import { isSheetNameEqual } from "../../helpers/sheet";
+import { createEmptySheet } from "../../migrations/data";
 import {
   Cell,
   CellPosition,
@@ -283,21 +284,11 @@ export class SheetPlugin extends CorePlugin<SheetState> implements SheetState {
     data.sheets = this.orderedSheetIds.filter(isDefined).map((id) => {
       const sheet = this.sheets[id]!;
       const sheetData: SheetData = {
+        ...createEmptySheet(sheet.id, sheet.name),
         id: sheet.id,
         name: sheet.name,
         colNumber: sheet.numberOfCols,
         rowNumber: this.getters.getNumberRows(sheet.id),
-        rows: {},
-        cols: {},
-        merges: [],
-        cells: {},
-        styles: {},
-        formats: {},
-        borders: {},
-        conditionalFormats: [],
-        dataValidationRules: [],
-        figures: [],
-        tables: [],
         areGridLinesVisible:
           sheet.areGridLinesVisible === undefined ? true : sheet.areGridLinesVisible,
         isVisible: sheet.isVisible,