From f640ddc2aa1320e674c7d8aed06e78572bf61213 Mon Sep 17 00:00:00 2001 From: David Luzar Date: Sun, 16 Apr 2023 17:22:16 +0200 Subject: [PATCH] fix: incorrectly duplicating items on paste/library insert (#6467 * fix: incorrectly duplicating items on paste/library insert * fix: deduplicate element ids on restore * tests --- src/components/App.tsx | 35 +++---- src/data/restore.ts | 7 ++ src/element/newElement.ts | 43 +++++---- .../regressionTests.test.tsx.snap | 12 +-- src/tests/helpers/api.ts | 5 +- src/tests/library.test.tsx | 94 +++++++++++++++++++ 6 files changed, 153 insertions(+), 43 deletions(-) diff --git a/src/components/App.tsx b/src/components/App.tsx index 1ebdc771..be556199 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -127,7 +127,11 @@ import { } from "../element/binding"; import { LinearElementEditor } from "../element/linearElementEditor"; import { mutateElement, newElementWith } from "../element/mutateElement"; -import { deepCopyElement, newFreeDrawElement } from "../element/newElement"; +import { + deepCopyElement, + duplicateElements, + newFreeDrawElement, +} from "../element/newElement"; import { hasBoundTextElement, isArrowElement, @@ -1625,35 +1629,22 @@ class App extends React.Component { const dx = x - elementsCenterX; const dy = y - elementsCenterY; - const groupIdMap = new Map(); const [gridX, gridY] = getGridPoint(dx, dy, this.state.gridSize); - const oldIdToDuplicatedId = new Map(); - const newElements = elements.map((element) => { - const newElement = duplicateElement( - this.state.editingGroupId, - groupIdMap, - element, - { + const newElements = duplicateElements( + elements.map((element) => { + return newElementWith(element, { x: element.x + gridX - minX, y: element.y + gridY - minY, - }, - ); - oldIdToDuplicatedId.set(element.id, newElement.id); - return newElement; - }); + }); + }), + ); - bindTextToShapeAfterDuplication(newElements, elements, oldIdToDuplicatedId); const nextElements = [ ...this.scene.getElementsIncludingDeleted(), ...newElements, ]; - fixBindingsAfterDuplication(nextElements, elements, oldIdToDuplicatedId); - - if (opts.files) { - this.files = { ...this.files, ...opts.files }; - } this.scene.replaceAllElements(nextElements); @@ -1664,6 +1655,10 @@ class App extends React.Component { } }); + if (opts.files) { + this.files = { ...this.files, ...opts.files }; + } + this.history.resumeRecording(); this.setState( diff --git a/src/data/restore.ts b/src/data/restore.ts index 2735d91d..fcf5fa13 100644 --- a/src/data/restore.ts +++ b/src/data/restore.ts @@ -369,6 +369,9 @@ export const restoreElements = ( localElements: readonly ExcalidrawElement[] | null | undefined, opts?: { refreshDimensions?: boolean; repairBindings?: boolean } | undefined, ): ExcalidrawElement[] => { + // used to detect duplicate top-level element ids + const existingIds = new Set(); + const localElementsMap = localElements ? arrayToMap(localElements) : null; const restoredElements = (elements || []).reduce((elements, element) => { // filtering out selection, which is legacy, no longer kept in elements, @@ -383,6 +386,10 @@ export const restoreElements = ( if (localElement && localElement.version > migratedElement.version) { migratedElement = bumpVersion(migratedElement, localElement.version); } + if (existingIds.has(migratedElement.id)) { + migratedElement = { ...migratedElement, id: randomId() }; + } + existingIds.add(migratedElement.id); elements.push(migratedElement); } } diff --git a/src/element/newElement.ts b/src/element/newElement.ts index 72aa5468..e3b25e84 100644 --- a/src/element/newElement.ts +++ b/src/element/newElement.ts @@ -439,6 +439,29 @@ export const deepCopyElement = ( return _deepCopyElement(val); }; +/** + * utility wrapper to generate new id. In test env it reuses the old + postfix + * for test assertions. + */ +const regenerateId = ( + /** supply null if no previous id exists */ + previousId: string | null, +) => { + if (isTestEnv() && previousId) { + let nextId = `${previousId}_copy`; + // `window.h` may not be defined in some unit tests + if ( + window.h?.app + ?.getSceneElementsIncludingDeleted() + .find((el) => el.id === nextId) + ) { + nextId += "_copy"; + } + return nextId; + } + return randomId(); +}; + /** * Duplicate an element, often used in the alt-drag operation. * Note that this method has gotten a bit complicated since the @@ -461,19 +484,7 @@ export const duplicateElement = ( ): Readonly => { let copy = deepCopyElement(element); - if (isTestEnv()) { - copy.id = `${copy.id}_copy`; - // `window.h` may not be defined in some unit tests - if ( - window.h?.app - ?.getSceneElementsIncludingDeleted() - .find((el) => el.id === copy.id) - ) { - copy.id += "_copy"; - } - } else { - copy.id = randomId(); - } + copy.id = regenerateId(copy.id); copy.boundElements = null; copy.updated = getUpdatedTimestamp(); copy.seed = randomInteger(); @@ -482,7 +493,7 @@ export const duplicateElement = ( editingGroupId, (groupId) => { if (!groupIdMapForOperation.has(groupId)) { - groupIdMapForOperation.set(groupId, randomId()); + groupIdMapForOperation.set(groupId, regenerateId(groupId)); } return groupIdMapForOperation.get(groupId)!; }, @@ -520,7 +531,7 @@ export const duplicateElements = (elements: readonly ExcalidrawElement[]) => { // if we haven't migrated the element id, but an old element with the same // id exists, generate a new id for it and return it if (origElementsMap.has(id)) { - const newId = randomId(); + const newId = regenerateId(id); elementNewIdsMap.set(id, newId); return newId; } @@ -538,7 +549,7 @@ export const duplicateElements = (elements: readonly ExcalidrawElement[]) => { if (clonedElement.groupIds) { clonedElement.groupIds = clonedElement.groupIds.map((groupId) => { if (!groupNewIdsMap.has(groupId)) { - groupNewIdsMap.set(groupId, randomId()); + groupNewIdsMap.set(groupId, regenerateId(groupId)); } return groupNewIdsMap.get(groupId)!; }); diff --git a/src/tests/__snapshots__/regressionTests.test.tsx.snap b/src/tests/__snapshots__/regressionTests.test.tsx.snap index 35f9eb7c..967a0cf6 100644 --- a/src/tests/__snapshots__/regressionTests.test.tsx.snap +++ b/src/tests/__snapshots__/regressionTests.test.tsx.snap @@ -13431,7 +13431,7 @@ Object { "boundElements": null, "fillStyle": "hachure", "groupIds": Array [ - "id6", + "id4_copy", ], "height": 10, "id": "id0_copy", @@ -13464,7 +13464,7 @@ Object { "boundElements": null, "fillStyle": "hachure", "groupIds": Array [ - "id6", + "id4_copy", ], "height": 10, "id": "id1_copy", @@ -13497,7 +13497,7 @@ Object { "boundElements": null, "fillStyle": "hachure", "groupIds": Array [ - "id6", + "id4_copy", ], "height": 10, "id": "id2_copy", @@ -13981,7 +13981,7 @@ Object { "boundElements": null, "fillStyle": "hachure", "groupIds": Array [ - "id6", + "id4_copy", ], "height": 10, "id": "id0_copy", @@ -14011,7 +14011,7 @@ Object { "boundElements": null, "fillStyle": "hachure", "groupIds": Array [ - "id6", + "id4_copy", ], "height": 10, "id": "id1_copy", @@ -14041,7 +14041,7 @@ Object { "boundElements": null, "fillStyle": "hachure", "groupIds": Array [ - "id6", + "id4_copy", ], "height": 10, "id": "id2_copy", diff --git a/src/tests/helpers/api.ts b/src/tests/helpers/api.ts index bc8bfc8a..705180d6 100644 --- a/src/tests/helpers/api.ts +++ b/src/tests/helpers/api.ts @@ -211,7 +211,10 @@ export class API { type, startArrowhead: null, endArrowhead: null, - points: rest.points ?? [], + points: rest.points ?? [ + [0, 0], + [100, 100], + ], }); break; case "image": diff --git a/src/tests/library.test.tsx b/src/tests/library.test.tsx index 6b3ff5dd..86847aee 100644 --- a/src/tests/library.test.tsx +++ b/src/tests/library.test.tsx @@ -72,6 +72,100 @@ describe("library", () => { }); }); + it("should regenerate ids but retain bindings on library insert", async () => { + const rectangle = API.createElement({ + id: "rectangle1", + type: "rectangle", + boundElements: [ + { type: "text", id: "text1" }, + { type: "arrow", id: "arrow1" }, + ], + }); + const text = API.createElement({ + id: "text1", + type: "text", + text: "ola", + containerId: "rectangle1", + }); + const arrow = API.createElement({ + id: "arrow1", + type: "arrow", + endBinding: { elementId: "rectangle1", focus: -1, gap: 0 }, + }); + + await API.drop( + new Blob( + [ + serializeLibraryAsJSON([ + { + id: "item1", + status: "published", + elements: [rectangle, text, arrow], + created: 1, + }, + ]), + ], + { + type: MIME_TYPES.excalidrawlib, + }, + ), + ); + + await waitFor(() => { + expect(h.elements).toEqual([ + expect.objectContaining({ + id: "rectangle1_copy", + boundElements: expect.arrayContaining([ + { type: "text", id: "text1_copy" }, + { type: "arrow", id: "arrow1_copy" }, + ]), + }), + expect.objectContaining({ + id: "text1_copy", + containerId: "rectangle1_copy", + }), + expect.objectContaining({ + id: "arrow1_copy", + endBinding: expect.objectContaining({ elementId: "rectangle1_copy" }), + }), + ]); + }); + }); + + it("should fix duplicate ids between items on insert", async () => { + // note, we're not testing for duplicate group ids and such because + // deduplication of that happens upstream in the library component + // which would be very hard to orchestrate in this test + + const elem1 = API.createElement({ + id: "elem1", + type: "rectangle", + }); + const item1: LibraryItem = { + id: "item1", + status: "published", + elements: [elem1], + created: 1, + }; + + await API.drop( + new Blob([serializeLibraryAsJSON([item1, item1])], { + type: MIME_TYPES.excalidrawlib, + }), + ); + + await waitFor(() => { + expect(h.elements).toEqual([ + expect.objectContaining({ + id: "elem1_copy", + }), + expect.objectContaining({ + id: expect.not.stringMatching(/^(elem1_copy|elem1)$/), + }), + ]); + }); + }); + it("inserting library item should revert to selection tool", async () => { UI.clickTool("rectangle"); expect(h.elements).toEqual([]);