diff --git a/packages/element/src/sizeHelpers.ts b/packages/element/src/sizeHelpers.ts index bd3d3fb0c..a8339b49f 100644 --- a/packages/element/src/sizeHelpers.ts +++ b/packages/element/src/sizeHelpers.ts @@ -3,13 +3,21 @@ 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 { + isArrowElement, + 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` @@ -17,8 +25,18 @@ export const isInvisiblySmallElement = ( element: ExcalidrawElement, ): boolean => { if (isLinearElement(element) || isFreeDrawElement(element)) { - return element.points.length < 2; + return ( + element.points.length < 2 || + (element.points.length === 2 && + isArrowElement(element) && + pointsEqual( + element.points[0], + element.points[element.points.length - 1], + INVISIBLY_SMALL_ELEMENT_SIZE, + )) + ); } + return element.width === 0 && element.height === 0; }; diff --git a/packages/element/tests/linearElementEditor.test.tsx b/packages/element/tests/linearElementEditor.test.tsx index 77b99fa7f..85987428e 100644 --- a/packages/element/tests/linearElementEditor.test.tsx +++ b/packages/element/tests/linearElementEditor.test.tsx @@ -292,7 +292,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(` @@ -333,7 +333,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, @@ -394,7 +394,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, @@ -462,7 +462,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); @@ -513,7 +513,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, @@ -554,7 +554,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, @@ -602,7 +602,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, @@ -660,7 +660,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) @@ -758,7 +758,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/actions/actionFinalize.tsx b/packages/excalidraw/actions/actionFinalize.tsx index 9738a62c2..bba3f4501 100644 --- a/packages/excalidraw/actions/actionFinalize.tsx +++ b/packages/excalidraw/actions/actionFinalize.tsx @@ -3,8 +3,9 @@ import { pointFrom } from "@excalidraw/math"; import { maybeBindLinearElement, bindOrUnbindLinearElement, -} from "@excalidraw/element"; -import { LinearElementEditor } from "@excalidraw/element"; + isBindingEnabled, +} from "@excalidraw/element/binding"; +import { LinearElementEditor } from "@excalidraw/element/linearElementEditor"; import { isBindingElement, isLinearElement } from "@excalidraw/element"; @@ -15,6 +16,12 @@ import { isInvisiblySmallElement } from "@excalidraw/element"; import { CaptureUpdateAction } from "@excalidraw/element"; +import type { + ExcalidrawElement, + ExcalidrawLinearElement, + NonDeleted, +} from "@excalidraw/element/types"; + import { t } from "../i18n"; import { resetCursor } from "../cursor"; import { done } from "../components/icons"; @@ -28,11 +35,50 @@ 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(); + 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) { + 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, + selectedPointsIndices: null, + }, + suggestedBindings: [], + }, + captureUpdate: CaptureUpdateAction.IMMEDIATELY, + }; + } + } + if (appState.editingLinearElement) { const { elementId, startBindingElement, endBindingElement } = appState.editingLinearElement; @@ -80,75 +126,85 @@ 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 (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 !== 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" - ) { - if (isLoop) { - const linePoints = multiPointElement.points; - const firstPoint = linePoints[0]; - scene.mutateElement(multiPointElement, { - 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(multiPointElement) && - !isLoop && - multiPointElement.points.length > 1 - ) { - const [x, y] = LinearElementEditor.getPointAtIndexGlobalCoordinates( - multiPointElement, - -1, - arrayToMap(elements), - ); - maybeBindLinearElement(multiPointElement, 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); + } } } if ( (!appState.activeTool.locked && appState.activeTool.type !== "freedraw") || - !multiPointElement + !element ) { resetCursor(interactiveCanvas); } @@ -175,7 +231,7 @@ export const actionFinalize = register({ activeTool: (appState.activeTool.locked || appState.activeTool.type === "freedraw") && - multiPointElement + element ? appState.activeTool : activeTool, activeEmbeddable: null, @@ -186,21 +242,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 8bef9703d..c8f16af9e 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -107,13 +107,11 @@ import { import { getCommonBounds, getElementAbsoluteCoords } from "@excalidraw/element"; import { - bindOrUnbindLinearElement, bindOrUnbindLinearElements, fixBindingsAfterDeletion, getHoveredElementForBinding, isBindingEnabled, isLinearElementSimpleAndAlreadyBound, - maybeBindLinearElement, shouldEnableBindingForPointerEvent, updateBoundElements, getSuggestedBindingsForArrows, @@ -2797,7 +2795,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 }); @@ -2944,27 +2941,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) - ) { - 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 @@ -9143,34 +9119,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)) { - 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, + }); } } @@ -9294,12 +9245,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) { diff --git a/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap index 0ee4ea46a..729e53d22 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", diff --git a/packages/excalidraw/tests/data/restore.test.ts b/packages/excalidraw/tests/data/restore.test.ts index d06daafba..cc3807382 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"; import * as sizeHelpers from "@excalidraw/element"; +import type { LocalPoint } from "@excalidraw/math"; + import type { + ExcalidrawArrowElement, ExcalidrawElement, ExcalidrawFreeDrawElement, ExcalidrawLinearElement, @@ -163,6 +166,109 @@ 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 keep 'imperceptibly' small freedraw/line elements", () => { + const freedrawElement = API.createElement({ + type: "freedraw", + points: [ + [0, 0], + [0.0001, 0.0001], + ] as LocalPoint[], + x: 0, + y: 0, + }); + const lineElement = API.createElement({ + type: "line", + points: [ + [0, 0], + [0.0001, 0.0001], + ] as LocalPoint[], + x: 0, + y: 0, + }); + + const restoredElements = restore.restoreElements( + [freedrawElement, lineElement], + null, + ); + + expect(restoredElements).toEqual([ + expect.objectContaining({ id: freedrawElement.id }), + expect.objectContaining({ id: lineElement.id }), + ]); + }); + + 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); 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(); 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; } /**