From 5f41313cf03e4d6df272654f28bc8b508f87ed9d Mon Sep 17 00:00:00 2001 From: Thomas Bouffard <27200110+tbouffard@users.noreply.github.com> Date: Sat, 2 Nov 2024 18:29:22 +0100 Subject: [PATCH 1/5] fix: allow to insert vertex without setting position and size This was allowed in mxGraph, and it is useful in scenario involving layouts (which is in charge of computing the position). The size of a vertex is sometime computed after the insertion, like in the OrgChart story. Update the signature of both the createVertex and insertVertex methods, and improve the related JSDOC. When the position is not set, the default values of the Geometry instance are used. Refactor tests: introduce helpers functions to remove duplication and make the expectations easier to read. --- .../__tests__/view/mixins/VertexMixin.test.ts | 208 +++++++++++------- packages/core/src/types.ts | 11 +- packages/core/src/view/geometry/Rectangle.ts | 7 +- packages/core/src/view/mixins/VertexMixin.ts | 10 +- .../core/src/view/mixins/VertexMixin.type.ts | 27 ++- 5 files changed, 160 insertions(+), 103 deletions(-) diff --git a/packages/core/__tests__/view/mixins/VertexMixin.test.ts b/packages/core/__tests__/view/mixins/VertexMixin.test.ts index cc3c9f96ae..b6fb55d15d 100644 --- a/packages/core/__tests__/view/mixins/VertexMixin.test.ts +++ b/packages/core/__tests__/view/mixins/VertexMixin.test.ts @@ -16,96 +16,140 @@ limitations under the License. import { describe, expect, test } from '@jest/globals'; import { createGraphWithoutContainer } from '../../utils'; -import { type CellStyle, Geometry } from '../../../src'; +import { type Cell, type CellStyle, Geometry, type Graph } from '../../../src'; + +const expectIsVertex = (cell: Cell, cellId: string) => { + expect(cell.getId()).toBe(cellId); + expect(cell.vertex).toBeTruthy(); + expect(cell.edge).toBeFalsy(); +}; + +const expectCellInModel = (graph: Graph, expectedCell: Cell, cellId: string) => { + const cellFromModel = graph.getDataModel().getCell(cellId); + expect(cellFromModel).toBe(expectedCell); +}; + +const expectIsChildOfDefaultParent = (cell: Cell) => { + expect(cell.parent).not.toBeNull(); + expect(cell.parent?.id).toBe('1'); // default parent + const children = cell.parent?.children; + expect(children).toContain(cell); + expect(children).toHaveLength(1); +}; + +const nonRelativeGeometry = (x: number, y: number, width: number, height: number) => { + const geometry = new Geometry(x, y, width, height); + geometry.relative = false; + return geometry; +}; + +const relativeGeometry = (x: number, y: number, width: number, height: number) => { + const geometry = new Geometry(x, y, width, height); + geometry.relative = true; + return geometry; +}; describe('insertVertex', () => { - test('with several parameters', () => { - const graph = createGraphWithoutContainer(); - const style: CellStyle = { rounded: true, shape: 'cloud' }; - const cell = graph.insertVertex(null, 'vertex_1', 'a value', 10, 20, 110, 120, style); - expect(cell.getId()).toBe('vertex_1'); - expect(cell.vertex).toBeTruthy(); - expect(cell.edge).toBeFalsy(); - expect(cell.value).toBe('a value'); - expect(cell.style).toStrictEqual(style); - - const geometry = new Geometry(10, 20, 110, 120); - geometry.relative = false; - expect(cell.geometry).toStrictEqual(geometry); - - // parent created with cell as child - expect(cell.parent).not.toBeNull(); - expect(cell.parent?.id).toBe('1'); // default parent - const children = cell.parent?.children; - expect(children).toContain(cell); - expect(children).toHaveLength(1); - - // ensure that the cell is in the model - const cellFromModel = graph.getDataModel().getCell('vertex_1'); - expect(cellFromModel).toBe(cell); - }); + describe('with several parameters', () => { + test('with position, size and style', () => { + const graph = createGraphWithoutContainer(); + const style: CellStyle = { rounded: true, shape: 'cloud' }; + const cellId = 'vertex_1'; + const cell = graph.insertVertex(null, cellId, 'a value', 10, 20, 110, 120, style); + + expectIsVertex(cell, cellId); + expect(cell.value).toBe('a value'); + expect(cell.style).toStrictEqual(style); + expect(cell.geometry).toStrictEqual(nonRelativeGeometry(10, 20, 110, 120)); + + expectIsChildOfDefaultParent(cell); + expectCellInModel(graph, cell, cellId); + }); - test('with single parameter', () => { - const graph = createGraphWithoutContainer(); - const style: CellStyle = { align: 'right', fillColor: 'red' }; - const cell = graph.insertVertex({ - value: 'a value', - x: 10, - y: 20, - size: [110, 120], - style, + test('with no position nor size', () => { + const graph = createGraphWithoutContainer(); + const cellId = 'noPositionNorSize_'; + const cell = graph.insertVertex( + null, + cellId, + 'a value for cell without position and size' + ); + + expectIsVertex(cell, cellId); + expect(cell.value).toBe('a value for cell without position and size'); + expect(cell.style).toStrictEqual({}); + expect(cell.geometry).toStrictEqual(nonRelativeGeometry(0, 0, 0, 0)); + + expectIsChildOfDefaultParent(cell); + expectCellInModel(graph, cell, cellId); }); - expect(cell.getId()).toBe('2'); // generated - expect(cell.vertex).toBeTruthy(); - expect(cell.edge).toBeFalsy(); - expect(cell.value).toBe('a value'); - expect(cell.style).toStrictEqual(style); - - const geometry = new Geometry(10, 20, 110, 120); - geometry.relative = false; - expect(cell.geometry).toStrictEqual(geometry); - - // parent created with cell as child - expect(cell.parent).not.toBeNull(); - expect(cell.parent?.id).toBe('1'); // default parent - const children = cell.parent?.children; - expect(children).toContain(cell); - expect(children).toHaveLength(1); - - // ensure that the cell is in the model - const cellFromModel = graph.getDataModel().getCell('2'); - expect(cellFromModel).toBe(cell); }); - test('with single parameter and non default parent', () => { - const graph = createGraphWithoutContainer(); + describe('with single parameter', () => { + const autoGeneratedCellId = '2'; + + test('with position, size and style', () => { + const graph = createGraphWithoutContainer(); + const style: CellStyle = { align: 'right', fillColor: 'red' }; + const cell = graph.insertVertex({ + value: 'another value', + x: 10, + y: 20, + size: [110, 120], + style, + }); + + expectIsVertex(cell, autoGeneratedCellId); + expect(cell.value).toBe('another value'); + expect(cell.style).toStrictEqual(style); + expect(cell.geometry).toStrictEqual(nonRelativeGeometry(10, 20, 110, 120)); + + expectIsChildOfDefaultParent(cell); + expectCellInModel(graph, cell, autoGeneratedCellId); + }); - const parentCell = graph.insertVertex({ - value: 'non default', - position: [10, 10], - size: [400, 400], + test('with non default parent', () => { + const graph = createGraphWithoutContainer(); + + const parentCell = graph.insertVertex({ + value: 'non default', + position: [10, 10], + size: [400, 400], + }); + expect(parentCell.getId()).toBe(autoGeneratedCellId); + expect(parentCell.value).toBe('non default'); + expect(parentCell.geometry).toStrictEqual(nonRelativeGeometry(10, 10, 400, 400)); + + const childCell = graph.insertVertex({ + parent: parentCell, + value: 'child', + position: [5, 5], + width: 400, + height: 400, + relative: true, + }); + expect(childCell.geometry).toStrictEqual(relativeGeometry(5, 5, 400, 400)); + + expect(childCell.parent).toBe(parentCell); + const children = parentCell.children; + expect(children).toContain(childCell); + expect(children).toHaveLength(1); }); - expect(parentCell.getId()).toBe('2'); // generated - expect(parentCell.value).toBe('non default'); - - const geometryOfParentCell = new Geometry(10, 10, 400, 400); - expect(parentCell.geometry).toStrictEqual(geometryOfParentCell); - - const childCell = graph.insertVertex({ - parent: parentCell, - value: 'child', - position: [5, 5], - width: 400, - height: 400, - relative: true, + + test('with no position nor size', () => { + const graph = createGraphWithoutContainer(); + const cellId = 'noPositionNorSize'; + const cell = graph.insertVertex({ + id: cellId, + value: 'a value for cell without position and size', + }); + expectIsVertex(cell, cellId); + expect(cell.value).toBe('a value for cell without position and size'); + expect(cell.style).toStrictEqual({}); + expect(cell.geometry).toStrictEqual(nonRelativeGeometry(0, 0, 0, 0)); + + expectIsChildOfDefaultParent(cell); + expectCellInModel(graph, cell, cellId); }); - const geometry = new Geometry(5, 5, 400, 400); - geometry.relative = true; - expect(childCell.geometry).toStrictEqual(geometry); - - expect(childCell.parent).toBe(parentCell); - const children = parentCell.children; - expect(children).toContain(childCell); - expect(children).toHaveLength(1); }); }); diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index d1d76a3fe5..8298dac883 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -1014,9 +1014,10 @@ export type VertexParameters = { */ parent?: Cell | null; /** - * Fallback when the {@link x} or the {@link y} parameters are not set. - * It is mandatory to set this value or the {@link x} and the {@link y} properties. + * Fallback when the {@link x} or the {@link y} properties are not set. * Order of the elements: x, y + * + * **NOTE:** If the position of the vertex is not set at vertex creation (by setting the {@link x} or the {@link y} properties or this property), it is advised to use a {@link GraphLayout} or a {@link LayoutManager} to automatically compute the actual position. */ position?: [number, number]; /** @@ -1025,9 +1026,11 @@ export type VertexParameters = { */ relative?: boolean; /** - * Fallback when the {@link width} or the {@link height} parameters are not set. - * It is mandatory to set this value or the {@link width} and the {@link height} properties. + * Fallback when the {@link width} or the {@link height} properties are not set. * Order of the elements: width, height + * + * **NOTE:** If the size of the vertex is not set at vertex creation (by setting the {@link width} and the {@link height} properties or this property), it is advised to later set the size on the geometry of the vertex instance. + * Otherwise, the vertex has no size and it is not displayed. */ size?: [number, number]; style?: CellStyle; diff --git a/packages/core/src/view/geometry/Rectangle.ts b/packages/core/src/view/geometry/Rectangle.ts index f5a0377554..eff390e6e6 100644 --- a/packages/core/src/view/geometry/Rectangle.ts +++ b/packages/core/src/view/geometry/Rectangle.ts @@ -31,18 +31,19 @@ class Rectangle extends Point { constructor(x = 0, y = 0, width = 0, height = 0) { super(x, y); - // replace super of mxPoint this.width = width; this.height = height; } /** - * Holds the width of the rectangle. Default is 0. + * Holds the width of the rectangle. + * @default 0 */ _width = 0; /** - * Holds the height of the rectangle. Default is 0. + * Holds the height of the rectangle. + * @default 0 */ _height = 0; diff --git a/packages/core/src/view/mixins/VertexMixin.ts b/packages/core/src/view/mixins/VertexMixin.ts index 23275bc0f9..855e979f0a 100644 --- a/packages/core/src/view/mixins/VertexMixin.ts +++ b/packages/core/src/view/mixins/VertexMixin.ts @@ -31,7 +31,7 @@ type PartialVertex = Pick< | 'isVertexLabelsMovable' | 'setVertexLabelsMovable' > & { - // handle the methods defined in the Graph interface with a single implementation + // handle the various methods defined in the Graph interface with a single implementation insertVertex: (...args: any[]) => Cell; }; type PartialType = PartialGraph & PartialVertex; @@ -68,10 +68,10 @@ export const VertexMixin: PartialType = { id = params.id; value = params.value; - x = 'x' in params ? params.x : params.position[0]; - y = 'y' in params ? params.y : params.position[1]; - width = 'width' in params ? params.width : params.size[0]; - height = 'height' in params ? params.height : params.size[1]; + x = 'x' in params ? params.x : params.position?.[0]; + y = 'y' in params ? params.y : params.position?.[1]; + width = 'width' in params ? params.width : params.size?.[0]; + height = 'height' in params ? params.height : params.size?.[1]; style = params.style; relative = params.relative; diff --git a/packages/core/src/view/mixins/VertexMixin.type.ts b/packages/core/src/view/mixins/VertexMixin.type.ts index 67b7a3847a..0e7952ffdc 100644 --- a/packages/core/src/view/mixins/VertexMixin.type.ts +++ b/packages/core/src/view/mixins/VertexMixin.type.ts @@ -44,7 +44,11 @@ declare module '../Graph' { * object and the given coordinates as the {@link Geometry} of the new vertex. * The id and style are used for the respective properties of the new `Cell`, which is returned. * - * **IMPORTANT**: this is a legacy method to ease the migration from `mxGraph`. Use the {@link insertVertex} method with a single object parameter instead. + * **IMPORTANT**: + * - This is a legacy method to ease the migration from `mxGraph`. Use the {@link insertVertex} method with a single object parameter instead. + * - If the position of the vertex is not set at vertex creation (by setting the `x` and `y` parameters), it is advised to use a {@link GraphLayout} or a {@link LayoutManager} to automatically compute the actual position. + * - If the size of the vertex is not set at vertex creation (by setting the `width` and the `height` parameters), it is advised to later set the size on the geometry of the vertex instance. + * Otherwise, the vertex has no size and it is not displayed. * * When adding new vertices from a mouse event, one should take into * account the offset of the graph container and the scale and translation @@ -83,10 +87,10 @@ declare module '../Graph' { parent: Cell | null, id: string | null | undefined, value: any, - x: number, - y: number, - width: number, - height: number, + x?: number, + y?: number, + width?: number, + height?: number, style?: CellStyle, relative?: boolean, geometryClass?: typeof Geometry @@ -130,6 +134,11 @@ declare module '../Graph' { /** * Hook method that creates the new vertex for {@link insertVertex}. * + * **IMPORTANT**: + * - If the position of the vertex is not set at vertex creation (by setting the `x` and `y` parameters), it is advised to use a {@link GraphLayout} or a {@link LayoutManager} to automatically compute the actual position. + * - If the size of the vertex is not set at vertex creation (by setting the `width` and the `height` parameters), it is advised to later set the size on the geometry of the vertex instance. + * Otherwise, the vertex has no size and it is not displayed. + * * @param parent the parent of the new vertex. If not set, use the default parent. * @param id Optional string that defines the id of the new vertex. If not set, the id is auto-generated when creating the vertex. * @param value Object to be used as the user object. @@ -146,10 +155,10 @@ declare module '../Graph' { parent: Cell | null, id: string | null | undefined, value: any, - x: number, - y: number, - width: number, - height: number, + x?: number, + y?: number, + width?: number, + height?: number, style?: CellStyle, relative?: boolean, geometryClass?: typeof Geometry From 48cebf61b7fa1495d0352595eb99c31d8cfa3d92 Mon Sep 17 00:00:00 2001 From: Thomas Bouffard <27200110+tbouffard@users.noreply.github.com> Date: Sun, 3 Nov 2024 11:25:54 +0100 Subject: [PATCH 2/5] createVertex impl better match type (optional parameter) --- packages/core/src/view/mixins/VertexMixin.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core/src/view/mixins/VertexMixin.ts b/packages/core/src/view/mixins/VertexMixin.ts index 855e979f0a..3fba4e536c 100644 --- a/packages/core/src/view/mixins/VertexMixin.ts +++ b/packages/core/src/view/mixins/VertexMixin.ts @@ -101,10 +101,10 @@ export const VertexMixin: PartialType = { _parent: Cell | null, id: string, value: any, - x: number, - y: number, - width: number, - height: number, + x?: number, + y?: number, + width?: number, + height?: number, style?: CellStyle, relative = false, geometryClass = Geometry From e2be64ff3532c388ea74c7bd98920afb2964b927 Mon Sep 17 00:00:00 2001 From: Thomas Bouffard <27200110+tbouffard@users.noreply.github.com> Date: Sun, 3 Nov 2024 11:28:44 +0100 Subject: [PATCH 3/5] test: simplify expectCellInModel --- .../core/__tests__/view/mixins/VertexMixin.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/core/__tests__/view/mixins/VertexMixin.test.ts b/packages/core/__tests__/view/mixins/VertexMixin.test.ts index b6fb55d15d..d6403f8d1b 100644 --- a/packages/core/__tests__/view/mixins/VertexMixin.test.ts +++ b/packages/core/__tests__/view/mixins/VertexMixin.test.ts @@ -24,8 +24,8 @@ const expectIsVertex = (cell: Cell, cellId: string) => { expect(cell.edge).toBeFalsy(); }; -const expectCellInModel = (graph: Graph, expectedCell: Cell, cellId: string) => { - const cellFromModel = graph.getDataModel().getCell(cellId); +const expectCellInModel = (graph: Graph, expectedCell: Cell) => { + const cellFromModel = graph.getDataModel().getCell(expectedCell.getId()!); expect(cellFromModel).toBe(expectedCell); }; @@ -63,7 +63,7 @@ describe('insertVertex', () => { expect(cell.geometry).toStrictEqual(nonRelativeGeometry(10, 20, 110, 120)); expectIsChildOfDefaultParent(cell); - expectCellInModel(graph, cell, cellId); + expectCellInModel(graph, cell); }); test('with no position nor size', () => { @@ -81,7 +81,7 @@ describe('insertVertex', () => { expect(cell.geometry).toStrictEqual(nonRelativeGeometry(0, 0, 0, 0)); expectIsChildOfDefaultParent(cell); - expectCellInModel(graph, cell, cellId); + expectCellInModel(graph, cell); }); }); @@ -105,7 +105,7 @@ describe('insertVertex', () => { expect(cell.geometry).toStrictEqual(nonRelativeGeometry(10, 20, 110, 120)); expectIsChildOfDefaultParent(cell); - expectCellInModel(graph, cell, autoGeneratedCellId); + expectCellInModel(graph, cell); }); test('with non default parent', () => { @@ -149,7 +149,7 @@ describe('insertVertex', () => { expect(cell.geometry).toStrictEqual(nonRelativeGeometry(0, 0, 0, 0)); expectIsChildOfDefaultParent(cell); - expectCellInModel(graph, cell, cellId); + expectCellInModel(graph, cell); }); }); }); From 35364d3b2b99405f0e95e9f1cb39af44ff65e623 Mon Sep 17 00:00:00 2001 From: Thomas Bouffard <27200110+tbouffard@users.noreply.github.com> Date: Sun, 3 Nov 2024 11:33:35 +0100 Subject: [PATCH 4/5] test: add more assertions --- .../__tests__/view/mixins/VertexMixin.test.ts | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/packages/core/__tests__/view/mixins/VertexMixin.test.ts b/packages/core/__tests__/view/mixins/VertexMixin.test.ts index d6403f8d1b..33b09dfcd5 100644 --- a/packages/core/__tests__/view/mixins/VertexMixin.test.ts +++ b/packages/core/__tests__/view/mixins/VertexMixin.test.ts @@ -37,6 +37,13 @@ const expectIsChildOfDefaultParent = (cell: Cell) => { expect(children).toHaveLength(1); }; +const isTheOnlyChildOf = (parentCell: Cell, childCell: Cell) => { + expect(childCell.parent).toBe(parentCell); + const children = parentCell.children; + expect(children).toContain(childCell); + expect(children).toHaveLength(1); +}; + const nonRelativeGeometry = (x: number, y: number, width: number, height: number) => { const geometry = new Geometry(x, y, width, height); geometry.relative = false; @@ -116,24 +123,29 @@ describe('insertVertex', () => { position: [10, 10], size: [400, 400], }); - expect(parentCell.getId()).toBe(autoGeneratedCellId); + expectIsVertex(parentCell, autoGeneratedCellId); expect(parentCell.value).toBe('non default'); + expect(parentCell.style).toStrictEqual({}); expect(parentCell.geometry).toStrictEqual(nonRelativeGeometry(10, 10, 400, 400)); + expectIsChildOfDefaultParent(parentCell); + expectCellInModel(graph, parentCell); + const childCellId = 'childId'; const childCell = graph.insertVertex({ parent: parentCell, + id: childCellId, value: 'child', position: [5, 5], width: 400, height: 400, relative: true, }); + expectIsVertex(childCell, childCellId); + expect(childCell.value).toBe('child'); + expect(childCell.style).toStrictEqual({}); expect(childCell.geometry).toStrictEqual(relativeGeometry(5, 5, 400, 400)); - - expect(childCell.parent).toBe(parentCell); - const children = parentCell.children; - expect(children).toContain(childCell); - expect(children).toHaveLength(1); + expectCellInModel(graph, childCell); + isTheOnlyChildOf(parentCell, childCell); }); test('with no position nor size', () => { From 94b64fbddb5151cf81c0535841b98aaa7ec56927 Mon Sep 17 00:00:00 2001 From: Thomas Bouffard <27200110+tbouffard@users.noreply.github.com> Date: Sun, 3 Nov 2024 11:39:31 +0100 Subject: [PATCH 5/5] test: rename expect helper function --- .../core/__tests__/view/mixins/VertexMixin.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/core/__tests__/view/mixins/VertexMixin.test.ts b/packages/core/__tests__/view/mixins/VertexMixin.test.ts index 33b09dfcd5..1517fc3d70 100644 --- a/packages/core/__tests__/view/mixins/VertexMixin.test.ts +++ b/packages/core/__tests__/view/mixins/VertexMixin.test.ts @@ -29,7 +29,7 @@ const expectCellInModel = (graph: Graph, expectedCell: Cell) => { expect(cellFromModel).toBe(expectedCell); }; -const expectIsChildOfDefaultParent = (cell: Cell) => { +const expectIsTheOnlyChildOfDefaultParent = (cell: Cell) => { expect(cell.parent).not.toBeNull(); expect(cell.parent?.id).toBe('1'); // default parent const children = cell.parent?.children; @@ -69,7 +69,7 @@ describe('insertVertex', () => { expect(cell.style).toStrictEqual(style); expect(cell.geometry).toStrictEqual(nonRelativeGeometry(10, 20, 110, 120)); - expectIsChildOfDefaultParent(cell); + expectIsTheOnlyChildOfDefaultParent(cell); expectCellInModel(graph, cell); }); @@ -87,7 +87,7 @@ describe('insertVertex', () => { expect(cell.style).toStrictEqual({}); expect(cell.geometry).toStrictEqual(nonRelativeGeometry(0, 0, 0, 0)); - expectIsChildOfDefaultParent(cell); + expectIsTheOnlyChildOfDefaultParent(cell); expectCellInModel(graph, cell); }); }); @@ -111,7 +111,7 @@ describe('insertVertex', () => { expect(cell.style).toStrictEqual(style); expect(cell.geometry).toStrictEqual(nonRelativeGeometry(10, 20, 110, 120)); - expectIsChildOfDefaultParent(cell); + expectIsTheOnlyChildOfDefaultParent(cell); expectCellInModel(graph, cell); }); @@ -127,7 +127,7 @@ describe('insertVertex', () => { expect(parentCell.value).toBe('non default'); expect(parentCell.style).toStrictEqual({}); expect(parentCell.geometry).toStrictEqual(nonRelativeGeometry(10, 10, 400, 400)); - expectIsChildOfDefaultParent(parentCell); + expectIsTheOnlyChildOfDefaultParent(parentCell); expectCellInModel(graph, parentCell); const childCellId = 'childId'; @@ -160,7 +160,7 @@ describe('insertVertex', () => { expect(cell.style).toStrictEqual({}); expect(cell.geometry).toStrictEqual(nonRelativeGeometry(0, 0, 0, 0)); - expectIsChildOfDefaultParent(cell); + expectIsTheOnlyChildOfDefaultParent(cell); expectCellInModel(graph, cell); }); });