From dde3dac931cd0d1db4c27f2809261f4e0e3843ca Mon Sep 17 00:00:00 2001 From: David Luzar Date: Tue, 17 Oct 2023 18:18:20 +0200 Subject: [PATCH] feat: remove bound-arrows from frames (#7157) --- 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, 81 insertions(+), 16 deletions(-) diff --git a/dev-docs/docs/codebase/frames.mdx b/dev-docs/docs/codebase/frames.mdx index 45a551f2..0dfd99ea 100644 --- a/dev-docs/docs/codebase/frames.mdx +++ b/dev-docs/docs/codebase/frames.mdx @@ -20,3 +20,13 @@ 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 a21260d5..fec91c4a 100644 --- a/src/actions/actionDuplicateSelection.tsx +++ b/src/actions/actionDuplicateSelection.tsx @@ -155,7 +155,12 @@ const duplicateElements = ( groupId, ).flatMap((element) => isFrameElement(element) - ? [...getFrameElements(elements, element.id), element] + ? [ + ...getFrameElements(elements, element.id, { + includeBoundArrows: true, + }), + element, + ] : [element], ); @@ -181,7 +186,9 @@ const duplicateElements = ( continue; } if (isElementAFrame) { - const elementsInFrame = getFrameElements(sortedElements, element.id); + const elementsInFrame = getFrameElements(sortedElements, element.id, { + includeBoundArrows: true, + }); elementsWithClones.push( ...markAsProcessed([ diff --git a/src/components/App.tsx b/src/components/App.tsx index d0f618d8..c9d58b35 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -2431,18 +2431,12 @@ 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; @@ -3456,6 +3450,11 @@ 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 d9ea999f..e951dbf5 100644 --- a/src/data/restore.ts +++ b/src/data/restore.ts @@ -43,6 +43,7 @@ import { measureBaseline, } from "../element/textElement"; import { normalizeLink } from "./url"; +import { isValidFrameChild } from "../frame"; type RestoredAppState = Omit< AppState, @@ -396,7 +397,7 @@ const repairBoundElement = ( }; /** - * Remove an element's frameId if its containing frame is non-existent + * resets `frameId` if no longer applicable. * * NOTE mutates elements. */ @@ -404,12 +405,16 @@ const repairFrameMembership = ( element: Mutable, elementsMap: Map>, ) => { - if (element.frameId) { - const containingFrame = elementsMap.get(element.frameId); + if (!element.frameId) { + return; + } - if (!containingFrame) { - element.frameId = null; - } + if ( + !isValidFrameChild(element) || + // target frame not exists + !elementsMap.get(element.frameId) + ) { + element.frameId = null; } }; @@ -453,6 +458,8 @@ 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 3f6cf002..6b39e1af 100644 --- a/src/element/binding.ts +++ b/src/element/binding.ts @@ -27,6 +27,7 @@ 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 @@ -211,6 +212,15 @@ 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 6dccedd2..40b08896 100644 --- a/src/frame.ts +++ b/src/frame.ts @@ -323,7 +323,24 @@ export const groupByFrames = (elements: readonly ExcalidrawElement[]) => { export const getFrameElements = ( allElements: ExcalidrawElementsIncludingDeleted, frameId: string, -) => allElements.filter((element) => element.frameId === frameId); + 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; + }); +}; export const getElementsInResizingFrame = ( allElements: ExcalidrawElementsIncludingDeleted, @@ -451,6 +468,14 @@ 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 ------------------------------- /** @@ -489,6 +514,9 @@ 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 6395ed0e..f7fb35cc 100644 --- a/src/renderer/renderScene.ts +++ b/src/renderer/renderScene.ts @@ -69,6 +69,7 @@ import { } from "../element/Hyperlink"; import { renderSnaps } from "./renderSnaps"; import { + isArrowElement, isEmbeddableElement, isFrameElement, isLinearElement, @@ -984,7 +985,10 @@ const _renderStaticScene = ({ // TODO do we need to check isElementInFrame here? if (frame && isElementInFrame(element, elements, appState)) { - frameClip(frame, context, renderConfig, appState); + // do not clip arrows + if (!isArrowElement(element)) { + frameClip(frame, context, renderConfig, appState); + } } renderElement(element, rc, context, renderConfig, appState); context.restore();