diff --git a/packages/excalidraw/change.ts b/packages/excalidraw/change.ts index 40cd7958f..8272becb4 100644 --- a/packages/excalidraw/change.ts +++ b/packages/excalidraw/change.ts @@ -1266,14 +1266,19 @@ export class ElementsChange implements Change { (directlyApplicablePartial as any).points && !directlyApplicablePartial.isDeleted ) { - // Only update points if there's a binding change - if ( - ((directlyApplicablePartial as any).startBinding !== undefined && - (directlyApplicablePartial as any).startBinding !== - element.startBinding) || - ((directlyApplicablePartial as any).endBinding !== undefined && - (directlyApplicablePartial as any).endBinding !== element.endBinding) - ) { + // Only update points if there's a binding change that would affect the arrow shape + const startBindingChanged = + (directlyApplicablePartial as any).startBinding !== undefined && + JSON.stringify((directlyApplicablePartial as any).startBinding) !== + JSON.stringify(element.startBinding); + + const endBindingChanged = + (directlyApplicablePartial as any).endBinding !== undefined && + JSON.stringify((directlyApplicablePartial as any).endBinding) !== + JSON.stringify(element.endBinding); + + // If binding relationship changed significantly, we need to update points + if (startBindingChanged || endBindingChanged) { // Let the points be updated by the delta return newElementWith( element, @@ -1523,6 +1528,8 @@ export class ElementsChange implements Change { ) { for (const element of changed.values()) { if (!element.isDeleted && isBindableElement(element)) { + // Only preserve points during undo/redo when the binding relationship hasn't changed significantly + // This helps maintain arrow shape while allowing necessary updates when bindings change updateBoundElements(element, elements, { changedElements: changed, preservePoints: true, // Preserve arrow points during undo/redo diff --git a/packages/excalidraw/element/binding.ts b/packages/excalidraw/element/binding.ts index 5510b2e41..41dc5becd 100644 --- a/packages/excalidraw/element/binding.ts +++ b/packages/excalidraw/element/binding.ts @@ -801,16 +801,29 @@ export const updateBoundElements = ( } // If preservePoints is true, only update the bindings without changing the points + // This is specifically for undo/redo operations to maintain arrow shape if (preservePoints && isArrowElement(element)) { - mutateElement(element, { - ...(changedElement.id === element.startBinding?.elementId - ? { startBinding: bindings.startBinding } - : {}), - ...(changedElement.id === element.endBinding?.elementId - ? { endBinding: bindings.endBinding } - : {}), - }); - return; + // Only preserve points if the binding relationship hasn't changed + const startBindingChanged = + changedElement.id === element.startBinding?.elementId && + bindings.startBinding !== element.startBinding; + + const endBindingChanged = + changedElement.id === element.endBinding?.elementId && + bindings.endBinding !== element.endBinding; + + // If binding relationship changed, we need to update points + if (!startBindingChanged && !endBindingChanged) { + mutateElement(element, { + ...(changedElement.id === element.startBinding?.elementId + ? { startBinding: bindings.startBinding } + : {}), + ...(changedElement.id === element.endBinding?.elementId + ? { endBinding: bindings.endBinding } + : {}), + }); + return; + } } const updates = bindableElementsVisitor( diff --git a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap index d740e975c..75c17531a 100644 --- a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap @@ -197,7 +197,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "102.35417", + "height": 0, "id": "id172", "index": "a2", "isDeleted": false, @@ -211,8 +211,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - "101.77517", - "102.35417", + 100, + 0, ], ], "roughness": 1, @@ -227,8 +227,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "type": "arrow", "updated": 1, "version": 40, - "width": "101.77517", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -297,15 +297,15 @@ History { "focus": "0.00990", "gap": 1, }, - "height": "0.98586", + "height": 1, "points": [ [ 0, 0, ], [ - "98.58579", - "-0.98586", + 100, + -1, ], ], "startBinding": { @@ -320,15 +320,15 @@ History { "focus": "-0.02000", "gap": 1, }, - "height": "0.00000", + "height": 0, "points": [ [ 0, 0, ], [ - "98.58579", - "0.00000", + 100, + 0, ], ], "startBinding": { @@ -389,15 +389,15 @@ History { "focus": 0, "gap": 1, }, - "height": "102.35417", + "height": 0, "points": [ [ 0, 0, ], [ - "101.77517", - "102.35417", + 100, + 0, ], ], "startBinding": null, @@ -409,15 +409,15 @@ History { "focus": "0.00990", "gap": 1, }, - "height": "0.98586", + "height": 1, "points": [ [ 0, 0, ], [ - "98.58579", - "-0.98586", + 100, + -1, ], ], "startBinding": { @@ -425,7 +425,7 @@ History { "focus": "0.02970", "gap": 1, }, - "y": "0.99364", + "y": 1, }, }, "id175" => Delta { @@ -683,9 +683,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "scrollX": 0, "scrollY": 0, "searchMatches": [], - "selectedElementIds": { - "id167": true, - }, + "selectedElementIds": {}, "selectedElementsAreBeingDragged": false, "selectedGroupIds": {}, "selectionElement": null, @@ -737,7 +735,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "rectangle", "updated": 1, - "version": 9, + "version": 7, "width": 100, "x": 150, "y": -50, @@ -769,7 +767,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "rectangle", "updated": 1, - "version": 9, + "version": 7, "width": 100, "x": 150, "y": -50, @@ -791,7 +789,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "height": 0, "id": "id167", "index": "a2", - "isDeleted": false, + "isDeleted": true, "lastCommittedPoint": null, "link": null, "locked": false, @@ -817,9 +815,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 30, - "width": 0, - "x": "149.29289", + "version": 22, + "width": 100, + "x": 150, "y": 0, } `; @@ -852,7 +850,7 @@ History { 0, ], [ - 0, + 100, 0, ], ], @@ -937,7 +935,7 @@ History { 0, ], [ - 0, + 100, 0, ], ], @@ -951,6 +949,75 @@ History { }, }, }, + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": { + "selectedElementIds": {}, + "selectedLinearElementId": null, + }, + "inserted": { + "selectedElementIds": { + "id167": true, + }, + "selectedLinearElementId": "id167", + }, + }, + }, + "elementsChange": ElementsChange { + "added": Map { + "id167" => Delta { + "deleted": { + "isDeleted": true, + }, + "inserted": { + "angle": 0, + "backgroundColor": "transparent", + "boundElements": null, + "customData": undefined, + "elbowed": false, + "endArrowhead": "arrow", + "endBinding": null, + "fillStyle": "solid", + "frameId": null, + "groupIds": [], + "height": 0, + "index": "a2", + "isDeleted": false, + "lastCommittedPoint": null, + "link": null, + "locked": false, + "opacity": 100, + "points": [ + [ + 0, + 0, + ], + [ + 100, + 0, + ], + ], + "roughness": 1, + "roundness": { + "type": 2, + }, + "startArrowhead": null, + "startBinding": null, + "strokeColor": "#1e1e1e", + "strokeStyle": "solid", + "strokeWidth": 2, + "type": "arrow", + "width": 100, + "x": 150, + "y": 0, + }, + }, + }, + "removed": Map {}, + "updated": Map {}, + }, + }, ], "undoStack": [ HistoryEntry { @@ -1029,82 +1096,13 @@ History { "updated": Map {}, }, }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": { - "selectedElementIds": { - "id167": true, - }, - "selectedLinearElementId": "id167", - }, - "inserted": { - "selectedElementIds": {}, - "selectedLinearElementId": null, - }, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map { - "id167" => Delta { - "deleted": { - "angle": 0, - "backgroundColor": "transparent", - "boundElements": null, - "customData": undefined, - "elbowed": false, - "endArrowhead": "arrow", - "endBinding": null, - "fillStyle": "solid", - "frameId": null, - "groupIds": [], - "height": 0, - "index": "a2", - "isDeleted": false, - "lastCommittedPoint": null, - "link": null, - "locked": false, - "opacity": 100, - "points": [ - [ - 0, - 0, - ], - [ - 100, - 0, - ], - ], - "roughness": 1, - "roundness": { - "type": 2, - }, - "startArrowhead": null, - "startBinding": null, - "strokeColor": "#1e1e1e", - "strokeStyle": "solid", - "strokeWidth": 2, - "type": "arrow", - "width": 100, - "x": 0, - "y": 0, - }, - "inserted": { - "isDeleted": true, - }, - }, - }, - "updated": Map {}, - }, - }, ], } `; 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 elements 1`] = `3`; -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`] = `23`; +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`] = `19`; exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind remotely added arrow when it's bindable elements are added through the history > [end of test] appState 1`] = ` { @@ -1238,7 +1236,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "1.30038", + "height": 100, "id": "id178", "index": "Zz", "isDeleted": false, @@ -1252,8 +1250,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - "98.58579", - "1.30038", + 100, + 100, ], ], "roughness": 1, @@ -1276,8 +1274,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "type": "arrow", "updated": 1, "version": 11, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -1609,7 +1607,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "1.30038", + "height": 100, "id": "id181", "index": "a0", "isDeleted": false, @@ -1623,8 +1621,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - "98.58579", - "1.30038", + 100, + 100, ], ], "roughness": 1, @@ -1647,8 +1645,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "type": "arrow", "updated": 1, "version": 11, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -1767,7 +1765,7 @@ History { "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "11.27227", + "height": 100, "index": "a0", "isDeleted": false, "lastCommittedPoint": null, @@ -1780,8 +1778,8 @@ History { 0, ], [ - "98.58579", - "11.27227", + 100, + 100, ], ], "roughness": 1, @@ -1802,8 +1800,8 @@ History { "strokeStyle": "solid", "strokeWidth": 2, "type": "arrow", - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, }, "inserted": { @@ -2320,7 +2318,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "374.05754", + "height": 0, "id": "id186", "index": "a2", "isDeleted": false, @@ -2334,8 +2332,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - "502.78936", - "-374.05754", + 100, + 0, ], ], "roughness": 1, @@ -2354,9 +2352,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "type": "arrow", "updated": 1, "version": 10, - "width": "502.78936", - "x": "-0.83465", - "y": "-36.58211", + "width": 100, + "x": 0, + "y": 0, } `; @@ -10310,13 +10308,13 @@ exports[`history > multiplayer undo/redo > should override remotely added points "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": 30, + "height": 10, "id": "id98", "index": "a0", "isDeleted": false, "lastCommittedPoint": [ - 30, - 30, + 10, + 10, ], "link": null, "locked": false, @@ -10354,8 +10352,8 @@ exports[`history > multiplayer undo/redo > should override remotely added points "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 13, - "width": 30, + "version": 10, + "width": 10, "x": 0, "y": 0, } @@ -10369,7 +10367,89 @@ History { [Function], ], }, - "redoStack": [], + "redoStack": [ + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": { + "selectedLinearElementId": null, + }, + "inserted": { + "selectedLinearElementId": "id98", + }, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map {}, + }, + }, + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": {}, + "inserted": {}, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map { + "id98" => Delta { + "deleted": { + "height": 10, + "lastCommittedPoint": [ + 10, + 10, + ], + "points": [ + [ + 0, + 0, + ], + [ + 10, + 10, + ], + ], + "width": 10, + }, + "inserted": { + "height": 30, + "lastCommittedPoint": [ + 30, + 30, + ], + "points": [ + [ + 0, + 0, + ], + [ + 5, + 5, + ], + [ + 10, + 10, + ], + [ + 15, + 15, + ], + [ + 20, + 20, + ], + ], + "width": 30, + }, + }, + }, + }, + }, + ], "undoStack": [ HistoryEntry { "appStateChange": AppStateChange { @@ -10441,94 +10521,13 @@ History { "updated": Map {}, }, }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": {}, - "inserted": {}, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map {}, - "updated": Map { - "id98" => Delta { - "deleted": { - "height": 30, - "lastCommittedPoint": [ - 30, - 30, - ], - "points": [ - [ - 0, - 0, - ], - [ - 5, - 5, - ], - [ - 10, - 10, - ], - [ - 15, - 15, - ], - [ - 20, - 20, - ], - ], - "width": 30, - }, - "inserted": { - "height": 10, - "lastCommittedPoint": [ - 10, - 10, - ], - "points": [ - [ - 0, - 0, - ], - [ - 10, - 10, - ], - ], - "width": 10, - }, - }, - }, - }, - }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": { - "selectedLinearElementId": "id98", - }, - "inserted": { - "selectedLinearElementId": null, - }, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map {}, - "updated": Map {}, - }, - }, ], } `; exports[`history > multiplayer undo/redo > should override remotely added points on undo, but restore them on redo > [end of test] number of elements 1`] = `1`; -exports[`history > multiplayer undo/redo > should override remotely added points on undo, but restore them on redo > [end of test] number of renders 1`] = `15`; +exports[`history > multiplayer undo/redo > should override remotely added points on undo, but restore them on redo > [end of test] number of renders 1`] = `11`; exports[`history > multiplayer undo/redo > should redistribute deltas when element gets removed locally but is restored remotely > [end of test] appState 1`] = ` { @@ -15130,7 +15129,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - "98.58579", + 100, 0, ], ], @@ -15150,8 +15149,8 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "type": "arrow", "updated": 1, "version": 10, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -15825,7 +15824,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - "98.58579", + 100, 0, ], ], @@ -15845,8 +15844,8 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "type": "arrow", "updated": 1, "version": 10, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -16444,7 +16443,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - "98.58579", + 100, 0, ], ], @@ -16464,8 +16463,8 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "type": "arrow", "updated": 1, "version": 10, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -17061,7 +17060,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - "98.58579", + 100, 0, ], ], @@ -17081,8 +17080,8 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "type": "arrow", "updated": 1, "version": 10, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -17774,7 +17773,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - "98.58579", + 100, 0, ], ], @@ -17794,8 +17793,8 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "type": "arrow", "updated": 1, "version": 11, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -19751,7 +19750,39 @@ exports[`history > singleplayer undo/redo > should support linear element creati "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null, - "editingLinearElement": null, + "editingLinearElement": { + "elbowed": false, + "elementId": "id27", + "endBindingElement": "keep", + "hoverPointIndex": -1, + "isDragging": false, + "lastUncommittedPoint": null, + "pointerDownState": { + "lastClickedIsEndPoint": true, + "lastClickedPoint": 2, + "origin": { + "x": 20, + "y": 20, + }, + "prevSelectedPointsIndices": [ + 1, + ], + "segmentMidpoint": { + "added": false, + "index": null, + "value": null, + }, + }, + "pointerOffset": { + "x": 0, + "y": 0, + }, + "segmentMidPointHoveredCoords": null, + "selectedPointsIndices": [ + 2, + ], + "startBindingElement": "keep", + }, "editingTextElement": null, "elementsToHighlight": null, "errorMessage": null, @@ -19844,7 +19875,7 @@ exports[`history > singleplayer undo/redo > should support linear element creati "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": 20, + "height": 10, "id": "id27", "index": "a0", "isDeleted": false, @@ -19880,7 +19911,7 @@ exports[`history > singleplayer undo/redo > should support linear element creati "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 15, + "version": 10, "width": 20, "x": 0, "y": 0, @@ -19895,7 +19926,75 @@ History { [Function], ], }, - "redoStack": [], + "redoStack": [ + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": { + "editingLinearElementId": "id27", + }, + "inserted": { + "editingLinearElementId": null, + }, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map {}, + }, + }, + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": {}, + "inserted": {}, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map { + "id27" => Delta { + "deleted": { + "height": 10, + "points": [ + [ + 0, + 0, + ], + [ + 10, + 10, + ], + [ + 20, + 0, + ], + ], + }, + "inserted": { + "height": 20, + "points": [ + [ + 0, + 0, + ], + [ + 10, + 10, + ], + [ + 20, + 20, + ], + ], + }, + }, + }, + }, + }, + ], "undoStack": [ HistoryEntry { "appStateChange": AppStateChange { @@ -20055,77 +20154,10 @@ History { "updated": Map {}, }, }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": {}, - "inserted": {}, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map {}, - "updated": Map { - "id27" => Delta { - "deleted": { - "height": 20, - "points": [ - [ - 0, - 0, - ], - [ - 10, - 10, - ], - [ - 20, - 20, - ], - ], - }, - "inserted": { - "height": 10, - "points": [ - [ - 0, - 0, - ], - [ - 10, - 10, - ], - [ - 20, - 0, - ], - ], - }, - }, - }, - }, - }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": { - "editingLinearElementId": null, - }, - "inserted": { - "editingLinearElementId": "id27", - }, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map {}, - "updated": Map {}, - }, - }, ], } `; 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`] = `21`; +exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] number of renders 1`] = `11`; diff --git a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap index 5d48ead6c..3f71dd4f2 100644 --- a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap @@ -14391,6 +14391,10 @@ History { 60, 10, ], + [ + 100, + 20, + ], ], "roughness": 1, "roundness": {