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,