Repository: maxGraph/maxGraph (ts)

Issue:
Graph.setCellStyles may not propagate style changes
Seen in #325
When calling Graph.setCellStyles to change the fillColor, the fill color doesn't change in the browser.

Pull request:
fix: correctly update style when calling Graph methods
`GraphDataModel.setStyle` does the actual style change. Its current implementation requires to pass an instance of CellStyle which is different from the current style hold by the Cell. Previously, the Graph methods directly update the Cell style instance prior calling the style update so nothing happened.
The problem was spotted in the Monitor story which is now fixed.
Also fix the signature of Graph.setCellStyle as the `cells` parameter is optional (previously, it was marked as mandatory).

Introduce a convenient `Cell.getClonedStyle` method to ease style update and update the JSDOC of several functions to warn users about how to correctly make the style update happen.

The fix has a slight side effects exhibited by the EdgeMixin tests. The `sourcePort` and `targetPort` cell style properties are now set to null at vertex insertion. This should not have any effects for the end user.


### Notes

Closes #326


### Monitor story

This PR fix the problem described in #325.

**_mxGraph_**
![mxGraph](https://github.com/maxGraph/maxGraph/assets/27200110/443a37b9-8e36-49f2-92f9-8cc122c912c1)

**_maxGraph PR 325_**
![maxGraph_PR_325](https://github.com/maxGraph/maxGraph/assets/27200110/46000481-99e7-4449-849a-583935ec1141)

**_now with this PR_**

https://github.com/maxGraph/maxGraph/assets/27200110/023ef670-5088-418b-a40b-59de880fe467




Patch:
diff --git a/packages/core/src/util/styleUtils.ts b/packages/core/src/util/styleUtils.ts
index a55904069..245b7ab1c 100644
--- a/packages/core/src/util/styleUtils.ts
+++ b/packages/core/src/util/styleUtils.ts
@@ -320,7 +320,7 @@ export const convertPoint = (container: HTMLElement, x: number, y: number) => {
  * removes the key from the styles if the value is null.
  *
  * @param model <Transactions> to execute the transaction in.
- * @param cells Array of {@link Cells} to be updated.
+ * @param cells Array of {@link Cell}s to be updated.
  * @param key Key of the style to be changed.
  * @param value New value for the given key.
  */
@@ -336,7 +336,8 @@ export const setCellStyles = (
         const cell = cells[i];
 
         if (cell) {
-          const style = cell.getStyle();
+          // Currently, the style object must be cloned, otherwise model.setStyle does not trigger the change event and the cell state in the view is not updated
+          const style = cell.getClonedStyle();
           style[key] = value;
 
           model.setStyle(cell, style);
@@ -379,7 +380,8 @@ export const setCellStyleFlags = (
         const cell = cells[i];
 
         if (cell) {
-          const style = setStyleFlag(cell.getStyle(), key, flag, value);
+          // Currently, the style object must be cloned, otherwise model.setStyle does not trigger the change event and the cell state in the view is not updated
+          const style = setStyleFlag(cell.getClonedStyle(), key, flag, value);
           model.setStyle(cell, style);
         }
       }
diff --git a/packages/core/src/view/GraphDataModel.ts b/packages/core/src/view/GraphDataModel.ts
index 948d6e371..ce87146e6 100644
--- a/packages/core/src/view/GraphDataModel.ts
+++ b/packages/core/src/view/GraphDataModel.ts
@@ -850,25 +850,30 @@ export class GraphDataModel extends EventSource {
   }
 
   /**
-   * Sets the style of the given {@link Cell} using {@link StyleChange} and
-   * adds the change to the current transaction.
+   * Sets the style of the given {@link Cell} using {@link StyleChange} and adds the change to the current transaction.
+   *
+   * **IMPORTANT**: Do not pass {@link Cell.getStyle} as value of the `style` parameter. Otherwise, no style change is performed, so the view won't be updated.
+   * Always get a clone of the style of the cell with {@link Cell.getClonedStyle}, then update it and pass the updated style to this method.
    *
    * @param cell  whose style should be changed.
    * @param style the new cell style to set.
    */
   setStyle(cell: Cell, style: CellStyle) {
+    // To investigate in the future: it may be more convenient to do a deep comparison to prevent unnecessary changes
+    // If the passed style is the same as the current style without being the same instance, we don't need to do anything
+    // With the current implementation, a style change is executed when the styles are deep equal.
     if (style !== cell.getStyle()) {
       this.execute(new StyleChange(this, cell, style));
     }
   }
 
   /**
-   * Inner callback to update the style of the given {@link Cell}
-   * using {@link Cell#setStyle} and return the previous style.
+   * Inner callback to update the style of the given {@link Cell}  using {@link Cell#setStyle} and return the previous style.
    *
-   * @param {Cell} cell  that specifies the cell to be updated.
-   * @param style  String of the form [stylename;|key=value;] to specify
-   * the new cell style.
+   * **IMPORTANT**: to fully work, this method should not receive `cell.getStyle` as value of the `style` parameter. See {@link setStyle} for more information.
+   *
+   * @param cell  whose style should be changed.
+   * @param style the new cell style to set.
    */
   styleForCellChanged(cell: Cell, style: CellStyle) {
     const previous = cell.getStyle();
@@ -896,7 +901,7 @@ export class GraphDataModel extends EventSource {
    * the previous collapsed state.
    *
    * @param {Cell} cell  that specifies the cell to be updated.
-   * @param collapsed  Boolean that specifies the new collpased state.
+   * @param collapsed  Boolean that specifies the new collapsed state.
    */
   collapsedStateForCellChanged(cell: Cell, collapsed: boolean): boolean {
     const previous = cell.isCollapsed();
diff --git a/packages/core/src/view/cell/Cell.ts b/packages/core/src/view/cell/Cell.ts
index fdd8d1a3a..429ec532b 100644
--- a/packages/core/src/view/cell/Cell.ts
+++ b/packages/core/src/view/cell/Cell.ts
@@ -242,14 +242,25 @@ export class Cell implements IdentityObject {
   }
 
   /**
-   * Returns a string that describes the <style>.
+   * Returns a string that describes the {@link style}.
+   *
+   * **IMPORTANT**: if you want to get the style object to later update it and propagate changes to the view, use {@link getClonedStyle} instead.
    */
   getStyle() {
     return this.style;
   }
 
   /**
-   * Sets the string to be used as the <style>.
+   * Use this method to get the style object to later update it and propagate changes to the view.
+   *
+   * See {@link GraphDataModel.setStyle} for more details.
+   */
+  getClonedStyle() {
+    return clone(this.getStyle());
+  }
+
+  /**
+   * Sets the string to be used as the {@link style}.
    */
   setStyle(style: CellStyle) {
     this.style = style;
diff --git a/packages/core/src/view/mixins/CellsMixin.ts b/packages/core/src/view/mixins/CellsMixin.ts
index 86b68b095..87b2ed2a3 100644
--- a/packages/core/src/view/mixins/CellsMixin.ts
+++ b/packages/core/src/view/mixins/CellsMixin.ts
@@ -70,7 +70,17 @@ declare module '../Graph' {
     getCurrentCellStyle: (cell: Cell, ignoreState?: boolean) => CellStateStyle;
     getCellStyle: (cell: Cell) => CellStateStyle;
     postProcessCellStyle: (style: CellStateStyle) => CellStateStyle;
-    setCellStyle: (style: CellStyle, cells: Cell[]) => void;
+    /**
+     * Sets the style of the specified cells. If no cells are given, then the selection cells are changed.
+     *
+     * **IMPORTANT**: Do not pass {@link Cell.getStyle} as value of the `style` parameter. Always get a clone of the style of the cell with {@link Cell.getClonedStyle}, then update it and pass the updated style to this method.
+     * For more details, see {@link GraphDataModel.setStyle}.
+     *
+     * @param style String representing the new style of the cells.
+     * @param cells Optional array of {@link Cell} to set the style for. Default is the
+     * selection cells.
+     */
+    setCellStyle: (style: CellStyle, cells?: Cell[]) => void;
     toggleCellStyle: (
       key: keyof CellStateStyle,
       defaultValue: boolean,
@@ -655,14 +665,6 @@ export const CellsMixin: PartialType = {
     return style;
   },
 
-  /**
-   * Sets the style of the specified cells. If no cells are given, then the
-   * selection cells are changed.
-   *
-   * @param style String representing the new style of the cells.
-   * @param cells Optional array of {@link Cell} to set the style for. Default is the
-   * selection cells.
-   */
   setCellStyle(style, cells?) {
     cells = cells ?? this.getSelectionCells();
 


Test patch:
diff --git a/packages/core/__tests__/util/styleUtils.test.ts b/packages/core/__tests__/util/styleUtils.test.ts
index a7c50dff4..ca62cfa7e 100644
--- a/packages/core/__tests__/util/styleUtils.test.ts
+++ b/packages/core/__tests__/util/styleUtils.test.ts
@@ -15,9 +15,15 @@ limitations under the License.
 */
 
 import { describe, expect, test } from '@jest/globals';
-import { matchBinaryMask, setStyleFlag } from '../../src/util/styleUtils';
+import {
+  matchBinaryMask,
+  setStyleFlag,
+  setCellStyleFlags,
+  setCellStyles,
+} from '../../src/util/styleUtils';
 import { FONT } from '../../src/util/Constants';
 import { type CellStyle } from '../../src/types';
+import { createGraphWithoutPlugins } from '../utils';
 
 describe('matchBinaryMask', () => {
   test('match self', () => {
@@ -94,3 +100,41 @@ describe('setStyleFlag', () => {
     expect(style.fontStyle).toBe(2);
   });
 });
+
+test('setCellStyleFlags on vertex', () => {
+  // Need a graph to have a view and ensure that the cell state is updated
+  const graph = createGraphWithoutPlugins();
+
+  const style: CellStyle = { fontStyle: 4, spacing: 8 };
+  const cell = graph.insertVertex({
+    value: 'a value',
+    x: 10,
+    y: 20,
+    size: [110, 120],
+    style,
+  });
+  expect(cell.style).toStrictEqual(style);
+
+  setCellStyleFlags(graph.getDataModel(), [cell], 'fontStyle', FONT.BOLD, true);
+  expect(cell.style.fontStyle).toBe(5);
+  expect(graph.getView().getState(cell)?.style?.fontStyle).toBe(5);
+});
+
+test('setCellStyles on vertex', () => {
+  // Need a graph to have a view and ensure that the cell state is updated
+  const graph = createGraphWithoutPlugins();
+
+  const style: CellStyle = { strokeColor: 'yellow', labelWidth: 100 };
+  const cell = graph.insertVertex({
+    value: 'a value',
+    x: 10,
+    y: 20,
+    size: [110, 120],
+    style,
+  });
+  expect(cell.style).toStrictEqual(style);
+
+  setCellStyles(graph.getDataModel(), [cell], 'strokeColor', 'chartreuse');
+  expect(cell.style.strokeColor).toBe('chartreuse');
+  expect(graph.getView().getState(cell)?.style?.strokeColor).toBe('chartreuse');
+});
diff --git a/packages/core/__tests__/view/mixins/CellMixin.test.ts b/packages/core/__tests__/view/mixins/CellMixin.test.ts
new file mode 100644
index 000000000..bba571270
--- /dev/null
+++ b/packages/core/__tests__/view/mixins/CellMixin.test.ts
@@ -0,0 +1,56 @@
+/*
+Copyright 2024-present The maxGraph project Contributors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import { expect, test } from '@jest/globals';
+import { createGraphWithoutPlugins } from '../../utils';
+import type { CellStyle } from '../../../src';
+import { FONT } from '../../../src/util/Constants';
+
+test('setCellStyles on vertex', () => {
+  const graph = createGraphWithoutPlugins();
+
+  const style: CellStyle = { align: 'right', fillColor: 'red' };
+  const cell = graph.insertVertex({
+    value: 'a value',
+    x: 10,
+    y: 20,
+    size: [110, 120],
+    style,
+  });
+  expect(cell.style).toStrictEqual(style);
+
+  graph.setCellStyles('fillColor', 'blue', [cell]);
+  expect(cell.style.fillColor).toBe('blue');
+  expect(graph.getView().getState(cell)?.style?.fillColor).toBe('blue');
+});
+
+test('setCellStyleFlags on vertex', () => {
+  const graph = createGraphWithoutPlugins();
+
+  const style: CellStyle = { fontStyle: 3, fillColor: 'red' };
+  const cell = graph.insertVertex({
+    value: 'a value',
+    x: 10,
+    y: 20,
+    size: [110, 120],
+    style,
+  });
+  expect(cell.style).toStrictEqual(style);
+
+  graph.setCellStyleFlags('fontStyle', FONT.UNDERLINE, null, [cell]);
+  expect(cell.style.fontStyle).toBe(7);
+  expect(graph.getView().getState(cell)?.style?.fontStyle).toBe(7);
+});
diff --git a/packages/core/__tests__/view/mixins/EdgeMixin.test.ts b/packages/core/__tests__/view/mixins/EdgeMixin.test.ts
index e0430b02e..10506fa11 100644
--- a/packages/core/__tests__/view/mixins/EdgeMixin.test.ts
+++ b/packages/core/__tests__/view/mixins/EdgeMixin.test.ts
@@ -32,7 +32,12 @@ describe('insertEdge', () => {
     expect(cell.value).toBe('a value');
     expect(cell.source).toBe(source);
     expect(cell.target).toBe(target);
-    expect(cell.style).toStrictEqual(style);
+    expect(cell.style).toStrictEqual({
+      ...style,
+      // added during insertion
+      sourcePort: null,
+      targetPort: null,
+    });
 
     const geometry = new Geometry();
     geometry.relative = true;
@@ -68,7 +73,12 @@ describe('insertEdge', () => {
     expect(cell.value).toBe('a value');
     expect(cell.source).toBe(source);
     expect(cell.target).toBe(target);
-    expect(cell.style).toStrictEqual(style);
+    expect(cell.style).toStrictEqual({
+      ...style,
+      // added during insertion
+      sourcePort: null,
+      targetPort: null,
+    });
 
     const geometry = new Geometry();
     geometry.relative = true;