From d47fd56d829a7b096d98cbc9ca8f2cdfdd77f9b9 Mon Sep 17 00:00:00 2001 From: David Sheldrick Date: Thu, 6 Jun 2024 10:48:23 +0100 Subject: [PATCH] Bindings onBeforeShapeIsolate? (#3871) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So we were kinda bending over backwards to capture the use case where we update the arrow's terminal x,y coords when unbinding, copy-pasting, and duplicating. - At first we abused the `onBeforeShapeDelete` callbacks, but that was footgunny. - Then we created a `onBeforeUnbind` callback, which was less footgunny but still subtly footgunny. This PR proposes reverting the `onBeforeUnbind` stuff, taking us back to having `onBeforeShapeDelete` stuff. But at the same time it adds `onBeforeShapeIsolate` callbacks which are triggered at the following times: - When you delete the other shape in a bound shape pair - When you copy/paste or duplicate one shape in a bound shape pair but not the other one - When you opt-in while deleting bindings e.g. `deleteBindings([...], {isolateShapes: true})` This PR also fixes the bound arrow drag interaction. We can probably extract that out to a separate PR if needed. ![Kapture 2024-06-04 at 12 42 40](https://github.com/tldraw/tldraw/assets/1242537/95b51e14-1119-4dad-91e4-8b19fdb5e862) ### Change Type - [x] `sdk` — Changes the tldraw SDK - [ ] `dotcom` — Changes the tldraw.com web app - [ ] `docs` — Changes to the documentation, examples, or templates. - [ ] `vs code` — Changes to the vscode plugin - [ ] `internal` — Does not affect user-facing stuff - [x] `bugfix` — Bug fix - [ ] `feature` — New feature - [x] `improvement` — Improving existing features - [ ] `chore` — Updating dependencies, other boring stuff - [ ] `galaxy brain` — Architectural changes - [ ] `tests` — Changes to any test code - [ ] `tools` — Changes to infrastructure, CI, internal scripts, debugging tools, etc. - [ ] `dunno` — I don't know ### Test Plan 1. Add a step-by-step description of how to test your PR here. 2. - [ ] Unit Tests - [ ] End to end tests ### Release Notes - Add a brief release note for your PR here. --- .../src/examples/pin-bindings/PinExample.tsx | 9 +- .../sticker-bindings/StickerExample.tsx | 9 +- packages/editor/api-report.md | 40 ++-- packages/editor/src/index.ts | 5 +- packages/editor/src/lib/editor/Editor.ts | 110 ++++----- .../src/lib/editor/bindings/BindingUtil.ts | 41 ++-- .../lib/editor/derivations/bindingsIndex.ts | 4 +- packages/editor/src/lib/options.ts | 4 +- .../lib/bindings/arrow/ArrowBindingUtil.ts | 56 +++-- .../src/lib/shapes/arrow/ArrowShapeUtil.tsx | 22 +- packages/tldraw/src/test/bindings.test.tsx | 210 ------------------ 11 files changed, 186 insertions(+), 324 deletions(-) delete mode 100644 packages/tldraw/src/test/bindings.test.tsx diff --git a/apps/examples/src/examples/pin-bindings/PinExample.tsx b/apps/examples/src/examples/pin-bindings/PinExample.tsx index 40813e1de..133787897 100644 --- a/apps/examples/src/examples/pin-bindings/PinExample.tsx +++ b/apps/examples/src/examples/pin-bindings/PinExample.tsx @@ -1,7 +1,6 @@ import { BindingOnShapeChangeOptions, - BindingOnUnbindOptions, - BindingUnbindReason, + BindingOnShapeDeleteOptions, BindingUtil, Box, DefaultFillStyle, @@ -257,10 +256,8 @@ class PinBindingUtil extends BindingUtil { } // when the thing we're stuck to is deleted, delete the pin too - override onBeforeUnbind({ binding, reason }: BindingOnUnbindOptions): void { - if (reason === BindingUnbindReason.DeletingToShape) { - this.editor.deleteShape(binding.fromId) - } + override onBeforeDeleteToShape({ binding }: BindingOnShapeDeleteOptions): void { + this.editor.deleteShape(binding.fromId) } } diff --git a/apps/examples/src/examples/sticker-bindings/StickerExample.tsx b/apps/examples/src/examples/sticker-bindings/StickerExample.tsx index 32c497541..a23f2fe3b 100644 --- a/apps/examples/src/examples/sticker-bindings/StickerExample.tsx +++ b/apps/examples/src/examples/sticker-bindings/StickerExample.tsx @@ -1,7 +1,6 @@ import { BindingOnShapeChangeOptions, - BindingOnUnbindOptions, - BindingUnbindReason, + BindingOnShapeDeleteOptions, BindingUtil, Box, DefaultToolbar, @@ -158,10 +157,8 @@ class StickerBindingUtil extends BindingUtil { } // when the thing we're stuck to is deleted, delete the sticker too - override onBeforeUnbind({ binding, reason }: BindingOnUnbindOptions): void { - if (reason === BindingUnbindReason.DeletingToShape) { - this.editor.deleteShape(binding.fromId) - } + override onBeforeDeleteToShape({ binding }: BindingOnShapeDeleteOptions): void { + this.editor.deleteShape(binding.fromId) } } diff --git a/packages/editor/api-report.md b/packages/editor/api-report.md index 2eb2494d7..3b8866b49 100644 --- a/packages/editor/api-report.md +++ b/packages/editor/api-report.md @@ -189,6 +189,12 @@ export interface BindingOnCreateOptions { binding: Binding; } +// @public (undocumented) +export interface BindingOnDeleteOptions { + // (undocumented) + binding: Binding; +} + // @public (undocumented) export interface BindingOnShapeChangeOptions { // (undocumented) @@ -200,21 +206,19 @@ export interface BindingOnShapeChangeOptions { } // @public (undocumented) -export interface BindingOnUnbindOptions { +export interface BindingOnShapeDeleteOptions { // (undocumented) binding: Binding; // (undocumented) - reason: BindingUnbindReason; + shape: TLShape; } // @public (undocumented) -export enum BindingUnbindReason { +export interface BindingOnShapeIsolateOptions { // (undocumented) - DeletingBinding = "deleting_binding", + binding: Binding; // (undocumented) - DeletingFromShape = "deleting_from_shape", - // (undocumented) - DeletingToShape = "deleting_to_shape" + shape: TLShape; } // @public (undocumented) @@ -234,13 +238,21 @@ export abstract class BindingUtil): void; // (undocumented) - onAfterUnbind?(options: BindingOnUnbindOptions): void; + onAfterDelete?(options: BindingOnDeleteOptions): void; // (undocumented) onBeforeChange?(options: BindingOnChangeOptions): Binding | void; // (undocumented) onBeforeCreate?(options: BindingOnCreateOptions): Binding | void; // (undocumented) - onBeforeUnbind?(options: BindingOnUnbindOptions): void; + onBeforeDelete?(options: BindingOnDeleteOptions): Binding | void; + // (undocumented) + onBeforeDeleteFromShape?(options: BindingOnShapeDeleteOptions): void; + // (undocumented) + onBeforeDeleteToShape?(options: BindingOnShapeDeleteOptions): void; + // (undocumented) + onBeforeIsolateFromShape?(options: BindingOnShapeIsolateOptions): void; + // (undocumented) + onBeforeIsolateToShape?(options: BindingOnShapeIsolateOptions): void; // (undocumented) onOperationComplete?(): void; // (undocumented) @@ -604,7 +616,7 @@ export const DefaultSvgDefs: () => null; export const defaultTldrawOptions: { readonly adjacentShapeMargin: 10; readonly animationMediumMs: 320; - readonly cameraMovingTimoutMs: 64; + readonly cameraMovingTimeoutMs: 64; readonly cameraSlideFriction: 0.09; readonly coarseDragDistanceSquared: 36; readonly coarseHandleRadius: 20; @@ -799,9 +811,11 @@ export class Editor extends EventEmitter { createShapes(shapes: OptionalKeys, 'id'>[]): this; deleteAssets(assets: TLAsset[] | TLAssetId[]): this; // (undocumented) - deleteBinding(binding: TLBinding | TLBindingId): this; + deleteBinding(binding: TLBinding | TLBindingId, opts?: Parameters[1]): this; // (undocumented) - deleteBindings(bindings: (TLBinding | TLBindingId)[]): this; + deleteBindings(bindings: (TLBinding | TLBindingId)[], { isolateShapes }?: { + isolateShapes?: boolean | undefined; + }): this; deleteOpenMenu(id: string): this; deletePage(page: TLPage | TLPageId): this; deleteShape(id: TLShapeId): this; @@ -2344,7 +2358,7 @@ export interface TldrawOptions { // (undocumented) readonly animationMediumMs: number; // (undocumented) - readonly cameraMovingTimoutMs: number; + readonly cameraMovingTimeoutMs: number; // (undocumented) readonly cameraSlideFriction: number; // (undocumented) diff --git a/packages/editor/src/index.ts b/packages/editor/src/index.ts index 6c4e4d3c0..309617afb 100644 --- a/packages/editor/src/index.ts +++ b/packages/editor/src/index.ts @@ -110,12 +110,13 @@ export { coreShapes, type TLAnyShapeUtilConstructor } from './lib/config/default export { DEFAULT_ANIMATION_OPTIONS, DEFAULT_CAMERA_OPTIONS, SIDES } from './lib/constants' export { Editor, type TLEditorOptions, type TLResizeShapeOptions } from './lib/editor/Editor' export { - BindingUnbindReason, BindingUtil, type BindingOnChangeOptions, type BindingOnCreateOptions, + type BindingOnDeleteOptions, type BindingOnShapeChangeOptions, - type BindingOnUnbindOptions, + type BindingOnShapeDeleteOptions, + type BindingOnShapeIsolateOptions, type TLBindingUtilConstructor, } from './lib/editor/bindings/BindingUtil' export { HistoryManager } from './lib/editor/managers/HistoryManager' diff --git a/packages/editor/src/lib/editor/Editor.ts b/packages/editor/src/lib/editor/Editor.ts index d9f954107..909aed9b6 100644 --- a/packages/editor/src/lib/editor/Editor.ts +++ b/packages/editor/src/lib/editor/Editor.ts @@ -110,8 +110,7 @@ import { getReorderingShapesChanges } from '../utils/reorderShapes' import { applyRotationToSnapshotShapes, getRotationSnapshot } from '../utils/rotation' import { uniqueId } from '../utils/uniqueId' import { - BindingOnUnbindOptions, - BindingUnbindReason, + BindingOnDeleteOptions, BindingUtil, TLBindingUtilConstructor, } from './bindings/BindingUtil' @@ -359,7 +358,7 @@ export class Editor extends EventEmitter { this.sideEffects = this.store.sideEffects - let deletedBindings = new Map>() + let deletedBindings = new Map>() const deletedShapeIds = new Set() const invalidParents = new Set() let invalidBindingTypes = new Set() @@ -395,7 +394,7 @@ export class Editor extends EventEmitter { const t = deletedBindings deletedBindings = new Map() for (const opts of t.values()) { - this.getBindingUtil(opts.binding).onAfterUnbind?.(opts) + this.getBindingUtil(opts.binding).onAfterDelete?.(opts) } } @@ -491,8 +490,19 @@ export class Editor extends EventEmitter { for (const binding of this.getBindingsInvolvingShape(shape)) { invalidBindingTypes.add(binding.type) deleteBindingIds.push(binding.id) + const util = this.getBindingUtil(binding) + if (binding.fromId === shape.id) { + util.onBeforeIsolateToShape?.({ binding, shape }) + util.onBeforeDeleteFromShape?.({ binding, shape }) + } else { + util.onBeforeIsolateFromShape?.({ binding, shape }) + util.onBeforeDeleteToShape?.({ binding, shape }) + } + } + + if (deleteBindingIds.length) { + this.deleteBindings(deleteBindingIds) } - this.deleteBindings(deleteBindingIds) const deletedIds = new Set([shape.id]) const updates = compact( @@ -529,25 +539,10 @@ export class Editor extends EventEmitter { this.getBindingUtil(bindingAfter).onAfterChange?.({ bindingBefore, bindingAfter }) }, beforeDelete: (binding) => { - const util = this.getBindingUtil(binding) - // No need to track this binding if it's util doesn't care about the unbind operation - if (!util.onBeforeUnbind && !util.onAfterUnbind) return - // We only want to call this once per binding and it might be possible that the onBeforeUnbind - // callback will trigger a nested delete operation on the same binding so let's bail out if - // that is happening - if (deletedBindings.has(binding.id)) return - const opts: BindingOnUnbindOptions = { - binding, - reason: deletedShapeIds.has(binding.fromId) - ? BindingUnbindReason.DeletingFromShape - : deletedShapeIds.has(binding.toId) - ? BindingUnbindReason.DeletingToShape - : BindingUnbindReason.DeletingBinding, - } - deletedBindings.set(binding.id, opts) - this.getBindingUtil(binding).onBeforeUnbind?.(opts) + this.getBindingUtil(binding).onBeforeDelete?.({ binding }) }, afterDelete: (binding) => { + this.getBindingUtil(binding).onAfterDelete?.({ binding }) invalidBindingTypes.add(binding.type) }, }, @@ -3461,7 +3456,7 @@ export class Editor extends EventEmitter { } private _tickCameraState = () => { // always reset the timeout - this._cameraStateTimeoutRemaining = this.options.cameraMovingTimoutMs + this._cameraStateTimeoutRemaining = this.options.cameraMovingTimeoutMs // If the state is idle, then start the tick if (this._cameraState.__unsafe__getWithoutCapture() !== 'idle') return this._cameraState.set('moving') @@ -5247,13 +5242,26 @@ export class Editor extends EventEmitter { return this.updateBindings([partial]) } - deleteBindings(bindings: (TLBinding | TLBindingId)[]) { + deleteBindings(bindings: (TLBinding | TLBindingId)[], { isolateShapes = false } = {}) { const ids = bindings.map((binding) => (typeof binding === 'string' ? binding : binding.id)) - this.store.remove(ids) + if (isolateShapes) { + this.store.atomic(() => { + for (const id of ids) { + const binding = this.getBinding(id) + if (!binding) continue + const util = this.getBindingUtil(binding) + util.onBeforeIsolateFromShape?.({ binding, shape: this.getShape(binding.fromId)! }) + util.onBeforeIsolateToShape?.({ binding, shape: this.getShape(binding.toId)! }) + this.store.remove([id]) + } + }) + } else { + this.store.remove(ids) + } return this } - deleteBinding(binding: TLBinding | TLBindingId) { - return this.deleteBindings([binding]) + deleteBinding(binding: TLBinding | TLBindingId, opts?: Parameters[1]) { + return this.deleteBindings([binding], opts) } canBindShapes({ fromShape, @@ -5406,7 +5414,7 @@ export class Editor extends EventEmitter { shapeIds.set(shapeId, createShapeId()) } - const { shapesToCreate, bindingsToCreate } = withoutBindingsToUnrelatedShapes( + const { shapesToCreate, bindingsToCreate } = withIsolatedShapes( this, shapeIdSet, (bindingIdsToMaintain) => { @@ -7664,7 +7672,7 @@ export class Editor extends EventEmitter { const shapeIds = this.getShapeAndDescendantIds(ids) - return withoutBindingsToUnrelatedShapes(this, shapeIds, (bindingIdsToKeep) => { + return withIsolatedShapes(this, shapeIds, (bindingIdsToKeep) => { const bindings: TLBinding[] = [] for (const id of bindingIdsToKeep) { const binding = this.getBinding(id) @@ -9005,36 +9013,36 @@ function pushShapeWithDescendants(editor: Editor, id: TLShapeId, result: TLShape * * The callback is given the set of bindings that should be maintained. */ -function withoutBindingsToUnrelatedShapes( +function withIsolatedShapes( editor: Editor, shapeIds: Set, callback: (bindingsWithBoth: Set) => T ): T { - const bindingsWithBoth = new Set() - const bindingsToRemove = new Set() - - for (const shapeId of shapeIds) { - const shape = editor.getShape(shapeId) - if (!shape) continue - - for (const binding of editor.getBindingsInvolvingShape(shapeId)) { - const hasFrom = shapeIds.has(binding.fromId) - const hasTo = shapeIds.has(binding.toId) - if (hasFrom && hasTo) { - bindingsWithBoth.add(binding.id) - continue - } - if (!hasFrom || !hasTo) { - bindingsToRemove.add(binding.id) - } - } - } - let result!: Result editor.history.ignore(() => { const changes = editor.store.extractingChanges(() => { - editor.deleteBindings([...bindingsToRemove]) + const bindingsWithBoth = new Set() + const bindingsToRemove = new Set() + + for (const shapeId of shapeIds) { + const shape = editor.getShape(shapeId) + if (!shape) continue + + for (const binding of editor.getBindingsInvolvingShape(shapeId)) { + const hasFrom = shapeIds.has(binding.fromId) + const hasTo = shapeIds.has(binding.toId) + if (hasFrom && hasTo) { + bindingsWithBoth.add(binding.id) + continue + } + if (!hasFrom || !hasTo) { + bindingsToRemove.add(binding.id) + } + } + } + + editor.deleteBindings([...bindingsToRemove], { isolateShapes: true }) try { result = Result.ok(callback(bindingsWithBoth)) diff --git a/packages/editor/src/lib/editor/bindings/BindingUtil.ts b/packages/editor/src/lib/editor/bindings/BindingUtil.ts index 476feacb3..22e722eb2 100644 --- a/packages/editor/src/lib/editor/bindings/BindingUtil.ts +++ b/packages/editor/src/lib/editor/bindings/BindingUtil.ts @@ -17,25 +17,17 @@ export interface BindingOnCreateOptions { binding: Binding } -/** @public */ -export enum BindingUnbindReason { - DeletingFromShape = 'deleting_from_shape', - DeletingToShape = 'deleting_to_shape', - DeletingBinding = 'deleting_binding', -} - -/** @public */ -export interface BindingOnUnbindOptions { - binding: Binding - reason: BindingUnbindReason -} - /** @public */ export interface BindingOnChangeOptions { bindingBefore: Binding bindingAfter: Binding } +/** @public */ +export interface BindingOnDeleteOptions { + binding: Binding +} + /** @public */ export interface BindingOnShapeChangeOptions { binding: Binding @@ -43,6 +35,18 @@ export interface BindingOnShapeChangeOptions { shapeAfter: TLShape } +/** @public */ +export interface BindingOnShapeIsolateOptions { + binding: Binding + shape: TLShape +} + +/** @public */ +export interface BindingOnShapeDeleteOptions { + binding: Binding + shape: TLShape +} + /** @public */ export abstract class BindingUtil { constructor(public editor: Editor) {} @@ -65,15 +69,20 @@ export abstract class BindingUtil): void - onAfterUnbind?(options: BindingOnUnbindOptions): void - // self lifecycle hooks onBeforeCreate?(options: BindingOnCreateOptions): Binding | void onAfterCreate?(options: BindingOnCreateOptions): void onBeforeChange?(options: BindingOnChangeOptions): Binding | void onAfterChange?(options: BindingOnChangeOptions): void + onBeforeDelete?(options: BindingOnDeleteOptions): Binding | void + onAfterDelete?(options: BindingOnDeleteOptions): void onAfterChangeFromShape?(options: BindingOnShapeChangeOptions): void onAfterChangeToShape?(options: BindingOnShapeChangeOptions): void + + onBeforeIsolateFromShape?(options: BindingOnShapeIsolateOptions): void + onBeforeIsolateToShape?(options: BindingOnShapeIsolateOptions): void + + onBeforeDeleteFromShape?(options: BindingOnShapeDeleteOptions): void + onBeforeDeleteToShape?(options: BindingOnShapeDeleteOptions): void } diff --git a/packages/editor/src/lib/editor/derivations/bindingsIndex.ts b/packages/editor/src/lib/editor/derivations/bindingsIndex.ts index 59bccd6d0..1adf209d5 100644 --- a/packages/editor/src/lib/editor/derivations/bindingsIndex.ts +++ b/packages/editor/src/lib/editor/derivations/bindingsIndex.ts @@ -50,14 +50,14 @@ export const bindingsIndex = (editor: Editor): Computed => { function removingBinding(binding: TLBinding) { nextValue ??= new Map(lastValue) - const prevFrom = lastValue.get(binding.fromId) + const prevFrom = nextValue.get(binding.fromId) const nextFrom = prevFrom?.filter((b) => b.id !== binding.id) if (!nextFrom?.length) { nextValue.delete(binding.fromId) } else { nextValue.set(binding.fromId, nextFrom) } - const prevTo = lastValue.get(binding.toId) + const prevTo = nextValue.get(binding.toId) const nextTo = prevTo?.filter((b) => b.id !== binding.id) if (!nextTo?.length) { nextValue.delete(binding.toId) diff --git a/packages/editor/src/lib/options.ts b/packages/editor/src/lib/options.ts index 56c29a314..bdf44040c 100644 --- a/packages/editor/src/lib/options.ts +++ b/packages/editor/src/lib/options.ts @@ -34,7 +34,7 @@ export interface TldrawOptions { readonly collaboratorInactiveTimeoutMs: number readonly collaboratorIdleTimeoutMs: number readonly collaboratorCheckIntervalMs: number - readonly cameraMovingTimoutMs: number + readonly cameraMovingTimeoutMs: number readonly hitTestMargin: number readonly edgeScrollSpeed: number readonly edgeScrollDistance: number @@ -67,7 +67,7 @@ export const defaultTldrawOptions = { collaboratorInactiveTimeoutMs: 60000, collaboratorIdleTimeoutMs: 3000, collaboratorCheckIntervalMs: 1200, - cameraMovingTimoutMs: 64, + cameraMovingTimeoutMs: 64, hitTestMargin: 8, edgeScrollSpeed: 20, edgeScrollDistance: 8, diff --git a/packages/tldraw/src/lib/bindings/arrow/ArrowBindingUtil.ts b/packages/tldraw/src/lib/bindings/arrow/ArrowBindingUtil.ts index 3583c3162..2ccef16a6 100644 --- a/packages/tldraw/src/lib/bindings/arrow/ArrowBindingUtil.ts +++ b/packages/tldraw/src/lib/bindings/arrow/ArrowBindingUtil.ts @@ -2,8 +2,7 @@ import { BindingOnChangeOptions, BindingOnCreateOptions, BindingOnShapeChangeOptions, - BindingOnUnbindOptions, - BindingUnbindReason, + BindingOnShapeIsolateOptions, BindingUtil, Editor, IndexKey, @@ -15,6 +14,7 @@ import { TLShapeId, TLShapePartial, Vec, + approximately, arrowBindingMigrations, arrowBindingProps, assert, @@ -60,13 +60,17 @@ export class ArrowBindingUtil extends BindingUtil { reparentArrow(this.editor, binding.fromId) } - // when the shape the arrow is pointing to is deleted - override onBeforeUnbind({ binding, reason }: BindingOnUnbindOptions): void { - // don't need to do anything if the arrow is being deleted - if (reason === BindingUnbindReason.DeletingFromShape) return + // when the arrow is isolated we need to update it's x,y positions + override onBeforeIsolateFromShape({ + binding, + }: BindingOnShapeIsolateOptions): void { const arrow = this.editor.getShape(binding.fromId) if (!arrow) return - unbindArrowTerminal(this.editor, arrow, binding.props.terminal) + updateArrowTerminal({ + editor: this.editor, + arrow, + terminal: binding.props.terminal, + }) } } @@ -171,7 +175,7 @@ function arrowDidUpdate(editor: Editor, arrow: TLArrowShape) { const isShapeInSamePageAsArrow = editor.getAncestorPageId(arrow) === editor.getAncestorPageId(boundShape) if (!boundShape || !isShapeInSamePageAsArrow) { - unbindArrowTerminal(editor, arrow, handle) + updateArrowTerminal({ editor, arrow, terminal: handle, unbind: true }) } } @@ -179,17 +183,34 @@ function arrowDidUpdate(editor: Editor, arrow: TLArrowShape) { reparentArrow(editor, arrow.id) } -function unbindArrowTerminal(editor: Editor, arrow: TLArrowShape, terminal: 'start' | 'end') { - const info = getArrowInfo(editor, arrow)! +/** @internal */ +export function updateArrowTerminal({ + editor, + arrow, + terminal, + unbind = false, + useHandle = false, +}: { + editor: Editor + arrow: TLArrowShape + terminal: 'start' | 'end' + unbind?: boolean + useHandle?: boolean +}) { + const info = getArrowInfo(editor, arrow) if (!info) { throw new Error('expected arrow info') } + const startPoint = useHandle ? info.start.handle : info.start.point + const endPoint = useHandle ? info.end.handle : info.end.point + const point = terminal === 'start' ? startPoint : endPoint + const update = { id: arrow.id, type: 'arrow', props: { - [terminal]: { x: info[terminal].point.x, y: info[terminal].point.y }, + [terminal]: { x: point.x, y: point.y }, bend: arrow.props.bend, }, } satisfies TLShapePartial @@ -197,8 +218,8 @@ function unbindArrowTerminal(editor: Editor, arrow: TLArrowShape, terminal: 'sta // fix up the bend: if (!info.isStraight) { // find the new start/end points of the resulting arrow - const newStart = terminal === 'start' ? info.start.point : info.start.handle - const newEnd = terminal === 'end' ? info.end.point : info.end.handle + const newStart = terminal === 'start' ? startPoint : info.start.handle + const newEnd = terminal === 'end' ? endPoint : info.end.handle const newMidPoint = Vec.Med(newStart, newEnd) // intersect a line segment perpendicular to the new arrow with the old arrow arc to @@ -218,9 +239,14 @@ function unbindArrowTerminal(editor: Editor, arrow: TLArrowShape, terminal: 'sta assert(intersections?.length === 1) const bend = Vec.Dist(newMidPoint, intersections[0]) * Math.sign(arrow.props.bend) - update.props.bend = bend + // use `approximately` to avoid endless update loops + if (!approximately(bend, update.props.bend)) { + update.props.bend = bend + } } editor.updateShape(update) - removeArrowBinding(editor, arrow, terminal) + if (unbind) { + removeArrowBinding(editor, arrow, terminal) + } } diff --git a/packages/tldraw/src/lib/shapes/arrow/ArrowShapeUtil.tsx b/packages/tldraw/src/lib/shapes/arrow/ArrowShapeUtil.tsx index 65847e430..5dcac83f7 100644 --- a/packages/tldraw/src/lib/shapes/arrow/ArrowShapeUtil.tsx +++ b/packages/tldraw/src/lib/shapes/arrow/ArrowShapeUtil.tsx @@ -34,6 +34,7 @@ import { useIsEditing, } from '@tldraw/editor' import React from 'react' +import { updateArrowTerminal } from '../../bindings/arrow/ArrowBindingUtil' import { ShapeFill, useDefaultColorTheme } from '../shared/ShapeFill' import { SvgTextLabel } from '../shared/SvgTextLabel' import { ARROW_LABEL_FONT_SIZES, STROKE_SIZES } from '../shared/default-shape-constants' @@ -355,6 +356,25 @@ export class ArrowShapeUtil extends ShapeUtil { }), }) + // update arrow terminal bindings eagerly to make sure the arrows unbind nicely when translating + if (bindings.start) { + updateArrowTerminal({ + editor: this.editor, + arrow: shape, + terminal: 'start', + useHandle: true, + }) + shape = this.editor.getShape(shape.id) as TLArrowShape + } + if (bindings.end) { + updateArrowTerminal({ + editor: this.editor, + arrow: shape, + terminal: 'end', + useHandle: true, + }) + } + for (const handleName of [ARROW_HANDLES.START, ARROW_HANDLES.END] as const) { const binding = bindings[handleName] if (!binding) continue @@ -570,7 +590,7 @@ export class ArrowShapeUtil extends ShapeUtil { {showArrowLabel && ( diff --git a/packages/tldraw/src/test/bindings.test.tsx b/packages/tldraw/src/test/bindings.test.tsx deleted file mode 100644 index abd928fd4..000000000 --- a/packages/tldraw/src/test/bindings.test.tsx +++ /dev/null @@ -1,210 +0,0 @@ -import { - BindingOnUnbindOptions, - BindingUnbindReason, - BindingUtil, - TLShapeId, - TLUnknownBinding, - createBindingId, - createShapeId, -} from '@tldraw/editor' -import { TestEditor } from './TestEditor' -import { TL } from './test-jsx' - -let editor: TestEditor - -const ids = { - box1: createShapeId('box1'), - box2: createShapeId('box2'), - box3: createShapeId('box3'), - box4: createShapeId('box4'), -} - -const mockOnBeforeUnbind = jest.fn() as jest.Mock]> -const mockOnAfterUnbind = jest.fn() as jest.Mock]> - -class TestBindingUtil extends BindingUtil { - static override type = 'test' - - static override props = {} - - override getDefaultProps(): object { - return {} - } - - override onBeforeUnbind(options: BindingOnUnbindOptions): void { - mockOnBeforeUnbind(options) - } - - override onAfterUnbind(options: BindingOnUnbindOptions): void { - mockOnAfterUnbind(options) - } -} - -beforeEach(() => { - editor = new TestEditor({ bindingUtils: [TestBindingUtil] }) - - editor.createShapesFromJsx([ - , - , - , - , - ]) - - mockOnBeforeUnbind.mockClear() - mockOnAfterUnbind.mockClear() -}) - -function bindShapes(fromId: TLShapeId, toId: TLShapeId) { - const bindingId = createBindingId() - editor.createBinding({ - id: bindingId, - type: 'test', - fromId, - toId, - }) - return bindingId -} - -test('deleting the from shape causes the reason to be "deleting_from_shape"', () => { - bindShapes(ids.box1, ids.box2) - editor.deleteShape(ids.box1) - expect(mockOnBeforeUnbind).toHaveBeenCalledTimes(1) - expect(mockOnBeforeUnbind).toHaveBeenCalledWith( - expect.objectContaining({ reason: BindingUnbindReason.DeletingFromShape }) - ) - expect(mockOnAfterUnbind).toHaveBeenCalledTimes(1) - expect(mockOnAfterUnbind).toHaveBeenCalledWith( - expect.objectContaining({ reason: BindingUnbindReason.DeletingFromShape }) - ) -}) - -test('deleting the to shape causes the reason to be "deleting_to_shape"', () => { - bindShapes(ids.box1, ids.box2) - editor.deleteShape(ids.box2) - expect(mockOnBeforeUnbind).toHaveBeenCalledTimes(1) - expect(mockOnBeforeUnbind).toHaveBeenCalledWith( - expect.objectContaining({ reason: BindingUnbindReason.DeletingToShape }) - ) - expect(mockOnAfterUnbind).toHaveBeenCalledTimes(1) - expect(mockOnAfterUnbind).toHaveBeenCalledWith( - expect.objectContaining({ reason: BindingUnbindReason.DeletingToShape }) - ) -}) - -test('deleting the binding itself causes the reason to be "deleting_binding"', () => { - const bindingId = bindShapes(ids.box1, ids.box2) - editor.deleteBinding(bindingId) - - expect(mockOnBeforeUnbind).toHaveBeenCalledTimes(1) - expect(mockOnBeforeUnbind).toHaveBeenCalledWith( - expect.objectContaining({ reason: BindingUnbindReason.DeletingBinding }) - ) - expect(mockOnAfterUnbind).toHaveBeenCalledTimes(1) - expect(mockOnAfterUnbind).toHaveBeenCalledWith( - expect.objectContaining({ reason: BindingUnbindReason.DeletingBinding }) - ) -}) - -test('copying both bound shapes does not trigger the unbind operation', () => { - bindShapes(ids.box1, ids.box2) - editor.select(ids.box1, ids.box2) - editor.copy() - expect(mockOnBeforeUnbind).not.toHaveBeenCalled() - expect(mockOnAfterUnbind).not.toHaveBeenCalled() -}) - -test('copying the from shape on its own does trigger the unbind operation', () => { - bindShapes(ids.box1, ids.box2) - editor.select(ids.box1) - editor.copy() - expect(mockOnBeforeUnbind).toHaveBeenCalledTimes(1) - expect(mockOnBeforeUnbind).toHaveBeenCalledWith( - expect.objectContaining({ reason: BindingUnbindReason.DeletingBinding }) - ) - expect(mockOnAfterUnbind).toHaveBeenCalledTimes(1) - expect(mockOnAfterUnbind).toHaveBeenCalledWith( - expect.objectContaining({ reason: BindingUnbindReason.DeletingBinding }) - ) -}) - -test('copying the to shape on its own does trigger the unbind operation', () => { - bindShapes(ids.box1, ids.box2) - editor.select(ids.box2) - editor.copy() - expect(mockOnBeforeUnbind).toHaveBeenCalledTimes(1) - expect(mockOnBeforeUnbind).toHaveBeenCalledWith( - expect.objectContaining({ reason: BindingUnbindReason.DeletingBinding }) - ) - expect(mockOnAfterUnbind).toHaveBeenCalledTimes(1) - expect(mockOnAfterUnbind).toHaveBeenCalledWith( - expect.objectContaining({ reason: BindingUnbindReason.DeletingBinding }) - ) -}) - -test('cascading deletes in afterUnbind are handled correctly', () => { - mockOnAfterUnbind.mockImplementation((options) => { - if (options.reason === BindingUnbindReason.DeletingFromShape) { - editor.deleteShape(options.binding.toId) - } - }) - - bindShapes(ids.box1, ids.box2) - bindShapes(ids.box2, ids.box3) - bindShapes(ids.box3, ids.box4) - - editor.deleteShape(ids.box1) - - expect(editor.getShape(ids.box1)).toBeUndefined() - expect(editor.getShape(ids.box2)).toBeUndefined() - expect(editor.getShape(ids.box3)).toBeUndefined() - expect(editor.getShape(ids.box4)).toBeUndefined() - - expect(mockOnBeforeUnbind).toHaveBeenCalledTimes(3) - expect(mockOnAfterUnbind).toHaveBeenCalledTimes(3) -}) - -test('cascading deletes in beforeUnbind are handled correctly', () => { - mockOnBeforeUnbind.mockImplementation((options) => { - if (options.reason === BindingUnbindReason.DeletingFromShape) { - editor.deleteShape(options.binding.toId) - } - }) - - bindShapes(ids.box1, ids.box2) - bindShapes(ids.box2, ids.box3) - bindShapes(ids.box3, ids.box4) - - editor.deleteShape(ids.box1) - - expect(editor.getShape(ids.box1)).toBeUndefined() - expect(editor.getShape(ids.box2)).toBeUndefined() - expect(editor.getShape(ids.box3)).toBeUndefined() - expect(editor.getShape(ids.box4)).toBeUndefined() - - expect(mockOnBeforeUnbind).toHaveBeenCalledTimes(3) - expect(mockOnAfterUnbind).toHaveBeenCalledTimes(3) -}) - -test('beforeUnbind is called before the from shape is deleted or the binding is deleted', () => { - mockOnBeforeUnbind.mockImplementationOnce(() => { - expect(editor.getShape(ids.box1)).toBeDefined() - expect(editor.getShape(ids.box2)).toBeDefined() - expect(editor.getBindingsFromShape(ids.box1, 'test')).toHaveLength(1) - }) - bindShapes(ids.box1, ids.box2) - editor.deleteShape(ids.box1) - - expect.assertions(3) -}) - -test('beforeUnbind is called before the to shape is deleted or the binding is deleted', () => { - mockOnBeforeUnbind.mockImplementationOnce(() => { - expect(editor.getShape(ids.box1)).toBeDefined() - expect(editor.getShape(ids.box2)).toBeDefined() - expect(editor.getBindingsToShape(ids.box2, 'test')).toHaveLength(1) - }) - bindShapes(ids.box1, ids.box2) - editor.deleteShape(ids.box2) - - expect.assertions(3) -})