fix: always re-generate index of defined moved elements (#8040)

2025-10-31 10:54:33 +01:00 · 2024-05-20 22:23:42 +01:00
parent 2f9526da24
commit eddbe55f50
5 changed files with 151 additions and 125 deletions
--- a/packages/excalidraw/change.ts
+++ b/packages/excalidraw/change.ts
@@ -1477,19 +1477,28 @@ export class ElementsChange implements Change<SceneElementsMap> {
      return elements;
    }

-    const previous = Array.from(elements.values());
-    const reordered = orderByFractionalIndex([...previous]);
+    const unordered = Array.from(elements.values());
+    const ordered = orderByFractionalIndex([...unordered]);
+    const moved = Delta.getRightDifferences(unordered, ordered, true).reduce(
+      (acc, arrayIndex) => {
+        const candidate = unordered[Number(arrayIndex)];
+        if (candidate && changed.has(candidate.id)) {
+          acc.set(candidate.id, candidate);
+        }

-    if (
-      !flags.containsVisibleDifference &&
-      Delta.isRightDifferent(previous, reordered, true)
-    ) {
+        return acc;
+      },
+      new Map(),
+    );
+
+    if (!flags.containsVisibleDifference && moved.size) {
      // we found a difference in order!
      flags.containsVisibleDifference = true;
    }

-    // let's synchronize all invalid indices of moved elements
-    return arrayToMap(syncMovedIndices(reordered, changed)) as typeof elements;
+    // synchronize all elements that were actually moved
+    // could fallback to synchronizing all invalid indices
+    return arrayToMap(syncMovedIndices(ordered, moved)) as typeof elements;
  }

  /**
--- a/packages/excalidraw/fractionalIndex.ts
+++ b/packages/excalidraw/fractionalIndex.ts
@@ -133,27 +133,11 @@ const getMovedIndicesGroups = (
  let i = 0;

  while (i < elements.length) {
-    if (
-      movedElements.has(elements[i].id) &&
-      !isValidFractionalIndex(
-        elements[i]?.index,
-        elements[i - 1]?.index,
-        elements[i + 1]?.index,
-      )
-    ) {
+    if (movedElements.has(elements[i].id)) {
      const indicesGroup = [i - 1, i]; // push the lower bound index as the first item

      while (++i < elements.length) {
-        if (
-          !(
-            movedElements.has(elements[i].id) &&
-            !isValidFractionalIndex(
-              elements[i]?.index,
-              elements[i - 1]?.index,
-              elements[i + 1]?.index,
-            )
-          )
-        ) {
+        if (!movedElements.has(elements[i].id)) {
          break;
        }

--- a/packages/excalidraw/tests/snapshots/history.test.tsx.snap
+++ b/packages/excalidraw/tests/snapshots/history.test.tsx.snap
@@ -17320,7 +17320,7 @@ exports[`history > singleplayer undo/redo > should support changes in elements'
  "strokeWidth": 2,
  "type": "rectangle",
  "updated": 1,
-  "version": 6,
+  "version": 7,
  "width": 10,
  "x": 10,
  "y": 0,
@@ -17352,7 +17352,7 @@ exports[`history > singleplayer undo/redo > should support changes in elements'
  "strokeWidth": 2,
  "type": "rectangle",
  "updated": 1,
-  "version": 9,
+  "version": 10,
  "width": 10,
  "x": 40,
  "y": 40,
@@ -17604,7 +17604,7 @@ History {
              "index": "a2",
            },
            "inserted": {
-              "index": "a0",
+              "index": "Zz",
            },
          },
          "id41" => Delta {
@@ -17612,7 +17612,7 @@ History {
              "index": "a3",
            },
            "inserted": {
-              "index": "a0V",
+              "index": "a0",
            },
          },
        },
--- a/packages/excalidraw/tests/snapshots/regressionTests.test.tsx.snap
+++ b/packages/excalidraw/tests/snapshots/regressionTests.test.tsx.snap
@@ -336,7 +336,7 @@ History {
              "groupIds": [
                "id5",
              ],
-              "index": "a1V",
+              "index": "a2",
            },
            "inserted": {
              "groupIds": [],
@@ -348,9 +348,11 @@ History {
              "groupIds": [
                "id5",
              ],
+              "index": "a3",
            },
            "inserted": {
              "groupIds": [],
+              "index": "a2",
            },
          },
        },
@@ -719,7 +721,7 @@ History {
              "groupIds": [
                "id4",
              ],
-              "index": "a1V",
+              "index": "a2",
            },
            "inserted": {
              "groupIds": [],
@@ -731,9 +733,11 @@ History {
              "groupIds": [
                "id4",
              ],
+              "index": "a3",
            },
            "inserted": {
              "groupIds": [],
+              "index": "a2",
            },
          },
        },
@@ -1856,7 +1860,7 @@ History {
              "groupIds": [
                "id5",
              ],
-              "index": "a1V",
+              "index": "a2",
            },
            "inserted": {
              "groupIds": [],
@@ -1868,9 +1872,11 @@ History {
              "groupIds": [
                "id5",
              ],
+              "index": "a3",
            },
            "inserted": {
              "groupIds": [],
+              "index": "a2",
            },
          },
        },
@@ -12810,7 +12816,7 @@ History {
                "id5",
                "id3",
              ],
-              "index": "a1V",
+              "index": "a2",
            },
            "inserted": {
              "groupIds": [
@@ -12825,11 +12831,13 @@ History {
                "id5",
                "id3",
              ],
+              "index": "a3",
            },
            "inserted": {
              "groupIds": [
                "id3",
              ],
+              "index": "a2",
            },
          },
        },
--- a/packages/excalidraw/tests/fractionalIndex.test.ts
+++ b/packages/excalidraw/tests/fractionalIndex.test.ts
@@ -77,97 +77,6 @@ describe("sync invalid indices with array order", () => {
    });
  });

-  describe("should NOT sync index when it is already valid", () => {
-    testMovedIndicesSync({
-      elements: [
-        { id: "A", index: "a2" },
-        { id: "B", index: "a4" },
-      ],
-      movedElements: ["A"],
-      expect: {
-        validInput: true,
-        unchangedElements: ["A", "B"],
-      },
-    });
-
-    testMovedIndicesSync({
-      elements: [
-        { id: "A", index: "a2" },
-        { id: "B", index: "a4" },
-      ],
-      movedElements: ["B"],
-      expect: {
-        validInput: true,
-        unchangedElements: ["A", "B"],
-      },
-    });
-  });
-
-  describe("should NOT sync indices when they are already valid", () => {
-    {
-      testMovedIndicesSync({
-        elements: [
-          { id: "A", index: "a1" },
-          { id: "B", index: "a0" },
-          { id: "C", index: "a2" },
-        ],
-        movedElements: ["B", "C"],
-        expect: {
-          // this should not sync 'C'
-          unchangedElements: ["A", "C"],
-        },
-      });
-
-      testMovedIndicesSync({
-        elements: [
-          { id: "A", index: "a1" },
-          { id: "B", index: "a0" },
-          { id: "C", index: "a2" },
-        ],
-        movedElements: ["A", "B"],
-        expect: {
-          // but this should sync 'A' as it's invalid!
-          unchangedElements: ["C"],
-        },
-      });
-    }
-
-    testMovedIndicesSync({
-      elements: [
-        { id: "A", index: "a0" },
-        { id: "B", index: "a2" },
-        { id: "C", index: "a1" },
-        { id: "D", index: "a1" },
-        { id: "E", index: "a2" },
-      ],
-      movedElements: ["B", "D", "E"],
-      expect: {
-        // should not sync 'E'
-        unchangedElements: ["A", "C", "E"],
-      },
-    });
-
-    testMovedIndicesSync({
-      elements: [
-        { id: "A" },
-        { id: "B" },
-        { id: "C", index: "a0" },
-        { id: "D", index: "a2" },
-        { id: "E" },
-        { id: "F", index: "a3" },
-        { id: "G" },
-        { id: "H", index: "a1" },
-        { id: "I", index: "a2" },
-        { id: "J" },
-      ],
-      movedElements: ["A", "B", "D", "E", "F", "G", "J"],
-      expect: {
-        // should not sync 'D' and 'F'
-        unchangedElements: ["C", "D", "F"],
-      },
-    });
-  });
-
  describe("should sync when fractional index is not defined", () => {
    testMovedIndicesSync({
      elements: [{ id: "A" }],
@@ -384,6 +293,122 @@ describe("sync invalid indices with array order", () => {
    });
  });

+  describe("should sync all moved elements regardless of their validity", () => {
+    testMovedIndicesSync({
+      elements: [
+        { id: "A", index: "a2" },
+        { id: "B", index: "a4" },
+      ],
+      movedElements: ["A"],
+      expect: {
+        validInput: true,
+        unchangedElements: ["B"],
+      },
+    });
+
+    testMovedIndicesSync({
+      elements: [
+        { id: "A", index: "a2" },
+        { id: "B", index: "a4" },
+      ],
+      movedElements: ["B"],
+      expect: {
+        validInput: true,
+        unchangedElements: ["A"],
+      },
+    });
+
+    testMovedIndicesSync({
+      elements: [
+        { id: "C", index: "a2" },
+        { id: "D", index: "a3" },
+        { id: "A", index: "a0" },
+        { id: "B", index: "a1" },
+      ],
+      movedElements: ["C", "D"],
+      expect: {
+        unchangedElements: ["A", "B"],
+      },
+    });
+
+    testMovedIndicesSync({
+      elements: [
+        { id: "A", index: "a1" },
+        { id: "B", index: "a2" },
+        { id: "D", index: "a4" },
+        { id: "C", index: "a3" },
+        { id: "F", index: "a6" },
+        { id: "E", index: "a5" },
+        { id: "H", index: "a8" },
+        { id: "G", index: "a7" },
+        { id: "I", index: "a9" },
+      ],
+      movedElements: ["D", "F", "H"],
+      expect: {
+        unchangedElements: ["A", "B", "C", "E", "G", "I"],
+      },
+    });
+
+    {
+      testMovedIndicesSync({
+        elements: [
+          { id: "A", index: "a1" },
+          { id: "B", index: "a0" },
+          { id: "C", index: "a2" },
+        ],
+        movedElements: ["B", "C"],
+        expect: {
+          unchangedElements: ["A"],
+        },
+      });
+
+      testMovedIndicesSync({
+        elements: [
+          { id: "A", index: "a1" },
+          { id: "B", index: "a0" },
+          { id: "C", index: "a2" },
+        ],
+        movedElements: ["A", "B"],
+        expect: {
+          unchangedElements: ["C"],
+        },
+      });
+    }
+
+    testMovedIndicesSync({
+      elements: [
+        { id: "A", index: "a0" },
+        { id: "B", index: "a2" },
+        { id: "C", index: "a1" },
+        { id: "D", index: "a1" },
+        { id: "E", index: "a2" },
+      ],
+      movedElements: ["B", "D", "E"],
+      expect: {
+        unchangedElements: ["A", "C"],
+      },
+    });
+
+    testMovedIndicesSync({
+      elements: [
+        { id: "A" },
+        { id: "B" },
+        { id: "C", index: "a0" },
+        { id: "D", index: "a2" },
+        { id: "E" },
+        { id: "F", index: "a3" },
+        { id: "G" },
+        { id: "H", index: "a1" },
+        { id: "I", index: "a2" },
+        { id: "J" },
+      ],
+      movedElements: ["A", "B", "D", "E", "F", "G", "J"],
+      expect: {
+        unchangedElements: ["C", "H", "I"],
+      },
+    });
+  });
+
  describe("should generate fractions for explicitly moved elements", () => {
    describe("should generate a fraction between 'A' and 'C'", () => {
      testMovedIndicesSync({