Merge 86a5b235a3 into 192c4e7658

packages/element/src/sizeHelpers.ts

@@ -3,6 +3,8 @@ import {
   viewportCoordsToSceneCoords,
 } from "@excalidraw/common";
 
+import { pointsEqual } from "@excalidraw/math";
+
 import type { AppState, Offsets, Zoom } from "@excalidraw/excalidraw/types";
 
 import { getCommonBounds, getElementBounds } from "./bounds";
@@ -10,6 +12,8 @@ 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`
@@ -17,8 +21,17 @@ 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 &&
+        pointsEqual(
+          element.points[0],
+          element.points[element.points.length - 1],
+          INVISIBLY_SMALL_ELEMENT_SIZE,
+        ))
+    );
   }
+
   return element.width === 0 && element.height === 0;
 };
 

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";
 
@@ -16,6 +17,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";
@@ -30,11 +37,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;
@@ -82,75 +128,85 @@ export const actionFinalize = register({
       focusContainer();
     }
 
-    const multiPointElement = appState.multiElement
+    let element: NonDeleted<ExcalidrawElement> | null = null;
-      ? appState.multiElement
+    if (appState.multiElement) {
-      : appState.newElement?.type === "freedraw"
+      element = appState.multiElement;
-      ? appState.newElement
+    } else if (
-      : null;
+      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<ExcalidrawLinearElement> | 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, {
+          scene.mutateElement(element, {
-            points: multiPointElement.points.slice(0, -1),
+            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(
+        newElements = newElements.filter((el) => el.id !== element!.id);
-          (el) => el.id !== multiPointElement.id,
-        );
       }
 
-      // If the multi point line closes the loop,
+      if (isLinearElement(element) || element.type === "freedraw") {
-      // set the last point to first point.
+        // If the multi point line closes the loop,
-      // This ensures that loop remains closed at different scales.
+        // set the last point to first point.
-      const isLoop = isPathALoop(multiPointElement.points, appState.zoom.value);
+        // This ensures that loop remains closed at different scales.
-      if (
+        const isLoop = isPathALoop(element.points, appState.zoom.value);
-        multiPointElement.type === "line" ||
+        if (element.type === "line" || element.type === "freedraw") {
-        multiPointElement.type === "freedraw"
+          if (isLoop) {
-      ) {
+            const linePoints = element.points;
-        if (isLoop) {
+            const firstPoint = linePoints[0];
-          const linePoints = multiPointElement.points;
+            scene.mutateElement(element, {
-          const firstPoint = linePoints[0];
+              points: linePoints.map((p, index) =>
-          scene.mutateElement(multiPointElement, {
+                index === linePoints.length - 1
-            points: linePoints.map((p, index) =>
+                  ? pointFrom(firstPoint[0], firstPoint[1])
-              index === linePoints.length - 1
+                  : p,
-                ? pointFrom(firstPoint[0], firstPoint[1])
+              ),
-                : p,
+            });
-            ),
+          }
-          });
         }
-      }
 
-      if (
+        if (
-        isBindingElement(multiPointElement) &&
+          isBindingElement(element) &&
-        !isLoop &&
+          !isLoop &&
-        multiPointElement.points.length > 1
+          element.points.length > 1 &&
-      ) {
+          isBindingEnabled(appState)
-        const [x, y] = LinearElementEditor.getPointAtIndexGlobalCoordinates(
+        ) {
-          multiPointElement,
+          const [x, y] = LinearElementEditor.getPointAtIndexGlobalCoordinates(
-          -1,
+            element,
-          arrayToMap(elements),
+            -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 +233,7 @@ export const actionFinalize = register({
         activeTool:
           (appState.activeTool.locked ||
             appState.activeTool.type === "freedraw") &&
-          multiPointElement
+          element
             ? appState.activeTool
             : activeTool,
         activeEmbeddable: null,
@@ -188,21 +244,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)
+          element && isLinearElement(element)
-            ? new LinearElementEditor(
+            ? new LinearElementEditor(element, arrayToMap(newElements))
-                multiPointElement,
-                arrayToMap(newElements),
-              )
             : appState.selectedLinearElement,
         pendingImageElementId: null,
       },

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<AppProps, AppState> {
     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 });
@@ -2928,27 +2925,6 @@ class App extends React.Component<AppProps, AppState> {
       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
@@ -9011,34 +8987,9 @@ class App extends React.Component<AppProps, AppState> {
             this.setState({ selectedLinearElement: null });
           }
         } else {
-          const linearElementEditor = LinearElementEditor.handlePointerUp(
+          this.actionManager.executeAction(actionFinalize, "ui", {
-            childEvent,
+            event: 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: [],
-            });
-          }
         }
       }
 
@@ -9162,12 +9113,7 @@ class App extends React.Component<AppProps, AppState> {
             isBindingEnabled(this.state) &&
             isBindingElement(newElement, false)
           ) {
-            maybeBindLinearElement(
+            this.actionManager.executeAction(actionFinalize);
-              newElement,
-              this.state,
-              pointerCoords,
-              this.scene,
-            );
           }
           this.setState({ suggestedBindings: [], startBoundElement: null });
           if (!activeTool.locked) {

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",

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);

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,

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();

packages/math/src/point.ts

@@ -91,9 +91,10 @@ export function isPoint(p: unknown): p is LocalPoint | GlobalPoint {
 export function pointsEqual<Point extends GlobalPoint | LocalPoint>(
   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;
 }
 
 /**