[IMP] style,format: add default style & format #6762

fdamhaut · 2025-07-07T07:46:35Z

Description:

description of this task, what is implemented and why it is implemented that way.

Task: TASK_ID

review checklist

robodoo · 2025-07-07T07:46:38Z

Add style and format sheet/col/row default. This task is groundwork for the infinite viewport. Task: 4852413

pro-odoo · 2025-07-08T08:47:31Z

src/clipboard_handlers/cell_clipboard.ts

@@ -58,8 +60,8 @@ export class CellClipboardHandler extends AbstractCellClipboardHandler<
            const pivotFormula = createPivotFormula(formulaPivotId, pivotCell);
            cell = {
              id: cell?.id || "",
-              style: cell?.style,
-              format: cell?.format,
+              style: style,


Suggested change

style: style,

style,

Same everywhere

LucasLefevre

it is unclear to me while reading what cell.style is supposed be when there are default values.

As a more general comment, I think now it might be a good idea to move all the style management out of the cell core plugin to its own dedicated plugin and remove cell.style. We could use cell ids to set a style on specific cells to avoid all range adaptations etc.)
I didn't check every use of cell.style to see how it would change (there seems be a bit more than 40 uses)
WDYT ?

LucasLefevre · 2025-07-10T12:52:20Z

src/clipboard_handlers/cell_clipboard.ts

-              style: cell?.style,
-              format: evaluatedCell.format,
+              style: style,
+              format: evaluatedCell.format ?? format,


shouldn't evaluatedCell.format be the actual format (including the default format)

LucasLefevre · 2025-07-10T12:52:40Z

src/plugins/core/cell.ts

@@ -47,9 +47,17 @@ import {
 import { CorePlugin } from "../core_plugin";
 import { PositionMap } from "../ui_core_views/cell_evaluation/position_map";

+interface sheetDefault<T> {


Suggested change

interface sheetDefault<T> {

interface SheetDefault<T> {

LucasLefevre · 2025-07-10T12:56:10Z

src/plugins/core/cell.ts

@@ -449,9 +498,18 @@ export class CellPlugin extends CorePlugin<CoreState> implements CoreState {
  }

  getCellStyle(position: CellPosition): Style {
-    return this.getters.getCell(position)?.style || {};
+    return this.getters.getCell(position)?.style || this.getDefaultCellStyle(position) || {};


Suggested change

return this.getters.getCell(position)?.style || this.getDefaultCellStyle(position) || {};

return this.getters.getCell(position)?.style ?? this.getDefaultCellStyle(position) ?? {};

LucasLefevre · 2025-07-10T13:01:56Z

src/plugins/core/cell.ts

+        this.defaultStyle[sheetId]?.cols?.[col]
+      );
+      this.history.update(
+        "defaultFormat",
+        sheetId,
+        "cols",
+        col,
+        this.defaultFormat[sheetId]?.cols?.[col]


shouldn't it be refColumn 🤔 ? (-> tests are missing)

Suggested change

this.defaultStyle[sheetId]?.cols?.[col]

);

this.history.update(

"defaultFormat",

sheetId,

"cols",

col,

this.defaultFormat[sheetId]?.cols?.[col]

this.defaultStyle[sheetId]?.cols?.[refColumn]

);

this.history.update(

"defaultFormat",

sheetId,

"cols",

col,

this.defaultFormat[sheetId]?.cols?.[refColumn]

LucasLefevre · 2025-07-10T13:08:24Z

src/plugins/core/cell.ts

+    for (const cell of this.getters.getCellFromZone(
+      sheetId,
+      this.getters.getColsZone(sheetId, refColumn, refColumn)
+    )) {
+      const { row } = this.getters.getCellPosition(cell.id);


Suggested change

for (const cell of this.getters.getCellFromZone(

sheetId,

this.getters.getColsZone(sheetId, refColumn, refColumn)

)) {

const { row } = this.getters.getCellPosition(cell.id);

for (let row = 0; row < this.getters.getSheetSize(sheetId).numberOfRows; row++) {

smaller, faster ?
Or maybe it's because sheet won't have a numberOfRows with the infinite viewport ?
In that case:

Suggested change

for (const cell of this.getters.getCellFromZone(

sheetId,

this.getters.getColsZone(sheetId, refColumn, refColumn)

)) {

const { row } = this.getters.getCellPosition(cell.id);

for (let row = 0; row < this.getters.getColsZone(sheetId, refColumn, refColumn).bottom; row++) {

LucasLefevre · 2025-07-10T14:06:40Z

src/plugins/core/cell.ts

+    }
+  }
+
+  private updateCellStyleWithDefault(cell: Cell, style: Style | undefined) {


I don't understand when this method should be called. There's "default" in it's name but there's nothing related to this.defaultStyle in here. Also, I don't understand the deepEquals(cellStyle, style) ? null : cellStyle part where the style is reset. Why should it be reset ?

EDIT:

Also, I don't understand the deepEquals(cellStyle, style) ? null : cellStyle part where the style is reset. Why should it be reset ?

Ah ! is it because style is the same as the default style and it means it can be removed from the cell ? If yes, then I think it doesn't really matter if cell.style is equal to the default style if it simplifies the code and it avoids a deepEquals.
It could also be named more simply to something like addStyle

LucasLefevre · 2025-07-10T14:27:51Z

src/plugins/core/cell.ts

+        sheetId,
+        col,
+        row,
+        style: deepEquals(cellStyle, style) ? null : cellStyle,


at this point, we know the cell doesn't exist, so updating the style to null is useless

LucasLefevre · 2025-07-10T14:41:45Z

src/plugins/core/cell.ts

+  defaultStyle: Record<UID, sheetDefault<Style> | undefined>;
+  defaultFormat: Record<UID, sheetDefault<Format> | undefined>;


missing import/export from/to json

LucasLefevre · 2025-07-10T14:45:55Z

src/plugins/ui_core_views/header_sizes_ui.ts

+    const cells = this.getters.getRowCells(sheetId, row);
    let maxHeight = 0;
    let tallestCell: CellWithSize | undefined = undefined;
-    for (let i = 0; i < cellIds.length; i++) {
-      const cell = this.getters.getCellById(cellIds[i]);
-      if (!cell) {
-        continue;
-      }
+    for (const cell of cells) {


LucasLefevre · 2025-07-10T14:49:44Z

tests/cells/style_plugin.test.ts

@@ -173,3 +173,38 @@ describe("styles", () => {
    );
  });
 });
+
+describe("Default Styles", () => {


missing tests for (at least):

copy-paste

add/remove col/rows

duplicate sheet

add default on top of existing style

add style on top of existing default

LucasLefevre · 2025-07-11T06:35:45Z

I was thinking more yesterday evening. Why do we make a difference between the global/full rows/full cols and specific cells ? Couldn't we just apply style on ranges everywhere ? (if we remove cell.style/cell.format and move everything in a dedicated plugin managing those ranges)

[IMP] style,format: add default style & format

4217257

Add style and format sheet/col/row default. This task is groundwork for the infinite viewport. Task: 4852413

fdamhaut force-pushed the master-infinite-style-flda branch from 0478283 to 4217257 Compare July 7, 2025 11:29

pro-odoo reviewed Jul 8, 2025

View reviewed changes

LucasLefevre reviewed Jul 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[IMP] style,format: add default style & format #6762

[IMP] style,format: add default style & format #6762

Uh oh!

fdamhaut commented Jul 7, 2025

Uh oh!

robodoo commented Jul 7, 2025

Uh oh!

pro-odoo Jul 8, 2025

Uh oh!

LucasLefevre left a comment

Uh oh!

LucasLefevre Jul 10, 2025

Uh oh!

LucasLefevre Jul 10, 2025

Uh oh!

LucasLefevre Jul 10, 2025

Uh oh!

LucasLefevre Jul 10, 2025

Uh oh!

LucasLefevre Jul 10, 2025

Uh oh!

LucasLefevre Jul 10, 2025

Uh oh!

LucasLefevre Jul 10, 2025

Uh oh!

LucasLefevre Jul 10, 2025

Uh oh!

LucasLefevre Jul 10, 2025

Uh oh!

LucasLefevre Jul 10, 2025

Uh oh!

LucasLefevre commented Jul 11, 2025 •

edited

Loading

Uh oh!

Uh oh!

	return this.getters.getCell(position)?.style \|\| this.getDefaultCellStyle(position) \|\| {};
	return this.getters.getCell(position)?.style ?? this.getDefaultCellStyle(position) ?? {};

		defaultStyle: Record<UID, sheetDefault<Style> \| undefined>;
		defaultFormat: Record<UID, sheetDefault<Format> \| undefined>;

[IMP] style,format: add default style & format #6762

Are you sure you want to change the base?

[IMP] style,format: add default style & format #6762

Uh oh!

Conversation

fdamhaut commented Jul 7, 2025

Description:

review checklist

Uh oh!

robodoo commented Jul 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucasLefevre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucasLefevre commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

LucasLefevre commented Jul 11, 2025 •

edited

Loading