From d9bbf1eda665f616f46f465f6122da22a850ef47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rk=20Tolm=C3=A1cs?= Date: Thu, 2 May 2024 08:32:12 +0200 Subject: [PATCH] feat: Allow binding only via linear element ends (#7946) Arrows now only bind to new shapes if their start or end point is dragged close to them. Arrows previously bound to shapes remain bound on move and drag if at the end of the drag/move the points remain in the original shapes' binding area. --------- Signed-off-by: Mark Tolmacs Co-authored-by: dwelle <5153846+dwelle@users.noreply.github.com> Co-authored-by: Sammy Lee --- packages/excalidraw/actions/actionFlip.ts | 15 +- packages/excalidraw/components/App.tsx | 72 ++-- packages/excalidraw/element/binding.ts | 372 +++++++++--------- .../tests/__snapshots__/history.test.tsx.snap | 282 ++++++++----- packages/excalidraw/tests/binding.test.tsx | 67 ++-- packages/excalidraw/tests/history.test.tsx | 35 +- 6 files changed, 498 insertions(+), 345 deletions(-) diff --git a/packages/excalidraw/actions/actionFlip.ts b/packages/excalidraw/actions/actionFlip.ts index aad901a86a..e767ca0b0e 100644 --- a/packages/excalidraw/actions/actionFlip.ts +++ b/packages/excalidraw/actions/actionFlip.ts @@ -12,13 +12,13 @@ import { arrayToMap } from "../utils"; import { CODES, KEYS } from "../keys"; import { getCommonBoundingBox } from "../element/bounds"; import { - bindOrUnbindSelectedElements, + bindOrUnbindLinearElements, isBindingEnabled, - unbindLinearElements, } from "../element/binding"; import { updateFrameMembershipOfSelectedElements } from "../frame"; import { flipHorizontal, flipVertical } from "../components/icons"; import { StoreAction } from "../store"; +import { isLinearElement } from "../element/typeChecks"; export const actionFlipHorizontal = register({ name: "flipHorizontal", @@ -89,7 +89,6 @@ const flipSelectedElements = ( const updatedElements = flipElements( selectedElements, - elements, elementsMap, appState, flipDirection, @@ -105,7 +104,6 @@ const flipSelectedElements = ( const flipElements = ( selectedElements: NonDeleted[], - elements: readonly ExcalidrawElement[], elementsMap: NonDeletedSceneElementsMap, appState: AppState, flipDirection: "horizontal" | "vertical", @@ -124,9 +122,12 @@ const flipElements = ( flipDirection === "horizontal" ? minY : maxY, ); - isBindingEnabled(appState) - ? bindOrUnbindSelectedElements(selectedElements, app) - : unbindLinearElements(selectedElements, elementsMap); + bindOrUnbindLinearElements( + selectedElements.filter(isLinearElement), + app, + isBindingEnabled(appState), + [], + ); return selectedElements; }; diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 8967f63c2f..5a08f1fe56 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -122,17 +122,16 @@ import { } from "../element"; import { bindOrUnbindLinearElement, - bindOrUnbindSelectedElements, + bindOrUnbindLinearElements, fixBindingsAfterDeletion, fixBindingsAfterDuplication, - getEligibleElementsForBinding, getHoveredElementForBinding, isBindingEnabled, isLinearElementSimpleAndAlreadyBound, maybeBindLinearElement, shouldEnableBindingForPointerEvent, - unbindLinearElements, updateBoundElements, + getSuggestedBindingsForArrows, } from "../element/binding"; import { LinearElementEditor } from "../element/linearElementEditor"; import { mutateElement, newElementWith } from "../element/mutateElement"; @@ -3938,7 +3937,12 @@ class App extends React.Component { }); }); - this.maybeSuggestBindingForAll(selectedElements); + this.setState({ + suggestedBindings: getSuggestedBindingsForArrows( + selectedElements, + this, + ), + }); event.preventDefault(); } else if (event.key === KEYS.ENTER) { @@ -4105,11 +4109,12 @@ class App extends React.Component { this.setState({ isBindingEnabled: true }); } if (isArrowKey(event.key)) { - const selectedElements = this.scene.getSelectedElements(this.state); - const elementsMap = this.scene.getNonDeletedElementsMap(); - isBindingEnabled(this.state) - ? bindOrUnbindSelectedElements(selectedElements, this) - : unbindLinearElements(selectedElements, elementsMap); + bindOrUnbindLinearElements( + this.scene.getSelectedElements(this.state).filter(isLinearElement), + this, + isBindingEnabled(this.state), + this.state.selectedLinearElement?.selectedPointsIndices ?? [], + ); this.setState({ suggestedBindings: [] }); } }); @@ -7453,7 +7458,12 @@ class App extends React.Component { event[KEYS.CTRL_OR_CMD] ? null : this.state.gridSize, ); - this.maybeSuggestBindingForAll(selectedElements); + this.setState({ + suggestedBindings: getSuggestedBindingsForArrows( + selectedElements, + this, + ), + }); // We duplicate the selected element if alt is pressed on pointer move if (event.altKey && !pointerDownState.hit.hasBeenDuplicated) { @@ -8500,15 +8510,18 @@ class App extends React.Component { } if (pointerDownState.drag.hasOccurred || isResizing || isRotating) { - isBindingEnabled(this.state) - ? bindOrUnbindSelectedElements( - this.scene.getSelectedElements(this.state), - this, - ) - : unbindLinearElements( - this.scene.getSelectedElements(this.state), - elementsMap, - ); + // We only allow binding via linear elements, specifically via dragging + // the endpoints ("start" or "end"). + const linearElements = this.scene + .getSelectedElements(this.state) + .filter(isLinearElement); + + bindOrUnbindLinearElements( + linearElements, + this, + isBindingEnabled(this.state), + this.state.selectedLinearElement?.selectedPointsIndices ?? [], + ); } if (activeTool.type === "laser") { @@ -9040,19 +9053,6 @@ class App extends React.Component { this.setState({ suggestedBindings }); }; - private maybeSuggestBindingForAll( - selectedElements: NonDeleted[], - ): void { - if (selectedElements.length > 50) { - return; - } - const suggestedBindings = getEligibleElementsForBinding( - selectedElements, - this, - ); - this.setState({ suggestedBindings }); - } - private clearSelection(hitElement: ExcalidrawElement | null): void { this.setState((prevState) => ({ selectedElementIds: makeNextSelectedElementIds({}, prevState), @@ -9439,8 +9439,6 @@ class App extends React.Component { this.state.originSnapOffset, ); - this.maybeSuggestBindingForAll([draggingElement]); - // highlight elements that are to be added to frames on frames creation if ( this.state.activeTool.type === TOOL_TYPE.frame || @@ -9563,7 +9561,10 @@ class App extends React.Component { pointerDownState.resize.center.y, ) ) { - this.maybeSuggestBindingForAll(selectedElements); + const suggestedBindings = getSuggestedBindingsForArrows( + selectedElements, + this, + ); const elementsToHighlight = new Set(); selectedFrames.forEach((frame) => { @@ -9577,6 +9578,7 @@ class App extends React.Component { this.setState({ elementsToHighlight: [...elementsToHighlight], + suggestedBindings, }); return true; diff --git a/packages/excalidraw/element/binding.ts b/packages/excalidraw/element/binding.ts index 6a1da1e7b7..a2ecc16786 100644 --- a/packages/excalidraw/element/binding.ts +++ b/packages/excalidraw/element/binding.ts @@ -131,73 +131,193 @@ const bindOrUnbindLinearElementEdge = ( unboundFromElementIds: Set, elementsMap: NonDeletedSceneElementsMap, ): void => { - if (bindableElement !== "keep") { - if (bindableElement != null) { - // Don't bind if we're trying to bind or are already bound to the same - // element on the other edge already ("start" edge takes precedence). - if ( - otherEdgeBindableElement == null || - (otherEdgeBindableElement === "keep" - ? !isLinearElementSimpleAndAlreadyBoundOnOppositeEdge( - linearElement, - bindableElement, - startOrEnd, - ) - : startOrEnd === "start" || - otherEdgeBindableElement.id !== bindableElement.id) - ) { - bindLinearElement( - linearElement, - bindableElement, - startOrEnd, - elementsMap, - ); - boundToElementIds.add(bindableElement.id); - } - } else { - const unbound = unbindLinearElement(linearElement, startOrEnd); - if (unbound != null) { - unboundFromElementIds.add(unbound); - } + // "keep" is for method chaining convenience, a "no-op", so just bail out + if (bindableElement === "keep") { + return; + } + + // null means break the bind, so nothing to consider here + if (bindableElement === null) { + const unbound = unbindLinearElement(linearElement, startOrEnd); + if (unbound != null) { + unboundFromElementIds.add(unbound); } + return; + } + + // While complext arrows can do anything, simple arrow with both ends trying + // to bind to the same bindable should not be allowed, start binding takes + // precedence + if (isLinearElementSimple(linearElement)) { + if ( + otherEdgeBindableElement == null || + (otherEdgeBindableElement === "keep" + ? // TODO: Refactor - Needlessly complex + !isLinearElementSimpleAndAlreadyBoundOnOppositeEdge( + linearElement, + bindableElement, + startOrEnd, + ) + : startOrEnd === "start" || + otherEdgeBindableElement.id !== bindableElement.id) + ) { + bindLinearElement( + linearElement, + bindableElement, + startOrEnd, + elementsMap, + ); + boundToElementIds.add(bindableElement.id); + } + } else { + bindLinearElement(linearElement, bindableElement, startOrEnd, elementsMap); + boundToElementIds.add(bindableElement.id); } }; -export const bindOrUnbindSelectedElements = ( - selectedElements: NonDeleted[], +const getOriginalBindingIfStillCloseOfLinearElementEdge = ( + linearElement: NonDeleted, + edge: "start" | "end", app: AppClassProperties, +): NonDeleted | null => { + const elementsMap = app.scene.getNonDeletedElementsMap(); + const coors = getLinearElementEdgeCoors(linearElement, edge, elementsMap); + const elementId = + edge === "start" + ? linearElement.startBinding?.elementId + : linearElement.endBinding?.elementId; + if (elementId) { + const element = elementsMap.get( + elementId, + ) as NonDeleted; + if (bindingBorderTest(element, coors, app)) { + return element; + } + } + + return null; +}; + +const getOriginalBindingsIfStillCloseToArrowEnds = ( + linearElement: NonDeleted, + app: AppClassProperties, +): (NonDeleted | null)[] => + ["start", "end"].map((edge) => + getOriginalBindingIfStillCloseOfLinearElementEdge( + linearElement, + edge as "start" | "end", + app, + ), + ); + +const getBindingStrategyForDraggingArrowEndpoints = ( + selectedElement: NonDeleted, + isBindingEnabled: boolean, + draggingPoints: readonly number[], + app: AppClassProperties, +): (NonDeleted | null | "keep")[] => { + const startIdx = 0; + const endIdx = selectedElement.points.length - 1; + const startDragged = draggingPoints.findIndex((i) => i === startIdx) > -1; + const endDragged = draggingPoints.findIndex((i) => i === endIdx) > -1; + const start = startDragged + ? isBindingEnabled + ? getElligibleElementForBindingElement(selectedElement, "start", app) + : null // If binding is disabled and start is dragged, break all binds + : // We have to update the focus and gap of the binding, so let's rebind + getElligibleElementForBindingElement(selectedElement, "start", app); + const end = endDragged + ? isBindingEnabled + ? getElligibleElementForBindingElement(selectedElement, "end", app) + : null // If binding is disabled and end is dragged, break all binds + : // We have to update the focus and gap of the binding, so let's rebind + getElligibleElementForBindingElement(selectedElement, "end", app); + + return [start, end]; +}; + +const getBindingStrategyForDraggingArrowOrJoints = ( + selectedElement: NonDeleted, + app: AppClassProperties, + isBindingEnabled: boolean, +): (NonDeleted | null | "keep")[] => { + const [startIsClose, endIsClose] = getOriginalBindingsIfStillCloseToArrowEnds( + selectedElement, + app, + ); + const start = startIsClose + ? isBindingEnabled + ? getElligibleElementForBindingElement(selectedElement, "start", app) + : null + : null; + const end = endIsClose + ? isBindingEnabled + ? getElligibleElementForBindingElement(selectedElement, "end", app) + : null + : null; + + return [start, end]; +}; + +export const bindOrUnbindLinearElements = ( + selectedElements: NonDeleted[], + app: AppClassProperties, + isBindingEnabled: boolean, + draggingPoints: readonly number[] | null, ): void => { selectedElements.forEach((selectedElement) => { - if (isBindingElement(selectedElement)) { - bindOrUnbindLinearElement( - selectedElement, - getElligibleElementForBindingElement(selectedElement, "start", app), - getElligibleElementForBindingElement(selectedElement, "end", app), - app.scene.getNonDeletedElementsMap(), - ); - } else if (isBindableElement(selectedElement)) { - maybeBindBindableElement( - selectedElement, - app.scene.getNonDeletedElementsMap(), - app, - ); - } + const [start, end] = draggingPoints?.length + ? // The arrow edge points are dragged (i.e. start, end) + getBindingStrategyForDraggingArrowEndpoints( + selectedElement, + isBindingEnabled, + draggingPoints ?? [], + app, + ) + : // The arrow itself (the shaft) or the inner joins are dragged + getBindingStrategyForDraggingArrowOrJoints( + selectedElement, + app, + isBindingEnabled, + ); + + bindOrUnbindLinearElement( + selectedElement, + start, + end, + app.scene.getNonDeletedElementsMap(), + ); }); }; -const maybeBindBindableElement = ( - bindableElement: NonDeleted, - elementsMap: NonDeletedSceneElementsMap, +export const getSuggestedBindingsForArrows = ( + selectedElements: NonDeleted[], app: AppClassProperties, -): void => { - getElligibleElementsForBindableElementAndWhere(bindableElement, app).forEach( - ([linearElement, where]) => - bindOrUnbindLinearElement( - linearElement, - where === "end" ? "keep" : bindableElement, - where === "start" ? "keep" : bindableElement, - elementsMap, - ), +): SuggestedBinding[] => { + // HOT PATH: Bail out if selected elements list is too large + if (selectedElements.length > 50) { + return []; + } + + return ( + selectedElements + .filter(isLinearElement) + .flatMap((element) => + getOriginalBindingsIfStillCloseToArrowEnds(element, app), + ) + .filter( + (element): element is NonDeleted => + element !== null, + ) + // Filter out bind candidates which are in the + // same selection / group with the arrow + // + // TODO: Is it worth turning the list into a set to avoid dupes? + .filter( + (element) => + selectedElements.filter((selected) => selected.id === element?.id) + .length === 0, + ) ); }; @@ -283,29 +403,14 @@ export const isLinearElementSimpleAndAlreadyBound = ( bindableElement: ExcalidrawBindableElement, ): boolean => { return ( - alreadyBoundToId === bindableElement.id && linearElement.points.length < 3 + alreadyBoundToId === bindableElement.id && + isLinearElementSimple(linearElement) ); }; -export const unbindLinearElements = ( - elements: readonly NonDeleted[], - elementsMap: NonDeletedSceneElementsMap, -): void => { - elements.forEach((element) => { - if (isBindingElement(element)) { - if (element.startBinding !== null && element.endBinding !== null) { - bindOrUnbindLinearElement(element, null, null, elementsMap); - } else { - bindOrUnbindLinearElement( - element, - element.startBinding ? "keep" : null, - element.endBinding ? "keep" : null, - elementsMap, - ); - } - } - }); -}; +const isLinearElementSimple = ( + linearElement: NonDeleted, +): boolean => linearElement.points.length < 3; const unbindLinearElement = ( linearElement: NonDeleted, @@ -552,42 +657,6 @@ const maybeCalculateNewGapWhenScaling = ( return { elementId, gap: newGap, focus }; }; -// TODO: this is a bottleneck, optimise -export const getEligibleElementsForBinding = ( - selectedElements: NonDeleted[], - app: AppClassProperties, -): SuggestedBinding[] => { - const includedElementIds = new Set(selectedElements.map(({ id }) => id)); - return selectedElements.flatMap((selectedElement) => - isBindingElement(selectedElement, false) - ? (getElligibleElementsForBindingElement( - selectedElement as NonDeleted, - app, - ).filter( - (element) => !includedElementIds.has(element.id), - ) as SuggestedBinding[]) - : isBindableElement(selectedElement, false) - ? getElligibleElementsForBindableElementAndWhere( - selectedElement, - app, - ).filter((binding) => !includedElementIds.has(binding[0].id)) - : [], - ); -}; - -const getElligibleElementsForBindingElement = ( - linearElement: NonDeleted, - app: AppClassProperties, -): NonDeleted[] => { - return [ - getElligibleElementForBindingElement(linearElement, "start", app), - getElligibleElementForBindingElement(linearElement, "end", app), - ].filter( - (element): element is NonDeleted => - element != null, - ); -}; - const getElligibleElementForBindingElement = ( linearElement: NonDeleted, startOrEnd: "start" | "end", @@ -618,67 +687,6 @@ const getLinearElementEdgeCoors = ( ); }; -const getElligibleElementsForBindableElementAndWhere = ( - bindableElement: NonDeleted, - app: AppClassProperties, -): SuggestedPointBinding[] => { - const scene = Scene.getScene(bindableElement)!; - return scene - .getNonDeletedElements() - .map((element) => { - if (!isBindingElement(element, false)) { - return null; - } - const canBindStart = isLinearElementEligibleForNewBindingByBindable( - element, - "start", - bindableElement, - scene.getNonDeletedElementsMap(), - app, - ); - const canBindEnd = isLinearElementEligibleForNewBindingByBindable( - element, - "end", - bindableElement, - scene.getNonDeletedElementsMap(), - app, - ); - if (!canBindStart && !canBindEnd) { - return null; - } - return [ - element, - canBindStart && canBindEnd ? "both" : canBindStart ? "start" : "end", - bindableElement, - ]; - }) - .filter((maybeElement) => maybeElement != null) as SuggestedPointBinding[]; -}; - -const isLinearElementEligibleForNewBindingByBindable = ( - linearElement: NonDeleted, - startOrEnd: "start" | "end", - bindableElement: NonDeleted, - elementsMap: NonDeletedSceneElementsMap, - app: AppClassProperties, -): boolean => { - const existingBinding = - linearElement[startOrEnd === "start" ? "startBinding" : "endBinding"]; - return ( - existingBinding == null && - !isLinearElementSimpleAndAlreadyBoundOnOppositeEdge( - linearElement, - bindableElement, - startOrEnd, - ) && - bindingBorderTest( - bindableElement, - getLinearElementEdgeCoors(linearElement, startOrEnd, elementsMap), - app, - ) - ); -}; - // We need to: // 1: Update elements not selected to point to duplicated elements // 2: Update duplicated elements to point to other duplicated elements @@ -814,7 +822,7 @@ const newBoundElements = ( return nextBoundElements; }; -export const bindingBorderTest = ( +const bindingBorderTest = ( element: NonDeleted, { x, y }: { x: number; y: number }, app: AppClassProperties, @@ -836,7 +844,7 @@ export const maxBindingGap = ( return Math.max(16, Math.min(0.25 * smallerDimension, 32)); }; -export const distanceToBindableElement = ( +const distanceToBindableElement = ( element: ExcalidrawBindableElement, point: Point, elementsMap: ElementsMap, @@ -893,7 +901,7 @@ const distanceToDiamond = ( return GAPoint.distanceToLine(pointRel, side); }; -export const distanceToEllipse = ( +const distanceToEllipse = ( element: ExcalidrawEllipseElement, point: Point, elementsMap: ElementsMap, @@ -1012,7 +1020,7 @@ const coordsCenter = ( // all focus points lie, so it's a number between -1 and 1. // The line going through `a` and `b` is a tangent to the "focus image" // of the element. -export const determineFocusDistance = ( +const determineFocusDistance = ( element: ExcalidrawBindableElement, // Point on the line, in absolute coordinates a: Point, @@ -1053,7 +1061,7 @@ export const determineFocusDistance = ( return ret || 0; }; -export const determineFocusPoint = ( +const determineFocusPoint = ( element: ExcalidrawBindableElement, // The oriented, relative distance from the center of `element` of the // returned focusPoint @@ -1093,7 +1101,7 @@ export const determineFocusPoint = ( // Returns 2 or 0 intersection points between line going through `a` and `b` // and the `element`, in ascending order of distance from `a`. -export const intersectElementWithLine = ( +const intersectElementWithLine = ( element: ExcalidrawBindableElement, // Point on the line, in absolute coordinates a: Point, @@ -1260,7 +1268,7 @@ const getEllipseIntersections = ( ]; }; -export const getCircleIntersections = ( +const getCircleIntersections = ( center: GA.Point, radius: number, line: GA.Line, @@ -1290,7 +1298,7 @@ export const getCircleIntersections = ( // The focus point is the tangent point of the "focus image" of the // `element`, where the tangent goes through `point`. -export const findFocusPointForEllipse = ( +const findFocusPointForEllipse = ( ellipse: ExcalidrawEllipseElement, // Between -1 and 1 (not 0) the relative size of the "focus image" of // the element on which the focus point lies @@ -1327,7 +1335,7 @@ export const findFocusPointForEllipse = ( return GA.point(x, (-m * x - 1) / n); }; -export const findFocusPointForRectangulars = ( +const findFocusPointForRectangulars = ( element: | ExcalidrawRectangleElement | ExcalidrawImageElement diff --git a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap index 03b1b6b312..be1883d648 100644 --- a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap @@ -71,13 +71,13 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "penMode": false, "pendingImageElementId": null, "previousSelectedElementIds": { - "id158": true, + "id159": true, }, "resizingElement": null, "scrollX": 0, "scrollY": 0, "selectedElementIds": { - "id158": true, + "id159": true, }, "selectedElementsAreBeingDragged": false, "selectedGroupIds": {}, @@ -112,7 +112,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 100, - "id": "id156", + "id": "id157", "index": "a0", "isDeleted": false, "link": null, @@ -127,7 +127,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "rectangle", "updated": 1, - "version": 8, + "version": 18, "width": 100, "x": -100, "y": -50, @@ -144,7 +144,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 100, - "id": "id157", + "id": "id158", "index": "a1", "isDeleted": false, "link": null, @@ -159,7 +159,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "rectangle", "updated": 1, - "version": 9, + "version": 19, "width": 100, "x": 100, "y": -50, @@ -174,7 +174,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "customData": undefined, "endArrowhead": "arrow", "endBinding": { - "elementId": "id160", + "elementId": "id162", "focus": 0, "gap": 1, }, @@ -182,7 +182,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 99.19725525211979, - "id": "id158", + "id": "id159", "index": "a2", "isDeleted": false, "lastCommittedPoint": null, @@ -210,9 +210,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 24, + "version": 54, "width": 98.40367721010284, - "x": 1.000000000000007, + "x": 1, "y": 0, } `; @@ -223,7 +223,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "backgroundColor": "transparent", "boundElements": [ { - "id": "id158", + "id": "id159", "type": "arrow", }, ], @@ -232,7 +232,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 50, - "id": "id160", + "id": "id162", "index": "a3", "isDeleted": false, "link": null, @@ -274,19 +274,68 @@ History { "added": Map {}, "removed": Map {}, "updated": Map { - "id156" => Delta { + "id159" => Delta { "deleted": { - "boundElements": [], + "endBinding": { + "elementId": "id158", + "focus": 0.009900990099009901, + "gap": 1, + }, + "height": 0.9800031696987099, + "points": [ + [ + 0, + 0, + ], + [ + 98, + -0.9800031696987099, + ], + ], + "startBinding": { + "elementId": "id157", + "focus": 0.0297029702970297, + "gap": 1, + }, }, "inserted": { - "boundElements": [ - { - "id": "id158", - "type": "arrow", - }, + "endBinding": { + "elementId": "id158", + "focus": -0.02, + "gap": 1, + }, + "height": 0.0002487679019458344, + "points": [ + [ + 0, + 0, + ], + [ + 98, + 0.0002487679019458344, + ], ], + "startBinding": { + "elementId": "id157", + "focus": 0.02, + "gap": 1, + }, }, }, + }, + }, + }, + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": {}, + "inserted": {}, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map { "id157" => Delta { "deleted": { "boundElements": [], @@ -294,16 +343,29 @@ History { "inserted": { "boundElements": [ { - "id": "id158", + "id": "id159", "type": "arrow", }, ], }, }, "id158" => Delta { + "deleted": { + "boundElements": [], + }, + "inserted": { + "boundElements": [ + { + "id": "id159", + "type": "arrow", + }, + ], + }, + }, + "id159" => Delta { "deleted": { "endBinding": { - "elementId": "id160", + "elementId": "id162", "focus": 0, "gap": 1, }, @@ -323,11 +385,11 @@ History { }, "inserted": { "endBinding": { - "elementId": "id157", + "elementId": "id158", "focus": 0.009900990099009901, "gap": 1, }, - "height": 0.9800000000000002, + "height": 0.9802432787444684, "points": [ [ 0, @@ -335,22 +397,22 @@ History { ], [ 98, - -0.9800000000000002, + -0.9802432787444684, ], ], "startBinding": { - "elementId": "id156", + "elementId": "id157", "focus": 0.0297029702970297, "gap": 1, }, - "y": 0.9900000000000004, + "y": 0.9903686540602428, }, }, - "id160" => Delta { + "id162" => Delta { "deleted": { "boundElements": [ { - "id": "id158", + "id": "id159", "type": "arrow", }, ], @@ -374,7 +436,7 @@ History { "elementsChange": ElementsChange { "added": Map {}, "removed": Map { - "id156" => Delta { + "id157" => Delta { "deleted": { "angle": 0, "backgroundColor": "transparent", @@ -405,7 +467,7 @@ History { "isDeleted": true, }, }, - "id157" => Delta { + "id158" => Delta { "deleted": { "angle": 0, "backgroundColor": "transparent", @@ -445,9 +507,9 @@ History { "delta": Delta { "deleted": { "selectedElementIds": { - "id158": true, + "id159": true, }, - "selectedLinearElementId": "id158", + "selectedLinearElementId": "id159", }, "inserted": { "selectedElementIds": {}, @@ -458,7 +520,7 @@ History { "elementsChange": ElementsChange { "added": Map {}, "removed": Map { - "id158" => Delta { + "id159" => Delta { "deleted": { "angle": 0, "backgroundColor": "transparent", @@ -514,7 +576,7 @@ History { exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and the arrow got bound to a different element in the meantime > [end of test] number of elements 1`] = `4`; -exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and the arrow got bound to a different element in the meantime > [end of test] number of renders 1`] = `15`; +exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and the arrow got bound to a different element in the meantime > [end of test] number of renders 1`] = `22`; exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and there are no conflicting updates in the meantime > [end of test] appState 1`] = ` { @@ -722,7 +784,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 23, + "version": 34, "width": 0, "x": 251, "y": 0, @@ -738,6 +800,46 @@ History { ], }, "redoStack": [ + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": {}, + "inserted": {}, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map { + "id154" => Delta { + "deleted": { + "points": [ + [ + 0, + 0, + ], + [ + 0, + 0, + ], + ], + }, + "inserted": { + "points": [ + [ + 0, + 0, + ], + [ + 100, + 0, + ], + ], + }, + }, + }, + }, + }, HistoryEntry { "appStateChange": AppStateChange { "delta": Delta { @@ -968,7 +1070,7 @@ History { exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and there are no conflicting updates in the meantime > [end of test] number of elements 1`] = `3`; -exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and there are no conflicting updates in the meantime > [end of test] number of renders 1`] = `16`; +exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and there are no conflicting updates in the meantime > [end of test] number of renders 1`] = `24`; exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind remotely added arrow when it's bindable elements are added through the history > [end of test] appState 1`] = ` { @@ -1079,7 +1181,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "customData": undefined, "endArrowhead": null, "endBinding": { - "elementId": "id162", + "elementId": "id164", "focus": 0, "gap": 1, }, @@ -1087,7 +1189,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 0.03596020595764898, - "id": "id163", + "id": "id165", "index": "Zz", "isDeleted": false, "lastCommittedPoint": null, @@ -1110,7 +1212,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl }, "startArrowhead": null, "startBinding": { - "elementId": "id161", + "elementId": "id163", "focus": 0, "gap": 1, }, @@ -1132,7 +1234,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "backgroundColor": "transparent", "boundElements": [ { - "id": "id163", + "id": "id165", "type": "arrow", }, ], @@ -1141,7 +1243,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 100, - "id": "id161", + "id": "id163", "index": "a0", "isDeleted": false, "link": null, @@ -1169,7 +1271,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "backgroundColor": "transparent", "boundElements": [ { - "id": "id163", + "id": "id165", "type": "arrow", }, ], @@ -1178,7 +1280,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 100, - "id": "id162", + "id": "id164", "index": "a1", "isDeleted": false, "link": null, @@ -1220,7 +1322,7 @@ History { "elementsChange": ElementsChange { "added": Map {}, "removed": Map { - "id161" => Delta { + "id163" => Delta { "deleted": { "angle": 0, "backgroundColor": "transparent", @@ -1251,7 +1353,7 @@ History { "isDeleted": true, }, }, - "id162" => Delta { + "id164" => Delta { "deleted": { "angle": 0, "backgroundColor": "transparent", @@ -1284,15 +1386,15 @@ History { }, }, "updated": Map { - "id163" => Delta { + "id165" => Delta { "deleted": { "endBinding": { - "elementId": "id162", + "elementId": "id164", "focus": 0, "gap": 1, }, "startBinding": { - "elementId": "id161", + "elementId": "id163", "focus": 0, "gap": 1, }, @@ -1422,7 +1524,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "customData": undefined, "endArrowhead": null, "endBinding": { - "elementId": "id165", + "elementId": "id167", "focus": 0, "gap": 1, }, @@ -1430,7 +1532,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 0.03596020595764898, - "id": "id166", + "id": "id168", "index": "a0", "isDeleted": false, "lastCommittedPoint": null, @@ -1453,7 +1555,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl }, "startArrowhead": null, "startBinding": { - "elementId": "id164", + "elementId": "id166", "focus": 0, "gap": 1, }, @@ -1475,7 +1577,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "backgroundColor": "transparent", "boundElements": [ { - "id": "id166", + "id": "id168", "type": "arrow", }, ], @@ -1484,7 +1586,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 100, - "id": "id164", + "id": "id166", "index": "a0V", "isDeleted": false, "link": null, @@ -1512,7 +1614,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "backgroundColor": "transparent", "boundElements": [ { - "id": "id166", + "id": "id168", "type": "arrow", }, ], @@ -1521,7 +1623,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 100, - "id": "id165", + "id": "id167", "index": "a1", "isDeleted": false, "link": null, @@ -1563,7 +1665,7 @@ History { "elementsChange": ElementsChange { "added": Map {}, "removed": Map { - "id166" => Delta { + "id168" => Delta { "deleted": { "angle": 0, "backgroundColor": "transparent", @@ -1571,7 +1673,7 @@ History { "customData": undefined, "endArrowhead": null, "endBinding": { - "elementId": "id165", + "elementId": "id167", "focus": 0, "gap": 1, }, @@ -1601,7 +1703,7 @@ History { }, "startArrowhead": null, "startBinding": { - "elementId": "id164", + "elementId": "id166", "focus": 0, "gap": 1, }, @@ -1619,11 +1721,11 @@ History { }, }, "updated": Map { - "id164" => Delta { + "id166" => Delta { "deleted": { "boundElements": [ { - "id": "id166", + "id": "id168", "type": "arrow", }, ], @@ -1632,11 +1734,11 @@ History { "boundElements": [], }, }, - "id165" => Delta { + "id167" => Delta { "deleted": { "boundElements": [ { - "id": "id166", + "id": "id168", "type": "arrow", }, ], @@ -1767,7 +1869,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 100, - "id": "id167", + "id": "id169", "index": "a0", "isDeleted": false, "link": null, @@ -1799,7 +1901,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 100, - "id": "id168", + "id": "id170", "index": "a1", "isDeleted": false, "link": null, @@ -1841,7 +1943,7 @@ History { "elementsChange": ElementsChange { "added": Map {}, "removed": Map { - "id167" => Delta { + "id169" => Delta { "deleted": { "angle": 0, "backgroundColor": "transparent", @@ -1872,7 +1974,7 @@ History { "isDeleted": true, }, }, - "id168" => Delta { + "id170" => Delta { "deleted": { "angle": 0, "backgroundColor": "transparent", @@ -1990,7 +2092,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "scrollX": 0, "scrollY": 0, "selectedElementIds": { - "id171": true, + "id173": true, }, "selectedElementsAreBeingDragged": false, "selectedGroupIds": {}, @@ -2021,7 +2123,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "backgroundColor": "transparent", "boundElements": [ { - "id": "id171", + "id": "id173", "type": "arrow", }, ], @@ -2030,7 +2132,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 100, - "id": "id169", + "id": "id171", "index": "a0", "isDeleted": false, "link": null, @@ -2058,7 +2160,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "backgroundColor": "transparent", "boundElements": [ { - "id": "id171", + "id": "id173", "type": "arrow", }, ], @@ -2067,7 +2169,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 100, - "id": "id170", + "id": "id172", "index": "a1", "isDeleted": false, "link": null, @@ -2097,7 +2199,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "customData": undefined, "endArrowhead": "arrow", "endBinding": { - "elementId": "id170", + "elementId": "id172", "focus": 0, "gap": 1, }, @@ -2105,7 +2207,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "frameId": null, "groupIds": [], "height": 373.7994222717614, - "id": "id171", + "id": "id173", "index": "a2", "isDeleted": false, "lastCommittedPoint": null, @@ -2128,7 +2230,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl }, "startArrowhead": null, "startBinding": { - "elementId": "id169", + "elementId": "id171", "focus": 0, "gap": 1, }, @@ -2164,7 +2266,7 @@ History { "elementsChange": ElementsChange { "added": Map {}, "removed": Map { - "id169" => Delta { + "id171" => Delta { "deleted": { "angle": 0, "backgroundColor": "transparent", @@ -2195,7 +2297,7 @@ History { "isDeleted": true, }, }, - "id170" => Delta { + "id172" => Delta { "deleted": { "angle": 0, "backgroundColor": "transparent", @@ -2235,9 +2337,9 @@ History { "delta": Delta { "deleted": { "selectedElementIds": { - "id171": true, + "id173": true, }, - "selectedLinearElementId": "id171", + "selectedLinearElementId": "id173", }, "inserted": { "selectedElementIds": {}, @@ -2248,7 +2350,7 @@ History { "elementsChange": ElementsChange { "added": Map {}, "removed": Map { - "id171" => Delta { + "id173" => Delta { "deleted": { "angle": 0, "backgroundColor": "transparent", @@ -2256,7 +2358,7 @@ History { "customData": undefined, "endArrowhead": "arrow", "endBinding": { - "elementId": "id170", + "elementId": "id172", "focus": 0, "gap": 1, }, @@ -2286,7 +2388,7 @@ History { }, "startArrowhead": null, "startBinding": { - "elementId": "id169", + "elementId": "id171", "focus": 0, "gap": 1, }, @@ -2304,11 +2406,11 @@ History { }, }, "updated": Map { - "id169" => Delta { + "id171" => Delta { "deleted": { "boundElements": [ { - "id": "id171", + "id": "id173", "type": "arrow", }, ], @@ -2317,11 +2419,11 @@ History { "boundElements": [], }, }, - "id170" => Delta { + "id172" => Delta { "deleted": { "boundElements": [ { - "id": "id171", + "id": "id173", "type": "arrow", }, ], @@ -5199,7 +5301,7 @@ exports[`history > multiplayer undo/redo > conflicts in frames and their childre "frameId": null, "groupIds": [], "height": 100, - "id": "id173", + "id": "id175", "index": "Zz", "isDeleted": false, "link": null, @@ -5231,7 +5333,7 @@ exports[`history > multiplayer undo/redo > conflicts in frames and their childre "frameId": null, "groupIds": [], "height": 500, - "id": "id172", + "id": "id174", "index": "a0", "isDeleted": true, "link": null, @@ -5274,7 +5376,7 @@ History { "elementsChange": ElementsChange { "added": Map {}, "removed": Map { - "id173" => Delta { + "id175" => Delta { "deleted": { "angle": 0, "backgroundColor": "transparent", @@ -5320,9 +5422,9 @@ History { "added": Map {}, "removed": Map {}, "updated": Map { - "id173" => Delta { + "id175" => Delta { "deleted": { - "frameId": "id172", + "frameId": "id174", }, "inserted": { "frameId": null, diff --git a/packages/excalidraw/tests/binding.test.tsx b/packages/excalidraw/tests/binding.test.tsx index 24d0bbf688..ece2d2fe3e 100644 --- a/packages/excalidraw/tests/binding.test.tsx +++ b/packages/excalidraw/tests/binding.test.tsx @@ -20,6 +20,7 @@ describe("element binding", () => { const rect = API.createElement({ type: "rectangle", x: 0, + y: 0, width: 50, height: 50, }); @@ -39,32 +40,43 @@ describe("element binding", () => { h.elements = [rect, arrow]; expect(arrow.startBinding).toBe(null); - API.setSelectedElements([arrow]); + // select arrow + mouse.clickAt(150, 0); - expect(API.getSelectedElements()).toEqual([arrow]); + // move arrow start to potential binding position mouse.downAt(100, 0); mouse.moveTo(55, 0); mouse.up(0, 0); - expect(API.getSelectedElements()).toEqual([arrow]); - expect(arrow.startBinding).toEqual({ - elementId: rect.id, - focus: expect.toBeNonNaNNumber(), - gap: expect.toBeNonNaNNumber(), - }); - mouse.downAt(100, 0); - mouse.move(-45, 0); - mouse.up(); - expect(arrow.startBinding).toEqual({ - elementId: rect.id, - focus: expect.toBeNonNaNNumber(), - gap: expect.toBeNonNaNNumber(), - }); - - mouse.down(); - mouse.move(-50, 0); - mouse.up(); + // Point selection is evaluated like the points are rendered, + // from right to left. So clicking on the first point should move the joint, + // not the start point. expect(arrow.startBinding).toBe(null); + + // Now that the start point is free, move it into overlapping position + mouse.downAt(100, 0); + mouse.moveTo(55, 0); + mouse.up(0, 0); + + expect(API.getSelectedElements()).toEqual([arrow]); + + expect(arrow.startBinding).toEqual({ + elementId: rect.id, + focus: expect.toBeNonNaNNumber(), + gap: expect.toBeNonNaNNumber(), + }); + + // Move the end point to the overlapping binding position + mouse.downAt(200, 0); + mouse.moveTo(55, 0); + mouse.up(0, 0); + + // Both the start and the end points should be bound + expect(arrow.startBinding).toEqual({ + elementId: rect.id, + focus: expect.toBeNonNaNNumber(), + gap: expect.toBeNonNaNNumber(), + }); expect(arrow.endBinding).toEqual({ elementId: rect.id, focus: expect.toBeNonNaNNumber(), @@ -144,7 +156,7 @@ describe("element binding", () => { }, ); - it("should bind/unbind arrow when moving it with keyboard", () => { + it("should unbind arrow when moving it with keyboard", () => { const rectangle = UI.createElement("rectangle", { x: 75, y: 0, @@ -160,11 +172,22 @@ describe("element binding", () => { expect(arrow.endBinding).toBe(null); + mouse.downAt(50, 50); + mouse.moveTo(51, 0); + mouse.up(0, 0); + + // Test sticky connection expect(API.getSelectedElement().type).toBe("arrow"); Keyboard.keyPress(KEYS.ARROW_RIGHT); expect(arrow.endBinding?.elementId).toBe(rectangle.id); - Keyboard.keyPress(KEYS.ARROW_LEFT); + expect(arrow.endBinding?.elementId).toBe(rectangle.id); + + // Sever connection + expect(API.getSelectedElement().type).toBe("arrow"); + Keyboard.keyPress(KEYS.ARROW_LEFT); + expect(arrow.endBinding).toBe(null); + Keyboard.keyPress(KEYS.ARROW_RIGHT); expect(arrow.endBinding).toBe(null); }); diff --git a/packages/excalidraw/tests/history.test.tsx b/packages/excalidraw/tests/history.test.tsx index 1f3d5e8d4e..e8fa06aa03 100644 --- a/packages/excalidraw/tests/history.test.tsx +++ b/packages/excalidraw/tests/history.test.tsx @@ -4014,12 +4014,18 @@ describe("history", () => { const arrowId = h.elements[2].id; - // create binding + // create start binding mouse.downAt(0, 0); mouse.moveTo(0, 1); mouse.moveTo(0, 0); mouse.up(); + // create end binding + mouse.downAt(100, 0); + mouse.moveTo(100, 1); + mouse.moveTo(100, 0); + mouse.up(); + expect(h.elements).toEqual([ expect.objectContaining({ id: rect1.id, @@ -4044,9 +4050,10 @@ describe("history", () => { }), ]); - Keyboard.undo(); + Keyboard.undo(); // undo start binding + Keyboard.undo(); // undo end binding expect(API.getUndoStack().length).toBe(2); - expect(API.getRedoStack().length).toBe(1); + expect(API.getRedoStack().length).toBe(2); expect(h.elements).toEqual([ expect.objectContaining({ id: rect1.id, @@ -4081,7 +4088,8 @@ describe("history", () => { runTwice(() => { Keyboard.redo(); - expect(API.getUndoStack().length).toBe(3); + Keyboard.redo(); + expect(API.getUndoStack().length).toBe(4); expect(API.getRedoStack().length).toBe(0); expect(h.elements).toEqual([ expect.objectContaining({ @@ -4107,9 +4115,10 @@ describe("history", () => { }), ]); + Keyboard.undo(); Keyboard.undo(); expect(API.getUndoStack().length).toBe(2); - expect(API.getRedoStack().length).toBe(1); + expect(API.getRedoStack().length).toBe(2); expect(h.elements).toEqual([ expect.objectContaining({ id: rect1.id, @@ -4135,11 +4144,16 @@ describe("history", () => { const arrowId = h.elements[2].id; - // create binding + // create start binding mouse.downAt(0, 0); mouse.moveTo(0, 1); mouse.upAt(0, 0); + // create end binding + mouse.downAt(100, 0); + mouse.moveTo(100, 1); + mouse.upAt(100, 0); + expect(h.elements).toEqual([ expect.objectContaining({ id: rect1.id, @@ -4164,9 +4178,10 @@ describe("history", () => { }), ]); + Keyboard.undo(); Keyboard.undo(); expect(API.getUndoStack().length).toBe(2); - expect(API.getRedoStack().length).toBe(1); + expect(API.getRedoStack().length).toBe(2); expect(h.elements).toEqual([ expect.objectContaining({ id: rect1.id, @@ -4202,7 +4217,8 @@ describe("history", () => { runTwice(() => { Keyboard.redo(); - expect(API.getUndoStack().length).toBe(3); + Keyboard.redo(); + expect(API.getUndoStack().length).toBe(4); expect(API.getRedoStack().length).toBe(0); expect(h.elements).toEqual([ expect.objectContaining({ @@ -4233,9 +4249,10 @@ describe("history", () => { }), ]); + Keyboard.undo(); Keyboard.undo(); expect(API.getUndoStack().length).toBe(2); - expect(API.getRedoStack().length).toBe(1); + expect(API.getRedoStack().length).toBe(2); expect(h.elements).toEqual([ expect.objectContaining({ id: rect1.id,