Fixing duplicate

Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
2025-05-03 10:00:07 -04:00 · 2025-03-17 22:58:56 +01:00 · 2025-03-17 22:58:56 +01:00 · 35c4c074d7
commit 35c4c074d7
parent d05c163aec
11 changed files with 184 additions and 246 deletions
--- a/packages/excalidraw/components/App.tsx
+++ b/packages/excalidraw/components/App.tsx
@ -8440,10 +8440,25 @@ class App extends React.Component<AppProps, AppState> {
                idsOfElementsToDuplicate: new Map(
                  selectedElements.map((el) => [el.id, el]),
                ),
+                overrides: (el) => {
+                  const origEl = pointerDownState.originalElements.get(el.id)!;
+                  return {
+                    x: origEl.x,
+                    y: origEl.y,
+                  };
+                },
              });
+            clonedElements.forEach((element) => {
+              pointerDownState.originalElements.set(element.id, element);
+            });
+
+            const mappedNewSceneElements = this.props.onDuplicate?.(
+              elementsWithClones,
+              elements,
+            );

            const nextSceneElements = syncMovedIndices(
-              elementsWithClones,
+              mappedNewSceneElements || elementsWithClones,
              arrayToMap(clonedElements),
            );

--- a/packages/excalidraw/element/binding.ts
+++ b/packages/excalidraw/element/binding.ts
@ -49,7 +49,6 @@ import { getBoundTextElement, handleBindTextResize } from "./textElement";
 import {
  isArrowElement,
  isBindableElement,
-  isBindingElement,
  isBoundToContainer,
  isElbowArrow,
  isFixedPointBinding,
@ -59,6 +58,10 @@ import {
  isTextElement,
 } from "./typeChecks";

+import { updateElbowArrowPoints } from "./elbowArrow";
+
+import type { Mutable } from "../utility-types";
+
 import type { Bounds } from "./bounds";
 import type { ElementUpdate } from "./mutateElement";
 import type {
@ -974,7 +977,6 @@ export const bindPointToSnapToElementOutline = (
          otherPoint,
        ),
      ),
-      FIXED_BINDING_DISTANCE,
    )[0];
  } else {
    intersection = intersectElementWithLineSegment(
@ -1147,7 +1149,7 @@ export const snapToMid = (
  ) {
    // LEFT
    return pointRotateRads(
-      pointFrom(x - 2 * FIXED_BINDING_DISTANCE, center[1]),
+      pointFrom(x - FIXED_BINDING_DISTANCE, center[1]),
      center,
      angle,
    );
@ -1158,7 +1160,7 @@ export const snapToMid = (
  ) {
    // TOP
    return pointRotateRads(
-      pointFrom(center[0], y - 2 * FIXED_BINDING_DISTANCE),
+      pointFrom(center[0], y - FIXED_BINDING_DISTANCE),
      center,
      angle,
    );
@ -1169,7 +1171,7 @@ export const snapToMid = (
  ) {
    // RIGHT
    return pointRotateRads(
-      pointFrom(x + width + 2 * FIXED_BINDING_DISTANCE, center[1]),
+      pointFrom(x + width + FIXED_BINDING_DISTANCE, center[1]),
      center,
      angle,
    );
@ -1180,7 +1182,7 @@ export const snapToMid = (
  ) {
    // DOWN
    return pointRotateRads(
-      pointFrom(center[0], y + height + 2 * FIXED_BINDING_DISTANCE),
+      pointFrom(center[0], y + height + FIXED_BINDING_DISTANCE),
      center,
      angle,
    );
@ -1412,107 +1414,75 @@ const getLinearElementEdgeCoors = (
  );
 };

-// We need to:
-// 1: Update elements not selected to point to duplicated elements
-// 2: Update duplicated elements to point to other duplicated elements
 export const fixBindingsAfterDuplication = (
-  sceneElements: readonly ExcalidrawElement[],
-  oldElements: readonly ExcalidrawElement[],
+  newElements: ExcalidrawElement[],
  oldIdToDuplicatedId: Map<ExcalidrawElement["id"], ExcalidrawElement["id"]>,
-  // There are three copying mechanisms: Copy-paste, duplication and alt-drag.
-  // Only when alt-dragging the new "duplicates" act as the "old", while
-  // the "old" elements act as the "new copy" - essentially working reverse
-  // to the other two.
-  duplicatesServeAsOld?: "duplicatesServeAsOld" | undefined,
-): void => {
-  // First collect all the binding/bindable elements, so we only update
-  // each once, regardless of whether they were duplicated or not.
-  const allBoundElementIds: Set<ExcalidrawElement["id"]> = new Set();
-  const allBindableElementIds: Set<ExcalidrawElement["id"]> = new Set();
-  const shouldReverseRoles = duplicatesServeAsOld === "duplicatesServeAsOld";
-  const duplicateIdToOldId = new Map(
-    [...oldIdToDuplicatedId].map(([key, value]) => [value, key]),
-  );
-  oldElements.forEach((oldElement) => {
-    const { boundElements } = oldElement;
-    if (boundElements != null && boundElements.length > 0) {
-      boundElements.forEach((boundElement) => {
-        if (shouldReverseRoles && !oldIdToDuplicatedId.has(boundElement.id)) {
-          allBoundElementIds.add(boundElement.id);
-        }
+  duplicatedElementsMap: NonDeletedSceneElementsMap,
+) => {
+  for (const element of newElements) {
+    if ("boundElements" in element && element.boundElements) {
+      Object.assign(element, {
+        boundElements: element.boundElements.reduce(
+          (
+            acc: Mutable<NonNullable<ExcalidrawElement["boundElements"]>>,
+            binding,
+          ) => {
+            const newBindingId = oldIdToDuplicatedId.get(binding.id);
+            if (newBindingId) {
+              acc.push({ ...binding, id: newBindingId });
+            }
+            return acc;
+          },
+          [],
+        ),
      });
-      allBindableElementIds.add(oldIdToDuplicatedId.get(oldElement.id)!);
    }
-    if (isBindingElement(oldElement)) {
-      if (oldElement.startBinding != null) {
-        const { elementId } = oldElement.startBinding;
-        if (shouldReverseRoles && !oldIdToDuplicatedId.has(elementId)) {
-          allBindableElementIds.add(elementId);
-        }
-      }
-      if (oldElement.endBinding != null) {
-        const { elementId } = oldElement.endBinding;
-        if (shouldReverseRoles && !oldIdToDuplicatedId.has(elementId)) {
-          allBindableElementIds.add(elementId);
-        }
-      }
-      if (oldElement.startBinding != null || oldElement.endBinding != null) {
-        allBoundElementIds.add(oldIdToDuplicatedId.get(oldElement.id)!);
-      }
+
+    if ("containerId" in element && element.containerId) {
+      Object.assign(element, {
+        containerId: oldIdToDuplicatedId.get(element.containerId) ?? null,
+      });
    }
-  });

-  // Update the linear elements
-  (
-    sceneElements.filter(({ id }) =>
-      allBoundElementIds.has(id),
-    ) as ExcalidrawLinearElement[]
-  ).forEach((element) => {
-    const { startBinding, endBinding } = element;
-    mutateElement(element, {
-      startBinding: newBindingAfterDuplication(
-        startBinding,
-        oldIdToDuplicatedId,
-      ),
-      endBinding: newBindingAfterDuplication(endBinding, oldIdToDuplicatedId),
-    });
-  });
+    if ("endBinding" in element && element.endBinding) {
+      const newEndBindingId = oldIdToDuplicatedId.get(
+        element.endBinding.elementId,
+      );
+      Object.assign(element, {
+        endBinding: newEndBindingId
+          ? {
+              ...element.endBinding,
+              elementId: newEndBindingId,
+            }
+          : null,
+      });
+    }
+    if ("startBinding" in element && element.startBinding) {
+      const newEndBindingId = oldIdToDuplicatedId.get(
+        element.startBinding.elementId,
+      );
+      Object.assign(element, {
+        startBinding: newEndBindingId
+          ? {
+              ...element.startBinding,
+              elementId: newEndBindingId,
+            }
+          : null,
+      });
+    }

-  // Update the bindable shapes
-  sceneElements
-    .filter(({ id }) => allBindableElementIds.has(id))
-    .forEach((bindableElement) => {
-      const oldElementId = duplicateIdToOldId.get(bindableElement.id);
-      const boundElements = sceneElements.find(
-        ({ id }) => id === oldElementId,
-      )?.boundElements;
-
-      if (boundElements && boundElements.length > 0) {
-        mutateElement(bindableElement, {
-          boundElements: boundElements.map((boundElement) =>
-            oldIdToDuplicatedId.has(boundElement.id)
-              ? {
-                  id: oldIdToDuplicatedId.get(boundElement.id)!,
-                  type: boundElement.type,
-                }
-              : boundElement,
-          ),
-        });
-      }
-    });
-};
-
-const newBindingAfterDuplication = (
-  binding: PointBinding | null,
-  oldIdToDuplicatedId: Map<ExcalidrawElement["id"], ExcalidrawElement["id"]>,
-): PointBinding | null => {
-  if (binding == null) {
-    return null;
+    if (isElbowArrow(element)) {
+      Object.assign(
+        element,
+        updateElbowArrowPoints(element, duplicatedElementsMap, {
+          points: [
+            element.points[0],
+            element.points[element.points.length - 1],
+          ],
+        }),
+      );
+    }
  }
-  return {
-    ...binding,
-    elementId: oldIdToDuplicatedId.get(binding.elementId) ?? binding.elementId,
-  };
 };

 export const fixBindingsAfterDeletion = (
--- a/packages/excalidraw/element/duplicate.test.ts
+++ b/packages/excalidraw/element/duplicate.test.ts
@ -209,6 +209,7 @@ describe("duplicating multiple elements", () => {
        type: clonedText1.type,
      }),
    );
+    expect(clonedRectangle.type).toBe("rectangle");

    clonedArrows.forEach((arrow) => {
      expect(
@ -302,9 +303,9 @@ describe("duplicating multiple elements", () => {
    // -------------------------------------------------------------------------

    const origElements = [rectangle1, text1, arrow1, arrow2, arrow3] as const;
-    const clonedElements = duplicateElements(
+    const { newElements: clonedElements } = duplicateElements(
      origElements,
-    ) as any as typeof origElements;
+    ) as any as { newElements: typeof origElements };
    const [
      clonedRectangle,
      clonedText1,
@ -324,7 +325,7 @@ describe("duplicating multiple elements", () => {
      elementId: clonedRectangle.id,
    });
    expect(clonedArrow2.endBinding).toBe(null);
-
+    console.log(clonedArrow3);
    expect(clonedArrow3.startBinding).toBe(null);
    expect(clonedArrow3.endBinding).toEqual({
      ...arrow3.endBinding,
@ -348,9 +349,9 @@ describe("duplicating multiple elements", () => {
      });

      const origElements = [rectangle1, rectangle2, rectangle3] as const;
-      const clonedElements = duplicateElements(
+      const { newElements: clonedElements } = duplicateElements(
        origElements,
-      ) as any as typeof origElements;
+      ) as any as { newElements: typeof origElements };
      const [clonedRectangle1, clonedRectangle2, clonedRectangle3] =
        clonedElements;

--- a/packages/excalidraw/element/duplicate.ts
+++ b/packages/excalidraw/element/duplicate.ts
@ -28,17 +28,12 @@ import { bumpVersion } from "./mutateElement";
 import {
  hasBoundTextElement,
  isBoundToContainer,
-  isElbowArrow,
  isFrameLikeElement,
 } from "./typeChecks";

-import {
-  bindTextToShapeAfterDuplication,
-  getBoundTextElement,
-  getContainerElement,
-} from "./textElement";
+import { getBoundTextElement, getContainerElement } from "./textElement";

-import { updateElbowArrowPoints } from "./elbowArrow";
+import { fixBindingsAfterDuplication } from "./binding";

 import type { AppState } from "../types";
 import type { Mutable } from "../utility-types";
@ -315,83 +310,12 @@ export const duplicateElements = (

  // ---------------------------------------------------------------------------

-  const fixBindingsAfterDuplication = (
-    newElements: ExcalidrawElement[],
-    oldIdToDuplicatedId: Map<ExcalidrawElement["id"], ExcalidrawElement["id"]>,
-    duplicatedElementsMap: NonDeletedSceneElementsMap,
-  ) => {
-    for (const element of newElements) {
-      if ("boundElements" in element && element.boundElements) {
-        Object.assign(element, {
-          boundElements: element.boundElements.reduce(
-            (
-              acc: Mutable<NonNullable<ExcalidrawElement["boundElements"]>>,
-              binding,
-            ) => {
-              const newBindingId = oldIdToDuplicatedId.get(binding.id);
-              if (newBindingId) {
-                acc.push({ ...binding, id: newBindingId });
-              }
-              return acc;
-            },
-            [],
-          ),
-        });
-      }
-
-      if ("endBinding" in element && element.endBinding) {
-        const newEndBindingId = oldIdToDuplicatedId.get(
-          element.endBinding.elementId,
-        );
-        Object.assign(element, {
-          endBinding: newEndBindingId
-            ? {
-                ...element.endBinding,
-                elementId: newEndBindingId,
-              }
-            : null,
-        });
-      }
-      if ("startBinding" in element && element.startBinding) {
-        const newEndBindingId = oldIdToDuplicatedId.get(
-          element.startBinding.elementId,
-        );
-        Object.assign(element, {
-          startBinding: newEndBindingId
-            ? {
-                ...element.startBinding,
-                elementId: newEndBindingId,
-              }
-            : null,
-        });
-      }
-
-      if (isElbowArrow(element)) {
-        Object.assign(
-          element,
-          updateElbowArrowPoints(element, duplicatedElementsMap, {
-            points: [
-              element.points[0],
-              element.points[element.points.length - 1],
-            ],
-          }),
-        );
-      }
-    }
-  };
-
  fixBindingsAfterDuplication(
    newElements,
    oldIdToDuplicatedId,
    duplicatedElementsMap as NonDeletedSceneElementsMap,
  );

-  bindTextToShapeAfterDuplication(
-    elementsWithClones,
-    oldElements,
-    oldIdToDuplicatedId,
-  );
-
  bindElementsToFramesAfterDuplication(
    elementsWithClones,
    oldElements,
--- a/packages/excalidraw/element/elbowArrow.test.tsx
+++ b/packages/excalidraw/element/elbowArrow.test.tsx
@ -358,7 +358,7 @@ describe("elbow arrow ui", () => {
    expect(arrow.endBinding).not.toBe(null);
  });

-  it("keeps arrow shape when only the bound arrow is duplicated", async () => {
+  it("changes arrow shape to unbind variant if only the connected elbow arrow is duplicated", async () => {
    UI.createElement("rectangle", {
      x: -150,
      y: -150,
@ -404,8 +404,8 @@ describe("elbow arrow ui", () => {
    expect(duplicatedArrow.elbowed).toBe(true);
    expect(duplicatedArrow.points).toEqual([
      [0, 0],
-      [45, 0],
-      [45, 200],
+      [0, 100],
+      [90, 100],
      [90, 200],
    ]);
  });
--- a/packages/excalidraw/element/textElement.ts
+++ b/packages/excalidraw/element/textElement.ts
@ -6,7 +6,7 @@ import {
  TEXT_ALIGN,
  VERTICAL_ALIGN,
 } from "../constants";
-import { getFontString, arrayToMap } from "../utils";
+import { getFontString } from "../utils";

 import {
  resetOriginalContainerCache,
@ -112,48 +112,6 @@ export const redrawTextBoundingBox = (
  mutateElement(textElement, boundTextUpdates, informMutation);
 };

-export const bindTextToShapeAfterDuplication = (
-  newElements: ExcalidrawElement[],
-  oldElements: ExcalidrawElement[],
-  oldIdToDuplicatedId: Map<ExcalidrawElement["id"], ExcalidrawElement["id"]>,
-): void => {
-  const newElementsMap = arrayToMap(newElements) as Map<
-    ExcalidrawElement["id"],
-    ExcalidrawElement
-  >;
-  oldElements.forEach((element) => {
-    const newElementId = oldIdToDuplicatedId.get(element.id) as string;
-    const boundTextElementId = getBoundTextElementId(element);
-
-    if (boundTextElementId) {
-      const newTextElementId = oldIdToDuplicatedId.get(boundTextElementId);
-      if (newTextElementId) {
-        const newContainer = newElementsMap.get(newElementId);
-        if (newContainer) {
-          mutateElement(newContainer, {
-            boundElements: (element.boundElements || [])
-              .filter(
-                (boundElement) =>
-                  boundElement.id !== newTextElementId &&
-                  boundElement.id !== boundTextElementId,
-              )
-              .concat({
-                type: "text",
-                id: newTextElementId,
-              }),
-          });
-        }
-        const newTextElement = newElementsMap.get(newTextElementId);
-        if (newTextElement && isTextElement(newTextElement)) {
-          mutateElement(newTextElement, {
-            containerId: newContainer ? newElementId : null,
-          });
-        }
-      }
-    }
-  });
-};
-
 export const handleBindTextResize = (
  container: NonDeletedExcalidrawElement,
  elementsMap: ElementsMap,
--- a/packages/excalidraw/tests/snapshots/history.test.tsx.snap
+++ b/packages/excalidraw/tests/snapshots/history.test.tsx.snap
@ -924,11 +924,12 @@ History {
                ],
              ],
              "startBinding": null,
+              "y": 0,
            },
            "inserted": {
              "endBinding": {
                "elementId": "id166",
-                "focus": -0,
+                "focus": "0.00000",
                "gap": 1,
              },
              "points": [
@ -943,9 +944,10 @@ History {
              ],
              "startBinding": {
                "elementId": "id165",
-                "focus": 0,
+                "focus": "-0.00000",
                "gap": 1,
              },
+              "y": "0.00000",
            },
          },
        },
--- a/packages/excalidraw/tests/snapshots/move.test.tsx.snap
+++ b/packages/excalidraw/tests/snapshots/move.test.tsx.snap
@ -1,5 +1,73 @@
 // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html

+exports[`duplicate element on move when ALT is clicked > rectangle 5`] = `
+{
+  "angle": 0,
+  "backgroundColor": "transparent",
+  "boundElements": null,
+  "customData": undefined,
+  "fillStyle": "solid",
+  "frameId": null,
+  "groupIds": [],
+  "height": 50,
+  "id": "id0",
+  "index": "a0",
+  "isDeleted": false,
+  "link": null,
+  "locked": false,
+  "opacity": 100,
+  "roughness": 1,
+  "roundness": {
+    "type": 3,
+  },
+  "seed": 1278240551,
+  "strokeColor": "#1e1e1e",
+  "strokeStyle": "solid",
+  "strokeWidth": 2,
+  "type": "rectangle",
+  "updated": 1,
+  "version": 5,
+  "versionNonce": 23633383,
+  "width": 30,
+  "x": -10,
+  "y": 60,
+}
+`;
+
+exports[`duplicate element on move when ALT is clicked > rectangle 6`] = `
+{
+  "angle": 0,
+  "backgroundColor": "transparent",
+  "boundElements": null,
+  "customData": undefined,
+  "fillStyle": "solid",
+  "frameId": null,
+  "groupIds": [],
+  "height": 50,
+  "id": "id2",
+  "index": "a1",
+  "isDeleted": false,
+  "link": null,
+  "locked": false,
+  "opacity": 100,
+  "roughness": 1,
+  "roundness": {
+    "type": 3,
+  },
+  "seed": 238820263,
+  "strokeColor": "#1e1e1e",
+  "strokeStyle": "solid",
+  "strokeWidth": 2,
+  "type": "rectangle",
+  "updated": 1,
+  "version": 6,
+  "versionNonce": 1604849351,
+  "width": 30,
+  "x": 30,
+  "y": 20,
+}
+`;
+
 exports[`move element > rectangle 5`] = `
 {
  "angle": 0,
--- a/packages/excalidraw/tests/snapshots/regressionTests.test.tsx.snap
+++ b/packages/excalidraw/tests/snapshots/regressionTests.test.tsx.snap
@ -2153,8 +2153,8 @@ History {
              "strokeWidth": 2,
              "type": "rectangle",
              "width": 10,
-              "x": 20,
-              "y": 20,
+              "x": 10,
+              "y": 10,
            },
            "inserted": {
              "isDeleted": true,
@ -10643,8 +10643,8 @@ History {
              "strokeWidth": 2,
              "type": "rectangle",
              "width": 10,
-              "x": 20,
-              "y": 20,
+              "x": 10,
+              "y": 10,
            },
            "inserted": {
              "isDeleted": true,
@ -10676,8 +10676,8 @@ History {
              "strokeWidth": 2,
              "type": "rectangle",
              "width": 10,
-              "x": 40,
-              "y": 20,
+              "x": 30,
+              "y": 10,
            },
            "inserted": {
              "isDeleted": true,
@ -10709,8 +10709,8 @@ History {
              "strokeWidth": 2,
              "type": "rectangle",
              "width": 10,
-              "x": 60,
-              "y": 20,
+              "x": 50,
+              "y": 10,
            },
            "inserted": {
              "isDeleted": true,
--- a/packages/excalidraw/tests/move.test.tsx
+++ b/packages/excalidraw/tests/move.test.tsx
@ -174,9 +174,9 @@ describe("duplicate element on move when ALT is clicked", () => {
    expect(h.state.selectionElement).toBeNull();
    expect(h.elements.length).toEqual(2);

-    // previous element should stay intact
-    expect([h.elements[0].x, h.elements[0].y]).toEqual([30, 20]);
-    expect([h.elements[1].x, h.elements[1].y]).toEqual([-10, 60]);
+    // behavior should be the same as Ctrl+D
+    expect([h.elements[0].x, h.elements[0].y]).toEqual([-10, 60]);
+    expect([h.elements[1].x, h.elements[1].y]).toEqual([30, 20]);

    h.elements.forEach((element) => expect(element).toMatchSnapshot());
  });
--- a/packages/excalidraw/tests/resize.test.tsx
+++ b/packages/excalidraw/tests/resize.test.tsx
@ -505,12 +505,12 @@ describe("arrow element", () => {
      h.state,
    )[0] as ExcalidrawElbowArrowElement;

-    expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(1);
+    expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(1, 0);
    expect(arrow.startBinding?.fixedPoint?.[1]).toBeCloseTo(0.75);

    UI.resize(rectangle, "se", [-200, -150]);

-    expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(1);
+    expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(1, 0);
    expect(arrow.startBinding?.fixedPoint?.[1]).toBeCloseTo(0.75);
  });

@ -533,11 +533,11 @@ describe("arrow element", () => {
      h.state,
    )[0] as ExcalidrawElbowArrowElement;

-    expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(1);
+    expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(1, 0);
    expect(arrow.startBinding?.fixedPoint?.[1]).toBeCloseTo(0.75);

    UI.resize([rectangle, arrow], "nw", [300, 350]);
-    expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(0);
+    expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(0, 0);
    expect(arrow.startBinding?.fixedPoint?.[1]).toBeCloseTo(0.25);
  });
 });