From 13a40d1b9c9f9e232b6a2b0aa7ef5fcc501bd37b Mon Sep 17 00:00:00 2001 From: Steve Ruiz Date: Sat, 4 Sep 2021 22:12:07 +0100 Subject: [PATCH] Fix binding bugs --- .../command/delete/delete.command.spec.ts | 2 + .../duplicate/duplicate.command.spec.ts | 2 + .../command/duplicate/duplicate.command.ts | 76 ++++++++++--------- .../move-to-page/move-to-page.command.spec.ts | 2 + .../command/utils/removeShapesFromPage.ts | 71 +++++++++-------- .../sessions/translate/translate.session.ts | 36 ++++----- 6 files changed, 103 insertions(+), 86 deletions(-) diff --git a/packages/tldraw/src/state/command/delete/delete.command.spec.ts b/packages/tldraw/src/state/command/delete/delete.command.spec.ts index 0051d0393..eb9fa85ca 100644 --- a/packages/tldraw/src/state/command/delete/delete.command.spec.ts +++ b/packages/tldraw/src/state/command/delete/delete.command.spec.ts @@ -112,4 +112,6 @@ describe('Delete command', () => { expect(tlstate.getShape('newGroup')).toBeUndefined() }) }) + + it.todo('Does not delete uneffected bindings.') }) diff --git a/packages/tldraw/src/state/command/duplicate/duplicate.command.spec.ts b/packages/tldraw/src/state/command/duplicate/duplicate.command.spec.ts index 12f2b1a1c..1d7771ce6 100644 --- a/packages/tldraw/src/state/command/duplicate/duplicate.command.spec.ts +++ b/packages/tldraw/src/state/command/duplicate/duplicate.command.spec.ts @@ -158,4 +158,6 @@ describe('Duplicate command', () => { expect(Object.keys(tlstate.page.shapes).length).toBe(beforeShapeIds.length + 1) }) }) + + it.todo('Does not delete uneffected bindings.') }) diff --git a/packages/tldraw/src/state/command/duplicate/duplicate.command.ts b/packages/tldraw/src/state/command/duplicate/duplicate.command.ts index ffc6ccf5e..db45ec07e 100644 --- a/packages/tldraw/src/state/command/duplicate/duplicate.command.ts +++ b/packages/tldraw/src/state/command/duplicate/duplicate.command.ts @@ -83,46 +83,48 @@ export function duplicate(data: Data, ids: string[]): TLDrawCommand { }) // Which ids did we end up duplicating? - const duplicatedShapeIds = Object.keys(duplicateMap) + const dupedShapeIds = new Set(Object.keys(duplicateMap)) // Handle bindings that effect duplicated shapes - Object.values(page.bindings).forEach((binding) => { - if (duplicatedShapeIds.includes(binding.fromId)) { - if (duplicatedShapeIds.includes(binding.toId)) { - // If the binding is between two duplicating shapes then - // duplicate the binding, too - const duplicatedBindingId = Utils.uniqueId() + Object.values(page.bindings) + .filter((binding) => dupedShapeIds.has(binding.fromId) || dupedShapeIds.has(binding.toId)) + .forEach((binding) => { + if (dupedShapeIds.has(binding.fromId)) { + if (dupedShapeIds.has(binding.toId)) { + // If the binding is between two duplicating shapes then + // duplicate the binding, too + const duplicatedBindingId = Utils.uniqueId() - const duplicatedBinding = { - ...Utils.deepClone(binding), - id: duplicatedBindingId, - fromId: duplicateMap[binding.fromId], - toId: duplicateMap[binding.toId], + const duplicatedBinding = { + ...Utils.deepClone(binding), + id: duplicatedBindingId, + fromId: duplicateMap[binding.fromId], + toId: duplicateMap[binding.toId], + } + + before.bindings[duplicatedBindingId] = undefined + after.bindings[duplicatedBindingId] = duplicatedBinding + + // Change the duplicated shape's handle so that it reference + // the duplicated binding + const boundShape = after.shapes[duplicatedBinding.fromId] + Object.values(boundShape!.handles!).forEach((handle) => { + if (handle!.bindingId === binding.id) { + handle!.bindingId = duplicatedBindingId + } + }) + } else { + // If only the fromId is selected, delete the binding on + // the duplicated shape's handles + const boundShape = after.shapes[duplicateMap[binding.fromId]] + Object.values(boundShape!.handles!).forEach((handle) => { + if (handle!.bindingId === binding.id) { + handle!.bindingId = undefined + } + }) } - - before.bindings[duplicatedBindingId] = undefined - after.bindings[duplicatedBindingId] = duplicatedBinding - - // Change the duplicated shape's handle so that it reference - // the duplicated binding - const boundShape = after.shapes[duplicatedBinding.fromId] - Object.values(boundShape!.handles!).forEach((handle) => { - if (handle!.bindingId === binding.id) { - handle!.bindingId = duplicatedBindingId - } - }) - } else { - // If only the fromId is selected, delete the binding on - // the duplicated shape's handles - const boundShape = after.shapes[duplicateMap[binding.fromId]] - Object.values(boundShape!.handles!).forEach((handle) => { - if (handle!.bindingId === binding.id) { - handle!.bindingId = undefined - } - }) } - } - }) + }) return { id: 'duplicate', @@ -142,7 +144,9 @@ export function duplicate(data: Data, ids: string[]): TLDrawCommand { [currentPageId]: after, }, pageStates: { - [currentPageId]: { selectedIds: duplicatedShapeIds.map((id) => duplicateMap[id]) }, + [currentPageId]: { + selectedIds: Array.from(dupedShapeIds.values()).map((id) => duplicateMap[id]), + }, }, }, }, diff --git a/packages/tldraw/src/state/command/move-to-page/move-to-page.command.spec.ts b/packages/tldraw/src/state/command/move-to-page/move-to-page.command.spec.ts index 05dd9d995..696360be2 100644 --- a/packages/tldraw/src/state/command/move-to-page/move-to-page.command.spec.ts +++ b/packages/tldraw/src/state/command/move-to-page/move-to-page.command.spec.ts @@ -247,4 +247,6 @@ describe('Move to page command', () => { ]) }) }) + + it.todo('Does not delete uneffected bindings.') }) diff --git a/packages/tldraw/src/state/command/utils/removeShapesFromPage.ts b/packages/tldraw/src/state/command/utils/removeShapesFromPage.ts index 11eda540d..9cd049eea 100644 --- a/packages/tldraw/src/state/command/utils/removeShapesFromPage.ts +++ b/packages/tldraw/src/state/command/utils/removeShapesFromPage.ts @@ -52,44 +52,49 @@ export function removeShapesFromPage(data: Data, ids: string[], pageId: string) const page = TLDR.getPage(data, pageId) // We also need to delete bindings that reference the deleted shapes - Object.values(page.bindings).forEach((binding) => { - for (const id of [binding.toId, binding.fromId]) { - // If the binding references a deleted shape... - if (after.shapes[id] === undefined) { - // Delete this binding - before.bindings[binding.id] = binding - after.bindings[binding.id] = undefined + Object.values(page.bindings) + .filter((binding) => deletedIds.has(binding.fromId) || deletedIds.has(binding.toId)) + .forEach((binding) => { + for (const id of [binding.toId, binding.fromId]) { + // If the binding references a deleted shape... + if (after.shapes[id] === undefined) { + // Delete this binding + before.bindings[binding.id] = binding + after.bindings[binding.id] = undefined - // Let's also look each the bound shape... - const shape = TLDR.getShape(data, id, pageId) + // Let's also look each the bound shape... + const shape = TLDR.getShape(data, id, pageId) - // If the bound shape has a handle that references the deleted binding... - if (shape.handles) { - Object.values(shape.handles) - .filter((handle) => handle.bindingId === binding.id) - .forEach((handle) => { - // Save the binding reference in the before patch - before.shapes[id] = { - ...before.shapes[id], - handles: { - ...before.shapes[id]?.handles, - [handle.id]: { bindingId: binding.id }, - }, - } - - // Unless we're currently deleting the shape, remove the - // binding reference from the after patch - if (!deletedIds.has(id)) { - after.shapes[id] = { - ...after.shapes[id], - handles: { ...after.shapes[id]?.handles, [handle.id]: { bindingId: undefined } }, + // If the bound shape has a handle that references the deleted binding... + if (shape.handles) { + Object.values(shape.handles) + .filter((handle) => handle.bindingId === binding.id) + .forEach((handle) => { + // Save the binding reference in the before patch + before.shapes[id] = { + ...before.shapes[id], + handles: { + ...before.shapes[id]?.handles, + [handle.id]: { bindingId: binding.id }, + }, } - } - }) + + // Unless we're currently deleting the shape, remove the + // binding reference from the after patch + if (!deletedIds.has(id)) { + after.shapes[id] = { + ...after.shapes[id], + handles: { + ...after.shapes[id]?.handles, + [handle.id]: { bindingId: undefined }, + }, + } + } + }) + } } } - } - }) + }) return { before, after } } diff --git a/packages/tldraw/src/state/session/sessions/translate/translate.session.ts b/packages/tldraw/src/state/session/sessions/translate/translate.session.ts index 31cf91989..33072125d 100644 --- a/packages/tldraw/src/state/session/sessions/translate/translate.session.ts +++ b/packages/tldraw/src/state/session/sessions/translate/translate.session.ts @@ -382,30 +382,32 @@ export function getTranslateSnapshot(data: Data) { // Potentially confusing name here: these are the ids of the // original shapes that were cloned, not their clones' ids. - const clonedShapeIds = Object.keys(cloneMap) + const clonedShapeIds = new Set(Object.keys(cloneMap)) const bindingsToDelete: TLDrawBinding[] = [] // Create cloned bindings for shapes where both to and from shapes are selected // (if the user clones, then we will create a new binding for the clones) - Object.values(page.bindings).forEach((binding) => { - if (clonedShapeIds.includes(binding.fromId)) { - if (clonedShapeIds.includes(binding.toId)) { - const cloneId = Utils.uniqueId() - const cloneBinding = { - ...Utils.deepClone(binding), - id: cloneId, - fromId: cloneMap[binding.fromId] || binding.fromId, - toId: cloneMap[binding.toId] || binding.toId, - } + Object.values(page.bindings) + .filter((binding) => clonedShapeIds.has(binding.fromId) || clonedShapeIds.has(binding.toId)) + .forEach((binding) => { + if (clonedShapeIds.has(binding.fromId)) { + if (clonedShapeIds.has(binding.toId)) { + const cloneId = Utils.uniqueId() + const cloneBinding = { + ...Utils.deepClone(binding), + id: cloneId, + fromId: cloneMap[binding.fromId] || binding.fromId, + toId: cloneMap[binding.toId] || binding.toId, + } - clonedBindingsMap[binding.id] = cloneId - clonedBindings.push(cloneBinding) - } else { - bindingsToDelete.push(binding) + clonedBindingsMap[binding.id] = cloneId + clonedBindings.push(cloneBinding) + } else { + bindingsToDelete.push(binding) + } } - } - }) + }) // Assign new binding ids to clones (or delete them!) clones.forEach((clone) => {