From 567c9f51e467361ceeaab58ef97cdff0afa814af Mon Sep 17 00:00:00 2001 From: Marcel Mraz Date: Tue, 15 Apr 2025 14:02:50 +0200 Subject: [PATCH] Passing in elementsMap in updatePoints, refactor to ElementsMap types --- packages/element/src/binding.ts | 45 +++++++++---------- packages/element/src/flowchart.ts | 18 +++----- packages/element/src/fractionalIndex.ts | 5 ++- packages/element/src/linearElementEditor.ts | 43 +++++++++++++----- .../element/tests/fractionalIndex.test.ts | 5 ++- .../actions/actionDeleteSelected.tsx | 6 ++- .../excalidraw/actions/actionProperties.tsx | 15 ++++--- packages/excalidraw/components/App.tsx | 1 - .../excalidraw/components/Stats/DragInput.tsx | 13 +++--- packages/excalidraw/scene/Scene.ts | 1 - .../tests/linearElementEditor.test.tsx | 2 +- 11 files changed, 86 insertions(+), 68 deletions(-) diff --git a/packages/element/src/binding.ts b/packages/element/src/binding.ts index 61f1966c8..1982a449b 100644 --- a/packages/element/src/binding.ts +++ b/packages/element/src/binding.ts @@ -177,6 +177,8 @@ const bindOrUnbindLinearElementEdge = ( unboundFromElementIds: Set, scene: Scene, ): void => { + const elementsMap = scene.getNonDeletedElementsMap(); + // "keep" is for method chaining convenience, a "no-op", so just bail out if (bindableElement === "keep") { return; @@ -211,7 +213,7 @@ const bindOrUnbindLinearElementEdge = ( linearElement, bindableElement, startOrEnd, - scene.getNonDeletedElementsMap(), + elementsMap, (...args) => scene.mutate(...args), ); boundToElementIds.add(bindableElement.id); @@ -221,7 +223,7 @@ const bindOrUnbindLinearElementEdge = ( linearElement, bindableElement, startOrEnd, - scene.getNonDeletedElementsMap(), + elementsMap, (...args) => scene.mutate(...args), ); boundToElementIds.add(bindableElement.id); @@ -434,12 +436,14 @@ export const maybeBindLinearElement = ( pointerCoords: { x: number; y: number }, scene: Scene, ): void => { + const elementsMap = scene.getNonDeletedElementsMap(); + if (appState.startBoundElement != null) { bindLinearElement( linearElement, appState.startBoundElement, "start", - scene.getNonDeletedElementsMap(), + elementsMap, (...args) => scene.mutate(...args), ); } @@ -447,7 +451,7 @@ export const maybeBindLinearElement = ( const hoveredElement = getHoveredElementForBinding( pointerCoords, scene.getNonDeletedElements(), - scene.getNonDeletedElementsMap(), + elementsMap, appState.zoom, isElbowArrow(linearElement), isElbowArrow(linearElement), @@ -465,7 +469,7 @@ export const maybeBindLinearElement = ( linearElement, hoveredElement, "end", - scene.getNonDeletedElementsMap(), + elementsMap, (...args) => scene.mutate(...args), ); } @@ -496,7 +500,7 @@ export const bindLinearElement = ( linearElement: NonDeleted, hoveredElement: ExcalidrawBindableElement, startOrEnd: "start" | "end", - elementsMap: Map, + elementsMap: ElementsMap, mutator: ( element: ExcalidrawElement, updates: ElementUpdate, @@ -753,7 +757,7 @@ const calculateFocusAndGap = ( // in explicitly. export const updateBoundElements = ( changedElement: NonDeletedExcalidrawElement, - elementsMap: Map, + elementsMap: ElementsMap, options?: { simultaneouslyUpdated?: readonly ExcalidrawElement[]; newSize?: { width: number; height: number }; @@ -856,19 +860,14 @@ export const updateBoundElements = ( }> => update !== null, ); - LinearElementEditor.movePoints( - element, - updates, - { - ...(changedElement.id === element.startBinding?.elementId - ? { startBinding: bindings.startBinding } - : {}), - ...(changedElement.id === element.endBinding?.elementId - ? { endBinding: bindings.endBinding } - : {}), - }, - elementsMap as NonDeletedSceneElementsMap, - ); + LinearElementEditor.movePoints(element, elementsMap, updates, { + ...(changedElement.id === element.startBinding?.elementId + ? { startBinding: bindings.startBinding } + : {}), + ...(changedElement.id === element.endBinding?.elementId + ? { endBinding: bindings.endBinding } + : {}), + }); const boundText = getBoundTextElement(element, elementsMap); if (boundText && !boundText.isDeleted) { @@ -1501,7 +1500,7 @@ export const fixDuplicatedBindingsAfterDuplication = ( const fixReversedBindingsForBindables = ( original: ExcalidrawBindableElement, duplicate: ExcalidrawBindableElement, - originalElements: Map, + originalElements: ElementsMap, elementsWithClones: ExcalidrawElement[], oldIdToDuplicatedId: Map, ) => { @@ -1589,7 +1588,7 @@ const fixReversedBindingsForBindables = ( const fixReversedBindingsForArrows = ( original: ExcalidrawArrowElement, duplicate: ExcalidrawArrowElement, - originalElements: Map, + originalElements: ElementsMap, bindingProp: "startBinding" | "endBinding", oldIdToDuplicatedId: Map, elementsWithClones: ExcalidrawElement[], @@ -1650,7 +1649,7 @@ const fixReversedBindingsForArrows = ( }; export const fixReversedBindings = ( - originalElements: Map, + originalElements: ElementsMap, elementsWithClones: ExcalidrawElement[], oldIdToDuplicatedId: Map, ) => { diff --git a/packages/element/src/flowchart.ts b/packages/element/src/flowchart.ts index f990c1cba..dca8fb77e 100644 --- a/packages/element/src/flowchart.ts +++ b/packages/element/src/flowchart.ts @@ -238,11 +238,11 @@ const getOffsets = ( const addNewNode = ( element: ExcalidrawFlowchartNodeElement, - elementsMap: ElementsMap, appState: AppState, direction: LinkDirection, scene: Scene, ) => { + const elementsMap = scene.getNonDeletedElementsMap(); const successors = getSuccessors(element, elementsMap, direction); const predeccessors = getPredecessors(element, elementsMap, direction); @@ -277,7 +277,6 @@ const addNewNode = ( const bindingArrow = createBindingArrow( element, nextNode, - elementsMap, direction, appState, scene, @@ -291,7 +290,6 @@ const addNewNode = ( export const addNewNodes = ( startNode: ExcalidrawFlowchartNodeElement, - elementsMap: ElementsMap, appState: AppState, direction: LinkDirection, scene: Scene, @@ -357,7 +355,6 @@ export const addNewNodes = ( const bindingArrow = createBindingArrow( startNode, nextNode, - elementsMap, direction, appState, scene, @@ -373,7 +370,6 @@ export const addNewNodes = ( const createBindingArrow = ( startBindingElement: ExcalidrawFlowchartNodeElement, endBindingElement: ExcalidrawFlowchartNodeElement, - elementsMap: ElementsMap, direction: LinkDirection, appState: AppState, scene: Scene, @@ -447,18 +443,20 @@ const createBindingArrow = ( elbowed: true, }); + const elementsMap = scene.getNonDeletedElementsMap(); + bindLinearElement( bindingArrow, startBindingElement, "start", - scene.getNonDeletedElementsMap(), + elementsMap, (...args) => scene.mutate(...args), ); bindLinearElement( bindingArrow, endBindingElement, "end", - scene.getNonDeletedElementsMap(), + elementsMap, (...args) => scene.mutate(...args), ); @@ -476,7 +474,7 @@ const createBindingArrow = ( bindingArrow as OrderedExcalidrawElement, ); - LinearElementEditor.movePoints(bindingArrow, [ + LinearElementEditor.movePoints(bindingArrow, elementsMap, [ { index: 1, point: bindingArrow.points[1], @@ -641,15 +639,14 @@ export class FlowChartCreator { createNodes( startNode: ExcalidrawFlowchartNodeElement, - elementsMap: ElementsMap, appState: AppState, direction: LinkDirection, scene: Scene, ) { + const elementsMap = scene.getNonDeletedElementsMap(); if (direction !== this.direction) { const { nextNode, bindingArrow } = addNewNode( startNode, - elementsMap, appState, direction, scene, @@ -663,7 +660,6 @@ export class FlowChartCreator { this.numberOfNodes += 1; const newNodes = addNewNodes( startNode, - elementsMap, appState, direction, scene, diff --git a/packages/element/src/fractionalIndex.ts b/packages/element/src/fractionalIndex.ts index 378b42f3c..6b4ba1ec5 100644 --- a/packages/element/src/fractionalIndex.ts +++ b/packages/element/src/fractionalIndex.ts @@ -7,6 +7,7 @@ import { getBoundTextElement } from "./textElement"; import { hasBoundTextElement } from "./typeChecks"; import type { + ElementsMap, ExcalidrawElement, FractionalIndex, OrderedExcalidrawElement, @@ -152,7 +153,7 @@ export const orderByFractionalIndex = ( */ export const syncMovedIndices = ( elements: readonly ExcalidrawElement[], - movedElements: Map, + movedElements: ElementsMap, ): OrderedExcalidrawElement[] => { try { const indicesGroups = getMovedIndicesGroups(elements, movedElements); @@ -210,7 +211,7 @@ export const syncInvalidIndices = ( */ const getMovedIndicesGroups = ( elements: readonly ExcalidrawElement[], - movedElements: Map, + movedElements: ElementsMap, ) => { const indicesGroups: number[][] = []; diff --git a/packages/element/src/linearElementEditor.ts b/packages/element/src/linearElementEditor.ts index f07c4262d..ac4424c3c 100644 --- a/packages/element/src/linearElementEditor.ts +++ b/packages/element/src/linearElementEditor.ts @@ -307,7 +307,7 @@ export class LinearElementEditor { event[KEYS.CTRL_OR_CMD] ? null : app.getEffectiveGridSize(), ); - LinearElementEditor.movePoints(element, [ + LinearElementEditor.movePoints(element, elementsMap, [ { index: selectedIndex, point: pointFrom( @@ -331,6 +331,7 @@ export class LinearElementEditor { LinearElementEditor.movePoints( element, + elementsMap, selectedPointsIndices.map((pointIndex) => { const newPointPosition: LocalPoint = pointIndex === lastClickedPoint @@ -451,7 +452,7 @@ export class LinearElementEditor { selectedPoint === element.points.length - 1 ) { if (isPathALoop(element.points, appState.zoom.value)) { - LinearElementEditor.movePoints(element, [ + LinearElementEditor.movePoints(element, elementsMap, [ { index: selectedPoint, point: @@ -948,7 +949,9 @@ export class LinearElementEditor { if (!event.altKey) { if (lastPoint === lastUncommittedPoint) { - LinearElementEditor.deletePoints(element, [points.length - 1]); + LinearElementEditor.deletePoints(element, elementsMap, [ + points.length - 1, + ]); } return { ...appState.editingLinearElement, @@ -986,14 +989,16 @@ export class LinearElementEditor { } if (lastPoint === lastUncommittedPoint) { - LinearElementEditor.movePoints(element, [ + LinearElementEditor.movePoints(element, elementsMap, [ { index: element.points.length - 1, point: newPoint, }, ]); } else { - LinearElementEditor.addPoints(element, [{ point: newPoint }]); + LinearElementEditor.addPoints(element, elementsMap, [ + { point: newPoint }, + ]); } return { ...appState.editingLinearElement, @@ -1226,7 +1231,7 @@ export class LinearElementEditor { // potentially expanding the bounding box if (pointAddedToEnd) { const lastPoint = element.points[element.points.length - 1]; - LinearElementEditor.movePoints(element, [ + LinearElementEditor.movePoints(element, elementsMap, [ { index: element.points.length - 1, point: pointFrom(lastPoint[0] + 30, lastPoint[1] + 30), @@ -1245,6 +1250,7 @@ export class LinearElementEditor { static deletePoints( element: NonDeleted, + elementsMap: ElementsMap, pointIndices: readonly number[], ) { let offsetX = 0; @@ -1275,28 +1281,41 @@ export class LinearElementEditor { return acc; }, []); - LinearElementEditor._updatePoints(element, nextPoints, offsetX, offsetY); + LinearElementEditor._updatePoints( + element, + elementsMap, + nextPoints, + offsetX, + offsetY, + ); } static addPoints( element: NonDeleted, + elementsMap: ElementsMap, targetPoints: { point: LocalPoint }[], ) { const offsetX = 0; const offsetY = 0; const nextPoints = [...element.points, ...targetPoints.map((x) => x.point)]; - LinearElementEditor._updatePoints(element, nextPoints, offsetX, offsetY); + LinearElementEditor._updatePoints( + element, + elementsMap, + nextPoints, + offsetX, + offsetY, + ); } static movePoints( element: NonDeleted, + elementsMap: ElementsMap, targetPoints: { index: number; point: LocalPoint; isDragging?: boolean }[], otherUpdates?: { startBinding?: PointBinding | null; endBinding?: PointBinding | null; }, - sceneElementsMap?: NonDeletedSceneElementsMap, ) { const { points } = element; @@ -1330,6 +1349,7 @@ export class LinearElementEditor { LinearElementEditor._updatePoints( element, + elementsMap, nextPoints, offsetX, offsetY, @@ -1340,7 +1360,6 @@ export class LinearElementEditor { dragging || targetPoint.isDragging === true, false, ), - sceneElementsMap, }, ); } @@ -1443,6 +1462,7 @@ export class LinearElementEditor { private static _updatePoints( element: NonDeleted, + elementsMap: ElementsMap, nextPoints: readonly LocalPoint[], offsetX: number, offsetY: number, @@ -1479,8 +1499,7 @@ export class LinearElementEditor { updates.points = Array.from(nextPoints); - // TODO_SCENE: fix - mutateElementWith(element, options?.sceneElementsMap!, updates, { + mutateElementWith(element, elementsMap, updates, { isDragging: options?.isDragging, }); } else { diff --git a/packages/element/tests/fractionalIndex.test.ts b/packages/element/tests/fractionalIndex.test.ts index 5d040e29f..c8cc9ad2d 100644 --- a/packages/element/tests/fractionalIndex.test.ts +++ b/packages/element/tests/fractionalIndex.test.ts @@ -14,6 +14,7 @@ import { deepCopyElement } from "@excalidraw/element/duplicate"; import { API } from "@excalidraw/excalidraw/tests/helpers/api"; import type { + ElementsMap, ExcalidrawElement, FractionalIndex, } from "@excalidraw/element/types"; @@ -749,7 +750,7 @@ function testInvalidIndicesSync(args: { function prepareArguments( elementsLike: { id: string; index?: string }[], movedElementsIds?: string[], -): [ExcalidrawElement[], Map | undefined] { +): [ExcalidrawElement[], ElementsMap | undefined] { const elements = elementsLike.map((x) => API.createElement({ id: x.id, index: x.index as FractionalIndex }), ); @@ -764,7 +765,7 @@ function prepareArguments( function test( name: string, elements: ExcalidrawElement[], - movedElements: Map | undefined, + movedElements: ElementsMap | undefined, expectUnchangedElements: Map, expectValidInput?: boolean, ) { diff --git a/packages/excalidraw/actions/actionDeleteSelected.tsx b/packages/excalidraw/actions/actionDeleteSelected.tsx index ab2ae05b0..2daf154ce 100644 --- a/packages/excalidraw/actions/actionDeleteSelected.tsx +++ b/packages/excalidraw/actions/actionDeleteSelected.tsx @@ -257,7 +257,11 @@ export const actionDeleteSelected = register({ : endBindingElement, }; - LinearElementEditor.deletePoints(element, selectedPointsIndices); + LinearElementEditor.deletePoints( + element, + elementsMap, + selectedPointsIndices, + ); return { elements, diff --git a/packages/excalidraw/actions/actionProperties.tsx b/packages/excalidraw/actions/actionProperties.tsx index b3b6a3bbe..337d7cffe 100644 --- a/packages/excalidraw/actions/actionProperties.tsx +++ b/packages/excalidraw/actions/actionProperties.tsx @@ -58,6 +58,7 @@ import type { LocalPoint } from "@excalidraw/math"; import type { Arrowhead, + ElementsMap, ExcalidrawBindableElement, ExcalidrawElement, ExcalidrawLinearElement, @@ -774,7 +775,7 @@ type ChangeFontFamilyData = Partial< > > & { /** cache of selected & editing elements populated on opened popup */ - cachedElements?: Map; + cachedElements?: ElementsMap; /** flag to reset all elements to their cached versions */ resetAll?: true; /** flag to reset all containers to their cached versions */ @@ -983,7 +984,7 @@ export const actionChangeFontFamily = register({ return result; }, PanelComponent: ({ elements, appState, app, updateData }) => { - const cachedElementsRef = useRef>(new Map()); + const cachedElementsRef = useRef(new Map()); const prevSelectedFontFamilyRef = useRef(null); // relying on state batching as multiple `FontPicker` handlers could be called in rapid succession and we want to combine them const [batchedData, setBatchedData] = useState({}); @@ -992,7 +993,7 @@ export const actionChangeFontFamily = register({ const selectedFontFamily = useMemo(() => { const getFontFamily = ( elementsArray: readonly ExcalidrawElement[], - elementsMap: Map, + elementsMap: ElementsMap, ) => getFormValue( elementsArray, @@ -1668,7 +1669,7 @@ export const actionChangeArrowType = register({ newElement, startHoveredElement, "start", - app.scene.getNonDeletedElementsMap(), + elementsMap, (...args) => app.scene.mutate(...args), ); endHoveredElement && @@ -1676,7 +1677,7 @@ export const actionChangeArrowType = register({ newElement, endHoveredElement, "end", - app.scene.getNonDeletedElementsMap(), + elementsMap, (...args) => app.scene.mutate(...args), ); @@ -1736,7 +1737,7 @@ export const actionChangeArrowType = register({ newElement, startElement, "start", - app.scene.getNonDeletedElementsMap(), + elementsMap, (...args) => app.scene.mutate(...args), ); } @@ -1750,7 +1751,7 @@ export const actionChangeArrowType = register({ newElement, endElement, "end", - app.scene.getNonDeletedElementsMap(), + elementsMap, (...args) => app.scene.mutate(...args), ); } diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 1a3c932dc..c27395b8b 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -4186,7 +4186,6 @@ class App extends React.Component { ) { this.flowChartCreator.createNodes( selectedElements[0], - this.scene.getNonDeletedElementsMap(), this.state, getLinkDirectionFromKey(event.key), this.scene, diff --git a/packages/excalidraw/components/Stats/DragInput.tsx b/packages/excalidraw/components/Stats/DragInput.tsx index b4795308d..1b97afff9 100644 --- a/packages/excalidraw/components/Stats/DragInput.tsx +++ b/packages/excalidraw/components/Stats/DragInput.tsx @@ -216,13 +216,12 @@ const StatsDragInput = < y: number; } | null = null; - let originalElementsMap: Map | null = - app.scene - .getNonDeletedElements() - .reduce((acc: ElementsMap, element) => { - acc.set(element.id, deepCopyElement(element)); - return acc; - }, new Map()); + let originalElementsMap: ElementsMap | null = app.scene + .getNonDeletedElements() + .reduce((acc: ElementsMap, element) => { + acc.set(element.id, deepCopyElement(element)); + return acc; + }, new Map()); let originalElements: readonly E[] | null = elements.map( (element) => originalElementsMap!.get(element.id) as E, diff --git a/packages/excalidraw/scene/Scene.ts b/packages/excalidraw/scene/Scene.ts index 971d1324d..e3b84b6b9 100644 --- a/packages/excalidraw/scene/Scene.ts +++ b/packages/excalidraw/scene/Scene.ts @@ -418,7 +418,6 @@ class Scene { }; // TODO_SCENE: should be accessed as app.scene through the API - // TODO_SCENE: in theory actions (and most of the App handlers) should not needs this as they are ending with "replaceAllElements" anyway // TODO_SCENE: inform mutation false is the new default, meaning all mutateElement with nothing should likely use scene instead // TODO_SCENE: think one more time about moving the scene inside element (probably we will end up with it either way) // Mutate an element with passed updates and trigger the component to update. Make sure you diff --git a/packages/excalidraw/tests/linearElementEditor.test.tsx b/packages/excalidraw/tests/linearElementEditor.test.tsx index 57c5d967a..e2922edd7 100644 --- a/packages/excalidraw/tests/linearElementEditor.test.tsx +++ b/packages/excalidraw/tests/linearElementEditor.test.tsx @@ -1384,7 +1384,7 @@ describe("Test Linear Elements", () => { const [origStartX, origStartY] = [line.x, line.y]; act(() => { - LinearElementEditor.movePoints(line, [ + LinearElementEditor.movePoints(line, arrayToMap(h.elements), [ { index: 0, point: pointFrom(line.points[0][0] + 10, line.points[0][1] + 10),