From f3f0ab7c8362750593ccc65a973d06b62925be35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rk=20Tolm=C3=A1cs?= Date: Thu, 19 Sep 2024 08:47:23 +0200 Subject: [PATCH 1/2] fix: Elbow arrow fixedpoint flipping now properly flips on inverted resize and flip action (#8324) * Flipping action now properly mirrors selections with elbow arrows * Flipping action now re-centers the selection to the original center to avoid "walking" selections on repeated flipping --- .../excalidraw/actions/actionFlip.test.tsx | 89 +++++++++++++++++++ packages/excalidraw/actions/actionFlip.ts | 51 ++++++++++- .../excalidraw/actions/actionProperties.tsx | 13 --- packages/excalidraw/components/App.tsx | 54 +++++++---- packages/excalidraw/data/restore.ts | 14 +-- packages/excalidraw/element/binding.ts | 5 +- packages/excalidraw/element/dragElements.ts | 9 +- .../excalidraw/element/linearElementEditor.ts | 6 +- packages/excalidraw/element/resizeElements.ts | 18 +--- packages/excalidraw/element/routing.ts | 10 +-- packages/excalidraw/element/typeChecks.ts | 7 +- packages/excalidraw/element/types.ts | 19 ++-- .../excalidraw/renderer/interactiveScene.ts | 2 - .../regressionTests.test.tsx.snap | 4 + packages/excalidraw/tests/helpers/api.ts | 6 +- packages/excalidraw/tests/history.test.tsx | 9 +- packages/excalidraw/tests/resize.test.tsx | 59 +++++++++++- 17 files changed, 290 insertions(+), 85 deletions(-) create mode 100644 packages/excalidraw/actions/actionFlip.test.tsx diff --git a/packages/excalidraw/actions/actionFlip.test.tsx b/packages/excalidraw/actions/actionFlip.test.tsx new file mode 100644 index 000000000000..0a1b9f41d513 --- /dev/null +++ b/packages/excalidraw/actions/actionFlip.test.tsx @@ -0,0 +1,89 @@ +import React from "react"; +import { Excalidraw } from "../index"; +import { render } from "../tests/test-utils"; +import { API } from "../tests/helpers/api"; +import { point } from "../../math"; +import { actionFlipHorizontal } from "./actionFlip"; + +const { h } = window; + +const testElements = [ + API.createElement({ + type: "rectangle", + id: "rec1", + x: 1046, + y: 541, + width: 100, + height: 100, + boundElements: [ + { + id: "arr", + type: "arrow", + }, + ], + }), + API.createElement({ + type: "rectangle", + id: "rec2", + x: 1169, + y: 777, + width: 102, + height: 115, + boundElements: [ + { + id: "arr", + type: "arrow", + }, + ], + }), + API.createElement({ + type: "arrow", + id: "arrow", + x: 1103.0717787616313, + y: 536.8531862198708, + width: 159.68539325842903, + height: 333.0396003698186, + startBinding: { + elementId: "rec1", + focus: 0.1366906474820229, + gap: 5.000000000000057, + fixedPoint: [0.5683453237410123, -0.05014327585315258], + }, + endBinding: { + elementId: "rec2", + focus: 0.0014925373134265828, + gap: 5, + fixedPoint: [-0.04862325174825108, 0.4992537313432874], + }, + points: [ + point(0, 0), + point(0, -35), + point(-97.80898876404626, -35), + point(-97.80898876404626, 298.0396003698186), + point(61.87640449438277, 298.0396003698186), + ], + elbowed: true, + }), +]; + +describe("flipping action", () => { + it("flip re-centers the selection even after multiple flip actions", async () => { + await render(); + + API.setSelectedElements(testElements); + + expect(Object.keys(h.state.selectedElementIds).length).toBe(3); + + API.executeAction(actionFlipHorizontal); + API.executeAction(actionFlipHorizontal); + API.executeAction(actionFlipHorizontal); + + const rec1 = h.elements.find((el) => el.id === "rec1"); + expect(rec1?.x).toBeCloseTo(1113.78, 0); + expect(rec1?.y).toBeCloseTo(541, 0); + + const rec2 = h.elements.find((el) => el.id === "rec2"); + expect(rec2?.x).toBeCloseTo(988.72, 0); + expect(rec2?.y).toBeCloseTo(777, 0); + }); +}); diff --git a/packages/excalidraw/actions/actionFlip.ts b/packages/excalidraw/actions/actionFlip.ts index a6dad249fb4f..45ca7a298c1e 100644 --- a/packages/excalidraw/actions/actionFlip.ts +++ b/packages/excalidraw/actions/actionFlip.ts @@ -2,6 +2,7 @@ import { register } from "./register"; import { getSelectedElements } from "../scene"; import { getNonDeletedElements } from "../element"; import type { + ExcalidrawElbowArrowElement, ExcalidrawElement, NonDeleted, NonDeletedSceneElementsMap, @@ -18,7 +19,9 @@ import { import { updateFrameMembershipOfSelectedElements } from "../frame"; import { flipHorizontal, flipVertical } from "../components/icons"; import { StoreAction } from "../store"; -import { isLinearElement } from "../element/typeChecks"; +import { isElbowArrow, isLinearElement } from "../element/typeChecks"; +import { mutateElbowArrow } from "../element/routing"; +import { mutateElement } from "../element/mutateElement"; export const actionFlipHorizontal = register({ name: "flipHorizontal", @@ -109,7 +112,8 @@ const flipElements = ( flipDirection: "horizontal" | "vertical", app: AppClassProperties, ): ExcalidrawElement[] => { - const { minX, minY, maxX, maxY } = getCommonBoundingBox(selectedElements); + const { minX, minY, maxX, maxY, midX, midY } = + getCommonBoundingBox(selectedElements); resizeMultipleElements( elementsMap, @@ -131,5 +135,48 @@ const flipElements = ( [], ); + // --------------------------------------------------------------------------- + // flipping arrow elements (and potentially other) makes the selection group + // "move" across the canvas because of how arrows can bump against the "wall" + // of the selection, so we need to center the group back to the original + // position so that repeated flips don't accumulate the offset + + const { elbowArrows, otherElements } = selectedElements.reduce( + ( + acc: { + elbowArrows: ExcalidrawElbowArrowElement[]; + otherElements: ExcalidrawElement[]; + }, + element, + ) => + isElbowArrow(element) + ? { ...acc, elbowArrows: acc.elbowArrows.concat(element) } + : { ...acc, otherElements: acc.otherElements.concat(element) }, + { elbowArrows: [], otherElements: [] }, + ); + + const { midX: newMidX, midY: newMidY } = + getCommonBoundingBox(selectedElements); + const [diffX, diffY] = [midX - newMidX, midY - newMidY]; + otherElements.forEach((element) => + mutateElement(element, { + x: element.x + diffX, + y: element.y + diffY, + }), + ); + elbowArrows.forEach((element) => + mutateElbowArrow( + element, + elementsMap, + element.points, + undefined, + undefined, + { + informMutation: false, + }, + ), + ); + // --------------------------------------------------------------------------- + return selectedElements; }; diff --git a/packages/excalidraw/actions/actionProperties.tsx b/packages/excalidraw/actions/actionProperties.tsx index 0fa705f23fb2..92fa3294731f 100644 --- a/packages/excalidraw/actions/actionProperties.tsx +++ b/packages/excalidraw/actions/actionProperties.tsx @@ -1685,19 +1685,6 @@ export const actionChangeArrowType = register({ : {}), }, ); - } else { - mutateElement( - newElement, - { - startBinding: newElement.startBinding - ? { ...newElement.startBinding, fixedPoint: null } - : null, - endBinding: newElement.endBinding - ? { ...newElement.endBinding, fixedPoint: null } - : null, - }, - false, - ); } return newElement; diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index a98974f41461..08ad13fa54ae 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -185,6 +185,7 @@ import type { MagicGenerationData, ExcalidrawNonSelectionElement, ExcalidrawArrowElement, + NonDeletedSceneElementsMap, } from "../element/types"; import { getCenter, getDistance } from "../gesture"; import { @@ -287,6 +288,7 @@ import { getDateTime, isShallowEqual, arrayToMap, + toBrandedType, } from "../utils"; import { createSrcDoc, @@ -3109,22 +3111,44 @@ class App extends React.Component { retainSeed?: boolean; fitToContent?: boolean; }) => { - let elements = opts.elements.map((el) => - isElbowArrow(el) - ? { - ...el, - ...updateElbowArrow( - { - ...el, - startBinding: null, - endBinding: null, - }, - this.scene.getNonDeletedElementsMap(), - [el.points[0], el.points[el.points.length - 1]], + let elements = opts.elements.map((el, _, elements) => { + if (isElbowArrow(el)) { + const startEndElements = [ + el.startBinding && + elements.find((l) => l.id === el.startBinding?.elementId), + el.endBinding && + elements.find((l) => l.id === el.endBinding?.elementId), + ]; + const startBinding = startEndElements[0] ? el.startBinding : null; + const endBinding = startEndElements[1] ? el.endBinding : null; + return { + ...el, + ...updateElbowArrow( + { + ...el, + startBinding, + endBinding, + }, + toBrandedType( + new Map( + startEndElements + .filter((x) => x != null) + .map( + (el) => + [el!.id, el] as [ + string, + Ordered, + ], + ), + ), ), - } - : el, - ); + [el.points[0], el.points[el.points.length - 1]], + ), + }; + } + + return el; + }); elements = restoreElements(elements, null, undefined); const [minX, minY, maxX, maxY] = getCommonBounds(elements); diff --git a/packages/excalidraw/data/restore.ts b/packages/excalidraw/data/restore.ts index 62652066fa4e..b476995da1a8 100644 --- a/packages/excalidraw/data/restore.ts +++ b/packages/excalidraw/data/restore.ts @@ -5,6 +5,7 @@ import type { ExcalidrawLinearElement, ExcalidrawSelectionElement, ExcalidrawTextElement, + FixedPointBinding, FontFamilyValues, OrderedExcalidrawElement, PointBinding, @@ -21,6 +22,7 @@ import { import { isArrowElement, isElbowArrow, + isFixedPointBinding, isLinearElement, isTextElement, isUsingAdaptiveRadius, @@ -101,8 +103,8 @@ const getFontFamilyByName = (fontFamilyName: string): FontFamilyValues => { const repairBinding = ( element: ExcalidrawLinearElement, - binding: PointBinding | null, -): PointBinding | null => { + binding: PointBinding | FixedPointBinding | null, +): PointBinding | FixedPointBinding | null => { if (!binding) { return null; } @@ -110,9 +112,11 @@ const repairBinding = ( return { ...binding, focus: binding.focus || 0, - fixedPoint: isElbowArrow(element) - ? normalizeFixedPoint(binding.fixedPoint ?? [0, 0]) - : null, + ...(isElbowArrow(element) && isFixedPointBinding(binding) + ? { + fixedPoint: normalizeFixedPoint(binding.fixedPoint ?? [0, 0]), + } + : {}), }; }; diff --git a/packages/excalidraw/element/binding.ts b/packages/excalidraw/element/binding.ts index fe820723f439..62e66f645be5 100644 --- a/packages/excalidraw/element/binding.ts +++ b/packages/excalidraw/element/binding.ts @@ -39,6 +39,7 @@ import { isBindingElement, isBoundToContainer, isElbowArrow, + isFixedPointBinding, isFrameLikeElement, isLinearElement, isRectangularElement, @@ -797,7 +798,7 @@ export const bindPointToSnapToElementOutline = ( isVertical ? Math.abs(p[1] - i[1]) < 0.1 : Math.abs(p[0] - i[0]) < 0.1, - )[0] ?? point; + )[0] ?? p; } return p; @@ -1013,7 +1014,7 @@ const updateBoundPoint = ( const direction = startOrEnd === "startBinding" ? -1 : 1; const edgePointIndex = direction === -1 ? 0 : linearElement.points.length - 1; - if (isElbowArrow(linearElement)) { + if (isElbowArrow(linearElement) && isFixedPointBinding(binding)) { const fixedPoint = normalizeFixedPoint(binding.fixedPoint) ?? calculateFixedPointForElbowArrowBinding( diff --git a/packages/excalidraw/element/dragElements.ts b/packages/excalidraw/element/dragElements.ts index 18d78fdbeff0..5775f0eb7456 100644 --- a/packages/excalidraw/element/dragElements.ts +++ b/packages/excalidraw/element/dragElements.ts @@ -35,7 +35,6 @@ export const dragSelectedElements = ( ) => { if ( _selectedElements.length === 1 && - isArrowElement(_selectedElements[0]) && isElbowArrow(_selectedElements[0]) && (_selectedElements[0].startBinding || _selectedElements[0].endBinding) ) { @@ -43,13 +42,7 @@ export const dragSelectedElements = ( } const selectedElements = _selectedElements.filter( - (el) => - !( - isArrowElement(el) && - isElbowArrow(el) && - el.startBinding && - el.endBinding - ), + (el) => !(isElbowArrow(el) && el.startBinding && el.endBinding), ); // we do not want a frame and its elements to be selected at the same time diff --git a/packages/excalidraw/element/linearElementEditor.ts b/packages/excalidraw/element/linearElementEditor.ts index 7607a2e162a5..e11c0b158c20 100644 --- a/packages/excalidraw/element/linearElementEditor.ts +++ b/packages/excalidraw/element/linearElementEditor.ts @@ -102,6 +102,7 @@ export class LinearElementEditor { public readonly endBindingElement: ExcalidrawBindableElement | null | "keep"; public readonly hoverPointIndex: number; public readonly segmentMidPointHoveredCoords: GlobalPoint | null; + public readonly elbowed: boolean; constructor(element: NonDeleted) { this.elementId = element.id as string & { @@ -131,6 +132,7 @@ export class LinearElementEditor { }; this.hoverPointIndex = -1; this.segmentMidPointHoveredCoords = null; + this.elbowed = isElbowArrow(element) && element.elbowed; } // --------------------------------------------------------------------------- @@ -1477,7 +1479,9 @@ export class LinearElementEditor { nextPoints, vector(offsetX, offsetY), bindings, - options, + { + isDragging: options?.isDragging, + }, ); } else { const nextCoords = getElementPointsCoords(element, nextPoints); diff --git a/packages/excalidraw/element/resizeElements.ts b/packages/excalidraw/element/resizeElements.ts index 3f3f8ef1e208..0a01459e694e 100644 --- a/packages/excalidraw/element/resizeElements.ts +++ b/packages/excalidraw/element/resizeElements.ts @@ -9,6 +9,7 @@ import type { ExcalidrawTextElementWithContainer, ExcalidrawImageElement, ElementsMap, + ExcalidrawArrowElement, NonDeletedSceneElementsMap, SceneElementsMap, } from "./types"; @@ -909,6 +910,8 @@ export const resizeMultipleElements = ( fontSize?: ExcalidrawTextElement["fontSize"]; scale?: ExcalidrawImageElement["scale"]; boundTextFontSize?: ExcalidrawTextElement["fontSize"]; + startBinding?: ExcalidrawArrowElement["startBinding"]; + endBinding?: ExcalidrawArrowElement["endBinding"]; }; }[] = []; @@ -993,19 +996,6 @@ export const resizeMultipleElements = ( mutateElement(element, update, false); - if (isArrowElement(element) && isElbowArrow(element)) { - mutateElbowArrow( - element, - elementsMap, - element.points, - undefined, - undefined, - { - informMutation: false, - }, - ); - } - updateBoundElements(element, elementsMap, { simultaneouslyUpdated: elementsToUpdate, oldSize: { width: oldWidth, height: oldHeight }, @@ -1059,7 +1049,7 @@ const rotateMultipleElements = ( (centerAngle + origAngle - element.angle) as Radians, ); - if (isArrowElement(element) && isElbowArrow(element)) { + if (isElbowArrow(element)) { const points = getArrowLocalFixedPoints(element, elementsMap); mutateElbowArrow(element, elementsMap, points); } else { diff --git a/packages/excalidraw/element/routing.ts b/packages/excalidraw/element/routing.ts index ac11cddb41b2..895340c91a2c 100644 --- a/packages/excalidraw/element/routing.ts +++ b/packages/excalidraw/element/routing.ts @@ -41,7 +41,6 @@ import { mutateElement } from "./mutateElement"; import { isBindableElement, isRectanguloidElement } from "./typeChecks"; import type { ExcalidrawElbowArrowElement, - FixedPointBinding, NonDeletedSceneElementsMap, SceneElementsMap, } from "./types"; @@ -73,13 +72,12 @@ export const mutateElbowArrow = ( elementsMap: NonDeletedSceneElementsMap | SceneElementsMap, nextPoints: readonly LocalPoint[], offset?: Vector, - otherUpdates?: { - startBinding?: FixedPointBinding | null; - endBinding?: FixedPointBinding | null; - }, + otherUpdates?: Omit< + ElementUpdate, + "angle" | "x" | "y" | "width" | "height" | "elbowed" | "points" + >, options?: { isDragging?: boolean; - disableBinding?: boolean; informMutation?: boolean; }, ) => { diff --git a/packages/excalidraw/element/typeChecks.ts b/packages/excalidraw/element/typeChecks.ts index 5ba089ab01a7..6bb4269f8705 100644 --- a/packages/excalidraw/element/typeChecks.ts +++ b/packages/excalidraw/element/typeChecks.ts @@ -320,9 +320,12 @@ export const getDefaultRoundnessTypeForElement = ( }; export const isFixedPointBinding = ( - binding: PointBinding, + binding: PointBinding | FixedPointBinding, ): binding is FixedPointBinding => { - return binding.fixedPoint != null; + return ( + Object.hasOwn(binding, "fixedPoint") && + (binding as FixedPointBinding).fixedPoint != null + ); }; // TODO: Move this to @excalidraw/math diff --git a/packages/excalidraw/element/types.ts b/packages/excalidraw/element/types.ts index 9b092542764e..5ebf505444a1 100644 --- a/packages/excalidraw/element/types.ts +++ b/packages/excalidraw/element/types.ts @@ -193,6 +193,7 @@ export type ExcalidrawElement = | ExcalidrawGenericElement | ExcalidrawTextElement | ExcalidrawLinearElement + | ExcalidrawArrowElement | ExcalidrawFreeDrawElement | ExcalidrawImageElement | ExcalidrawFrameElement @@ -268,15 +269,19 @@ export type PointBinding = { elementId: ExcalidrawBindableElement["id"]; focus: number; gap: number; - // Represents the fixed point binding information in form of a vertical and - // horizontal ratio (i.e. a percentage value in the 0.0-1.0 range). This ratio - // gives the user selected fixed point by multiplying the bound element width - // with fixedPoint[0] and the bound element height with fixedPoint[1] to get the - // bound element-local point coordinate. - fixedPoint: FixedPoint | null; }; -export type FixedPointBinding = Merge; +export type FixedPointBinding = Merge< + PointBinding, + { + // Represents the fixed point binding information in form of a vertical and + // horizontal ratio (i.e. a percentage value in the 0.0-1.0 range). This ratio + // gives the user selected fixed point by multiplying the bound element width + // with fixedPoint[0] and the bound element height with fixedPoint[1] to get the + // bound element-local point coordinate. + fixedPoint: FixedPoint; + } +>; export type Arrowhead = | "arrow" diff --git a/packages/excalidraw/renderer/interactiveScene.ts b/packages/excalidraw/renderer/interactiveScene.ts index 0d03b0f5a046..7dc84db9967e 100644 --- a/packages/excalidraw/renderer/interactiveScene.ts +++ b/packages/excalidraw/renderer/interactiveScene.ts @@ -52,7 +52,6 @@ import { } from "./helpers"; import oc from "open-color"; import { - isArrowElement, isElbowArrow, isFrameLikeElement, isLinearElement, @@ -807,7 +806,6 @@ const _renderInteractiveScene = ({ // Elbow arrow elements cannot be selected when bound on either end ( isSingleLinearElementSelected && - isArrowElement(element) && isElbowArrow(element) && (element.startBinding || element.endBinding) ) diff --git a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap index c4683267c96a..6e1f5350368f 100644 --- a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap @@ -8430,6 +8430,7 @@ exports[`regression tests > key 5 selects arrow tool > [end of test] appState 1` "selectedElementsAreBeingDragged": false, "selectedGroupIds": {}, "selectedLinearElement": LinearElementEditor { + "elbowed": false, "elementId": "id0", "endBindingElement": "keep", "hoverPointIndex": -1, @@ -8649,6 +8650,7 @@ exports[`regression tests > key 6 selects line tool > [end of test] appState 1`] "selectedElementsAreBeingDragged": false, "selectedGroupIds": {}, "selectedLinearElement": LinearElementEditor { + "elbowed": false, "elementId": "id0", "endBindingElement": "keep", "hoverPointIndex": -1, @@ -9058,6 +9060,7 @@ exports[`regression tests > key a selects arrow tool > [end of test] appState 1` "selectedElementsAreBeingDragged": false, "selectedGroupIds": {}, "selectedLinearElement": LinearElementEditor { + "elbowed": false, "elementId": "id0", "endBindingElement": "keep", "hoverPointIndex": -1, @@ -9454,6 +9457,7 @@ exports[`regression tests > key l selects line tool > [end of test] appState 1`] "selectedElementsAreBeingDragged": false, "selectedGroupIds": {}, "selectedLinearElement": LinearElementEditor { + "elbowed": false, "elementId": "id0", "endBindingElement": "keep", "hoverPointIndex": -1, diff --git a/packages/excalidraw/tests/helpers/api.ts b/packages/excalidraw/tests/helpers/api.ts index 6c16e51909b2..d98a908f7ab6 100644 --- a/packages/excalidraw/tests/helpers/api.ts +++ b/packages/excalidraw/tests/helpers/api.ts @@ -9,6 +9,8 @@ import type { ExcalidrawFrameElement, ExcalidrawElementType, ExcalidrawMagicFrameElement, + ExcalidrawElbowArrowElement, + ExcalidrawArrowElement, } from "../../element/types"; import { newElement, newTextElement, newLinearElement } from "../../element"; import { DEFAULT_VERTICAL_ALIGN, ROUNDNESS } from "../../constants"; @@ -179,10 +181,10 @@ export class API { scale?: T extends "image" ? ExcalidrawImageElement["scale"] : never; status?: T extends "image" ? ExcalidrawImageElement["status"] : never; startBinding?: T extends "arrow" - ? ExcalidrawLinearElement["startBinding"] + ? ExcalidrawArrowElement["startBinding"] | ExcalidrawElbowArrowElement["startBinding"] : never; endBinding?: T extends "arrow" - ? ExcalidrawLinearElement["endBinding"] + ? ExcalidrawArrowElement["endBinding"] | ExcalidrawElbowArrowElement["endBinding"] : never; elbowed?: boolean; }): T extends "arrow" | "line" diff --git a/packages/excalidraw/tests/history.test.tsx b/packages/excalidraw/tests/history.test.tsx index 8e825e41489a..3c807cf9154e 100644 --- a/packages/excalidraw/tests/history.test.tsx +++ b/packages/excalidraw/tests/history.test.tsx @@ -31,6 +31,7 @@ import type { ExcalidrawGenericElement, ExcalidrawLinearElement, ExcalidrawTextElement, + FixedPointBinding, FractionalIndex, SceneElementsMap, } from "../element/types"; @@ -2049,13 +2050,13 @@ describe("history", () => { focus: -0.001587301587301948, gap: 5, fixedPoint: [1.0318471337579618, 0.49920634920634904], - }, + } as FixedPointBinding, endBinding: { elementId: "u2JGnnmoJ0VATV4vCNJE5", focus: -0.0016129032258049847, gap: 3.537079145500037, fixedPoint: [0.4991935483870975, -0.03875193720914723], - }, + } as FixedPointBinding, }, ], storeAction: StoreAction.CAPTURE, @@ -4455,7 +4456,7 @@ describe("history", () => { elements: [ h.elements[0], newElementWith(h.elements[1], { boundElements: [] }), - newElementWith(h.elements[2] as ExcalidrawLinearElement, { + newElementWith(h.elements[2] as ExcalidrawElbowArrowElement, { endBinding: { elementId: remoteContainer.id, gap: 1, @@ -4655,7 +4656,7 @@ describe("history", () => { // Simulate remote update API.updateScene({ elements: [ - newElementWith(h.elements[0] as ExcalidrawLinearElement, { + newElementWith(h.elements[0] as ExcalidrawElbowArrowElement, { startBinding: { elementId: rect1.id, gap: 1, diff --git a/packages/excalidraw/tests/resize.test.tsx b/packages/excalidraw/tests/resize.test.tsx index d18f5cd498ad..8de7157b18d9 100644 --- a/packages/excalidraw/tests/resize.test.tsx +++ b/packages/excalidraw/tests/resize.test.tsx @@ -4,6 +4,7 @@ import { render } from "./test-utils"; import { reseed } from "../random"; import { UI, Keyboard, Pointer } from "./helpers/ui"; import type { + ExcalidrawElbowArrowElement, ExcalidrawFreeDrawElement, ExcalidrawLinearElement, } from "../element/types"; @@ -333,6 +334,62 @@ describe("arrow element", () => { expect(label.angle).toBeCloseTo(0); expect(label.fontSize).toEqual(20); }); + + it("flips the fixed point binding on negative resize for single bindable", () => { + const rectangle = UI.createElement("rectangle", { + x: -100, + y: -75, + width: 95, + height: 100, + }); + UI.clickTool("arrow"); + UI.clickOnTestId("elbow-arrow"); + mouse.reset(); + mouse.moveTo(-5, 0); + mouse.click(); + mouse.moveTo(120, 200); + mouse.click(); + + const arrow = h.scene.getSelectedElements( + h.state, + )[0] as ExcalidrawElbowArrowElement; + + expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(1.05); + expect(arrow.startBinding?.fixedPoint?.[1]).toBeCloseTo(0.75); + + UI.resize(rectangle, "se", [-200, -150]); + + expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(1.05); + expect(arrow.startBinding?.fixedPoint?.[1]).toBeCloseTo(0.75); + }); + + it("flips the fixed point binding on negative resize for group selection", () => { + const rectangle = UI.createElement("rectangle", { + x: -100, + y: -75, + width: 95, + height: 100, + }); + UI.clickTool("arrow"); + UI.clickOnTestId("elbow-arrow"); + mouse.reset(); + mouse.moveTo(-5, 0); + mouse.click(); + mouse.moveTo(120, 200); + mouse.click(); + + const arrow = h.scene.getSelectedElements( + h.state, + )[0] as ExcalidrawElbowArrowElement; + + expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(1.05); + expect(arrow.startBinding?.fixedPoint?.[1]).toBeCloseTo(0.75); + + UI.resize([rectangle, arrow], "nw", [300, 350]); + + expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(-0.144, 2); + expect(arrow.startBinding?.fixedPoint?.[1]).toBeCloseTo(0.25); + }); }); describe("text element", () => { @@ -828,7 +885,6 @@ describe("multiple selection", () => { expect(leftBoundArrow.endBinding?.elementId).toBe( leftArrowBinding.elementId, ); - expect(leftBoundArrow.endBinding?.fixedPoint).toBeNull(); expect(leftBoundArrow.endBinding?.focus).toBe(leftArrowBinding.focus); expect(rightBoundArrow.x).toBeCloseTo(210); @@ -843,7 +899,6 @@ describe("multiple selection", () => { expect(rightBoundArrow.endBinding?.elementId).toBe( rightArrowBinding.elementId, ); - expect(rightBoundArrow.endBinding?.fixedPoint).toBeNull(); expect(rightBoundArrow.endBinding?.focus).toBe(rightArrowBinding.focus); }); From 8ca4cf32605343c1631ed7ef57f0c9d87f4c23b1 Mon Sep 17 00:00:00 2001 From: David Luzar <5153846+dwelle@users.noreply.github.com> Date: Thu, 19 Sep 2024 15:46:36 +0200 Subject: [PATCH 2/2] feat: flip arrowheads if only arrow(s) selected (#8525) Co-authored-by: Mark Tolmacs --- .../excalidraw/actions/actionFlip.test.tsx | 256 +++++++++++++----- packages/excalidraw/actions/actionFlip.ts | 24 +- packages/excalidraw/tests/helpers/api.ts | 12 + 3 files changed, 223 insertions(+), 69 deletions(-) diff --git a/packages/excalidraw/actions/actionFlip.test.tsx b/packages/excalidraw/actions/actionFlip.test.tsx index 0a1b9f41d513..c8a6239cdff2 100644 --- a/packages/excalidraw/actions/actionFlip.test.tsx +++ b/packages/excalidraw/actions/actionFlip.test.tsx @@ -3,87 +3,209 @@ import { Excalidraw } from "../index"; import { render } from "../tests/test-utils"; import { API } from "../tests/helpers/api"; import { point } from "../../math"; -import { actionFlipHorizontal } from "./actionFlip"; +import { actionFlipHorizontal, actionFlipVertical } from "./actionFlip"; const { h } = window; -const testElements = [ - API.createElement({ - type: "rectangle", - id: "rec1", - x: 1046, - y: 541, - width: 100, - height: 100, - boundElements: [ - { - id: "arr", +describe("flipping re-centers selection", () => { + it("elbow arrow touches group selection side yet it remains in place after multiple moves", async () => { + const elements = [ + API.createElement({ + type: "rectangle", + id: "rec1", + x: 100, + y: 100, + width: 100, + height: 100, + boundElements: [{ id: "arr", type: "arrow" }], + }), + API.createElement({ + type: "rectangle", + id: "rec2", + x: 220, + y: 250, + width: 100, + height: 100, + boundElements: [{ id: "arr", type: "arrow" }], + }), + API.createElement({ type: "arrow", - }, - ], - }), - API.createElement({ - type: "rectangle", - id: "rec2", - x: 1169, - y: 777, - width: 102, - height: 115, - boundElements: [ - { id: "arr", - type: "arrow", - }, - ], - }), - API.createElement({ - type: "arrow", - id: "arrow", - x: 1103.0717787616313, - y: 536.8531862198708, - width: 159.68539325842903, - height: 333.0396003698186, - startBinding: { - elementId: "rec1", - focus: 0.1366906474820229, - gap: 5.000000000000057, - fixedPoint: [0.5683453237410123, -0.05014327585315258], - }, - endBinding: { - elementId: "rec2", - focus: 0.0014925373134265828, - gap: 5, - fixedPoint: [-0.04862325174825108, 0.4992537313432874], - }, - points: [ - point(0, 0), - point(0, -35), - point(-97.80898876404626, -35), - point(-97.80898876404626, 298.0396003698186), - point(61.87640449438277, 298.0396003698186), - ], - elbowed: true, - }), -]; - -describe("flipping action", () => { - it("flip re-centers the selection even after multiple flip actions", async () => { - await render(); - - API.setSelectedElements(testElements); + x: 149.9, + y: 95, + width: 156, + height: 239.9, + startBinding: { + elementId: "rec1", + focus: 0, + gap: 5, + fixedPoint: [0.49, -0.05], + }, + endBinding: { + elementId: "rec2", + focus: 0, + gap: 5, + fixedPoint: [-0.05, 0.49], + }, + startArrowhead: null, + endArrowhead: "arrow", + points: [ + point(0, 0), + point(0, -35), + point(-90.9, -35), + point(-90.9, 204.9), + point(65.1, 204.9), + ], + elbowed: true, + }), + ]; + await render(); + + API.setSelectedElements(elements); expect(Object.keys(h.state.selectedElementIds).length).toBe(3); API.executeAction(actionFlipHorizontal); API.executeAction(actionFlipHorizontal); API.executeAction(actionFlipHorizontal); + API.executeAction(actionFlipHorizontal); const rec1 = h.elements.find((el) => el.id === "rec1"); - expect(rec1?.x).toBeCloseTo(1113.78, 0); - expect(rec1?.y).toBeCloseTo(541, 0); + expect(rec1?.x).toBeCloseTo(100); + expect(rec1?.y).toBeCloseTo(100); const rec2 = h.elements.find((el) => el.id === "rec2"); - expect(rec2?.x).toBeCloseTo(988.72, 0); - expect(rec2?.y).toBeCloseTo(777, 0); + expect(rec2?.x).toBeCloseTo(220); + expect(rec2?.y).toBeCloseTo(250); + }); +}); + +describe("flipping arrowheads", () => { + beforeEach(async () => { + await render(); + }); + + it("flipping bound arrow should flip arrowheads only", () => { + const rect = API.createElement({ + type: "rectangle", + boundElements: [{ type: "arrow", id: "arrow1" }], + }); + const arrow = API.createElement({ + type: "arrow", + id: "arrow1", + startArrowhead: "arrow", + endArrowhead: null, + endBinding: { + elementId: rect.id, + focus: 0.5, + gap: 5, + }, + }); + + API.setElements([rect, arrow]); + API.setSelectedElements([arrow]); + + expect(API.getElement(arrow).startArrowhead).toBe("arrow"); + expect(API.getElement(arrow).endArrowhead).toBe(null); + + API.executeAction(actionFlipHorizontal); + expect(API.getElement(arrow).startArrowhead).toBe(null); + expect(API.getElement(arrow).endArrowhead).toBe("arrow"); + + API.executeAction(actionFlipHorizontal); + expect(API.getElement(arrow).startArrowhead).toBe("arrow"); + expect(API.getElement(arrow).endArrowhead).toBe(null); + + API.executeAction(actionFlipVertical); + expect(API.getElement(arrow).startArrowhead).toBe(null); + expect(API.getElement(arrow).endArrowhead).toBe("arrow"); + }); + + it("flipping bound arrow should flip arrowheads only 2", () => { + const rect = API.createElement({ + type: "rectangle", + boundElements: [{ type: "arrow", id: "arrow1" }], + }); + const rect2 = API.createElement({ + type: "rectangle", + boundElements: [{ type: "arrow", id: "arrow1" }], + }); + const arrow = API.createElement({ + type: "arrow", + id: "arrow1", + startArrowhead: "arrow", + endArrowhead: "circle", + startBinding: { + elementId: rect.id, + focus: 0.5, + gap: 5, + }, + endBinding: { + elementId: rect2.id, + focus: 0.5, + gap: 5, + }, + }); + + API.setElements([rect, rect2, arrow]); + API.setSelectedElements([arrow]); + + expect(API.getElement(arrow).startArrowhead).toBe("arrow"); + expect(API.getElement(arrow).endArrowhead).toBe("circle"); + + API.executeAction(actionFlipHorizontal); + expect(API.getElement(arrow).startArrowhead).toBe("circle"); + expect(API.getElement(arrow).endArrowhead).toBe("arrow"); + + API.executeAction(actionFlipVertical); + expect(API.getElement(arrow).startArrowhead).toBe("arrow"); + expect(API.getElement(arrow).endArrowhead).toBe("circle"); + }); + + it("flipping unbound arrow shouldn't flip arrowheads", () => { + const arrow = API.createElement({ + type: "arrow", + id: "arrow1", + startArrowhead: "arrow", + endArrowhead: "circle", + }); + + API.setElements([arrow]); + API.setSelectedElements([arrow]); + + expect(API.getElement(arrow).startArrowhead).toBe("arrow"); + expect(API.getElement(arrow).endArrowhead).toBe("circle"); + + API.executeAction(actionFlipHorizontal); + expect(API.getElement(arrow).startArrowhead).toBe("arrow"); + expect(API.getElement(arrow).endArrowhead).toBe("circle"); + }); + + it("flipping bound arrow shouldn't flip arrowheads if selected alongside non-arrow eleemnt", () => { + const rect = API.createElement({ + type: "rectangle", + boundElements: [{ type: "arrow", id: "arrow1" }], + }); + const arrow = API.createElement({ + type: "arrow", + id: "arrow1", + startArrowhead: "arrow", + endArrowhead: null, + endBinding: { + elementId: rect.id, + focus: 0.5, + gap: 5, + }, + }); + + API.setElements([rect, arrow]); + API.setSelectedElements([rect, arrow]); + + expect(API.getElement(arrow).startArrowhead).toBe("arrow"); + expect(API.getElement(arrow).endArrowhead).toBe(null); + + API.executeAction(actionFlipHorizontal); + expect(API.getElement(arrow).startArrowhead).toBe("arrow"); + expect(API.getElement(arrow).endArrowhead).toBe(null); }); }); diff --git a/packages/excalidraw/actions/actionFlip.ts b/packages/excalidraw/actions/actionFlip.ts index 45ca7a298c1e..6b75b8facd71 100644 --- a/packages/excalidraw/actions/actionFlip.ts +++ b/packages/excalidraw/actions/actionFlip.ts @@ -2,6 +2,7 @@ import { register } from "./register"; import { getSelectedElements } from "../scene"; import { getNonDeletedElements } from "../element"; import type { + ExcalidrawArrowElement, ExcalidrawElbowArrowElement, ExcalidrawElement, NonDeleted, @@ -19,9 +20,13 @@ import { import { updateFrameMembershipOfSelectedElements } from "../frame"; import { flipHorizontal, flipVertical } from "../components/icons"; import { StoreAction } from "../store"; -import { isElbowArrow, isLinearElement } from "../element/typeChecks"; +import { + isArrowElement, + isElbowArrow, + isLinearElement, +} from "../element/typeChecks"; import { mutateElbowArrow } from "../element/routing"; -import { mutateElement } from "../element/mutateElement"; +import { mutateElement, newElementWith } from "../element/mutateElement"; export const actionFlipHorizontal = register({ name: "flipHorizontal", @@ -112,6 +117,21 @@ const flipElements = ( flipDirection: "horizontal" | "vertical", app: AppClassProperties, ): ExcalidrawElement[] => { + if ( + selectedElements.every( + (element) => + isArrowElement(element) && (element.startBinding || element.endBinding), + ) + ) { + return selectedElements.map((element) => { + const _element = element as ExcalidrawArrowElement; + return newElementWith(_element, { + startArrowhead: _element.endArrowhead, + endArrowhead: _element.startArrowhead, + }); + }); + } + const { minX, minY, maxX, maxY, midX, midY } = getCommonBoundingBox(selectedElements); diff --git a/packages/excalidraw/tests/helpers/api.ts b/packages/excalidraw/tests/helpers/api.ts index d98a908f7ab6..b7dc6e10d6bc 100644 --- a/packages/excalidraw/tests/helpers/api.ts +++ b/packages/excalidraw/tests/helpers/api.ts @@ -129,6 +129,10 @@ export class API { expect(API.getSelectedElements().length).toBe(0); }; + static getElement = (element: T): T => { + return h.app.scene.getElementsMapIncludingDeleted().get(element.id) as T || element; + } + static createElement = < T extends Exclude = "rectangle", >({ @@ -186,6 +190,12 @@ export class API { endBinding?: T extends "arrow" ? ExcalidrawArrowElement["endBinding"] | ExcalidrawElbowArrowElement["endBinding"] : never; + startArrowhead?: T extends "arrow" + ? ExcalidrawArrowElement["startArrowhead"] | ExcalidrawElbowArrowElement["startArrowhead"] + : never; + endArrowhead?: T extends "arrow" + ? ExcalidrawArrowElement["endArrowhead"] | ExcalidrawElbowArrowElement["endArrowhead"] + : never; elbowed?: boolean; }): T extends "arrow" | "line" ? ExcalidrawLinearElement @@ -342,6 +352,8 @@ export class API { if (element.type === "arrow") { element.startBinding = rest.startBinding ?? null; element.endBinding = rest.endBinding ?? null; + element.startArrowhead = rest.startArrowhead ?? null; + element.endArrowhead = rest.endArrowhead ?? null; } if (id) { element.id = id;