From a524eeb66e77f15c9621ab332bdbf8eccd917785 Mon Sep 17 00:00:00 2001 From: David Luzar Date: Sat, 7 May 2022 21:01:37 +0200 Subject: [PATCH] fix: eraser removed deleted elements (#5155) * fix: eraser removed deleted elements * rename `getElements` API * fix one more case of not including deleted elements --- src/components/App.tsx | 135 ++++++++++++++++++++++------------------- src/element/binding.ts | 107 ++++++++++++++++++-------------- src/scene/Scene.ts | 18 +----- 3 files changed, 135 insertions(+), 125 deletions(-) diff --git a/src/components/App.tsx b/src/components/App.tsx index 868ea1716d..be515e76a5 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -468,7 +468,7 @@ class App extends React.Component { public render() { const { zenModeEnabled, viewModeEnabled } = this.state; const selectedElement = getSelectedElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), this.state, ); const { @@ -501,7 +501,7 @@ class App extends React.Component { files={this.files} setAppState={this.setAppState} actionManager={this.actionManager} - elements={this.scene.getElements()} + elements={this.scene.getNonDeletedElements()} onCollabButtonClick={onCollabButtonClick} onLockToggle={this.toggleLock} onPenModeToggle={this.togglePenMode} @@ -549,7 +549,7 @@ class App extends React.Component { @@ -578,7 +578,7 @@ class App extends React.Component { }; public getSceneElements = () => { - return this.scene.getElements(); + return this.scene.getNonDeletedElements(); }; private syncActionResult = withBatchedUpdates( @@ -1195,24 +1195,26 @@ class App extends React.Component { ); cursorButton[socketId] = user.button; }); - const renderingElements = this.scene.getElements().filter((element) => { - if (isImageElement(element)) { - if ( - // not placed on canvas yet (but in elements array) - this.state.pendingImageElement && - element.id === this.state.pendingImageElement.id - ) { - return false; + const renderingElements = this.scene + .getNonDeletedElements() + .filter((element) => { + if (isImageElement(element)) { + if ( + // not placed on canvas yet (but in elements array) + this.state.pendingImageElement && + element.id === this.state.pendingImageElement.id + ) { + return false; + } } - } - // don't render text element that's being currently edited (it's - // rendered on remote only) - return ( - !this.state.editingElement || - this.state.editingElement.type !== "text" || - element.id !== this.state.editingElement.id - ); - }); + // don't render text element that's being currently edited (it's + // rendered on remote only) + return ( + !this.state.editingElement || + this.state.editingElement.type !== "text" || + element.id !== this.state.editingElement.id + ); + }); const { atLeastOneVisibleElement, scrollBars } = renderScene( renderingElements, this.state, @@ -1525,7 +1527,7 @@ class App extends React.Component { }, {} as any), selectedGroupIds: {}, }, - this.scene.getElements(), + this.scene.getNonDeletedElements(), ), () => { if (opts.files) { @@ -1626,7 +1628,7 @@ class App extends React.Component { scrollToContent = ( target: | ExcalidrawElement - | readonly ExcalidrawElement[] = this.scene.getElements(), + | readonly ExcalidrawElement[] = this.scene.getNonDeletedElements(), ) => { this.setState({ ...calculateScrollCenter( @@ -1671,7 +1673,7 @@ class App extends React.Component { this.files = { ...this.files, ...Object.fromEntries(filesMap) }; - this.scene.getElements().forEach((element) => { + this.scene.getNonDeletedElements().forEach((element) => { if ( isInitializedImageElement(element) && filesMap.has(element.fileId) @@ -1816,7 +1818,7 @@ class App extends React.Component { : ELEMENT_TRANSLATE_AMOUNT); const selectedElements = getSelectedElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), this.state, true, ); @@ -1850,7 +1852,7 @@ class App extends React.Component { event.preventDefault(); } else if (event.key === KEYS.ENTER) { const selectedElements = getSelectedElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), this.state, ); @@ -1918,7 +1920,7 @@ class App extends React.Component { !event[KEYS.CTRL_OR_CMD] ) { const selectedElements = getSelectedElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), this.state, ); if ( @@ -1965,7 +1967,7 @@ class App extends React.Component { } if (isArrowKey(event.key)) { const selectedElements = getSelectedElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), this.state, ); isBindingEnabled(this.state) @@ -2115,7 +2117,9 @@ class App extends React.Component { })); } if (isDeleted) { - fixBindingsAfterDeletion(this.scene.getElements(), [element]); + fixBindingsAfterDeletion(this.scene.getNonDeletedElements(), [ + element, + ]); } if (!isDeleted || isExistingElement) { this.history.resumeRecording(); @@ -2217,9 +2221,9 @@ class App extends React.Component { ): NonDeleted[] { const elements = includeBoundTextElement && includeLockedElements - ? this.scene.getElements() + ? this.scene.getNonDeletedElements() : this.scene - .getElements() + .getNonDeletedElements() .filter( (element) => (includeLockedElements || !element.locked) && @@ -2260,7 +2264,7 @@ class App extends React.Component { let container: ExcalidrawTextContainer | null = null; const selectedElements = getSelectedElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), this.state, ); @@ -2284,7 +2288,7 @@ class App extends React.Component { ) { container = getTextBindableContainerAtPosition( this.scene - .getElements() + .getNonDeletedElements() .filter( (ele) => isTextBindableContainer(ele, false) && !getBoundTextElement(ele), @@ -2388,7 +2392,7 @@ class App extends React.Component { } const selectedElements = getSelectedElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), this.state, ); @@ -2433,7 +2437,7 @@ class App extends React.Component { selectedElementIds: { [hitElement!.id]: true }, selectedGroupIds: {}, }, - this.scene.getElements(), + this.scene.getNonDeletedElements(), ), ); return; @@ -2443,7 +2447,7 @@ class App extends React.Component { resetCursor(this.canvas); if (!event[KEYS.CTRL_OR_CMD] && !this.state.viewModeEnabled) { const selectedElements = getSelectedElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), this.state, ); if (selectedElements.length === 1) { @@ -2469,7 +2473,7 @@ class App extends React.Component { ): ExcalidrawElement | undefined => { // Reversing so we traverse the elements in decreasing order // of z-index - const elements = this.scene.getElements().slice().reverse(); + const elements = this.scene.getNonDeletedElements().slice().reverse(); let hitElementIndex = Infinity; return elements.find((element, index) => { @@ -2731,7 +2735,7 @@ class App extends React.Component { return; } - const elements = this.scene.getElements(); + const elements = this.scene.getNonDeletedElements(); const selectedElements = getSelectedElements(elements, this.state); if ( @@ -2905,7 +2909,7 @@ class App extends React.Component { point.y = nextY; } - const elements = this.scene.getElements().map((ele) => { + const elements = this.scene.getElementsIncludingDeleted().map((ele) => { const id = isBoundToContainer(ele) && idsToUpdate.includes(ele.containerId) ? ele.containerId @@ -3287,7 +3291,7 @@ class App extends React.Component { ): PointerDownState { const origin = viewportCoordsToSceneCoords(event, this.state); const selectedElements = getSelectedElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), this.state, ); const [minX, minY, maxX, maxY] = getCommonBounds(selectedElements); @@ -3305,10 +3309,12 @@ class App extends React.Component { ), // we need to duplicate because we'll be updating this state lastCoords: { ...origin }, - originalElements: this.scene.getElements().reduce((acc, element) => { - acc.set(element.id, deepCopyElement(element)); - return acc; - }, new Map() as PointerDownState["originalElements"]), + originalElements: this.scene + .getNonDeletedElements() + .reduce((acc, element) => { + acc.set(element.id, deepCopyElement(element)); + return acc; + }, new Map() as PointerDownState["originalElements"]), resize: { handleType: false, isResizing: false, @@ -3405,7 +3411,7 @@ class App extends React.Component { pointerDownState: PointerDownState, ): boolean => { if (this.state.activeTool.type === "selection") { - const elements = this.scene.getElements(); + const elements = this.scene.getNonDeletedElements(); const selectedElements = getSelectedElements(elements, this.state); if (selectedElements.length === 1 && !this.state.editingLinearElement) { const elementWithTransformHandleType = @@ -3578,7 +3584,7 @@ class App extends React.Component { }, showHyperlinkPopup: hitElement.link ? "info" : false, }, - this.scene.getElements(), + this.scene.getNonDeletedElements(), ); }); pointerDownState.hit.wasAddedToSelection = true; @@ -3928,7 +3934,7 @@ class App extends React.Component { if (pointerDownState.drag.offset === null) { pointerDownState.drag.offset = tupleToCoors( getDragOffsetXY( - getSelectedElements(this.scene.getElements(), this.state), + getSelectedElements(this.scene.getNonDeletedElements(), this.state), pointerDownState.origin.x, pointerDownState.origin.y, ), @@ -4023,7 +4029,7 @@ class App extends React.Component { pointerDownState.hit.hasHitElementInside) ) { const selectedElements = getSelectedElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), this.state, ); if (selectedElements.every((element) => element.locked)) { @@ -4192,7 +4198,7 @@ class App extends React.Component { if (this.state.activeTool.type === "selection") { pointerDownState.boxSelection.hasOccurred = true; - const elements = this.scene.getElements(); + const elements = this.scene.getNonDeletedElements(); if ( !event.shiftKey && // allows for box-selecting points (without shift) @@ -4208,7 +4214,7 @@ class App extends React.Component { [pointerDownState.hit.element!.id]: true, }, }, - this.scene.getElements(), + this.scene.getNonDeletedElements(), ), ); } else { @@ -4257,7 +4263,7 @@ class App extends React.Component { ? "info" : false, }, - this.scene.getElements(), + this.scene.getNonDeletedElements(), ), ); } @@ -4578,7 +4584,10 @@ class App extends React.Component { // hitElement is part of const idsOfSelectedElementsThatAreInGroups = hitElement.groupIds .flatMap((groupId) => - getElementsInGroup(this.scene.getElements(), groupId), + getElementsInGroup( + this.scene.getNonDeletedElements(), + groupId, + ), ) .map((element) => ({ [element.id]: false })) .reduce((prevId, acc) => ({ ...prevId, ...acc }), {}); @@ -4607,7 +4616,7 @@ class App extends React.Component { [hitElement!.id]: false, }, }, - this.scene.getElements(), + this.scene.getNonDeletedElements(), ), ); } @@ -4629,7 +4638,7 @@ class App extends React.Component { ...prevState, selectedElementIds: { [hitElement.id]: true }, }, - this.scene.getElements(), + this.scene.getNonDeletedElements(), ), })); } @@ -4674,7 +4683,7 @@ class App extends React.Component { if ( activeTool.type !== "selection" || - isSomeElementSelected(this.scene.getElements(), this.state) + isSomeElementSelected(this.scene.getNonDeletedElements(), this.state) ) { this.history.resumeRecording(); } @@ -4683,7 +4692,7 @@ class App extends React.Component { (isBindingEnabled(this.state) ? bindOrUnbindSelectedElements : unbindLinearElements)( - getSelectedElements(this.scene.getElements(), this.state), + getSelectedElements(this.scene.getNonDeletedElements(), this.state), ); } @@ -4706,7 +4715,7 @@ class App extends React.Component { private restoreReadyToEraseElements = ( pointerDownState: PointerDownState, ) => { - const elements = this.scene.getElements().map((ele) => { + const elements = this.scene.getElementsIncludingDeleted().map((ele) => { if ( pointerDownState.elementIdsToErase[ele.id] && pointerDownState.elementIdsToErase[ele.id].erase @@ -4730,7 +4739,7 @@ class App extends React.Component { }; private eraseElements = (pointerDownState: PointerDownState) => { - const elements = this.scene.getElements().map((ele) => { + const elements = this.scene.getElementsIncludingDeleted().map((ele) => { if ( pointerDownState.elementIdsToErase[ele.id] && pointerDownState.elementIdsToErase[ele.id].erase @@ -5099,7 +5108,7 @@ class App extends React.Component { /** adds new images to imageCache and re-renders if needed */ private addNewImagesToImageCache = async ( imageElements: InitializedExcalidrawImageElement[] = getInitializedImageElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), ), files: BinaryFiles = this.files, ) => { @@ -5392,7 +5401,7 @@ class App extends React.Component { ...this.state, selectedElementIds: { [element.id]: true }, }, - this.scene.getElements(), + this.scene.getNonDeletedElements(), ), () => { this._openContextMenu({ top, left }, type); @@ -5468,7 +5477,7 @@ class App extends React.Component { event: MouseEvent | KeyboardEvent, ): boolean => { const selectedElements = getSelectedElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), this.state, ); const transformHandleType = pointerDownState.resize.handleType; @@ -5555,10 +5564,10 @@ class App extends React.Component { const separator = "separator"; - const elements = this.scene.getElements(); + const elements = this.scene.getNonDeletedElements(); const selectedElements = getSelectedElements( - this.scene.getElements(), + this.scene.getNonDeletedElements(), this.state, ); diff --git a/src/element/binding.ts b/src/element/binding.ts index ff23dd07b3..70e9db1d3e 100644 --- a/src/element/binding.ts +++ b/src/element/binding.ts @@ -47,6 +47,20 @@ export const isBindingEnabled = (appState: AppState): boolean => { return appState.isBindingEnabled; }; +const getNonDeletedElements = ( + scene: Scene, + ids: readonly ExcalidrawElement["id"][], +): NonDeleted[] => { + const result: NonDeleted[] = []; + ids.forEach((id) => { + const element = scene.getNonDeletedElement(id); + if (element != null) { + result.push(element); + } + }); + return result; +}; + export const bindOrUnbindLinearElement = ( linearElement: NonDeleted, startBindingElement: ExcalidrawBindableElement | null | "keep", @@ -74,16 +88,17 @@ export const bindOrUnbindLinearElement = ( const onlyUnbound = Array.from(unboundFromElementIds).filter( (id) => !boundToElementIds.has(id), ); - Scene.getScene(linearElement)! - .getNonDeletedElements(onlyUnbound) - .forEach((element) => { + + getNonDeletedElements(Scene.getScene(linearElement)!, onlyUnbound).forEach( + (element) => { mutateElement(element, { boundElements: element.boundElements?.filter( (element) => element.type !== "arrow" || element.id !== linearElement.id, ), }); - }); + }, + ); }; const bindOrUnbindLinearElementEdge = ( @@ -253,7 +268,7 @@ export const getHoveredElementForBinding = ( scene: Scene, ): NonDeleted | null => { const hoveredElement = getElementAtPosition( - scene.getElements(), + scene.getNonDeletedElements(), (element) => isBindableElement(element, false) && bindingBorderTest(element, pointerCoords), @@ -305,46 +320,48 @@ export const updateBoundElements = ( const simultaneouslyUpdatedElementIds = getSimultaneouslyUpdatedElementIds( simultaneouslyUpdated, ); - Scene.getScene(changedElement)! - .getNonDeletedElements(boundLinearElements.map((el) => el.id)) - .forEach((element) => { - if (!isLinearElement(element)) { - return; - } - const bindableElement = changedElement as ExcalidrawBindableElement; - // In case the boundElements are stale - if (!doesNeedUpdate(element, bindableElement)) { - return; - } - const startBinding = maybeCalculateNewGapWhenScaling( - bindableElement, - element.startBinding, - newSize, - ); - const endBinding = maybeCalculateNewGapWhenScaling( - bindableElement, - element.endBinding, - newSize, - ); - // `linearElement` is being moved/scaled already, just update the binding - if (simultaneouslyUpdatedElementIds.has(element.id)) { - mutateElement(element, { startBinding, endBinding }); - return; - } - updateBoundPoint( - element, - "start", - startBinding, - changedElement as ExcalidrawBindableElement, - ); - updateBoundPoint( - element, - "end", - endBinding, - changedElement as ExcalidrawBindableElement, - ); - }); + getNonDeletedElements( + Scene.getScene(changedElement)!, + boundLinearElements.map((el) => el.id), + ).forEach((element) => { + if (!isLinearElement(element)) { + return; + } + + const bindableElement = changedElement as ExcalidrawBindableElement; + // In case the boundElements are stale + if (!doesNeedUpdate(element, bindableElement)) { + return; + } + const startBinding = maybeCalculateNewGapWhenScaling( + bindableElement, + element.startBinding, + newSize, + ); + const endBinding = maybeCalculateNewGapWhenScaling( + bindableElement, + element.endBinding, + newSize, + ); + // `linearElement` is being moved/scaled already, just update the binding + if (simultaneouslyUpdatedElementIds.has(element.id)) { + mutateElement(element, { startBinding, endBinding }); + return; + } + updateBoundPoint( + element, + "start", + startBinding, + changedElement as ExcalidrawBindableElement, + ); + updateBoundPoint( + element, + "end", + endBinding, + changedElement as ExcalidrawBindableElement, + ); + }); }; const doesNeedUpdate = ( @@ -507,7 +524,7 @@ const getElligibleElementsForBindableElementAndWhere = ( bindableElement: NonDeleted, ): SuggestedPointBinding[] => { return Scene.getScene(bindableElement)! - .getElements() + .getNonDeletedElements() .map((element) => { if (!isBindingElement(element, false)) { return null; diff --git a/src/scene/Scene.ts b/src/scene/Scene.ts index 4f05f52eb9..cf078e1468 100644 --- a/src/scene/Scene.ts +++ b/src/scene/Scene.ts @@ -52,13 +52,11 @@ class Scene { private elements: readonly ExcalidrawElement[] = []; private elementsMap = new Map(); - // TODO: getAllElementsIncludingDeleted getElementsIncludingDeleted() { return this.elements; } - // TODO: getAllNonDeletedElements - getElements(): readonly NonDeletedExcalidrawElement[] { + getNonDeletedElements(): readonly NonDeletedExcalidrawElement[] { return this.nonDeletedElements; } @@ -76,20 +74,6 @@ class Scene { return null; } - // TODO: Rename methods here, this is confusing - getNonDeletedElements( - ids: readonly ExcalidrawElement["id"][], - ): NonDeleted[] { - const result: NonDeleted[] = []; - ids.forEach((id) => { - const element = this.getNonDeletedElement(id); - if (element != null) { - result.push(element); - } - }); - return result; - } - replaceAllElements(nextElements: readonly ExcalidrawElement[]) { this.elements = nextElements; this.elementsMap.clear();