Fix arrow rebinding on rotation (take 2) (#2104)

* Clear up test, fix simple rotation
* Fix eligibility rules

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<ExcalidrawProps, AppState> {
           )
         ) {
           this.maybeSuggestBindingForAll(selectedElements);
-          bindOrUnbindSelectedElements(selectedElements);
           return;
         }
       }
@@ -3201,6 +3199,7 @@ class App extends React.Component<ExcalidrawProps, AppState> {
         elementType,
         elementLocked,
         isResizing,
+        isRotating,
       } = this.state;
 
       this.setState({
@@ -3313,7 +3312,6 @@ class App extends React.Component<ExcalidrawProps, AppState> {
             }));
           }
         }
-
         return;
       }
 
@@ -3337,12 +3335,6 @@ class App extends React.Component<ExcalidrawProps, AppState> {
           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<ExcalidrawProps, AppState> {
         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<ExcalidrawProps, AppState> {
     // into `linearElement`
     oppositeBindingBoundElement?: ExcalidrawBindableElement | null,
   ): void => {
-    const hoveredBindableElement = getElligibleElementForBindingElementAtCoors(
-      linearElement,
-      startOrEnd,
+    const hoveredBindableElement = getHoveredElementForBinding(
       pointerCoords,
+      this.scene,
     );
     this.setState({
       suggestedBindings:

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<ExcalidrawLinearElement>,
   bindableElement: ExcalidrawBindableElement | null | "keep",
+  otherEdgeBindableElement: ExcalidrawBindableElement | null | "keep",
   startOrEnd: "start" | "end",
   // Is mutated
   boundToElementIds: Set<ExcalidrawBindableElement["id"]>,
@@ -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<ExcalidrawBindableElement>,
 ): 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<ExcalidrawLinearElement>,
   startOrEnd: "start" | "end",
 ): NonDeleted<ExcalidrawBindableElement> | null => {
-  return getElligibleElementForBindingElementAtCoors(
-    linearElement,
-    startOrEnd,
+  return getHoveredElementForBinding(
     getLinearElementEdgeCoors(linearElement, startOrEnd),
-  );
-};
-
-export const getElligibleElementForBindingElementAtCoors = (
-  linearElement: NonDeleted<ExcalidrawLinearElement>,
-  startOrEnd: "start" | "end",
-  pointerCoords: {
-    x: number;
-    y: number;
-  },
-): NonDeleted<ExcalidrawBindableElement> | 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 = (

src/tests/binding.test.tsx

@@ -13,37 +13,36 @@ describe("element binding", () => {
     render(<App />);
   });
 
-  // 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);
   });
 });