From 14bd9d3829f8452aeb9be0b55ae660d5a7994d10 Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Tue, 29 Apr 2025 18:15:14 +0200 Subject: [PATCH 1/8] First iter Signed-off-by: Mark Tolmacs --- .../excalidraw/actions/actionFinalize.tsx | 79 +++++++++++-------- packages/excalidraw/components/App.tsx | 20 ++--- 2 files changed, 53 insertions(+), 46 deletions(-) diff --git a/packages/excalidraw/actions/actionFinalize.tsx b/packages/excalidraw/actions/actionFinalize.tsx index 22638ee91..89acad682 100644 --- a/packages/excalidraw/actions/actionFinalize.tsx +++ b/packages/excalidraw/actions/actionFinalize.tsx @@ -16,6 +16,12 @@ import { isPathALoop } from "@excalidraw/element/shapes"; import { isInvisiblySmallElement } from "@excalidraw/element/sizeHelpers"; +import type { + ExcalidrawElement, + ExcalidrawLinearElement, + NonDeleted, +} from "@excalidraw/element/types"; + import { t } from "../i18n"; import { resetCursor } from "../cursor"; import { done } from "../components/icons"; @@ -82,48 +88,55 @@ export const actionFinalize = register({ focusContainer(); } - const multiPointElement = appState.multiElement - ? appState.multiElement - : appState.newElement?.type === "freedraw" - ? appState.newElement - : null; + let element: NonDeleted | null = null; + if (appState.multiElement) { + element = appState.multiElement; + } else if ( + appState.newElement?.type === "freedraw" || + isBindingElement(appState.newElement) + ) { + element = appState.newElement; + } else if (Object.keys(appState.selectedElementIds).length === 1) { + const candidate = elementsMap.get( + Object.keys(appState.selectedElementIds)[0], + ) as NonDeleted | undefined; + if (candidate) { + element = candidate; + } + } - if (multiPointElement) { + if (element) { // pen and mouse have hover if ( - multiPointElement.type !== "freedraw" && + appState.multiElement && + element.type !== "freedraw" && appState.lastPointerDownWith !== "touch" ) { - const { points, lastCommittedPoint } = multiPointElement; + const { points, lastCommittedPoint } = element; if ( !lastCommittedPoint || points[points.length - 1] !== lastCommittedPoint ) { - scene.mutateElement(multiPointElement, { - points: multiPointElement.points.slice(0, -1), + scene.mutateElement(element, { + points: element.points.slice(0, -1), }); } } - if (isInvisiblySmallElement(multiPointElement)) { + if (isInvisiblySmallElement(element)) { // TODO: #7348 in theory this gets recorded by the store, so the invisible elements could be restored by the undo/redo, which might be not what we would want - newElements = newElements.filter( - (el) => el.id !== multiPointElement.id, - ); + newElements = newElements.filter((el) => el.id !== element.id); } // If the multi point line closes the loop, // set the last point to first point. // This ensures that loop remains closed at different scales. - const isLoop = isPathALoop(multiPointElement.points, appState.zoom.value); - if ( - multiPointElement.type === "line" || - multiPointElement.type === "freedraw" - ) { + const isLoop = isPathALoop(element.points, appState.zoom.value); + if (element.type === "line" || element.type === "freedraw") { if (isLoop) { - const linePoints = multiPointElement.points; + const linePoints = element.points; const firstPoint = linePoints[0]; - scene.mutateElement(multiPointElement, { + scene.mutateElement(element, { points: linePoints.map((p, index) => index === linePoints.length - 1 ? pointFrom(firstPoint[0], firstPoint[1]) @@ -134,23 +147,24 @@ export const actionFinalize = register({ } if ( - isBindingElement(multiPointElement) && + isBindingElement(element) && !isLoop && - multiPointElement.points.length > 1 + element.points.length > 1 && + !appState.selectedElementIds[element.id] ) { const [x, y] = LinearElementEditor.getPointAtIndexGlobalCoordinates( - multiPointElement, + element, -1, arrayToMap(elements), ); - maybeBindLinearElement(multiPointElement, appState, { x, y }, scene); + maybeBindLinearElement(element, appState, { x, y }, scene); } } if ( (!appState.activeTool.locked && appState.activeTool.type !== "freedraw") || - !multiPointElement + !element ) { resetCursor(interactiveCanvas); } @@ -177,7 +191,7 @@ export const actionFinalize = register({ activeTool: (appState.activeTool.locked || appState.activeTool.type === "freedraw") && - multiPointElement + element ? appState.activeTool : activeTool, activeEmbeddable: null, @@ -188,21 +202,18 @@ export const actionFinalize = register({ startBoundElement: null, suggestedBindings: [], selectedElementIds: - multiPointElement && + element && !appState.activeTool.locked && appState.activeTool.type !== "freedraw" ? { ...appState.selectedElementIds, - [multiPointElement.id]: true, + [element.id]: true, } : appState.selectedElementIds, // To select the linear element when user has finished mutipoint editing selectedLinearElement: - multiPointElement && isLinearElement(multiPointElement) - ? new LinearElementEditor( - multiPointElement, - arrayToMap(newElements), - ) + element && isLinearElement(element) + ? new LinearElementEditor(element, arrayToMap(newElements)) : appState.selectedLinearElement, pendingImageElementId: null, }, diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index ddb071981..5ce8ee376 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -9022,12 +9022,13 @@ class App extends React.Component { linearElementEditor; const element = this.scene.getElement(linearElementEditor.elementId); if (isBindingElement(element)) { - bindOrUnbindLinearElement( - element, - startBindingElement, - endBindingElement, - this.scene, - ); + this.actionManager.executeAction(actionFinalize); + // bindOrUnbindLinearElement( + // element, + // startBindingElement, + // endBindingElement, + // this.scene, + // ); } if (linearElementEditor !== this.state.selectedLinearElement) { @@ -9162,12 +9163,7 @@ class App extends React.Component { isBindingEnabled(this.state) && isBindingElement(newElement, false) ) { - maybeBindLinearElement( - newElement, - this.state, - pointerCoords, - this.scene, - ); + this.actionManager.executeAction(actionFinalize); } this.setState({ suggestedBindings: [], startBoundElement: null }); if (!activeTool.locked) { From be69271fcdc66d48bd5a875eb1b01f1e3260486c Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Tue, 29 Apr 2025 18:24:59 +0200 Subject: [PATCH 2/8] Restore binding Signed-off-by: Mark Tolmacs --- packages/excalidraw/actions/actionFinalize.tsx | 2 +- packages/excalidraw/components/App.tsx | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/excalidraw/actions/actionFinalize.tsx b/packages/excalidraw/actions/actionFinalize.tsx index 89acad682..247dd44a9 100644 --- a/packages/excalidraw/actions/actionFinalize.tsx +++ b/packages/excalidraw/actions/actionFinalize.tsx @@ -123,7 +123,7 @@ export const actionFinalize = register({ } } - if (isInvisiblySmallElement(element)) { + if (element && isInvisiblySmallElement(element)) { // TODO: #7348 in theory this gets recorded by the store, so the invisible elements could be restored by the undo/redo, which might be not what we would want newElements = newElements.filter((el) => el.id !== element.id); } diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 5ce8ee376..f963a3f9b 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -9023,12 +9023,12 @@ class App extends React.Component { const element = this.scene.getElement(linearElementEditor.elementId); if (isBindingElement(element)) { this.actionManager.executeAction(actionFinalize); - // bindOrUnbindLinearElement( - // element, - // startBindingElement, - // endBindingElement, - // this.scene, - // ); + bindOrUnbindLinearElement( + element, + startBindingElement, + endBindingElement, + this.scene, + ); } if (linearElementEditor !== this.state.selectedLinearElement) { From 9b643abceeaf1fb20406145333df0240ddcae0fe Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Wed, 30 Apr 2025 22:30:40 +0200 Subject: [PATCH 3/8] More actionFinalize Signed-off-by: Mark Tolmacs --- .../excalidraw/actions/actionFinalize.tsx | 41 ++++++++++++- packages/excalidraw/components/App.tsx | 60 +++++-------------- 2 files changed, 54 insertions(+), 47 deletions(-) diff --git a/packages/excalidraw/actions/actionFinalize.tsx b/packages/excalidraw/actions/actionFinalize.tsx index 247dd44a9..cd640860a 100644 --- a/packages/excalidraw/actions/actionFinalize.tsx +++ b/packages/excalidraw/actions/actionFinalize.tsx @@ -3,6 +3,7 @@ import { pointFrom } from "@excalidraw/math"; import { maybeBindLinearElement, bindOrUnbindLinearElement, + isBindingEnabled, } from "@excalidraw/element/binding"; import { LinearElementEditor } from "@excalidraw/element/linearElementEditor"; @@ -36,10 +37,43 @@ export const actionFinalize = register({ name: "finalize", label: "", trackEvent: false, - perform: (elements, appState, _, app) => { + perform: (elements, appState, data, app) => { const { interactiveCanvas, focusContainer, scene } = app; const elementsMap = scene.getNonDeletedElementsMap(); + console.log("actionFinalize"); + if (data?.event && appState.selectedLinearElement) { + const linearElementEditor = LinearElementEditor.handlePointerUp( + data.event, + appState.selectedLinearElement, + appState, + app.scene, + ); + + const { startBindingElement, endBindingElement } = linearElementEditor; + const element = app.scene.getElement(linearElementEditor.elementId); + if (isBindingElement(element)) { + bindOrUnbindLinearElement( + element, + startBindingElement, + endBindingElement, + app.scene, + ); + } + + if (linearElementEditor !== appState.selectedLinearElement) { + return { + appState: { + selectedLinearElement: { + ...linearElementEditor, + selectedPointsIndices: null, + }, + suggestedBindings: [], + }, + captureUpdate: CaptureUpdateAction.IMMEDIATELY, + }; + } + } if (appState.editingLinearElement) { const { elementId, startBindingElement, endBindingElement } = @@ -125,7 +159,7 @@ export const actionFinalize = register({ if (element && isInvisiblySmallElement(element)) { // TODO: #7348 in theory this gets recorded by the store, so the invisible elements could be restored by the undo/redo, which might be not what we would want - newElements = newElements.filter((el) => el.id !== element.id); + newElements = newElements.filter((el) => el.id !== element!.id); } // If the multi point line closes the loop, @@ -150,7 +184,8 @@ export const actionFinalize = register({ isBindingElement(element) && !isLoop && element.points.length > 1 && - !appState.selectedElementIds[element.id] + !appState.selectedElementIds[element.id] && + isBindingEnabled(appState) ) { const [x, y] = LinearElementEditor.getPointAtIndexGlobalCoordinates( element, diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index f963a3f9b..8763045eb 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -109,13 +109,11 @@ import { } from "@excalidraw/element/bounds"; import { - bindOrUnbindLinearElement, bindOrUnbindLinearElements, fixBindingsAfterDeletion, getHoveredElementForBinding, isBindingEnabled, isLinearElementSimpleAndAlreadyBound, - maybeBindLinearElement, shouldEnableBindingForPointerEvent, updateBoundElements, getSuggestedBindingsForArrows, @@ -2781,7 +2779,6 @@ class App extends React.Component { this.updateEmbeddables(); const elements = this.scene.getElementsIncludingDeleted(); const elementsMap = this.scene.getElementsMapIncludingDeleted(); - const nonDeletedElementsMap = this.scene.getNonDeletedElementsMap(); if (!this.state.showWelcomeScreen && !elements.length) { this.setState({ showWelcomeScreen: true }); @@ -2935,18 +2932,19 @@ class App extends React.Component { isBindingEnabled(this.state) && isBindingElement(multiElement, false) ) { - maybeBindLinearElement( - multiElement, - this.state, - tupleToCoors( - LinearElementEditor.getPointAtIndexGlobalCoordinates( - multiElement, - -1, - nonDeletedElementsMap, - ), - ), - this.scene, - ); + this.actionManager.executeAction(actionFinalize); + // maybeBindLinearElement( + // multiElement, + // this.state, + // tupleToCoors( + // LinearElementEditor.getPointAtIndexGlobalCoordinates( + // multiElement, + // -1, + // nonDeletedElementsMap, + // ), + // ), + // this.scene, + // ); } this.store.commit(elementsMap, this.state); @@ -9011,35 +9009,9 @@ class App extends React.Component { this.setState({ selectedLinearElement: null }); } } else { - const linearElementEditor = LinearElementEditor.handlePointerUp( - childEvent, - this.state.selectedLinearElement, - this.state, - this.scene, - ); - - const { startBindingElement, endBindingElement } = - linearElementEditor; - const element = this.scene.getElement(linearElementEditor.elementId); - if (isBindingElement(element)) { - this.actionManager.executeAction(actionFinalize); - bindOrUnbindLinearElement( - element, - startBindingElement, - endBindingElement, - this.scene, - ); - } - - if (linearElementEditor !== this.state.selectedLinearElement) { - this.setState({ - selectedLinearElement: { - ...linearElementEditor, - selectedPointsIndices: null, - }, - suggestedBindings: [], - }); - } + this.actionManager.executeAction(actionFinalize, "ui", { + event: childEvent, + }); } } From 4ffe7fd99144eb20814544dbceb0e90e3c06fe17 Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Thu, 1 May 2025 09:07:50 +0200 Subject: [PATCH 4/8] Additional fixes Signed-off-by: Mark Tolmacs --- .../excalidraw/actions/actionFinalize.tsx | 61 ++++++++++--------- packages/excalidraw/components/App.tsx | 22 ------- .../tests/__snapshots__/move.test.tsx.snap | 4 +- 3 files changed, 33 insertions(+), 54 deletions(-) diff --git a/packages/excalidraw/actions/actionFinalize.tsx b/packages/excalidraw/actions/actionFinalize.tsx index cd640860a..a6a6fe63d 100644 --- a/packages/excalidraw/actions/actionFinalize.tsx +++ b/packages/excalidraw/actions/actionFinalize.tsx @@ -41,7 +41,7 @@ export const actionFinalize = register({ const { interactiveCanvas, focusContainer, scene } = app; const elementsMap = scene.getNonDeletedElementsMap(); - console.log("actionFinalize"); + if (data?.event && appState.selectedLinearElement) { const linearElementEditor = LinearElementEditor.handlePointerUp( data.event, @@ -162,37 +162,38 @@ export const actionFinalize = register({ newElements = newElements.filter((el) => el.id !== element!.id); } - // If the multi point line closes the loop, - // set the last point to first point. - // This ensures that loop remains closed at different scales. - const isLoop = isPathALoop(element.points, appState.zoom.value); - if (element.type === "line" || element.type === "freedraw") { - if (isLoop) { - const linePoints = element.points; - const firstPoint = linePoints[0]; - scene.mutateElement(element, { - points: linePoints.map((p, index) => - index === linePoints.length - 1 - ? pointFrom(firstPoint[0], firstPoint[1]) - : p, - ), - }); + if (isLinearElement(element) || element.type === "freedraw") { + // If the multi point line closes the loop, + // set the last point to first point. + // This ensures that loop remains closed at different scales. + const isLoop = isPathALoop(element.points, appState.zoom.value); + if (element.type === "line" || element.type === "freedraw") { + if (isLoop) { + const linePoints = element.points; + const firstPoint = linePoints[0]; + scene.mutateElement(element, { + points: linePoints.map((p, index) => + index === linePoints.length - 1 + ? pointFrom(firstPoint[0], firstPoint[1]) + : p, + ), + }); + } } - } - if ( - isBindingElement(element) && - !isLoop && - element.points.length > 1 && - !appState.selectedElementIds[element.id] && - isBindingEnabled(appState) - ) { - const [x, y] = LinearElementEditor.getPointAtIndexGlobalCoordinates( - element, - -1, - arrayToMap(elements), - ); - maybeBindLinearElement(element, appState, { x, y }, scene); + if ( + isBindingElement(element) && + !isLoop && + element.points.length > 1 && + isBindingEnabled(appState) + ) { + const [x, y] = LinearElementEditor.getPointAtIndexGlobalCoordinates( + element, + -1, + arrayToMap(elements), + ); + maybeBindLinearElement(element, appState, { x, y }, scene); + } } } diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 8763045eb..317ee0ad1 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -2925,28 +2925,6 @@ class App extends React.Component { this.setState({ selectedLinearElement: null }); } - const { multiElement } = prevState; - if ( - prevState.activeTool !== this.state.activeTool && - multiElement != null && - isBindingEnabled(this.state) && - isBindingElement(multiElement, false) - ) { - this.actionManager.executeAction(actionFinalize); - // maybeBindLinearElement( - // multiElement, - // this.state, - // tupleToCoors( - // LinearElementEditor.getPointAtIndexGlobalCoordinates( - // multiElement, - // -1, - // nonDeletedElementsMap, - // ), - // ), - // this.scene, - // ); - } - this.store.commit(elementsMap, this.state); // Do not notify consumers if we're still loading the scene. Among other diff --git a/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap index 5078a31a0..39d04ab8c 100644 --- a/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap @@ -173,7 +173,7 @@ exports[`move element > rectangles with binding arrow 6`] = ` "type": "rectangle", "updated": 1, "version": 7, - "versionNonce": 745419401, + "versionNonce": 1051383431, "width": 300, "x": 201, "y": 2, @@ -231,7 +231,7 @@ exports[`move element > rectangles with binding arrow 7`] = ` "type": "arrow", "updated": 1, "version": 11, - "versionNonce": 1051383431, + "versionNonce": 1996028265, "width": "86.85786", "x": "107.07107", "y": "47.07107", From 25a2ec4b492f8205151b2df447094ad446c1a4ea Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Thu, 1 May 2025 09:37:55 +0200 Subject: [PATCH 5/8] New elbow arrow is removed if too small Signed-off-by: Mark Tolmacs --- packages/element/src/sizeHelpers.ts | 10 +++++++++- packages/excalidraw/actions/actionFinalize.tsx | 6 ++++++ .../tests/linearElementEditor.test.tsx | 18 +++++++++--------- packages/excalidraw/tests/selection.test.tsx | 4 ++-- 4 files changed, 26 insertions(+), 12 deletions(-) diff --git a/packages/element/src/sizeHelpers.ts b/packages/element/src/sizeHelpers.ts index bd3d3fb0c..02f6ea923 100644 --- a/packages/element/src/sizeHelpers.ts +++ b/packages/element/src/sizeHelpers.ts @@ -3,10 +3,12 @@ import { viewportCoordsToSceneCoords, } from "@excalidraw/common"; +import { pointsEqual } from "@excalidraw/math"; + import type { AppState, Offsets, Zoom } from "@excalidraw/excalidraw/types"; import { getCommonBounds, getElementBounds } from "./bounds"; -import { isFreeDrawElement, isLinearElement } from "./typeChecks"; +import { isElbowArrow, isFreeDrawElement, isLinearElement } from "./typeChecks"; import type { ElementsMap, ExcalidrawElement } from "./types"; @@ -16,6 +18,12 @@ import type { ElementsMap, ExcalidrawElement } from "./types"; export const isInvisiblySmallElement = ( element: ExcalidrawElement, ): boolean => { + if (isElbowArrow(element)) { + return ( + element.points.length < 2 || + pointsEqual(element.points[0], element.points[element.points.length - 1]) + ); + } if (isLinearElement(element) || isFreeDrawElement(element)) { return element.points.length < 2; } diff --git a/packages/excalidraw/actions/actionFinalize.tsx b/packages/excalidraw/actions/actionFinalize.tsx index a6a6fe63d..92ea7bbb7 100644 --- a/packages/excalidraw/actions/actionFinalize.tsx +++ b/packages/excalidraw/actions/actionFinalize.tsx @@ -62,7 +62,13 @@ export const actionFinalize = register({ } if (linearElementEditor !== appState.selectedLinearElement) { + let newElements = elements; + if (element && isInvisiblySmallElement(element)) { + // TODO: #7348 in theory this gets recorded by the store, so the invisible elements could be restored by the undo/redo, which might be not what we would want + newElements = newElements.filter((el) => el.id !== element!.id); + } return { + elements: newElements, appState: { selectedLinearElement: { ...linearElementEditor, diff --git a/packages/excalidraw/tests/linearElementEditor.test.tsx b/packages/excalidraw/tests/linearElementEditor.test.tsx index 2e32e8821..5212e23bf 100644 --- a/packages/excalidraw/tests/linearElementEditor.test.tsx +++ b/packages/excalidraw/tests/linearElementEditor.test.tsx @@ -296,7 +296,7 @@ describe("Test Linear Elements", () => { expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot( `12`, ); - expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`6`); + expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`7`); expect(line.points.length).toEqual(3); expect(line.points).toMatchInlineSnapshot(` @@ -337,7 +337,7 @@ describe("Test Linear Elements", () => { expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot( `9`, ); - expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`6`); + expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`7`); const midPointsWithRoundEdge = LinearElementEditor.getEditorMidPoints( h.elements[0] as ExcalidrawLinearElement, @@ -398,7 +398,7 @@ describe("Test Linear Elements", () => { expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot( `12`, ); - expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`7`); + expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`8`); expect([line.x, line.y]).toEqual([ points[0][0] + deltaX, @@ -466,7 +466,7 @@ describe("Test Linear Elements", () => { expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot( `16`, ); - expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`7`); + expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`8`); expect(line.points.length).toEqual(5); @@ -517,7 +517,7 @@ describe("Test Linear Elements", () => { expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot( `12`, ); - expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`6`); + expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`7`); const newPoints = LinearElementEditor.getPointsGlobalCoordinates( line, @@ -558,7 +558,7 @@ describe("Test Linear Elements", () => { expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot( `12`, ); - expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`6`); + expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`7`); const newPoints = LinearElementEditor.getPointsGlobalCoordinates( line, @@ -606,7 +606,7 @@ describe("Test Linear Elements", () => { expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot( `18`, ); - expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`7`); + expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`8`); const newMidPoints = LinearElementEditor.getEditorMidPoints( line, @@ -664,7 +664,7 @@ describe("Test Linear Elements", () => { expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot( `16`, ); - expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`7`); + expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`8`); expect(line.points.length).toEqual(5); expect((h.elements[0] as ExcalidrawLinearElement).points) @@ -762,7 +762,7 @@ describe("Test Linear Elements", () => { expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot( `12`, ); - expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`6`); + expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`7`); const newPoints = LinearElementEditor.getPointsGlobalCoordinates( line, diff --git a/packages/excalidraw/tests/selection.test.tsx b/packages/excalidraw/tests/selection.test.tsx index 10f4f7ad9..50d392651 100644 --- a/packages/excalidraw/tests/selection.test.tsx +++ b/packages/excalidraw/tests/selection.test.tsx @@ -426,7 +426,7 @@ describe("select single element on the scene", () => { fireEvent.pointerUp(canvas); expect(renderInteractiveScene).toHaveBeenCalledTimes(8); - expect(renderStaticScene).toHaveBeenCalledTimes(6); + expect(renderStaticScene).toHaveBeenCalledTimes(7); expect(h.state.selectionElement).toBeNull(); expect(h.elements.length).toEqual(1); expect(h.state.selectedElementIds[h.elements[0].id]).toBeTruthy(); @@ -470,7 +470,7 @@ describe("select single element on the scene", () => { fireEvent.pointerUp(canvas); expect(renderInteractiveScene).toHaveBeenCalledTimes(8); - expect(renderStaticScene).toHaveBeenCalledTimes(6); + expect(renderStaticScene).toHaveBeenCalledTimes(7); expect(h.state.selectionElement).toBeNull(); expect(h.elements.length).toEqual(1); expect(h.state.selectedElementIds[h.elements[0].id]).toBeTruthy(); From 470250950075ed1245be7bd6d9885cf2334e3dff Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Thu, 1 May 2025 13:38:54 +0200 Subject: [PATCH 6/8] Remove very small arrows --- packages/element/src/sizeHelpers.ts | 15 +++++++++------ packages/math/src/point.ts | 3 ++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/element/src/sizeHelpers.ts b/packages/element/src/sizeHelpers.ts index 02f6ea923..b31237857 100644 --- a/packages/element/src/sizeHelpers.ts +++ b/packages/element/src/sizeHelpers.ts @@ -8,25 +8,28 @@ import { pointsEqual } from "@excalidraw/math"; import type { AppState, Offsets, Zoom } from "@excalidraw/excalidraw/types"; import { getCommonBounds, getElementBounds } from "./bounds"; -import { isElbowArrow, isFreeDrawElement, isLinearElement } from "./typeChecks"; +import { isFreeDrawElement, isLinearElement } from "./typeChecks"; import type { ElementsMap, ExcalidrawElement } from "./types"; +export const INVISIBLY_SMALL_ELEMENT_SIZE = 0.1; + // TODO: remove invisible elements consistently actions, so that invisible elements are not recorded by the store, exported, broadcasted or persisted // - perhaps could be as part of a standalone 'cleanup' action, in addition to 'finalize' // - could also be part of `_clearElements` export const isInvisiblySmallElement = ( element: ExcalidrawElement, ): boolean => { - if (isElbowArrow(element)) { + if (isLinearElement(element) || isFreeDrawElement(element)) { return ( element.points.length < 2 || - pointsEqual(element.points[0], element.points[element.points.length - 1]) + pointsEqual( + element.points[0], + element.points[element.points.length - 1], + INVISIBLY_SMALL_ELEMENT_SIZE, + ) ); } - if (isLinearElement(element) || isFreeDrawElement(element)) { - return element.points.length < 2; - } return element.width === 0 && element.height === 0; }; diff --git a/packages/math/src/point.ts b/packages/math/src/point.ts index b6054a10a..863febfd4 100644 --- a/packages/math/src/point.ts +++ b/packages/math/src/point.ts @@ -91,9 +91,10 @@ export function isPoint(p: unknown): p is LocalPoint | GlobalPoint { export function pointsEqual( a: Point, b: Point, + tolerance: number = PRECISION, ): boolean { const abs = Math.abs; - return abs(a[0] - b[0]) < PRECISION && abs(a[1] - b[1]) < PRECISION; + return abs(a[0] - b[0]) < tolerance && abs(a[1] - b[1]) < tolerance; } /** From 570fa803d6a4deaa9219f726ba0620c5092ea8da Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Thu, 1 May 2025 14:07:57 +0200 Subject: [PATCH 7/8] Still allow loops --- packages/element/src/sizeHelpers.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/element/src/sizeHelpers.ts b/packages/element/src/sizeHelpers.ts index b31237857..7bf09ab55 100644 --- a/packages/element/src/sizeHelpers.ts +++ b/packages/element/src/sizeHelpers.ts @@ -23,13 +23,15 @@ export const isInvisiblySmallElement = ( if (isLinearElement(element) || isFreeDrawElement(element)) { return ( element.points.length < 2 || - pointsEqual( - element.points[0], - element.points[element.points.length - 1], - INVISIBLY_SMALL_ELEMENT_SIZE, - ) + (element.points.length === 2 && + pointsEqual( + element.points[0], + element.points[element.points.length - 1], + INVISIBLY_SMALL_ELEMENT_SIZE, + )) ); } + return element.width === 0 && element.height === 0; }; From 86a5b235a382e80812ef08e626dadf0353708456 Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Thu, 1 May 2025 19:57:52 +0200 Subject: [PATCH 8/8] Restore tests Signed-off-by: Mark Tolmacs --- .../excalidraw/tests/data/restore.test.ts | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/packages/excalidraw/tests/data/restore.test.ts b/packages/excalidraw/tests/data/restore.test.ts index 4b414bbf1..f520a7b78 100644 --- a/packages/excalidraw/tests/data/restore.test.ts +++ b/packages/excalidraw/tests/data/restore.test.ts @@ -6,7 +6,10 @@ import { DEFAULT_SIDEBAR, FONT_FAMILY, ROUNDNESS } from "@excalidraw/common"; import { newElementWith } from "@excalidraw/element/mutateElement"; import * as sizeHelpers from "@excalidraw/element/sizeHelpers"; +import type { LocalPoint } from "@excalidraw/math"; + import type { + ExcalidrawArrowElement, ExcalidrawElement, ExcalidrawFreeDrawElement, ExcalidrawLinearElement, @@ -163,6 +166,78 @@ describe("restoreElements", () => { }); }); + it("should remove imperceptibly small elements", () => { + const arrowElement = API.createElement({ + type: "arrow", + points: [ + [0, 0], + [0.02, 0.05], + ] as LocalPoint[], + x: 0, + y: 0, + }); + + const restoredElements = restore.restoreElements([arrowElement], null); + + const restoredArrow = restoredElements[0] as + | ExcalidrawArrowElement + | undefined; + + expect(restoredArrow).toBeUndefined(); + }); + + it("should restore loop linears correctly", () => { + const linearElement = API.createElement({ + type: "line", + points: [ + [0, 0], + [100, 100], + [100, 200], + [0, 0], + ] as LocalPoint[], + x: 0, + y: 0, + }); + const arrowElement = API.createElement({ + type: "arrow", + points: [ + [0, 0], + [100, 100], + [100, 200], + [0, 0], + ] as LocalPoint[], + x: 500, + y: 500, + }); + + const restoredElements = restore.restoreElements( + [linearElement, arrowElement], + null, + ); + + const restoredLinear = restoredElements[0] as + | ExcalidrawLinearElement + | undefined; + const restoredArrow = restoredElements[1] as + | ExcalidrawArrowElement + | undefined; + + expect(restoredLinear?.type).toBe("line"); + expect(restoredLinear?.points).toEqual([ + [0, 0], + [100, 100], + [100, 200], + [0, 0], + ] as LocalPoint[]); + expect(restoredArrow?.type).toBe("arrow"); + expect(restoredArrow?.points).toEqual([ + [0, 0], + [100, 100], + [100, 200], + [0, 0], + ] as LocalPoint[]); + }); + it('should set arrow element endArrowHead as "arrow" when arrow element endArrowHead is null', () => { const arrowElement = API.createElement({ type: "arrow" }); const restoredElements = restore.restoreElements([arrowElement], null);