From 7ebeae2d387270d8cb064d9645ec1ac812dd2a7b Mon Sep 17 00:00:00 2001 From: Michal Srb Date: Sat, 29 Aug 2020 17:56:03 +0200 Subject: [PATCH] Fix arrow rebinding on rotation (take 2) (#2104) * Clear up test, fix simple rotation * Fix eligibility rules --- src/components/App.tsx | 17 +++------ src/element/binding.ts | 73 +++++++++++++++----------------------- src/tests/binding.test.tsx | 47 ++++++++++++------------ 3 files changed, 56 insertions(+), 81 deletions(-) diff --git a/src/components/App.tsx b/src/components/App.tsx index 11b0b235..3d53a85a 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -167,7 +167,6 @@ import { bindOrUnbindSelectedElements, unbindLinearElements, fixBindingsAfterDuplication, - getElligibleElementForBindingElementAtCoors, fixBindingsAfterDeletion, isLinearElementSimpleAndAlreadyBound, isBindingEnabled, @@ -2944,7 +2943,6 @@ class App extends React.Component { ) ) { this.maybeSuggestBindingForAll(selectedElements); - bindOrUnbindSelectedElements(selectedElements); return; } } @@ -3201,6 +3199,7 @@ class App extends React.Component { elementType, elementLocked, isResizing, + isRotating, } = this.state; this.setState({ @@ -3313,7 +3312,6 @@ class App extends React.Component { })); } } - return; } @@ -3337,12 +3335,6 @@ class App extends React.Component { draggingElement, getNormalizedDimensions(draggingElement), ); - - if (isBindingEnabled(this.state)) { - bindOrUnbindSelectedElements( - getSelectedElements(this.scene.getElements(), this.state), - ); - } } if (resizingElement) { @@ -3470,7 +3462,7 @@ class App extends React.Component { history.resumeRecording(); } - if (pointerDownState.drag.hasOccurred || isResizing) { + if (pointerDownState.drag.hasOccurred || isResizing || isRotating) { (isBindingEnabled(this.state) ? bindOrUnbindSelectedElements : unbindLinearElements)( @@ -3519,10 +3511,9 @@ class App extends React.Component { // into `linearElement` oppositeBindingBoundElement?: ExcalidrawBindableElement | null, ): void => { - const hoveredBindableElement = getElligibleElementForBindingElementAtCoors( - linearElement, - startOrEnd, + const hoveredBindableElement = getHoveredElementForBinding( pointerCoords, + this.scene, ); this.setState({ suggestedBindings: diff --git a/src/element/binding.ts b/src/element/binding.ts index a38cac43..5599866c 100644 --- a/src/element/binding.ts +++ b/src/element/binding.ts @@ -46,6 +46,7 @@ export const bindOrUnbindLinearElement = ( bindOrUnbindLinearElementEdge( linearElement, startBindingElement, + endBindingElement, "start", boundToElementIds, unboundFromElementIds, @@ -53,6 +54,7 @@ export const bindOrUnbindLinearElement = ( bindOrUnbindLinearElementEdge( linearElement, endBindingElement, + startBindingElement, "end", boundToElementIds, unboundFromElementIds, @@ -75,6 +77,7 @@ export const bindOrUnbindLinearElement = ( const bindOrUnbindLinearElementEdge = ( linearElement: NonDeleted, bindableElement: ExcalidrawBindableElement | null | "keep", + otherEdgeBindableElement: ExcalidrawBindableElement | null | "keep", startOrEnd: "start" | "end", // Is mutated boundToElementIds: Set, @@ -83,8 +86,22 @@ const bindOrUnbindLinearElementEdge = ( ): void => { if (bindableElement !== "keep") { if (bindableElement != null) { - bindLinearElement(linearElement, bindableElement, startOrEnd); - boundToElementIds.add(bindableElement.id); + // 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); + boundToElementIds.add(bindableElement.id); + } } else { const unbound = unbindLinearElement(linearElement, startOrEnd); if (unbound != null) { @@ -110,7 +127,7 @@ export const bindOrUnbindSelectedElements = ( }); }; -export const maybeBindBindableElement = ( +const maybeBindBindableElement = ( bindableElement: NonDeleted, ): void => { getElligibleElementsForBindableElementAndWhere( @@ -134,7 +151,14 @@ export const maybeBindLinearElement = ( bindLinearElement(linearElement, appState.startBoundElement, "start"); } const hoveredElement = getHoveredElementForBinding(pointerCoords, scene); - if (hoveredElement != null) { + if ( + hoveredElement != null && + !isLinearElementSimpleAndAlreadyBoundOnOppositeEdge( + linearElement, + hoveredElement, + "end", + ) + ) { bindLinearElement(linearElement, hoveredElement, "end"); } }; @@ -144,15 +168,6 @@ const bindLinearElement = ( hoveredElement: ExcalidrawBindableElement, startOrEnd: "start" | "end", ): void => { - if ( - isLinearElementSimpleAndAlreadyBoundOnOppositeEdge( - linearElement, - hoveredElement, - startOrEnd, - ) - ) { - return; - } mutateElement(linearElement, { [startOrEnd === "start" ? "startBinding" : "endBinding"]: { elementId: hoveredElement.id, @@ -442,40 +457,10 @@ const getElligibleElementForBindingElement = ( linearElement: NonDeleted, startOrEnd: "start" | "end", ): NonDeleted | null => { - return getElligibleElementForBindingElementAtCoors( - linearElement, - startOrEnd, + return getHoveredElementForBinding( getLinearElementEdgeCoors(linearElement, startOrEnd), - ); -}; - -export const getElligibleElementForBindingElementAtCoors = ( - linearElement: NonDeleted, - startOrEnd: "start" | "end", - pointerCoords: { - x: number; - y: number; - }, -): NonDeleted | null => { - const bindableElement = getHoveredElementForBinding( - pointerCoords, Scene.getScene(linearElement)!, ); - if (bindableElement == null) { - return null; - } - // Note: We could push this check inside a version of - // `getHoveredElementForBinding`, but it's unlikely this is needed. - if ( - isLinearElementSimpleAndAlreadyBoundOnOppositeEdge( - linearElement, - bindableElement, - startOrEnd, - ) - ) { - return null; - } - return bindableElement; }; const getLinearElementEdgeCoors = ( diff --git a/src/tests/binding.test.tsx b/src/tests/binding.test.tsx index b4204640..173e15c3 100644 --- a/src/tests/binding.test.tsx +++ b/src/tests/binding.test.tsx @@ -13,37 +13,36 @@ describe("element binding", () => { render(); }); - // NOTE if this tests fails, skip it -- it was really flaky at one point it("rotation of arrow should rebind both ends", () => { - const rect1 = UI.createElement("rectangle", { + const rectLeft = UI.createElement("rectangle", { x: 0, - width: 100, - height: 1000, + width: 200, + height: 500, }); - const rect2 = UI.createElement("rectangle", { - x: 200, - width: 100, - height: 1000, + const rectRight = UI.createElement("rectangle", { + x: 400, + width: 200, + height: 500, }); const arrow = UI.createElement("arrow", { - x: 110, - y: 50, - width: 80, + x: 220, + y: 250, + width: 160, height: 1, }); - expect(arrow.startBinding?.elementId).toBe(rect1.id); - expect(arrow.endBinding?.elementId).toBe(rect2.id); + expect(arrow.startBinding?.elementId).toBe(rectLeft.id); + expect(arrow.endBinding?.elementId).toBe(rectRight.id); - const { rotation } = getTransformHandles(arrow, h.state.zoom, "mouse"); - if (rotation) { - const rotationHandleX = rotation[0] + rotation[2] / 2; - const rotationHandleY = rotation[1] + rotation[3] / 2; - mouse.down(rotationHandleX, rotationHandleY); - mouse.move(0, 1000); - mouse.up(); - } - expect(arrow.angle).toBeGreaterThan(3); - expect(arrow.startBinding?.elementId).toBe(rect2.id); - expect(arrow.endBinding?.elementId).toBe(rect1.id); + const rotation = getTransformHandles(arrow, h.state.zoom, "mouse") + .rotation!; + const rotationHandleX = rotation[0] + rotation[2] / 2; + const rotationHandleY = rotation[1] + rotation[3] / 2; + mouse.down(rotationHandleX, rotationHandleY); + mouse.move(300, 400); + mouse.up(); + expect(arrow.angle).toBeGreaterThan(0.7 * Math.PI); + expect(arrow.angle).toBeLessThan(1.3 * Math.PI); + expect(arrow.startBinding?.elementId).toBe(rectRight.id); + expect(arrow.endBinding?.elementId).toBe(rectLeft.id); }); });