From 104f64f1dc9debe44397478e980ecf51c2872536 Mon Sep 17 00:00:00 2001 From: David Luzar Date: Wed, 25 Oct 2023 10:39:19 +0200 Subject: [PATCH] revert: remove bound-arrows from frames (#7190) --- dev-docs/docs/codebase/frames.mdx | 10 -------- src/actions/actionDuplicateSelection.tsx | 11 ++------- src/components/App.tsx | 11 +++++---- src/data/restore.ts | 19 +++++---------- src/element/binding.ts | 10 -------- src/frame.ts | 30 +----------------------- src/renderer/renderScene.ts | 6 +---- 7 files changed, 16 insertions(+), 81 deletions(-) diff --git a/dev-docs/docs/codebase/frames.mdx b/dev-docs/docs/codebase/frames.mdx index 0dfd99ea..45a551f2 100644 --- a/dev-docs/docs/codebase/frames.mdx +++ b/dev-docs/docs/codebase/frames.mdx @@ -20,13 +20,3 @@ Frames should be ordered where frame children come first, followed by the frame ``` If not oredered correctly, the editor will still function, but the elements may not be rendered and clipped correctly. Further, the renderer relies on this ordering for performance optimizations. - -# Arrows - -An arrow can be a child of a frame only if it has no binding (either start or end) to any other element, regardless of whether the bound element is inside the frame or not. - -This ensures that when an arrow is bound to an element outside the frame, it's rendered and behaves correctly. - -Therefore, when an arrow (that's a child of a frame) gets bound to an element, it's automatically removed from the frame. - -Bound-arrow is duplicated alongside a frame only if the arrow start is bound to an element within that frame. diff --git a/src/actions/actionDuplicateSelection.tsx b/src/actions/actionDuplicateSelection.tsx index fec91c4a..a21260d5 100644 --- a/src/actions/actionDuplicateSelection.tsx +++ b/src/actions/actionDuplicateSelection.tsx @@ -155,12 +155,7 @@ const duplicateElements = ( groupId, ).flatMap((element) => isFrameElement(element) - ? [ - ...getFrameElements(elements, element.id, { - includeBoundArrows: true, - }), - element, - ] + ? [...getFrameElements(elements, element.id), element] : [element], ); @@ -186,9 +181,7 @@ const duplicateElements = ( continue; } if (isElementAFrame) { - const elementsInFrame = getFrameElements(sortedElements, element.id, { - includeBoundArrows: true, - }); + const elementsInFrame = getFrameElements(sortedElements, element.id); elementsWithClones.push( ...markAsProcessed([ diff --git a/src/components/App.tsx b/src/components/App.tsx index 880462bb..a697b28b 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -2554,12 +2554,18 @@ class App extends React.Component { const lineHeight = getDefaultLineHeight(textElementProps.fontFamily); if (text.length) { + const topLayerFrame = this.getTopLayerFrameAtSceneCoords({ + x, + y: currentY, + }); + const element = newTextElement({ ...textElementProps, x, y: currentY, text, lineHeight, + frameId: topLayerFrame ? topLayerFrame.id : null, }); acc.push(element); currentY += element.height + LINE_GAP; @@ -3574,11 +3580,6 @@ class App extends React.Component { return getElementsAtPosition(elements, (element) => hitTest(element, this.state, this.frameNameBoundsCache, x, y), ).filter((element) => { - // arrows don't clip even if they're children of frames, - // so always allow hitbox regardless of beinging contained in frame - if (isArrowElement(element)) { - return true; - } // hitting a frame's element from outside the frame is not considered a hit const containingFrame = getContainingFrame(element); return containingFrame && diff --git a/src/data/restore.ts b/src/data/restore.ts index e951dbf5..d9ea999f 100644 --- a/src/data/restore.ts +++ b/src/data/restore.ts @@ -43,7 +43,6 @@ import { measureBaseline, } from "../element/textElement"; import { normalizeLink } from "./url"; -import { isValidFrameChild } from "../frame"; type RestoredAppState = Omit< AppState, @@ -397,7 +396,7 @@ const repairBoundElement = ( }; /** - * resets `frameId` if no longer applicable. + * Remove an element's frameId if its containing frame is non-existent * * NOTE mutates elements. */ @@ -405,16 +404,12 @@ const repairFrameMembership = ( element: Mutable, elementsMap: Map>, ) => { - if (!element.frameId) { - return; - } + if (element.frameId) { + const containingFrame = elementsMap.get(element.frameId); - if ( - !isValidFrameChild(element) || - // target frame not exists - !elementsMap.get(element.frameId) - ) { - element.frameId = null; + if (!containingFrame) { + element.frameId = null; + } } }; @@ -458,8 +453,6 @@ export const restoreElements = ( // repair binding. Mutates elements. const restoredElementsMap = arrayToMap(restoredElements); for (const element of restoredElements) { - // repair frame membership *after* bindings we do in restoreElement() - // since we rely on bindings to be correct if (element.frameId) { repairFrameMembership(element, restoredElementsMap); } diff --git a/src/element/binding.ts b/src/element/binding.ts index 6b39e1af..3f6cf002 100644 --- a/src/element/binding.ts +++ b/src/element/binding.ts @@ -27,7 +27,6 @@ import { LinearElementEditor } from "./linearElementEditor"; import { arrayToMap, tupleToCoors } from "../utils"; import { KEYS } from "../keys"; import { getBoundTextElement, handleBindTextResize } from "./textElement"; -import { isValidFrameChild } from "../frame"; export type SuggestedBinding = | NonDeleted @@ -212,15 +211,6 @@ export const bindLinearElement = ( }), }); } - if (linearElement.frameId && !isValidFrameChild(linearElement)) { - mutateElement( - linearElement, - { - frameId: null, - }, - false, - ); - } }; // Don't bind both ends of a simple segment diff --git a/src/frame.ts b/src/frame.ts index 40b08896..6dccedd2 100644 --- a/src/frame.ts +++ b/src/frame.ts @@ -323,24 +323,7 @@ export const groupByFrames = (elements: readonly ExcalidrawElement[]) => { export const getFrameElements = ( allElements: ExcalidrawElementsIncludingDeleted, frameId: string, - opts?: { includeBoundArrows?: boolean }, -) => { - return allElements.filter((element) => { - if (element.frameId === frameId) { - return true; - } - if (opts?.includeBoundArrows && element.type === "arrow") { - const bindingId = element.startBinding?.elementId; - if (bindingId) { - const boundElement = Scene.getScene(element)?.getElement(bindingId); - if (boundElement?.frameId === frameId) { - return true; - } - } - } - return false; - }); -}; +) => allElements.filter((element) => element.frameId === frameId); export const getElementsInResizingFrame = ( allElements: ExcalidrawElementsIncludingDeleted, @@ -468,14 +451,6 @@ export const getContainingFrame = ( return null; }; -export const isValidFrameChild = (element: ExcalidrawElement) => { - return ( - element.type !== "frame" && - // arrows that are bound to elements cannot be frame children - (element.type !== "arrow" || (!element.startBinding && !element.endBinding)) - ); -}; - // --------------------------- Frame Operations ------------------------------- /** @@ -514,9 +489,6 @@ export const addElementsToFrame = ( elementsToAdd, )) { if (!currTargetFrameChildrenMap.has(element.id)) { - if (!isValidFrameChild(element)) { - continue; - } finalElementsToAdd.push(element); } diff --git a/src/renderer/renderScene.ts b/src/renderer/renderScene.ts index f7fb35cc..6395ed0e 100644 --- a/src/renderer/renderScene.ts +++ b/src/renderer/renderScene.ts @@ -69,7 +69,6 @@ import { } from "../element/Hyperlink"; import { renderSnaps } from "./renderSnaps"; import { - isArrowElement, isEmbeddableElement, isFrameElement, isLinearElement, @@ -985,10 +984,7 @@ const _renderStaticScene = ({ // TODO do we need to check isElementInFrame here? if (frame && isElementInFrame(element, elements, appState)) { - // do not clip arrows - if (!isArrowElement(element)) { - frameClip(frame, context, renderConfig, appState); - } + frameClip(frame, context, renderConfig, appState); } renderElement(element, rc, context, renderConfig, appState); context.restore();