From 25c6056b0335c8db4de34d13f74953b068783b77 Mon Sep 17 00:00:00 2001 From: Aakansha Doshi Date: Tue, 29 Nov 2022 00:01:53 +0530 Subject: [PATCH] feat: Don't add midpoint until dragged beyond a threshold (#5927) * Don't add midpoint until dragged beyond a threshold * remove unnecessary code * fix tests * fix * add spec * remove isMidpoint * cleanup * fix threshold for zoom * split into shouldAddMidpoint and addMidpoint * wrap in flushSync for synchronous updates * remove threshold for line editor and add spec * [unrelated] fix stack overflow state update * fix tests * don't drag arrow when dragging to add mid point * add specs Co-authored-by: dwelle --- src/components/App.tsx | 58 ++++++- src/element/linearElementEditor.ts | 158 ++++++++++++++---- .../regressionTests.test.tsx.snap | 24 +++ .../__snapshots__/selection.test.tsx.snap | 16 +- src/tests/linearElementEditor.test.tsx | 65 +++++-- 5 files changed, 258 insertions(+), 63 deletions(-) diff --git a/src/components/App.tsx b/src/components/App.tsx index be62bc5c7..f865079af 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -2885,7 +2885,10 @@ class App extends React.Component { if (editingLinearElement?.lastUncommittedPoint != null) { this.maybeSuggestBindingAtCursor(scenePointer); } else { - this.setState({ suggestedBindings: [] }); + // causes stack overflow if not sync + flushSync(() => { + this.setState({ suggestedBindings: [] }); + }); } } @@ -3825,7 +3828,7 @@ class App extends React.Component { this.setState({ editingLinearElement: ret.linearElementEditor }); } } - if (ret.didAddPoint && !ret.isMidPoint) { + if (ret.didAddPoint) { return true; } } @@ -4315,7 +4318,6 @@ class App extends React.Component { // to ensure we don't create a 2-point arrow by mistake when // user clicks mouse in a way that it moves a tiny bit (thus // triggering pointermove) - if ( !pointerDownState.drag.hasOccurred && (this.state.activeTool.type === "arrow" || @@ -4343,6 +4345,56 @@ class App extends React.Component { if (this.state.selectedLinearElement) { const linearElementEditor = this.state.editingLinearElement || this.state.selectedLinearElement; + + if ( + LinearElementEditor.shouldAddMidpoint( + this.state.selectedLinearElement, + pointerCoords, + this.state, + ) + ) { + const ret = LinearElementEditor.addMidpoint( + this.state.selectedLinearElement, + pointerCoords, + this.state, + ); + if (!ret) { + return; + } + + // Since we are reading from previous state which is not possible with + // automatic batching in React 18 hence using flush sync to synchronously + // update the state. Check https://github.com/excalidraw/excalidraw/pull/5508 for more details. + + flushSync(() => { + if (this.state.selectedLinearElement) { + this.setState({ + selectedLinearElement: { + ...this.state.selectedLinearElement, + pointerDownState: ret.pointerDownState, + selectedPointsIndices: ret.selectedPointsIndices, + }, + }); + } + if (this.state.editingLinearElement) { + this.setState({ + editingLinearElement: { + ...this.state.editingLinearElement, + pointerDownState: ret.pointerDownState, + selectedPointsIndices: ret.selectedPointsIndices, + }, + }); + } + }); + + return; + } else if ( + linearElementEditor.pointerDownState.segmentMidpoint.value !== null && + !linearElementEditor.pointerDownState.segmentMidpoint.added + ) { + return; + } + const didDrag = LinearElementEditor.handlePointDragging( event, this.state, diff --git a/src/element/linearElementEditor.ts b/src/element/linearElementEditor.ts index df9157486..f0d0a2153 100644 --- a/src/element/linearElementEditor.ts +++ b/src/element/linearElementEditor.ts @@ -20,7 +20,7 @@ import { } from "../math"; import { getElementAbsoluteCoords, getLockedLinearCursorAlignSize } from "."; import { getElementPointsCoords } from "./bounds"; -import { Point, AppState } from "../types"; +import { Point, AppState, PointerCoords } from "../types"; import { mutateElement } from "./mutateElement"; import History from "../history"; @@ -33,6 +33,7 @@ import { import { tupleToCoors } from "../utils"; import { isBindingElement } from "./typeChecks"; import { shouldRotateWithDiscreteAngle } from "../keys"; +import { DRAGGING_THRESHOLD } from "../constants"; const editorMidPointsCache: { version: number | null; @@ -51,6 +52,12 @@ export class LinearElementEditor { prevSelectedPointsIndices: readonly number[] | null; /** index */ lastClickedPoint: number; + origin: Readonly<{ x: number; y: number }> | null; + segmentMidpoint: { + value: Point | null; + index: number | null; + added: boolean; + }; }>; /** whether you're dragging a point */ @@ -81,6 +88,13 @@ export class LinearElementEditor { this.pointerDownState = { prevSelectedPointsIndices: null, lastClickedPoint: -1, + origin: null, + + segmentMidpoint: { + value: null, + index: null, + added: false, + }, }; this.hoverPointIndex = -1; this.segmentMidPointHoveredCoords = null; @@ -180,6 +194,7 @@ export class LinearElementEditor { const draggingPoint = element.points[ linearElementEditor.pointerDownState.lastClickedPoint ] as [number, number] | undefined; + if (selectedPointsIndices && draggingPoint) { if ( shouldRotateWithDiscreteAngle(event) && @@ -551,7 +566,7 @@ export class LinearElementEditor { } const midPoints = LinearElementEditor.getEditorMidPoints(element, appState); let index = 0; - while (index < midPoints.length - 1) { + while (index < midPoints.length) { if (LinearElementEditor.arePointsEqual(midPoint, midPoints[index])) { return index + 1; } @@ -570,13 +585,11 @@ export class LinearElementEditor { didAddPoint: boolean; hitElement: NonDeleted | null; linearElementEditor: LinearElementEditor | null; - isMidPoint: boolean; } { const ret: ReturnType = { didAddPoint: false, hitElement: null, linearElementEditor: null, - isMidPoint: false, }; if (!linearElementEditor) { @@ -589,43 +602,18 @@ export class LinearElementEditor { if (!element) { return ret; } - const segmentMidPoint = LinearElementEditor.getSegmentMidpointHitCoords( + const segmentMidpoint = LinearElementEditor.getSegmentMidpointHitCoords( linearElementEditor, scenePointer, appState, ); - if (segmentMidPoint) { - const index = LinearElementEditor.getSegmentMidPointIndex( + let segmentMidpointIndex = null; + if (segmentMidpoint) { + segmentMidpointIndex = LinearElementEditor.getSegmentMidPointIndex( linearElementEditor, appState, - segmentMidPoint, + segmentMidpoint, ); - const newMidPoint = LinearElementEditor.createPointAt( - element, - segmentMidPoint[0], - segmentMidPoint[1], - appState.gridSize, - ); - const points = [ - ...element.points.slice(0, index), - newMidPoint, - ...element.points.slice(index), - ]; - mutateElement(element, { - points, - }); - - ret.didAddPoint = true; - ret.isMidPoint = true; - ret.linearElementEditor = { - ...linearElementEditor, - selectedPointsIndices: element.points[1], - pointerDownState: { - prevSelectedPointsIndices: linearElementEditor.selectedPointsIndices, - lastClickedPoint: -1, - }, - lastUncommittedPoint: null, - }; } if (event.altKey && appState.editingLinearElement) { if (linearElementEditor.lastUncommittedPoint == null) { @@ -648,6 +636,12 @@ export class LinearElementEditor { pointerDownState: { prevSelectedPointsIndices: linearElementEditor.selectedPointsIndices, lastClickedPoint: -1, + origin: { x: scenePointer.x, y: scenePointer.y }, + segmentMidpoint: { + value: segmentMidpoint, + index: segmentMidpointIndex, + added: false, + }, }, selectedPointsIndices: [element.points.length - 1], lastUncommittedPoint: null, @@ -670,7 +664,7 @@ export class LinearElementEditor { // if we clicked on a point, set the element as hitElement otherwise // it would get deselected if the point is outside the hitbox area - if (clickedPointIndex >= 0 || segmentMidPoint) { + if (clickedPointIndex >= 0 || segmentMidpoint) { ret.hitElement = element; } else { // You might be wandering why we are storing the binding elements on @@ -716,6 +710,12 @@ export class LinearElementEditor { pointerDownState: { prevSelectedPointsIndices: linearElementEditor.selectedPointsIndices, lastClickedPoint: clickedPointIndex, + origin: { x: scenePointer.x, y: scenePointer.y }, + segmentMidpoint: { + value: segmentMidpoint, + index: segmentMidpointIndex, + added: false, + }, }, selectedPointsIndices: nextSelectedPointsIndices, pointerOffset: targetPoint @@ -1111,6 +1111,94 @@ export class LinearElementEditor { ); } + static shouldAddMidpoint( + linearElementEditor: LinearElementEditor, + pointerCoords: PointerCoords, + appState: AppState, + ) { + const element = LinearElementEditor.getElement( + linearElementEditor.elementId, + ); + + if (!element) { + return false; + } + + const { segmentMidpoint } = linearElementEditor.pointerDownState; + + if ( + segmentMidpoint.added || + segmentMidpoint.value === null || + segmentMidpoint.index === null || + linearElementEditor.pointerDownState.origin === null + ) { + return false; + } + + const origin = linearElementEditor.pointerDownState.origin!; + const dist = distance2d( + origin.x, + origin.y, + pointerCoords.x, + pointerCoords.y, + ); + if ( + !appState.editingLinearElement && + dist < DRAGGING_THRESHOLD / appState.zoom.value + ) { + return false; + } + return true; + } + + static addMidpoint( + linearElementEditor: LinearElementEditor, + pointerCoords: PointerCoords, + appState: AppState, + ) { + const element = LinearElementEditor.getElement( + linearElementEditor.elementId, + ); + if (!element) { + return; + } + const { segmentMidpoint } = linearElementEditor.pointerDownState; + const ret: { + pointerDownState: LinearElementEditor["pointerDownState"]; + selectedPointsIndices: LinearElementEditor["selectedPointsIndices"]; + } = { + pointerDownState: linearElementEditor.pointerDownState, + selectedPointsIndices: linearElementEditor.selectedPointsIndices, + }; + + const midpoint = LinearElementEditor.createPointAt( + element, + pointerCoords.x, + pointerCoords.y, + appState.gridSize, + ); + const points = [ + ...element.points.slice(0, segmentMidpoint.index!), + midpoint, + ...element.points.slice(segmentMidpoint.index!), + ]; + + mutateElement(element, { + points, + }); + + ret.pointerDownState = { + ...linearElementEditor.pointerDownState, + segmentMidpoint: { + ...linearElementEditor.pointerDownState.segmentMidpoint, + added: true, + }, + lastClickedPoint: segmentMidpoint.index!, + }; + ret.selectedPointsIndices = [segmentMidpoint.index!]; + return ret; + } + private static _updatePoints( element: NonDeleted, nextPoints: readonly Point[], diff --git a/src/tests/__snapshots__/regressionTests.test.tsx.snap b/src/tests/__snapshots__/regressionTests.test.tsx.snap index 53e721001..b7de2aa05 100644 --- a/src/tests/__snapshots__/regressionTests.test.tsx.snap +++ b/src/tests/__snapshots__/regressionTests.test.tsx.snap @@ -10989,7 +10989,13 @@ Object { "lastUncommittedPoint": null, "pointerDownState": Object { "lastClickedPoint": -1, + "origin": null, "prevSelectedPointsIndices": null, + "segmentMidpoint": Object { + "added": false, + "index": null, + "value": null, + }, }, "pointerOffset": Object { "x": 0, @@ -11216,7 +11222,13 @@ Object { "lastUncommittedPoint": null, "pointerDownState": Object { "lastClickedPoint": -1, + "origin": null, "prevSelectedPointsIndices": null, + "segmentMidpoint": Object { + "added": false, + "index": null, + "value": null, + }, }, "pointerOffset": Object { "x": 0, @@ -11671,7 +11683,13 @@ Object { "lastUncommittedPoint": null, "pointerDownState": Object { "lastClickedPoint": -1, + "origin": null, "prevSelectedPointsIndices": null, + "segmentMidpoint": Object { + "added": false, + "index": null, + "value": null, + }, }, "pointerOffset": Object { "x": 0, @@ -12078,7 +12096,13 @@ Object { "lastUncommittedPoint": null, "pointerDownState": Object { "lastClickedPoint": -1, + "origin": null, "prevSelectedPointsIndices": null, + "segmentMidpoint": Object { + "added": false, + "index": null, + "value": null, + }, }, "pointerOffset": Object { "x": 0, diff --git a/src/tests/__snapshots__/selection.test.tsx.snap b/src/tests/__snapshots__/selection.test.tsx.snap index 1bf265d1b..d3f6ae527 100644 --- a/src/tests/__snapshots__/selection.test.tsx.snap +++ b/src/tests/__snapshots__/selection.test.tsx.snap @@ -21,10 +21,6 @@ Object { 0, 0, ], - Array [ - 15, - 25, - ], Array [ 30, 50, @@ -40,8 +36,8 @@ Object { "strokeWidth": 1, "type": "arrow", "updated": 1, - "version": 4, - "versionNonce": 453191, + "version": 3, + "versionNonce": 449462985, "width": 30, "x": 10, "y": 10, @@ -69,10 +65,6 @@ Object { 0, 0, ], - Array [ - 15, - 25, - ], Array [ 30, 50, @@ -88,8 +80,8 @@ Object { "strokeWidth": 1, "type": "line", "updated": 1, - "version": 4, - "versionNonce": 453191, + "version": 3, + "versionNonce": 449462985, "width": 30, "x": 10, "y": 10, diff --git a/src/tests/linearElementEditor.test.tsx b/src/tests/linearElementEditor.test.tsx index 56993b72c..e46311e40 100644 --- a/src/tests/linearElementEditor.test.tsx +++ b/src/tests/linearElementEditor.test.tsx @@ -129,6 +129,28 @@ describe("Test Linear Elements", () => { Keyboard.keyPress(KEYS.DELETE); }; + it("should not drag line and add midpoint until dragged beyond a threshold", () => { + createTwoPointerLinearElement("line"); + const line = h.elements[0] as ExcalidrawLinearElement; + const originalX = line.x; + const originalY = line.y; + expect(line.points.length).toEqual(2); + + mouse.clickAt(midpoint[0], midpoint[1]); + drag(midpoint, [midpoint[0] + 1, midpoint[1] + 1]); + + expect(line.points.length).toEqual(2); + + expect(line.x).toBe(originalX); + expect(line.y).toBe(originalY); + expect(line.points.length).toEqual(2); + + drag(midpoint, [midpoint[0] + delta, midpoint[1] + delta]); + expect(line.x).toBe(originalX); + expect(line.y).toBe(originalY); + expect(line.points.length).toEqual(3); + }); + it("should allow dragging line from midpoint in 2 pointer lines outside editor", async () => { createTwoPointerLinearElement("line"); const line = h.elements[0] as ExcalidrawLinearElement; @@ -138,7 +160,7 @@ describe("Test Linear Elements", () => { // drag line from midpoint drag(midpoint, [midpoint[0] + delta, midpoint[1] + delta]); - expect(renderScene).toHaveBeenCalledTimes(10); + expect(renderScene).toHaveBeenCalledTimes(11); expect(line.points.length).toEqual(3); expect(line.points).toMatchInlineSnapshot(` Array [ @@ -195,6 +217,24 @@ describe("Test Linear Elements", () => { }); describe("Inside editor", () => { + it("should not drag line and add midpoint when dragged irrespective of threshold", () => { + createTwoPointerLinearElement("line"); + const line = h.elements[0] as ExcalidrawLinearElement; + const originalX = line.x; + const originalY = line.y; + enterLineEditingMode(line); + + expect(line.points.length).toEqual(2); + + mouse.clickAt(midpoint[0], midpoint[1]); + expect(line.points.length).toEqual(2); + + drag(midpoint, [midpoint[0] + 1, midpoint[1] + 1]); + expect(line.x).toBe(originalX); + expect(line.y).toBe(originalY); + expect(line.points.length).toEqual(3); + }); + it("should allow dragging line from midpoint in 2 pointer lines", async () => { createTwoPointerLinearElement("line"); @@ -203,7 +243,7 @@ describe("Test Linear Elements", () => { // drag line from midpoint drag(midpoint, [midpoint[0] + delta, midpoint[1] + delta]); - expect(renderScene).toHaveBeenCalledTimes(14); + expect(renderScene).toHaveBeenCalledTimes(15); expect(line.points.length).toEqual(3); expect(line.points).toMatchInlineSnapshot(` @@ -282,7 +322,7 @@ describe("Test Linear Elements", () => { // Move the element drag(startPoint, endPoint); - expect(renderScene).toHaveBeenCalledTimes(15); + expect(renderScene).toHaveBeenCalledTimes(16); expect([line.x, line.y]).toEqual([ points[0][0] + deltaX, points[0][1] + deltaY, @@ -339,7 +379,7 @@ describe("Test Linear Elements", () => { lastSegmentMidpoint[1] + delta, ]); - expect(renderScene).toHaveBeenCalledTimes(19); + expect(renderScene).toHaveBeenCalledTimes(21); expect(line.points.length).toEqual(5); expect((h.elements[0] as ExcalidrawLinearElement).points) @@ -359,7 +399,7 @@ describe("Test Linear Elements", () => { ], Array [ 105, - 75, + 70, ], Array [ 40, @@ -378,7 +418,7 @@ describe("Test Linear Elements", () => { // Drag from first point drag(hitCoords, [hitCoords[0] - delta, hitCoords[1] - delta]); - expect(renderScene).toHaveBeenCalledTimes(15); + expect(renderScene).toHaveBeenCalledTimes(16); const newPoints = LinearElementEditor.getPointsGlobalCoordinates(line); expect([newPoints[0][0], newPoints[0][1]]).toEqual([ @@ -404,7 +444,7 @@ describe("Test Linear Elements", () => { // Drag from first point drag(hitCoords, [hitCoords[0] + delta, hitCoords[1] + delta]); - expect(renderScene).toHaveBeenCalledTimes(15); + expect(renderScene).toHaveBeenCalledTimes(16); const newPoints = LinearElementEditor.getPointsGlobalCoordinates(line); expect([newPoints[0][0], newPoints[0][1]]).toEqual([ @@ -438,7 +478,7 @@ describe("Test Linear Elements", () => { // delete 3rd point deletePoint(points[2]); expect(line.points.length).toEqual(3); - expect(renderScene).toHaveBeenCalledTimes(20); + expect(renderScene).toHaveBeenCalledTimes(21); const newMidPoints = LinearElementEditor.getEditorMidPoints( line, @@ -483,7 +523,7 @@ describe("Test Linear Elements", () => { lastSegmentMidpoint[0] + delta, lastSegmentMidpoint[1] + delta, ]); - expect(renderScene).toHaveBeenCalledTimes(19); + expect(renderScene).toHaveBeenCalledTimes(21); expect(line.points.length).toEqual(5); @@ -503,8 +543,8 @@ describe("Test Linear Elements", () => { 50, ], Array [ - 104.58050066266131, - 74.24758482724201, + 106.08587175006699, + 73.29416593965323, ], Array [ 40, @@ -559,7 +599,7 @@ describe("Test Linear Elements", () => { // Drag from first point drag(hitCoords, [hitCoords[0] + delta, hitCoords[1] + delta]); - expect(renderScene).toHaveBeenCalledTimes(15); + expect(renderScene).toHaveBeenCalledTimes(16); const newPoints = LinearElementEditor.getPointsGlobalCoordinates(line); expect([newPoints[0][0], newPoints[0][1]]).toEqual([ @@ -627,7 +667,6 @@ describe("Test Linear Elements", () => { fillStyle: "solid", }), ]; - const origPoints = line.points.map((point) => [...point]); const dragEndPositionOffset = [100, 100] as const; API.setSelectedElements([line]);