[IMP] Cells: set explicit style on hyperlink-like content

Currently, we apply a custom style to the cells that contain a content
similar to a hyper/markdown link during the rendering process. This
means that this style disappears once we export the spreadsheet to other
formats (.xlsx for instance ). Furthermore, the detection depends on the
elements added to `urlRegistry`.

In some cases (specifically when loading a spreadsheet in a public page
in Odoo) we might populate this registry differently, which leads to a
difference of behaviour between renderers of the same raw content.

Since the style was computed during the rendering process, it means that
you could not just "get rid" of that style, you had to define a
dedicated style to override it, so 'clear format' would never work.

This revision changes our strategy and tries to explicitely apply a
specific style when adding a content detected as a
hyperlink/markdownling. This way, we ensure that the style is hardcoded
in the data and will be consistent when we export it in .xslx files or
if we show it on a public page in Odoo.

Task: 5410289

Author: rrahir

demo/data.js

@@ -116,6 +116,7 @@ export const demoData = {
         F2: 5,
         F3: 6,
         F4: 7,
+        A21: 30,
       },
       formats: {
         C22: 1,
@@ -3526,6 +3527,11 @@ export const demoData = {
     11: {
       bold: true,
     },
+    30: {
+      bold: true,
+      fontSize: 18,
+      fillColor: "#FFFF00",
+    },
   },
   formats: {
     1: "0.00%",

packages/o-spreadsheet-engine/src/helpers/data_normalization.ts

@@ -15,7 +15,8 @@ const globalIdCounter = new WeakMap<ItemsDic<any>, number>();
 export function getItemId<T>(item: T, itemsDic: ItemsDic<T>) {
   if (!globalReverseLookup.has(itemsDic)) {
     globalReverseLookup.set(itemsDic, new Map());
-    globalIdCounter.set(itemsDic, 0);
+    const x = Math.max(...Object.keys(itemsDic).map((k) => Number(k)), 0);
+    globalIdCounter.set(itemsDic, x);
   }
   const reverseLookup = globalReverseLookup.get(itemsDic)!;
   const canonical = getCanonicalRepresentation(item);

packages/o-spreadsheet-engine/src/helpers/misc.ts

@@ -236,6 +236,10 @@ export function parseSheetUrl(sheetLink: string) {
   throw new Error(`${sheetLink} is not a valid sheet link`);
 }
 
+export function hasHyperlinkContent(content: string): boolean {
+  return isMarkdownLink(content) || isWebLink(content) || content.startsWith("=HYPERLINK(");
+}
+
 /**
  * This helper function can be used as a type guard when filtering arrays.
  * const foo: number[] = [1, 2, undefined, 4].filter(isDefined)

packages/o-spreadsheet-engine/src/migrations/data.ts

@@ -218,6 +218,7 @@ export function repairInitialMessages(
   initialMessages = fixChartDefinitions(data, initialMessages);
   initialMessages = fixFigureOffset(data, initialMessages);
   initialMessages = fixTranslatedDuplicateSheetName(data, initialMessages);
+  initialMessages = fixHyperlinkCellUpdate(data, initialMessages);
   return initialMessages;
 }
 
@@ -408,6 +409,13 @@ function fixTranslatedDuplicateSheetName(
   return initialMessages;
 }
 
+function fixHyperlinkCellUpdate(
+  data: Partial<WorkbookData>,
+  initialMessages: StateUpdateMessage[]
+) {
+  return initialMessages;
+}
+
 // -----------------------------------------------------------------------------
 // Helpers
 // -----------------------------------------------------------------------------

packages/o-spreadsheet-engine/src/migrations/migration_steps.ts

@@ -1,10 +1,11 @@
-import { BACKGROUND_CHART_COLOR, FORMULA_REF_IDENTIFIER } from "../constants";
+import { BACKGROUND_CHART_COLOR, FORMULA_REF_IDENTIFIER, LINK_COLOR } from "../constants";
 import { toXC } from "../helpers/coordinates";
 import { getItemId } from "../helpers/data_normalization";
-import { getUniqueText, sanitizeSheetName } from "../helpers/misc";
+import { getUniqueText, isMarkdownLink, isWebLink, sanitizeSheetName } from "../helpers/misc";
 import { getMaxObjectId } from "../helpers/pivot/pivot_helpers";
+import { recomputeZones } from "../helpers/recompute_zones";
 import { DEFAULT_TABLE_CONFIG } from "../helpers/table_presets";
-import { overlap, toZone, zoneToXc } from "../helpers/zones";
+import { isZoneInside, overlap, toZone, zoneToXc } from "../helpers/zones";
 import { Registry } from "../registry";
 import { CustomizedDataSet, schemeToColorScale } from "../types/chart";
 import { Format } from "../types/format";
@@ -576,6 +577,11 @@ migrationStepRegistry
       }
       return data;
     },
+  })
+  .add("19.2.0", {
+    migrate(data: WorkbookData): any {
+      return addHyperlinkStyle(data);
+    },
   });
 
 function fixOverlappingFilters(data: any): any {
@@ -600,3 +606,56 @@ function fixOverlappingFilters(data: any): any {
   }
   return data;
 }
+
+function addHyperlinkStyle(data: WorkbookData): WorkbookData {
+  for (const sheet of data.sheets || []) {
+    for (const xc in sheet.cells || {}) {
+      const content = sheet.cells[xc];
+      if (content && (isMarkdownLink(content) || isWebLink(content))) {
+        // debugger;
+        // find the XC in the styles
+        const styleXc = Object.keys(sheet.styles).find((styleXC) =>
+          isZoneInside(toZone(xc), toZone(styleXC))
+        );
+        if (styleXc) {
+          // vérifier qu'il y a textcolor; si c'est le cas, on ne fait rien
+          if (data.styles[sheet.styles[styleXc]].textColor) {
+            continue;
+          }
+          // sinon, on split le truc existant et on ajourte la couleur juste pour nous.
+
+          const existingStyleId = sheet.styles[styleXc];
+          const existingStyle = data.styles[existingStyleId];
+
+          const zones = recomputeZones([toZone(styleXc)], [toZone(xc)]);
+
+          delete sheet.styles[styleXc];
+
+          zones.forEach((zone) => {
+            const zoneXc = zoneToXc(zone);
+            sheet.styles[zoneXc] = existingStyleId;
+          });
+
+          sheet.styles[xc] = getItemId({ ...existingStyle, textColor: LINK_COLOR }, data.styles);
+
+          /**
+           * Stylexc > tozone
+           * zones << recomputezone without xc
+           *
+           * delete sheet styleXc
+           * add all zones with the existing style
+           * add xc with the existing style + textcolor as a new style
+           *
+           *
+           */
+          // utiliser getItemId
+        }
+        // Si on n'a pas de style existant, ou pas de textcolor, on ajoute la couleur et un style a part
+        else {
+          sheet.styles[xc] = getItemId({ textColor: LINK_COLOR }, data.styles);
+        }
+      }
+    }
+  }
+  return data;
+}

packages/o-spreadsheet-engine/src/plugins/ui_feature/cell_computed_style.ts

@@ -1,4 +1,3 @@
-import { LINK_COLOR } from "../../constants";
 import { PositionMap } from "../../helpers/cells/position_map";
 import { isObjectEmptyRecursive, removeFalsyAttributes } from "../../helpers/misc";
 import { positionToZone } from "../../helpers/zones";
@@ -107,10 +106,6 @@ export class CellComputedStylePlugin extends UIPlugin {
           ...removeFalsyAttributes(styles.get(position)),
           ...removeFalsyAttributes(this.getters.getCellConditionalFormatStyle(position)),
         };
-        const evaluatedCell = this.getters.getEvaluatedCell(position);
-        if (evaluatedCell.link && !computedStyle.textColor) {
-          computedStyle.textColor = LINK_COLOR;
-        }
         this.styles.set(position, computedStyle);
       }
     }

src/components/composer/composer/cell_composer_store.ts

@@ -1,4 +1,5 @@
-import { parseTokens } from "@odoo/o-spreadsheet-engine";
+import { hasHyperlinkContent, parseTokens, Style } from "@odoo/o-spreadsheet-engine";
+import { LINK_COLOR } from "@odoo/o-spreadsheet-engine/constants";
 import { prettify } from "@odoo/o-spreadsheet-engine/formulas/formula_formatter";
 import {
   isMultipleElementMatrix,
@@ -28,9 +29,9 @@ import {
   Direction,
   Format,
   FormulaCell,
+  isMatrix,
   Locale,
   RemoveColumnsRowsCommand,
-  isMatrix,
 } from "../../../types";
 import { AbstractComposerStore, ComposerSelection } from "./abstract_composer_store";
 
@@ -189,10 +190,19 @@ export class CellComposerStore extends AbstractComposerStore {
       if (cell.link && !isFormula(content)) {
         content = markdownLink(content, cell.link.url);
       }
+      /** si pas de cell core || cell core a un style */
+      let style: Style | undefined;
+      // if ()
+      const cellStyle = this.getters.getCellStyle(this.currentEditedCell);
+      if ((!cellStyle || !cellStyle.textColor) && hasHyperlinkContent(content)) {
+        style = { ...cellStyle, textColor: LINK_COLOR };
+      }
+
       this.addHeadersForSpreadingFormula(content);
       this.model.dispatch("UPDATE_CELL", {
         ...this.currentEditedCell,
         content,
+        style,
       });
     } else {
       this.model.dispatch("UPDATE_CELL", {

src/components/link/link_editor/link_editor.ts

@@ -1,3 +1,4 @@
+import { LINK_COLOR } from "@odoo/o-spreadsheet-engine/constants";
 import { detectLink, urlRepresentation } from "@odoo/o-spreadsheet-engine/helpers/links";
 import { canonicalizeNumberContent } from "@odoo/o-spreadsheet-engine/helpers/locale";
 import { SpreadsheetChildEnv } from "@odoo/o-spreadsheet-engine/types/spreadsheet_env";
@@ -86,16 +87,20 @@ export class LinkEditor extends Component<LinkEditorProps, SpreadsheetChildEnv>
   }
 
   save() {
+    const sheetId = this.env.model.getters.getActiveSheetId();
     const { col, row } = this.props.cellPosition;
     const locale = this.env.model.getters.getLocale();
     const label = this.link.label
       ? canonicalizeNumberContent(this.link.label, locale)
       : this.link.url;
+    // TODORAR add this feature to UPDATE_CELL in the composr as well ?
+    const style = this.env.model.getters.getCellStyle({ sheetId, col, row });
     this.env.model.dispatch("UPDATE_CELL", {
-      col: col,
-      row: row,
-      sheetId: this.env.model.getters.getActiveSheetId(),
+      col,
+      row,
+      sheetId,
       content: markdownLink(label, this.link.url),
+      style: { ...style, textColor: LINK_COLOR },
     });
     this.props.onClosed?.();
   }

tests/cells/cell_plugin.test.ts

@@ -1,4 +1,3 @@
-import { LINK_COLOR } from "@odoo/o-spreadsheet-engine/constants";
 import { urlRepresentation } from "@odoo/o-spreadsheet-engine/helpers/links";
 import { corePluginRegistry } from "@odoo/o-spreadsheet-engine/plugins";
 import { CoreCommand, CorePlugin, Model } from "../../src";
@@ -31,7 +30,6 @@ import {
   getCellStyle,
   getCellText,
   getEvaluatedCell,
-  getStyle,
 } from "../test_helpers/getters_helpers";
 import { addTestPlugin, getGrid, setGrid, target } from "../test_helpers/helpers";
 
@@ -266,7 +264,6 @@ describe("link cell", () => {
       expect(cell.link?.url).toBe(url);
       expect(urlRepresentation(cell.link!, model.getters)).toBe(url);
       expect(getCell(model, "A1")?.content).toBe(`[my label](${url})`);
-      expect(getStyle(model, "A1")).toEqual({ textColor: LINK_COLOR });
       expect(getCellText(model, "A1")).toBe("my label");
     }
   );
@@ -281,7 +278,6 @@ describe("link cell", () => {
       expect(cell.link?.url).toBe(url);
       expect(urlRepresentation(cell.link!, model.getters)).toBe(url);
       expect(getCell(model, "B1")?.content).toBe(`=HYPERLINK("${url}", "Odoo")`);
-      expect(getStyle(model, "B1")).toEqual({ textColor: LINK_COLOR });
       expect(getCellText(model, "B1")).toBe(`=HYPERLINK("${url}", "Odoo")`);
     }
   );

tests/clipboard/clipboard_plugin.test.ts

@@ -1,4 +1,4 @@
-import { DEFAULT_BORDER_DESC, LINK_COLOR } from "@odoo/o-spreadsheet-engine/constants";
+import { DEFAULT_BORDER_DESC } from "@odoo/o-spreadsheet-engine/constants";
 import {
   getClipboardDataPositions,
   parseOSClipboardContent,
@@ -3026,7 +3026,6 @@ describe("cross spreadsheet copy/paste", () => {
     expect(cell.link?.url).toBe(url);
     expect(urlRepresentation(cell.link!, modelB.getters)).toBe(url);
     expect(getCell(modelB, "D1")?.content).toBe("[Odoo Website](https://www.odoo.com)");
-    expect(getStyle(modelB, "D1")).toEqual({ textColor: LINK_COLOR });
     expect(getCellText(modelB, "D1")).toBe("Odoo Website");
   });