From 87fbbedc48febf2e3e9c7930701b5c8d8127099f Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Thu, 14 Aug 2025 12:52:40 +0200 Subject: [PATCH] fix: Refactor actionFinalize for linear --- .../excalidraw/actions/actionFinalize.tsx | 82 +-- .../tests/__snapshots__/history.test.tsx.snap | 523 ++---------------- .../regressionTests.test.tsx.snap | 4 +- 3 files changed, 70 insertions(+), 539 deletions(-) diff --git a/packages/excalidraw/actions/actionFinalize.tsx b/packages/excalidraw/actions/actionFinalize.tsx index 04d315491..10bd2814a 100644 --- a/packages/excalidraw/actions/actionFinalize.tsx +++ b/packages/excalidraw/actions/actionFinalize.tsx @@ -2,7 +2,6 @@ import { pointFrom } from "@excalidraw/math"; import { bindOrUnbindBindingElement } from "@excalidraw/element/binding"; import { - isSimpleArrow, isValidPolygon, LinearElementEditor, newElementWith, @@ -29,7 +28,6 @@ import { CaptureUpdateAction } from "@excalidraw/element"; import type { GlobalPoint, LocalPoint } from "@excalidraw/math"; import type { - ExcalidrawArrowElement, ExcalidrawElement, ExcalidrawLinearElement, NonDeleted, @@ -61,7 +59,7 @@ export const actionFinalize = register({ if (data && appState.selectedLinearElement) { const { event, sceneCoords } = data; - const element = LinearElementEditor.getElement( + const element = LinearElementEditor.getElement( appState.selectedLinearElement.elementId, elementsMap, ); @@ -83,7 +81,7 @@ export const actionFinalize = register({ app.scene, ); - if (isSimpleArrow(element)) { + if (isBindingElement(element)) { const newArrow = !appState.selectedLinearElement?.selectedPointsIndices; const selectedPointsIndices = newArrow @@ -106,6 +104,16 @@ export const actionFinalize = register({ bindOrUnbindBindingElement(element, draggedPoints, scene, appState, { newArrow, }); + } else if (isLineElement(element)) { + if ( + appState.selectedLinearElement?.isEditing && + !appState.newElement && + !isValidPolygon(element.points) + ) { + scene.mutateElement(element, { + polygon: false, + }); + } } if (linearElementEditor !== appState.selectedLinearElement) { @@ -125,57 +133,6 @@ export const actionFinalize = register({ }); } - return { - elements: newElements, - appState: { - ...appState, - selectedLinearElement: { - ...linearElementEditor, - selectedPointsIndices: null, - }, - suggestedBindings: [], - newElement: null, - multiElement: null, - }, - captureUpdate: CaptureUpdateAction.IMMEDIATELY, - }; - } - } - - if (appState.selectedLinearElement?.isEditing && !appState.newElement) { - const { elementId } = appState.selectedLinearElement; - const element = LinearElementEditor.getElement(elementId, elementsMap); - - if (element) { - if (isBindingElement(element)) { - const updates = - appState.selectedLinearElement?.pointerDownState.prevSelectedPointsIndices?.reduce( - (updates, index) => { - updates.set(index, { - point: element.points[index], - draggedPoints: true, - }); - - return updates; - }, - new Map(), - ) ?? new Map(); - const allPointsSelected = - appState.selectedLinearElement?.pointerDownState - .prevSelectedPointsIndices?.length === element.points.length; - - // Dragging the entire arrow doesn't allow binding. - if (!allPointsSelected) { - bindOrUnbindBindingElement(element, updates, scene, appState); - } - } - - if (isLineElement(element) && !isValidPolygon(element.points)) { - scene.mutateElement(element, { - polygon: false, - }); - } - return { elements: element.points.length < 2 || isInvisiblySmallElement(element) @@ -185,15 +142,18 @@ export const actionFinalize = register({ } return el; }) - : undefined, + : newElements, appState: { ...appState, cursorButton: "up", - selectedLinearElement: new LinearElementEditor( - element, - arrayToMap(elementsMap), - false, // exit editing mode - ), + selectedLinearElement: { + ...linearElementEditor, + selectedPointsIndices: null, + isEditing: false, + }, + suggestedBindings: [], + newElement: null, + multiElement: null, }, captureUpdate: CaptureUpdateAction.IMMEDIATELY, }; diff --git a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap index db0d73bad..c1549cc2f 100644 --- a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap @@ -31,7 +31,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "currentItemStrokeStyle": "solid", "currentItemStrokeWidth": 2, "currentItemTextAlign": "left", - "cursorButton": "down", + "cursorButton": "up", "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null, @@ -279,195 +279,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 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`] = `10`; -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`] = `21`; - -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] redo stack 1`] = ` -[ - { - "appState": AppStateDelta { - "delta": Delta { - "deleted": {}, - "inserted": {}, - }, - }, - "elements": { - "added": {}, - "removed": {}, - "updated": { - "id0": { - "deleted": { - "version": 12, - }, - "inserted": { - "version": 11, - }, - }, - "id1": { - "deleted": { - "boundElements": [], - "version": 9, - }, - "inserted": { - "boundElements": [ - { - "id": "id4", - "type": "arrow", - }, - ], - "version": 8, - }, - }, - "id15": { - "deleted": { - "boundElements": [ - { - "id": "id4", - "type": "arrow", - }, - ], - "version": 9, - }, - "inserted": { - "boundElements": [], - "version": 8, - }, - }, - "id4": { - "deleted": { - "endBinding": { - "elementId": "id15", - "fixedPoint": [ - "0.50000", - 1, - ], - "focus": 0, - "gap": 1, - }, - "height": "104.34908", - "points": [ - [ - 0, - 0, - ], - [ - "124.00500", - "104.34908", - ], - ], - "startBinding": { - "elementId": "id0", - "focus": "0.02970", - "gap": 1, - }, - "version": 33, - }, - "inserted": { - "endBinding": { - "elementId": "id1", - "focus": "-0.02000", - "gap": 1, - }, - "height": "0.00849", - "points": [ - [ - 0, - 0, - ], - [ - "98.00000", - "-0.00849", - ], - ], - "startBinding": { - "elementId": "id0", - "focus": "0.02000", - "gap": 1, - }, - "version": 30, - }, - }, - }, - }, - "id": "id22", - }, - { - "appState": AppStateDelta { - "delta": Delta { - "deleted": {}, - "inserted": {}, - }, - }, - "elements": { - "added": {}, - "removed": {}, - "updated": { - "id0": { - "deleted": { - "boundElements": [], - "version": 13, - }, - "inserted": { - "boundElements": [ - { - "id": "id4", - "type": "arrow", - }, - ], - "version": 12, - }, - }, - "id15": { - "deleted": { - "version": 10, - }, - "inserted": { - "version": 9, - }, - }, - "id4": { - "deleted": { - "height": 150, - "points": [ - [ - 0, - 0, - ], - [ - "124.00500", - 150, - ], - ], - "startBinding": null, - "version": 35, - "y": 0, - }, - "inserted": { - "height": "104.34908", - "points": [ - [ - 0, - 0, - ], - [ - "124.00500", - "104.34908", - ], - ], - "startBinding": { - "elementId": "id0", - "focus": "0.02970", - "gap": 1, - }, - "version": 33, - "y": "45.65092", - }, - }, - }, - }, - "id": "id23", - }, -] -`; +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] redo stack 1`] = `[]`; 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] undo stack 1`] = ` [ @@ -958,7 +770,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "currentItemStrokeStyle": "solid", "currentItemStrokeWidth": 2, "currentItemTextAlign": "left", - "cursorButton": "down", + "cursorButton": "up", "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null, @@ -1206,106 +1018,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 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`] = `12`; -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] redo stack 1`] = ` -[ - { - "appState": AppStateDelta { - "delta": Delta { - "deleted": {}, - "inserted": {}, - }, - }, - "elements": { - "added": {}, - "removed": {}, - "updated": { - "id0": { - "deleted": { - "version": 13, - }, - "inserted": { - "version": 12, - }, - }, - "id1": { - "deleted": { - "boundElements": [], - "version": 9, - }, - "inserted": { - "boundElements": [ - { - "id": "id4", - "type": "arrow", - }, - ], - "version": 8, - }, - }, - "id4": { - "deleted": { - "endBinding": null, - "version": 30, - }, - "inserted": { - "endBinding": { - "elementId": "id1", - "focus": -0, - "gap": 1, - }, - "version": 28, - }, - }, - }, - }, - "id": "id21", - }, - { - "appState": AppStateDelta { - "delta": Delta { - "deleted": {}, - "inserted": {}, - }, - }, - "elements": { - "added": {}, - "removed": {}, - "updated": { - "id0": { - "deleted": { - "boundElements": [], - "version": 14, - }, - "inserted": { - "boundElements": [ - { - "id": "id4", - "type": "arrow", - }, - ], - "version": 13, - }, - }, - "id4": { - "deleted": { - "startBinding": null, - "version": 31, - }, - "inserted": { - "startBinding": { - "elementId": "id0", - "focus": 0, - "gap": 1, - }, - "version": 30, - }, - }, - }, - }, - "id": "id22", - }, -] -`; +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] redo stack 1`] = `[]`; 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] undo stack 1`] = ` [ @@ -2896,7 +2609,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "currentItemStrokeStyle": "solid", "currentItemStrokeWidth": 2, "currentItemTextAlign": "left", - "cursorButton": "down", + "cursorButton": "up", "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null, @@ -8093,7 +7806,7 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh exports[`history > multiplayer undo/redo > should iterate through the history when selected or editing linear element was remotely deleted > [end of test] number of elements 1`] = `1`; -exports[`history > multiplayer undo/redo > should iterate through the history when selected or editing linear element was remotely deleted > [end of test] number of renders 1`] = `9`; +exports[`history > multiplayer undo/redo > should iterate through the history when selected or editing linear element was remotely deleted > [end of test] number of renders 1`] = `10`; exports[`history > multiplayer undo/redo > should iterate through the history when selected or editing linear element was remotely deleted > [end of test] redo stack 1`] = `[]`; @@ -16629,7 +16342,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "currentItemStrokeStyle": "solid", "currentItemStrokeWidth": 2, "currentItemTextAlign": "left", - "cursorButton": "down", + "cursorButton": "up", "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null, @@ -16892,85 +16605,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding exports[`history > singleplayer undo/redo > should support bidirectional bindings > should unbind arrow from non deleted bindable elements on deletion and rebind on undo > [end of test] number of renders 1`] = `10`; -exports[`history > singleplayer undo/redo > should support bidirectional bindings > should unbind arrow from non deleted bindable elements on deletion and rebind on undo > [end of test] redo stack 1`] = ` -[ - { - "appState": AppStateDelta { - "delta": Delta { - "deleted": { - "selectedElementIds": { - "id13": true, - }, - "selectedLinearElement": { - "elementId": "id13", - "isEditing": false, - }, - }, - "inserted": { - "selectedElementIds": {}, - "selectedLinearElement": null, - }, - }, - }, - "elements": { - "added": {}, - "removed": { - "id13": { - "deleted": { - "isDeleted": false, - "version": 10, - }, - "inserted": { - "isDeleted": true, - "version": 7, - }, - }, - }, - "updated": { - "id0": { - "deleted": { - "boundElements": [ - { - "id": "id13", - "type": "arrow", - }, - ], - "version": 6, - }, - "inserted": { - "boundElements": [], - "version": 5, - }, - }, - "id1": { - "deleted": { - "version": 5, - }, - "inserted": { - "version": 4, - }, - }, - "id2": { - "deleted": { - "boundElements": [ - { - "id": "id13", - "type": "arrow", - }, - ], - "version": 5, - }, - "inserted": { - "boundElements": [], - "version": 4, - }, - }, - }, - }, - "id": "id18", - }, -] -`; +exports[`history > singleplayer undo/redo > should support bidirectional bindings > should unbind arrow from non deleted bindable elements on deletion and rebind on undo > [end of test] redo stack 1`] = `[]`; exports[`history > singleplayer undo/redo > should support bidirectional bindings > should unbind arrow from non deleted bindable elements on deletion and rebind on undo > [end of test] undo stack 1`] = ` [ @@ -17345,7 +16980,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "currentItemStrokeStyle": "solid", "currentItemStrokeWidth": 2, "currentItemTextAlign": "left", - "cursorButton": "down", + "cursorButton": "up", "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null, @@ -17927,7 +17562,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding }, "inserted": { "boundElements": [], - "version": 5, + "version": 3, }, }, "id1": { @@ -17991,7 +17626,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "currentItemStrokeStyle": "solid", "currentItemStrokeWidth": 2, "currentItemTextAlign": "left", - "cursorButton": "down", + "cursorButton": "up", "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null, @@ -18573,7 +18208,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding }, "inserted": { "boundElements": [], - "version": 9, + "version": 3, }, }, "id1": { @@ -18637,7 +18272,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "currentItemStrokeStyle": "solid", "currentItemStrokeWidth": 2, "currentItemTextAlign": "left", - "cursorButton": "down", + "cursorButton": "up", "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null, @@ -18900,74 +18535,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding exports[`history > singleplayer undo/redo > should support bidirectional bindings > should unbind rectangle from arrow on deletion and rebind on undo > [end of test] number of renders 1`] = `10`; -exports[`history > singleplayer undo/redo > should support bidirectional bindings > should unbind rectangle from arrow on deletion and rebind on undo > [end of test] redo stack 1`] = ` -[ - { - "appState": AppStateDelta { - "delta": Delta { - "deleted": { - "selectedElementIds": { - "id0": true, - }, - }, - "inserted": { - "selectedElementIds": {}, - }, - }, - }, - "elements": { - "added": {}, - "removed": { - "id0": { - "deleted": { - "isDeleted": false, - "version": 6, - }, - "inserted": { - "isDeleted": true, - "version": 5, - }, - }, - "id1": { - "deleted": { - "isDeleted": false, - "version": 6, - }, - "inserted": { - "isDeleted": true, - "version": 5, - }, - }, - }, - "updated": { - "id13": { - "deleted": { - "startBinding": { - "elementId": "id0", - "focus": 0, - "gap": 1, - }, - "version": 10, - }, - "inserted": { - "startBinding": null, - "version": 7, - }, - }, - "id2": { - "deleted": { - "version": 4, - }, - "inserted": { - "version": 3, - }, - }, - }, - }, - "id": "id21", - }, -] -`; +exports[`history > singleplayer undo/redo > should support bidirectional bindings > should unbind rectangle from arrow on deletion and rebind on undo > [end of test] redo stack 1`] = `[]`; exports[`history > singleplayer undo/redo > should support bidirectional bindings > should unbind rectangle from arrow on deletion and rebind on undo > [end of test] undo stack 1`] = ` [ @@ -19342,7 +18910,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "currentItemStrokeStyle": "solid", "currentItemStrokeWidth": 2, "currentItemTextAlign": "left", - "cursorButton": "down", + "cursorButton": "up", "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null, @@ -21496,7 +21064,7 @@ exports[`history > singleplayer undo/redo > should support linear element creati "frameId": null, "groupIds": [], "height": 0, - "id": "id9", + "id": "id17", "index": null, "isDeleted": false, "link": null, @@ -21504,7 +21072,7 @@ exports[`history > singleplayer undo/redo > should support linear element creati "opacity": 100, "roughness": 1, "roundness": null, - "seed": 23633383, + "seed": 640725609, "strokeColor": "#1e1e1e", "strokeStyle": "solid", "strokeWidth": 2, @@ -21514,7 +21082,7 @@ exports[`history > singleplayer undo/redo > should support linear element creati "versionNonce": 0, "width": 0, "x": 20, - "y": 0, + "y": 20, }, "shouldCacheIgnoreZoom": false, "showHyperlinkPopup": false, @@ -21596,9 +21164,36 @@ exports[`history > singleplayer undo/redo > should support linear element creati exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] number of elements 1`] = `1`; -exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] number of renders 1`] = `10`; +exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] number of renders 1`] = `15`; -exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] redo stack 1`] = `[]`; +exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] redo stack 1`] = ` +[ + { + "appState": AppStateDelta { + "delta": Delta { + "deleted": { + "selectedLinearElement": { + "elementId": "id0", + "isEditing": true, + }, + }, + "inserted": { + "selectedLinearElement": { + "elementId": "id0", + "isEditing": false, + }, + }, + }, + }, + "elements": { + "added": {}, + "removed": {}, + "updated": {}, + }, + "id": "id14", + }, +] +`; exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] undo stack 1`] = ` [ @@ -21863,29 +21458,5 @@ exports[`history > singleplayer undo/redo > should support linear element creati }, "id": "id11", }, - { - "appState": AppStateDelta { - "delta": Delta { - "deleted": { - "selectedLinearElement": { - "elementId": "id0", - "isEditing": false, - }, - }, - "inserted": { - "selectedLinearElement": { - "elementId": "id0", - "isEditing": true, - }, - }, - }, - }, - "elements": { - "added": {}, - "removed": {}, - "updated": {}, - }, - "id": "id13", - }, ] `; diff --git a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap index b9665b0e1..7aa2f42d2 100644 --- a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap @@ -8717,7 +8717,7 @@ exports[`regression tests > key 5 selects arrow tool > [end of test] appState 1` "currentItemStrokeStyle": "solid", "currentItemStrokeWidth": 2, "currentItemTextAlign": "left", - "cursorButton": "down", + "cursorButton": "up", "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null, @@ -9366,7 +9366,7 @@ exports[`regression tests > key a selects arrow tool > [end of test] appState 1` "currentItemStrokeStyle": "solid", "currentItemStrokeWidth": 2, "currentItemTextAlign": "left", - "cursorButton": "down", + "cursorButton": "up", "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null,