feat: Call actionFinalize at the end of arrow creation and drag (#9453)

* First iter

Signed-off-by: Mark Tolmacs <[email protected]>

* Restore binding

Signed-off-by: Mark Tolmacs <[email protected]>

* More actionFinalize

Signed-off-by: Mark Tolmacs <[email protected]>

* Additional fixes

Signed-off-by: Mark Tolmacs <[email protected]>

* New elbow arrow is removed if  too small

Signed-off-by: Mark Tolmacs <[email protected]>

* Remove very small arrows

* Still allow loops

* Restore tests

Signed-off-by: Mark Tolmacs <[email protected]>

* Update history snapshot

* More history snapshot updates

* keep invisible 2-point lines/freedraw elements

---------

Signed-off-by: Mark Tolmacs <[email protected]>
Co-authored-by: dwelle <[email protected]>

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

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,

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<ExcalidrawElement> | 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<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, {
-            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,
       },

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<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 });
@@ -2944,27 +2941,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
@@ -9143,34 +9119,9 @@ class App extends React.Component<AppProps, AppState> {
             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<AppProps, AppState> {
             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) {

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

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;
 }
 
 /**