From 3544a15ed21c4f1976cfe59e7a398b07f84341b2 Mon Sep 17 00:00:00 2001 From: Sean Parsons Date: Thu, 5 Sep 2024 17:15:22 +0100 Subject: [PATCH 1/6] performance(editor) Improved `buildTree`. - Reworked `buildTree_MUTATE` to build the tree from the root downwards, combining it with some of the logic taken from `getChildrenPaths`. - Further improvement to `isDescendantOf` replacing the previous implementation. - `getReorderedPaths` now avoids a lot of repeated logic for checking the element type. --- editor/src/benchmarks.benchmark.ts | 4 +- .../shared/element-path-tree.benchmark.ts | 56 ++++++ editor/src/core/shared/element-path-tree.ts | 167 +++++++++--------- editor/src/core/shared/element-path.ts | 39 +++- 4 files changed, 169 insertions(+), 97 deletions(-) diff --git a/editor/src/benchmarks.benchmark.ts b/editor/src/benchmarks.benchmark.ts index 4cb01c8292cf..57192a78c629 100644 --- a/editor/src/benchmarks.benchmark.ts +++ b/editor/src/benchmarks.benchmark.ts @@ -5,9 +5,9 @@ import { benchmarkOptics } from './core/shared/optics.benchmark' import { benchmarkPropertyPathFunction } from './core/shared/property-path.benchmark' import { benchmarkGetUniqueUids } from './core/model/get-unique-ids.benchmark' -// await benchmarkBuildTree() +await benchmarkBuildTree() // await benchmarkElementPathFunction() // await benchmarkPropertyPathFunction() // await benchmarkAttributes() // await benchmarkOptics() -await benchmarkGetUniqueUids() +// await benchmarkGetUniqueUids() diff --git a/editor/src/core/shared/element-path-tree.benchmark.ts b/editor/src/core/shared/element-path-tree.benchmark.ts index cc55e77ac41e..ffd7351e1ff3 100644 --- a/editor/src/core/shared/element-path-tree.benchmark.ts +++ b/editor/src/core/shared/element-path-tree.benchmark.ts @@ -17,6 +17,62 @@ function dummyMetadataFromPaths(elementPaths: ElementPath[]): ElementInstanceMet } export async function benchmarkBuildTree(): Promise { + await Benny.suite( + 'isDescendantOf - for immediate descendant', + Benny.add('for immediate descendant', () => { + const path1 = EP.fromString(`aaa/bbb/ccc`) + const path2 = EP.fromString(`aaa/bbb/ccc:ddd`) + + return () => { + EP.isDescendantOf(path2, path1) + } + }), + Benny.cycle(), + Benny.complete(), + Benny.save({ file: 'buildTree immediate descendant', details: true }), + ) + await Benny.suite( + 'isDescendantOf - non-descendant', + Benny.add('non-descendant', () => { + const path1 = EP.fromString(`aaa/bbb/ccc`) + const path2 = EP.fromString(`aaa/bbb/ddd`) + + return () => { + EP.isDescendantOf(path2, path1) + } + }), + Benny.cycle(), + Benny.complete(), + Benny.save({ file: 'buildTree non-descendant', details: true }), + ) + await Benny.suite( + 'isChildOf - child', + Benny.add('child', () => { + const path1 = EP.fromString(`aaa/bbb/ccc`) + const path2 = EP.fromString(`aaa/bbb/ccc:ddd`) + + return () => { + EP.isChildOf(path2, path1) + } + }), + Benny.cycle(), + Benny.complete(), + Benny.save({ file: 'isChildOf child', details: true }), + ) + await Benny.suite( + 'isChildOf - not child', + Benny.add('not child', () => { + const path1 = EP.fromString(`aaa/bbb/ccc`) + const path2 = EP.fromString(`aaa/bbb/ddd`) + + return () => { + EP.isChildOf(path2, path1) + } + }), + Benny.cycle(), + Benny.complete(), + Benny.save({ file: 'isChildOf not child', details: true }), + ) await Benny.suite( 'buildTree - deeply nested elements', Benny.add('deeply nested elements', () => { diff --git a/editor/src/core/shared/element-path-tree.ts b/editor/src/core/shared/element-path-tree.ts index 1c223657b4f9..6d1bdf3a26a9 100644 --- a/editor/src/core/shared/element-path-tree.ts +++ b/editor/src/core/shared/element-path-tree.ts @@ -1,18 +1,9 @@ import { isFeatureEnabled } from '../../utils/feature-switches' import { MetadataUtils } from '../model/element-metadata-utils' -import { isLeft, isRight } from './either' +import { forEachRight, isLeft } from './either' import * as EP from './element-path' -import type { ElementInstanceMetadataMap } from './element-template' -import { - isJSElementAccess, - isJSExpressionMapOrOtherJavaScript, - isJSIdentifier, - isJSPropertyAccess, - isJSXConditionalExpression, - isJSXElement, - isJSXFragment, - isJSXTextBlock, -} from './element-template' +import type { ElementInstanceMetadataMap, JSXElementChild } from './element-template' +import { isJSXConditionalExpression, isJSXElement, isJSXFragment } from './element-template' import type { ElementPath } from './project-file-types' import { fastForEach } from './utils' @@ -51,90 +42,88 @@ export function buildTree(...metadatas: Array): Elem const missingParents = getMissingParentPaths(elementPaths, metadata) const paths = getReorderedPaths(elementPaths, metadata, missingParents) - buildTreeRecursive_MUTATE(root, tree, paths, metadata) + buildTree_MUTATE(root, tree, paths, metadata) } return tree } -function buildTreeRecursive_MUTATE( +function buildTree_MUTATE( rootPath: ElementPath, trees: ElementPathTrees, originalPaths: ElementPath[], metadata: ElementInstanceMetadataMap, -): ElementPathTree[] { +): void { const rootPathString = EP.toString(rootPath) trees[rootPathString] = elementPathTree(rootPath, rootPathString, []) - const childrenPaths = getChildrenPaths(metadata, rootPath, originalPaths) - - let children: ElementPathTree[] = [] - for (const path of childrenPaths) { + function addPathToTree(path: ElementPath): void { const pathString = EP.toString(path) - const subTree = elementPathTree( - path, - pathString, - buildTreeRecursive_MUTATE(path, trees, originalPaths, metadata), - ) - trees[rootPathString].children.push(subTree) - children.push(subTree) + const alreadyAdded = pathString in trees + if (!alreadyAdded) { + const parentPath = EP.parentPath(path) + const subTree = elementPathTree(path, pathString, []) + trees[pathString] = subTree + trees[EP.toString(parentPath)].children.push(subTree) + } } - return children -} - -function getChildrenPaths( - metadata: ElementInstanceMetadataMap, - rootPath: ElementPath, - paths: ElementPath[], -): ElementPath[] { - const element = metadata[EP.toString(rootPath)] + let workingPaths = [...originalPaths] + workingPaths.sort((a, b) => { + return EP.fullDepth(a) - EP.fullDepth(b) + }) + for (const workingPath of workingPaths) { + addPathToTree(workingPath) + } - // Grab the child elements from the element metadata, if available. - let childrenFromElement: ElementPath[] = [] - if ( - element != null && - isRight(element.element) && - (isJSXElement(element.element.value) || isJSXFragment(element.element.value)) && - element.element.value.children.length > 0 - ) { - childrenFromElement = element.element.value.children - .filter((child) => { - if (isFeatureEnabled('Condensed Navigator Entries')) { - // if Data Entries are enabled, we should always show them in the Navigator - return true - } else { - return ( - !isJSXTextBlock(child) && - !isJSExpressionMapOrOtherJavaScript(child) && - !isJSIdentifier(child) && - !isJSPropertyAccess(child) && - !isJSElementAccess(child) - ) - } - }) - .map((child) => EP.appendToPath(rootPath, child.uid)) + for (const elementMetadata of Object.values(metadata)) { + forEachRight(elementMetadata.element, (element) => { + switch (element.type) { + case 'JSX_ELEMENT': + case 'JSX_FRAGMENT': + { + let elementChildren: Array + if (isFeatureEnabled('Condensed Navigator Entries')) { + elementChildren = element.children + } else { + elementChildren = element.children.filter((child) => { + switch (child.type) { + case 'JSX_TEXT_BLOCK': + case 'ATTRIBUTE_OTHER_JAVASCRIPT': + case 'JSX_MAP_EXPRESSION': + case 'JS_IDENTIFIER': + case 'JS_PROPERTY_ACCESS': + case 'JS_ELEMENT_ACCESS': + return false + default: + return true + } + }) + } + for (const child of elementChildren) { + const childPath = EP.appendToPath(elementMetadata.elementPath, child.uid) + addPathToTree(childPath) + } + } + break + default: + break + } + }) } - // Then, grab any other child from the paths array, which is not included in the - // elements from the metadata. - const otherChildrenFromPaths = paths.filter( - (path) => - EP.isChildOf(path, rootPath) && - !childrenFromElement.some((other) => EP.pathsEqual(other, path)), - ) + function walkTreeSortingChildren(tree: ElementPathTree): void { + tree.children.sort( + (a, b) => + originalPaths.findIndex((p) => EP.pathsEqual(p, a.path)) - + originalPaths.findIndex((p) => EP.pathsEqual(p, b.path)), + ) + for (const child of tree.children) { + walkTreeSortingChildren(child) + } + } - // If there are children outside the element metadata, need to merge the two and sort them. - // Otherwise, return the children from the meta. - return otherChildrenFromPaths.length > 0 - ? childrenFromElement - .concat(otherChildrenFromPaths) - .sort( - (a, b) => - paths.findIndex((p) => EP.pathsEqual(p, a)) - - paths.findIndex((p) => EP.pathsEqual(p, b)), - ) - : childrenFromElement + walkTreeSortingChildren(trees[rootPathString]) } function getMissingParentPaths( @@ -160,18 +149,22 @@ function getReorderedPaths( missingParents: ElementPath[], ): ElementPath[] { const elementsToReorder = original.filter((path) => { - const element = MetadataUtils.findElementByElementPath(metadata, path) - if (element == null) { + const elementMetadata = MetadataUtils.findElementByElementPath(metadata, path) + if (elementMetadata == null || isLeft(elementMetadata.element)) { return false } - return ( - MetadataUtils.isConditionalFromMetadata(element) || - MetadataUtils.isExpressionOtherJavascriptFromMetadata(element) || - MetadataUtils.isJSXMapExpressionFromMetadata(element) || - MetadataUtils.isIdentifierFromMetadata(element) || - MetadataUtils.isPropertyAccessFromMetadata(element) || - MetadataUtils.isElementAccessFromMetadata(element) - ) + const element = elementMetadata.element.value + switch (element.type) { + case 'JSX_CONDITIONAL_EXPRESSION': + case 'ATTRIBUTE_OTHER_JAVASCRIPT': + case 'JSX_MAP_EXPRESSION': + case 'JS_IDENTIFIER': + case 'JS_ELEMENT_ACCESS': + case 'JS_PROPERTY_ACCESS': + return true + default: + return false + } }) const pathsToBeReordered = original.filter( (path) => diff --git a/editor/src/core/shared/element-path.ts b/editor/src/core/shared/element-path.ts index 8bfb72740ed1..17dfeac9da7f 100644 --- a/editor/src/core/shared/element-path.ts +++ b/editor/src/core/shared/element-path.ts @@ -642,14 +642,37 @@ export function findAmongAncestorsOfPath( } export function isDescendantOf(target: ElementPath, maybeAncestor: ElementPath): boolean { - const targetString = toString(target) - const maybeAncestorString = toString(maybeAncestor) - return ( - targetString !== maybeAncestorString && - targetString.startsWith(maybeAncestorString) && - (targetString[maybeAncestorString.length] === SceneSeparator || - targetString[maybeAncestorString.length] === ElementSeparator) - ) + if (target.parts.length >= maybeAncestor.parts.length) { + for (let index = 0; index < maybeAncestor.parts.length - 1; index++) { + const maybeAncestorPart = maybeAncestor.parts[index] + const targetPart = target.parts[index] + if (!elementPathPartsEqual(maybeAncestorPart, targetPart)) { + return false + } + } + const lastTargetPart = target.parts[maybeAncestor.parts.length - 1] + const lastAncestorPart = maybeAncestor.parts[maybeAncestor.parts.length - 1] + if (lastTargetPart.length >= lastAncestorPart.length) { + for (let partIndex = 0; partIndex < lastAncestorPart.length; partIndex++) { + const part = lastAncestorPart[partIndex] + if (lastTargetPart[partIndex] !== part) { + return false + } + } + if ( + target.parts.length === maybeAncestor.parts.length && + lastTargetPart.length === lastAncestorPart.length + ) { + // This caters for the case where the paths are the same. + return false + } + return true + } else { + return false + } + } else { + return false + } } export function getAncestorsForLastPart(path: ElementPath): ElementPath[] { From 646b9b552c590d3a9018891263fd034e86543351 Mon Sep 17 00:00:00 2001 From: Sean Parsons Date: Tue, 10 Sep 2024 11:01:34 +0100 Subject: [PATCH 2/6] fix(tests) Partially fixed tests. --- editor/src/core/shared/element-path-tree.ts | 117 ++++++++++---------- 1 file changed, 61 insertions(+), 56 deletions(-) diff --git a/editor/src/core/shared/element-path-tree.ts b/editor/src/core/shared/element-path-tree.ts index 6d1bdf3a26a9..56151358a2ae 100644 --- a/editor/src/core/shared/element-path-tree.ts +++ b/editor/src/core/shared/element-path-tree.ts @@ -30,13 +30,16 @@ export type ElementPathTrees = { [key: string]: ElementPathTree } export function buildTree(...metadatas: Array): ElementPathTrees { let tree: ElementPathTrees = {} for (const metadata of metadatas) { + // The tree should maintain the ordering of the statically defined elements, + // But the metadata itself may not match that ordering. const elementPaths = Object.values(metadata).map((m) => m.elementPath) - if (elementPaths.length === 0) { - return {} + const firstElementPath = elementPaths.at(0) + if (firstElementPath == null) { + continue } - const root = EP.getStoryboardPathFromPath(elementPaths[0]) + const root = EP.getStoryboardPathFromPath(firstElementPath) if (root == null) { - return {} + continue } const missingParents = getMissingParentPaths(elementPaths, metadata) @@ -48,14 +51,20 @@ export function buildTree(...metadatas: Array): Elem return tree } +function addRootToTree(rootPath: ElementPath, trees: ElementPathTrees): void { + const rootPathString = EP.toString(rootPath) + if (!(rootPathString in trees)) { + trees[rootPathString] = elementPathTree(rootPath, rootPathString, []) + } +} + function buildTree_MUTATE( rootPath: ElementPath, trees: ElementPathTrees, originalPaths: ElementPath[], metadata: ElementInstanceMetadataMap, ): void { - const rootPathString = EP.toString(rootPath) - trees[rootPathString] = elementPathTree(rootPath, rootPathString, []) + addRootToTree(rootPath, trees) function addPathToTree(path: ElementPath): void { const pathString = EP.toString(path) @@ -68,6 +77,7 @@ function buildTree_MUTATE( } } + //console.log('originalPaths', JSON.stringify(originalPaths.map(EP.toString), null, 2)) let workingPaths = [...originalPaths] workingPaths.sort((a, b) => { return EP.fullDepth(a) - EP.fullDepth(b) @@ -75,6 +85,7 @@ function buildTree_MUTATE( for (const workingPath of workingPaths) { addPathToTree(workingPath) } + //console.log('after workingPaths', printTree(trees)) for (const elementMetadata of Object.values(metadata)) { forEachRight(elementMetadata.element, (element) => { @@ -111,19 +122,6 @@ function buildTree_MUTATE( } }) } - - function walkTreeSortingChildren(tree: ElementPathTree): void { - tree.children.sort( - (a, b) => - originalPaths.findIndex((p) => EP.pathsEqual(p, a.path)) - - originalPaths.findIndex((p) => EP.pathsEqual(p, b.path)), - ) - for (const child of tree.children) { - walkTreeSortingChildren(child) - } - } - - walkTreeSortingChildren(trees[rootPathString]) } function getMissingParentPaths( @@ -134,7 +132,7 @@ function getMissingParentPaths( for (const path of paths) { let parent = EP.parentPath(path) while (parent.parts.length > 0) { - if (metadata[EP.toString(parent)] == null) { + if (!(EP.toString(parent) in metadata)) { missingParentPaths.add(parent) } parent = EP.parentPath(parent) @@ -148,36 +146,19 @@ function getReorderedPaths( metadata: ElementInstanceMetadataMap, missingParents: ElementPath[], ): ElementPath[] { - const elementsToReorder = original.filter((path) => { - const elementMetadata = MetadataUtils.findElementByElementPath(metadata, path) - if (elementMetadata == null || isLeft(elementMetadata.element)) { - return false - } - const element = elementMetadata.element.value - switch (element.type) { - case 'JSX_CONDITIONAL_EXPRESSION': - case 'ATTRIBUTE_OTHER_JAVASCRIPT': - case 'JSX_MAP_EXPRESSION': - case 'JS_IDENTIFIER': - case 'JS_ELEMENT_ACCESS': - case 'JS_PROPERTY_ACCESS': - return true - default: - return false - } + const pathsToBeReordered = original.filter((path) => { + return !missingParents.some((parentPath) => EP.isDescendantOf(path, parentPath)) // omit elements that have a missing parent }) - const pathsToBeReordered = original.filter( - (path) => - !elementsToReorder.some((other) => EP.pathsEqual(other, path)) && // omit conditionals, that will be reordered - !missingParents.some((parentPath) => EP.isDescendantOf(path, parentPath)), // omit elements that have a missing parent - ) - return elementsToReorder.reduce((paths, path) => { + + return original.reduce((paths, path) => { const index = getReorderedIndexInPaths(paths, metadata, path) if (index === 'do-not-reorder') { + //console.log('do-not-reorder', JSON.stringify(paths.map(EP.toString), null, 2)) return paths } - const before = paths.slice(0, index) - const after = paths.slice(index) + const pathsWithout = paths.filter((pathEntry) => !EP.pathsEqual(pathEntry, path)) + const before = pathsWithout.slice(0, index) + const after = pathsWithout.slice(index) return [...before, path, ...after] }, pathsToBeReordered) } @@ -185,34 +166,58 @@ function getReorderedPaths( function getReorderedIndexInPaths( paths: ElementPath[], metadata: ElementInstanceMetadataMap, - conditionalPath: ElementPath, + pathToReorder: ElementPath, ): number | 'do-not-reorder' { - const index = paths.findIndex((path) => EP.isDescendantOf(path, conditionalPath)) + const index = paths.findIndex((path) => EP.isDescendantOf(path, pathToReorder)) if (index >= 0) { return index } - const parent = MetadataUtils.getParent(metadata, conditionalPath) + const parent = MetadataUtils.getParent(metadata, pathToReorder) if (parent == null || isLeft(parent.element)) { return 'do-not-reorder' } + const parentElement = parent.element.value - if (isJSXElement(parent.element.value) || isJSXFragment(parent.element.value)) { - const innerIndex = parent.element.value.children.findIndex((child) => { + if (isJSXElement(parentElement) || isJSXFragment(parentElement)) { + const innerIndex = parentElement.children.findIndex((child) => { const childPath = EP.appendToPath(parent.elementPath, child.uid) - return EP.pathsEqual(childPath, conditionalPath) + return EP.pathsEqual(childPath, pathToReorder) }) + const priorSiblings = parentElement.children.slice(0, innerIndex) const parentIndex = paths.findIndex((path) => EP.pathsEqual(parent.elementPath, path)) - if (parentIndex < 0) { - return 'do-not-reorder' + + if (priorSiblings.length === 0) { + if (parentIndex < 0) { + return 'do-not-reorder' + } else { + return parentIndex + 1 + innerIndex + } + } else { + const priorSiblingPaths = priorSiblings.map((sibling) => + EP.appendToPath(parent.elementPath, sibling.uid), + ) + const priorSiblingDescendants = paths.reduce((workingCount, path) => { + if ( + priorSiblingPaths.some((priorSiblingPath) => { + return EP.isDescendantOfOrEqualTo(path, priorSiblingPath) + }) + ) { + return workingCount + 1 + } else { + return workingCount + } + }, 0) + + return parentIndex + 1 + priorSiblingDescendants } - return parentIndex + innerIndex - } else if (isJSXConditionalExpression(parent.element.value)) { + } else if (isJSXConditionalExpression(parentElement)) { const parentIndex = paths.findIndex((path) => EP.pathsEqual(parent.elementPath, path)) if (parentIndex < 0) { return 'do-not-reorder' + } else { + return parentIndex } - return parentIndex } else { return 'do-not-reorder' } From 85f6741ed36124e0a83e9d5069375fa132f92206 Mon Sep 17 00:00:00 2001 From: Sean Parsons Date: Tue, 10 Sep 2024 15:48:12 +0100 Subject: [PATCH 3/6] performance(editor) Further buildTree improvements. - Broke apart `ElementPathTree.children` into two separate fields to make for easier ordering. - Added `getElementPathTreeChildren` and `forEachElementPathTreeChild` utility functions. - `buildTree_MUTATE` now adds to the two different children fields based on the element path representing inner children over property children. - `getReorderedIndexInPaths` further reworked to handle shifting by the siblings. - Fixed some tests that had expressions incorrectly ordered. --- editor/src/components/canvas/canvas.ts | 4 +- .../controls/select-mode/scene-label.tsx | 4 +- .../canvas/dom-sampler.spec.browser2.tsx | 6 +- editor/src/components/canvas/gap-utils.ts | 8 ++- .../store/store-deep-equality-instances.ts | 6 +- .../components/navigator/navigator-utils.ts | 18 +++-- .../navigator/navigator.spec.browser2.tsx | 6 +- .../model/element-metadata-utils.spec.tsx | 14 ++++ .../src/core/model/element-metadata-utils.ts | 19 +++-- .../element-path-tree.spec.browser2.tsx | 6 +- editor/src/core/shared/element-path-tree.ts | 70 +++++++++++++------ 11 files changed, 110 insertions(+), 51 deletions(-) diff --git a/editor/src/components/canvas/canvas.ts b/editor/src/components/canvas/canvas.ts index ca9b4009d4db..d59c126e1400 100644 --- a/editor/src/components/canvas/canvas.ts +++ b/editor/src/components/canvas/canvas.ts @@ -18,7 +18,7 @@ import type { } from '../editor/store/editor-state' import * as EP from '../../core/shared/element-path' import type { ElementPathTree, ElementPathTrees } from '../../core/shared/element-path-tree' -import { getSubTree } from '../../core/shared/element-path-tree' +import { forEachElementPathTreeChild, getSubTree } from '../../core/shared/element-path-tree' import { assertNever, fastForEach } from '../../core/shared/utils' import { memoize } from '../../core/shared/memoize' import { maybeToArray } from '../../core/shared/optional-utils' @@ -73,7 +73,7 @@ function getFramesInCanvasContextUncached( let children: Array = [] let unfurledComponents: Array = [] - fastForEach(Object.values(componentTree.children), (childTree) => { + forEachElementPathTreeChild(componentTree, (childTree) => { if (EP.isRootElementOfInstance(childTree.path)) { unfurledComponents.push(childTree) } else { diff --git a/editor/src/components/canvas/controls/select-mode/scene-label.tsx b/editor/src/components/canvas/controls/select-mode/scene-label.tsx index fc5db3cd09f0..36e9227f4157 100644 --- a/editor/src/components/canvas/controls/select-mode/scene-label.tsx +++ b/editor/src/components/canvas/controls/select-mode/scene-label.tsx @@ -18,7 +18,7 @@ import { boundingArea, createInteractionViaMouse } from '../../canvas-strategies import { windowToCanvasCoordinates } from '../../dom-lookup' import { CanvasOffsetWrapper } from '../canvas-offset-wrapper' import { isCommentMode, isSelectModeWithArea } from '../../../editor/editor-modes' -import { getSubTree } from '../../../../core/shared/element-path-tree' +import { getElementPathTreeChildren, getSubTree } from '../../../../core/shared/element-path-tree' interface SceneLabelControlProps { maybeHighlightOnHover: (target: ElementPath) => void @@ -65,7 +65,7 @@ const SceneLabel = React.memo((props) => { if (subTree == null) { return false } else { - return subTree.children.length === 1 + return getElementPathTreeChildren(subTree).length === 1 } }, 'SceneLabel sceneHasSingleChild', diff --git a/editor/src/components/canvas/dom-sampler.spec.browser2.tsx b/editor/src/components/canvas/dom-sampler.spec.browser2.tsx index f1ca07c54a40..3b93654cda41 100644 --- a/editor/src/components/canvas/dom-sampler.spec.browser2.tsx +++ b/editor/src/components/canvas/dom-sampler.spec.browser2.tsx @@ -296,16 +296,14 @@ export var storyboard = ( navigatorEntryToKey, ), ).toEqual([ - 'regular-sb/1e7', 'regular-sb/sc', 'regular-sb/sc/app', 'regular-sb/sc/app:app-root', 'regular-sb/sc/app:app-root/card', 'regular-sb/sc/app:app-root/card:card-root', - 'regular-sb/sc/app:app-root/card:card-root/30d', 'regular-sb/sc/app:app-root/card:card-root/card-span', + 'regular-sb/sc/app:app-root/card:card-root/30d', 'regular-sb/sc/app:app-root/card/card-child', - 'regular-sb/sc/app:app-root/children-code-block', 'regular-sb/sc/app:app-root/frag', 'regular-sb/sc/app:app-root/frag/frag-child', 'regular-sb/sc/app:app-root/frag/cond-1', @@ -319,7 +317,9 @@ export var storyboard = ( 'synthetic-sb/sc/app:app-root/frag/cond-1/cond-1-true/cond-2/d84-attribute', 'conditional-clause-sb/sc/app:app-root/frag/cond-1-false-case', 'synthetic-sb/sc/app:app-root/frag/cond-1/019-attribute', + 'regular-sb/sc/app:app-root/children-code-block', 'regular-sb/sc/app/app-child', + 'regular-sb/1e7', ]) }) diff --git a/editor/src/components/canvas/gap-utils.ts b/editor/src/components/canvas/gap-utils.ts index 99a70eeae5c4..c30454a2afeb 100644 --- a/editor/src/components/canvas/gap-utils.ts +++ b/editor/src/components/canvas/gap-utils.ts @@ -22,7 +22,11 @@ import type { CSSNumber, FlexDirection } from '../inspector/common/css-utils' import type { Sides } from 'utopia-api/core' import { sides } from 'utopia-api/core' import { styleStringInArray } from '../../utils/common-constants' -import { getSubTree, type ElementPathTrees } from '../../core/shared/element-path-tree' +import { + getElementPathTreeChildren, + getSubTree, + type ElementPathTrees, +} from '../../core/shared/element-path-tree' import { isReversedFlexDirection } from '../../core/model/flex-utils' import * as EP from '../../core/shared/element-path' import { treatElementAsFragmentLike } from './canvas-strategies/strategies/fragment-like-helpers' @@ -380,7 +384,7 @@ export function recurseIntoChildrenOfMapOrFragment( return [] } - return subTree.children.flatMap((element) => { + return getElementPathTreeChildren(subTree).flatMap((element) => { const elementPath = element.path if (EP.isRootElementOfInstance(elementPath)) { return [] diff --git a/editor/src/components/editor/store/store-deep-equality-instances.ts b/editor/src/components/editor/store/store-deep-equality-instances.ts index 6f3b13d23626..e19d4f36fab4 100644 --- a/editor/src/components/editor/store/store-deep-equality-instances.ts +++ b/editor/src/components/editor/store/store-deep-equality-instances.ts @@ -2996,12 +2996,14 @@ export function ElementPathTreeKeepDeepEquality( oldValue: ElementPathTree, newValue: ElementPathTree, ): KeepDeepEqualityResult { - return combine3EqualityCalls( + return combine4EqualityCalls( (pathTree) => pathTree.path, ElementPathKeepDeepEquality, (pathTree) => pathTree.pathString, createCallWithTripleEquals(), - (pathTree) => pathTree.children, + (pathTree) => pathTree.innerChildren, + arrayDeepEquality(ElementPathTreeKeepDeepEquality), + (pathTree) => pathTree.propsChildren, arrayDeepEquality(ElementPathTreeKeepDeepEquality), elementPathTree, )(oldValue, newValue) diff --git a/editor/src/components/navigator/navigator-utils.ts b/editor/src/components/navigator/navigator-utils.ts index c3a8c358566e..d84324561244 100644 --- a/editor/src/components/navigator/navigator-utils.ts +++ b/editor/src/components/navigator/navigator-utils.ts @@ -37,7 +37,11 @@ import { syntheticNavigatorEntry, } from '../editor/store/editor-state' import type { ElementPathTree, ElementPathTrees } from '../../core/shared/element-path-tree' -import { getCanvasRoots, getSubTree } from '../../core/shared/element-path-tree' +import { + getCanvasRoots, + getElementPathTreeChildren, + getSubTree, +} from '../../core/shared/element-path-tree' import { assertNever } from '../../core/shared/utils' import type { ConditionalCase } from '../../core/model/conditionals' import { getConditionalClausePath } from '../../core/model/conditionals' @@ -320,7 +324,9 @@ function walkRegularNavigatorEntry( const childPath = EP.appendToPath(elementPath, getUtopiaID(propValue)) - const subTreeChild = subTree?.children.find((child) => EP.pathsEqual(child.path, childPath)) + const subTreeChild = getElementPathTreeChildren(subTree).find((child) => + EP.pathsEqual(child.path, childPath), + ) if (subTreeChild != null) { const childTreeEntry = createNavigatorSubtree( metadata, @@ -359,7 +365,7 @@ function walkRegularNavigatorEntry( }) } - const childrenPaths = subTree.children.filter( + const childrenPaths = getElementPathTreeChildren(subTree).filter( (child) => !processedAccumulator.has(EP.toString(child.path)), ) const children: Array = mapDropNulls((child) => { @@ -457,7 +463,7 @@ function walkConditionalClause( const branch = conditionalCase === 'true-case' ? conditional.whenTrue : conditional.whenFalse // Walk the clause of the conditional. - const clausePathTrees = Object.values(conditionalSubTree.children).filter((childPath) => { + const clausePathTrees = getElementPathTreeChildren(conditionalSubTree).filter((childPath) => { if (isDynamic(childPath.path) && hasElementsWithin(branch)) { for (const element of Object.values(branch.elementsWithin)) { const firstChildPath = EP.appendToPath(EP.parentPath(clausePath), element.uid) @@ -513,7 +519,7 @@ function walkMapExpression( const commentFlag = findUtopiaCommentFlag(element.comments, 'map-count') const mapCountOverride = isUtopiaPropOrCommentFlagMapCount(commentFlag) ? commentFlag.value : null - const mappedChildren = Object.values(subTree.children).map((child) => + const mappedChildren = getElementPathTreeChildren(subTree).map((child) => createNavigatorSubtree( metadata, elementPathTrees, @@ -531,7 +537,7 @@ function walkMapExpression( } let invalidEntries: Array = [] - for (let i = Object.values(subTree.children).length; i < mapCountOverride; i++) { + for (let i = getElementPathTreeChildren(subTree).length; i < mapCountOverride; i++) { const entry = invalidOverrideNavigatorEntry( EP.appendToPath(subTree.path, `invalid-override-${i + 1}`), 'data source not found', diff --git a/editor/src/components/navigator/navigator.spec.browser2.tsx b/editor/src/components/navigator/navigator.spec.browser2.tsx index 2da3ac334c1d..e8911f87814c 100644 --- a/editor/src/components/navigator/navigator.spec.browser2.tsx +++ b/editor/src/components/navigator/navigator.spec.browser2.tsx @@ -5495,16 +5495,14 @@ describe('Navigator row order', () => { navigatorEntryToKey, ), ).toEqual([ - 'regular-sb/1e7', 'regular-sb/sc', 'regular-sb/sc/app', 'regular-sb/sc/app:app-root', 'regular-sb/sc/app:app-root/card', 'regular-sb/sc/app:app-root/card:card-root', - 'regular-sb/sc/app:app-root/card:card-root/30d', 'regular-sb/sc/app:app-root/card:card-root/card-span', + 'regular-sb/sc/app:app-root/card:card-root/30d', 'regular-sb/sc/app:app-root/card/card-child', - 'regular-sb/sc/app:app-root/children-code-block', 'regular-sb/sc/app:app-root/frag', 'regular-sb/sc/app:app-root/frag/frag-child', 'regular-sb/sc/app:app-root/frag/cond-1', @@ -5518,7 +5516,9 @@ describe('Navigator row order', () => { 'synthetic-sb/sc/app:app-root/frag/cond-1/cond-1-true/cond-2/d84-attribute', 'conditional-clause-sb/sc/app:app-root/frag/cond-1-false-case', 'synthetic-sb/sc/app:app-root/frag/cond-1/019-attribute', + 'regular-sb/sc/app:app-root/children-code-block', 'regular-sb/sc/app/app-child', + 'regular-sb/1e7', ]) }) diff --git a/editor/src/core/model/element-metadata-utils.spec.tsx b/editor/src/core/model/element-metadata-utils.spec.tsx index 49ab92bb84da..695b9a2de175 100644 --- a/editor/src/core/model/element-metadata-utils.spec.tsx +++ b/editor/src/core/model/element-metadata-utils.spec.tsx @@ -1101,6 +1101,7 @@ describe('getElementLabel', () => { spanElementMetadata.elementPath, EP.toString(spanElementMetadata.elementPath), [], + [], ), [EP.toString(divElementMetadata.elementPath)]: elementPathTree( divElementMetadata.elementPath, @@ -1110,8 +1111,10 @@ describe('getElementLabel', () => { spanElementMetadata.elementPath, EP.toString(spanElementMetadata.elementPath), [], + [], ), ], + [], ), } it('the label of a spin containing text is that text', () => { @@ -1138,16 +1141,19 @@ describe('getting the root paths', () => { testComponentSceneElement.elementPath, EP.toString(testComponentSceneElement.elementPath), [], + [], ) const testStoryboardChildTree = elementPathTree( testStoryboardChildElement.elementPath, EP.toString(testStoryboardChildElement.elementPath), [], + [], ) const storyboardTree = elementPathTree( EP.elementPath([[BakedInStoryboardUID]]), EP.toString(EP.elementPath([[BakedInStoryboardUID]])), [testComponentSceneTree, testStoryboardChildTree], + [], ) const pathTrees: ElementPathTrees = { [EP.toString(EP.elementPath([[BakedInStoryboardUID]]))]: storyboardTree, @@ -1170,41 +1176,49 @@ describe('getting the root paths', () => { testComponentSceneChildElement.elementPath, EP.toString(testComponentSceneChildElement.elementPath), [], + [], ) const testComponentChild1Tree = elementPathTree( testComponentMetadataChild1.elementPath, EP.toString(testComponentMetadataChild1.elementPath), [], + [], ) const testComponentChild2Tree = elementPathTree( testComponentMetadataChild2.elementPath, EP.toString(testComponentMetadataChild2.elementPath), [], + [], ) const testComponentChild3Tree = elementPathTree( testComponentMetadataChild3.elementPath, EP.toString(testComponentMetadataChild3.elementPath), [], + [], ) const testComponentRoot1Tree = elementPathTree( testComponentRoot1.elementPath, EP.toString(testComponentRoot1.elementPath), [testComponentChild1Tree, testComponentChild2Tree, testComponentChild3Tree], + [], ) const testComponentSceneTree = elementPathTree( testComponentSceneElement.elementPath, EP.toString(testComponentSceneElement.elementPath), [testComponentSceneChildTree, testComponentRoot1Tree], + [], ) const testStoryboardChildTree = elementPathTree( testStoryboardChildElement.elementPath, EP.toString(testStoryboardChildElement.elementPath), [], + [], ) const storyboardTree = elementPathTree( EP.elementPath([[BakedInStoryboardUID]]), EP.toString(EP.elementPath([[BakedInStoryboardUID]])), [testComponentSceneChildTree, testStoryboardChildTree], + [], ) const pathTrees: ElementPathTrees = { [EP.toString(EP.elementPath([[BakedInStoryboardUID]]))]: storyboardTree, diff --git a/editor/src/core/model/element-metadata-utils.ts b/editor/src/core/model/element-metadata-utils.ts index de4faaa17ad5..6c089afe8b5f 100644 --- a/editor/src/core/model/element-metadata-utils.ts +++ b/editor/src/core/model/element-metadata-utils.ts @@ -138,7 +138,15 @@ import { import type { ProjectContentTreeRoot } from '../../components/assets' import { memoize } from '../shared/memoize' import type { ElementPathTree, ElementPathTrees } from '../shared/element-path-tree' -import { buildTree, getSubTree, getCanvasRoots, elementPathTree } from '../shared/element-path-tree' +import { + buildTree, + getSubTree, + getCanvasRoots, + elementPathTree, + getElementPathTreeChildren, + forEachElementPathTreeChild, + printTree, +} from '../shared/element-path-tree' import type { PropertyControlsInfo } from '../../components/custom-code/code-file' import { findUnderlyingTargetComponentImplementationFromImportInfo } from '../../components/custom-code/code-file' import type { @@ -323,7 +331,6 @@ export const MetadataUtils = { if (target == null) { return [] } - const parentPath = EP.parentPath(target) const siblingPathsOrNull = EP.isRootElementOfInstance(target) ? MetadataUtils.getRootViewPathsOrdered(metadata, pathTree, parentPath) @@ -724,7 +731,7 @@ export const MetadataUtils = { if (subTree == null) { return [] } else { - return subTree.children + return getElementPathTreeChildren(subTree) .map((child) => child.path) .filter((path) => !EP.isRootElementOfInstance(path)) } @@ -845,7 +852,7 @@ export const MetadataUtils = { let result: Array = [] function recurseElement(tree: ElementPathTree): void { result.push(tree.path) - fastForEach(Object.values(tree.children), (childTree) => { + forEachElementPathTreeChild(tree, (childTree) => { recurseElement(childTree) }) } @@ -878,7 +885,7 @@ export const MetadataUtils = { if (tree != null) { result.push(tree.path) - fastForEach(Object.values(tree.children), (childTree) => { + forEachElementPathTreeChild(tree, (childTree) => { recurseElement(childTree) }) } @@ -1375,7 +1382,7 @@ export const MetadataUtils = { let unfurledComponents: Array = [] - fastForEach(Object.values(subTree.children), (child) => { + forEachElementPathTreeChild(subTree, (child) => { if (EP.isRootElementOfInstance(child.path)) { unfurledComponents.push(child) } else { diff --git a/editor/src/core/shared/element-path-tree.spec.browser2.tsx b/editor/src/core/shared/element-path-tree.spec.browser2.tsx index 16348c6b1b1d..321414400399 100644 --- a/editor/src/core/shared/element-path-tree.spec.browser2.tsx +++ b/editor/src/core/shared/element-path-tree.spec.browser2.tsx @@ -126,16 +126,14 @@ describe('Building and ordering the element path tree for a real project', () => expect(printTree(renderResult.getEditorState().editor.elementPathTree)).toEqual( `sb - sb/1e7 sb/sc sb/sc/app sb/sc/app:app-root sb/sc/app:app-root/card sb/sc/app:app-root/card:card-root - sb/sc/app:app-root/card:card-root/30d sb/sc/app:app-root/card:card-root/card-span + sb/sc/app:app-root/card:card-root/30d sb/sc/app:app-root/card/card-child - sb/sc/app:app-root/52e sb/sc/app:app-root/frag sb/sc/app:app-root/frag/frag-child sb/sc/app:app-root/frag/cond-1 @@ -144,7 +142,9 @@ describe('Building and ordering the element path tree for a real project', () => sb/sc/app:app-root/frag/cond-1/cond-1-true/cond-2 sb/sc/app:app-root/frag/cond-1/cond-1-true/cond-2/e26 sb/sc/app:app-root/frag/cond-1/cond-1-true/cond-2/e26/cond-2-child~~~1 + sb/sc/app:app-root/52e sb/sc/app/app-child + sb/1e7 `, ) }) diff --git a/editor/src/core/shared/element-path-tree.ts b/editor/src/core/shared/element-path-tree.ts index 56151358a2ae..e12c3b86b748 100644 --- a/editor/src/core/shared/element-path-tree.ts +++ b/editor/src/core/shared/element-path-tree.ts @@ -5,26 +5,40 @@ import * as EP from './element-path' import type { ElementInstanceMetadataMap, JSXElementChild } from './element-template' import { isJSXConditionalExpression, isJSXElement, isJSXFragment } from './element-template' import type { ElementPath } from './project-file-types' -import { fastForEach } from './utils' export interface ElementPathTree { path: ElementPath pathString: string - children: Array + innerChildren: Array + propsChildren: Array } export function elementPathTree( path: ElementPath, pathString: string, - children: Array, + innerChildren: Array, + propsChildren: Array, ): ElementPathTree { return { path: path, pathString: pathString, - children: children, + innerChildren: innerChildren, + propsChildren: propsChildren, } } +export function getElementPathTreeChildren(tree: ElementPathTree): Array { + return [...tree.innerChildren, ...tree.propsChildren] +} + +export function forEachElementPathTreeChild( + tree: ElementPathTree, + handler: (elementPathTree: ElementPathTree) => void, +): void { + tree.innerChildren.forEach(handler) + tree.propsChildren.forEach(handler) +} + export type ElementPathTrees = { [key: string]: ElementPathTree } export function buildTree(...metadatas: Array): ElementPathTrees { @@ -54,7 +68,7 @@ export function buildTree(...metadatas: Array): Elem function addRootToTree(rootPath: ElementPath, trees: ElementPathTrees): void { const rootPathString = EP.toString(rootPath) if (!(rootPathString in trees)) { - trees[rootPathString] = elementPathTree(rootPath, rootPathString, []) + trees[rootPathString] = elementPathTree(rootPath, rootPathString, [], []) } } @@ -71,9 +85,14 @@ function buildTree_MUTATE( const alreadyAdded = pathString in trees if (!alreadyAdded) { const parentPath = EP.parentPath(path) - const subTree = elementPathTree(path, pathString, []) + const subTree = elementPathTree(path, pathString, [], []) trees[pathString] = subTree - trees[EP.toString(parentPath)].children.push(subTree) + const pathLastPart = EP.lastElementPathForPath(path) + if (pathLastPart?.length === 1) { + trees[EP.toString(parentPath)].innerChildren.push(subTree) + } else { + trees[EP.toString(parentPath)].propsChildren.push(subTree) + } } } @@ -168,11 +187,6 @@ function getReorderedIndexInPaths( metadata: ElementInstanceMetadataMap, pathToReorder: ElementPath, ): number | 'do-not-reorder' { - const index = paths.findIndex((path) => EP.isDescendantOf(path, pathToReorder)) - if (index >= 0) { - return index - } - const parent = MetadataUtils.getParent(metadata, pathToReorder) if (parent == null || isLeft(parent.element)) { return 'do-not-reorder' @@ -180,11 +194,23 @@ function getReorderedIndexInPaths( const parentElement = parent.element.value if (isJSXElement(parentElement) || isJSXFragment(parentElement)) { - const innerIndex = parentElement.children.findIndex((child) => { + let innerIndex: number | null = null + let priorSiblings: Array = [] + for (let childIndex = 0; childIndex < parentElement.children.length; childIndex++) { + const child = parentElement.children[childIndex] const childPath = EP.appendToPath(parent.elementPath, child.uid) - return EP.pathsEqual(childPath, pathToReorder) - }) - const priorSiblings = parentElement.children.slice(0, innerIndex) + if (EP.pathsEqual(childPath, pathToReorder)) { + innerIndex = childIndex + break + } + if (innerIndex == null) { + priorSiblings.push(child) + } + } + if (innerIndex == null) { + return 'do-not-reorder' + } + const parentIndex = paths.findIndex((path) => EP.pathsEqual(parent.elementPath, path)) if (priorSiblings.length === 0) { @@ -194,9 +220,9 @@ function getReorderedIndexInPaths( return parentIndex + 1 + innerIndex } } else { - const priorSiblingPaths = priorSiblings.map((sibling) => - EP.appendToPath(parent.elementPath, sibling.uid), - ) + const priorSiblingPaths = priorSiblings.map((sibling) => { + return EP.appendToPath(parent.elementPath, sibling.uid) + }) const priorSiblingDescendants = paths.reduce((workingCount, path) => { if ( priorSiblingPaths.some((priorSiblingPath) => { @@ -234,7 +260,7 @@ export function getCanvasRoots(trees: ElementPathTrees): Array return [] } - return storyboardTree.children + return getElementPathTreeChildren(storyboardTree) } export function printTree(treeRoot: ElementPathTrees): string { @@ -245,7 +271,7 @@ export function printTree(treeRoot: ElementPathTrees): string { } outputText += EP.toString(element.path) outputText += '\n' - for (const child of element.children) { + for (const child of getElementPathTreeChildren(element)) { printElement(child, depth + 1) } } @@ -263,7 +289,7 @@ export function forEachChildOfTarget( ): void { const foundTree = getSubTree(treeRoot, target) if (foundTree != null) { - fastForEach(Object.values(foundTree.children), (subTree) => { + forEachElementPathTreeChild(foundTree, (subTree) => { handler(subTree.path) }) } From cc8834a9bee5489ebf9019ec94853f9487f440a0 Mon Sep 17 00:00:00 2001 From: Sean Parsons Date: Tue, 10 Sep 2024 16:10:19 +0100 Subject: [PATCH 4/6] chore(pr) Added some explanatory comments and removed some commented out code. --- editor/src/core/shared/element-path-tree.ts | 26 ++++++++++++++------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/editor/src/core/shared/element-path-tree.ts b/editor/src/core/shared/element-path-tree.ts index e12c3b86b748..ad69c1b2a076 100644 --- a/editor/src/core/shared/element-path-tree.ts +++ b/editor/src/core/shared/element-path-tree.ts @@ -88,15 +88,18 @@ function buildTree_MUTATE( const subTree = elementPathTree(path, pathString, [], []) trees[pathString] = subTree const pathLastPart = EP.lastElementPathForPath(path) + if (pathLastPart?.length === 1) { + // If the last part of the path is a single element, it should be added to the inner children. + // This would be a path like `a/b/c:d`. trees[EP.toString(parentPath)].innerChildren.push(subTree) } else { + // Otherwise this should be added to the props children. trees[EP.toString(parentPath)].propsChildren.push(subTree) } } } - //console.log('originalPaths', JSON.stringify(originalPaths.map(EP.toString), null, 2)) let workingPaths = [...originalPaths] workingPaths.sort((a, b) => { return EP.fullDepth(a) - EP.fullDepth(b) @@ -104,7 +107,6 @@ function buildTree_MUTATE( for (const workingPath of workingPaths) { addPathToTree(workingPath) } - //console.log('after workingPaths', printTree(trees)) for (const elementMetadata of Object.values(metadata)) { forEachRight(elementMetadata.element, (element) => { @@ -166,13 +168,13 @@ function getReorderedPaths( missingParents: ElementPath[], ): ElementPath[] { const pathsToBeReordered = original.filter((path) => { - return !missingParents.some((parentPath) => EP.isDescendantOf(path, parentPath)) // omit elements that have a missing parent + // Omit elements that have a missing parent. + return !missingParents.some((parentPath) => EP.isDescendantOf(path, parentPath)) }) return original.reduce((paths, path) => { const index = getReorderedIndexInPaths(paths, metadata, path) if (index === 'do-not-reorder') { - //console.log('do-not-reorder', JSON.stringify(paths.map(EP.toString), null, 2)) return paths } const pathsWithout = paths.filter((pathEntry) => !EP.pathsEqual(pathEntry, path)) @@ -200,10 +202,13 @@ function getReorderedIndexInPaths( const child = parentElement.children[childIndex] const childPath = EP.appendToPath(parent.elementPath, child.uid) if (EP.pathsEqual(childPath, pathToReorder)) { + // We've found the item that we're trying to reorder, so record the index + // and stop searching. innerIndex = childIndex break - } - if (innerIndex == null) { + } else { + // As we haven't reached the element of interest, record + // this prior sibling for later use. priorSiblings.push(child) } } @@ -213,16 +218,19 @@ function getReorderedIndexInPaths( const parentIndex = paths.findIndex((path) => EP.pathsEqual(parent.elementPath, path)) - if (priorSiblings.length === 0) { + if (innerIndex === 0) { if (parentIndex < 0) { return 'do-not-reorder' } else { - return parentIndex + 1 + innerIndex + // As this is the first item, shift the index past the parent. + return parentIndex + 1 } } else { const priorSiblingPaths = priorSiblings.map((sibling) => { return EP.appendToPath(parent.elementPath, sibling.uid) }) + // As there are prior siblings, we need to count those and their descendants, + // so as to shift this element past them. const priorSiblingDescendants = paths.reduce((workingCount, path) => { if ( priorSiblingPaths.some((priorSiblingPath) => { @@ -235,6 +243,8 @@ function getReorderedIndexInPaths( } }, 0) + // Shift by the parent position, add 1 to put it past the parent and then shift it by the + // count of the prior siblings and their descendants. return parentIndex + 1 + priorSiblingDescendants } } else if (isJSXConditionalExpression(parentElement)) { From 8c91af726f6881b8967bd78c71d454f5107378fa Mon Sep 17 00:00:00 2001 From: Sean Parsons Date: Tue, 10 Sep 2024 18:00:12 +0100 Subject: [PATCH 5/6] chore(pr) Tweaks from feedback. --- editor/src/core/shared/element-path-tree.ts | 62 ++++++++++----------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/editor/src/core/shared/element-path-tree.ts b/editor/src/core/shared/element-path-tree.ts index ad69c1b2a076..0046924801dc 100644 --- a/editor/src/core/shared/element-path-tree.ts +++ b/editor/src/core/shared/element-path-tree.ts @@ -3,7 +3,12 @@ import { MetadataUtils } from '../model/element-metadata-utils' import { forEachRight, isLeft } from './either' import * as EP from './element-path' import type { ElementInstanceMetadataMap, JSXElementChild } from './element-template' -import { isJSXConditionalExpression, isJSXElement, isJSXFragment } from './element-template' +import { + isJSXConditionalExpression, + isJSXElement, + isJSXElementLike, + isJSXFragment, +} from './element-template' import type { ElementPath } from './project-file-types' export interface ElementPathTree { @@ -110,36 +115,29 @@ function buildTree_MUTATE( for (const elementMetadata of Object.values(metadata)) { forEachRight(elementMetadata.element, (element) => { - switch (element.type) { - case 'JSX_ELEMENT': - case 'JSX_FRAGMENT': - { - let elementChildren: Array - if (isFeatureEnabled('Condensed Navigator Entries')) { - elementChildren = element.children - } else { - elementChildren = element.children.filter((child) => { - switch (child.type) { - case 'JSX_TEXT_BLOCK': - case 'ATTRIBUTE_OTHER_JAVASCRIPT': - case 'JSX_MAP_EXPRESSION': - case 'JS_IDENTIFIER': - case 'JS_PROPERTY_ACCESS': - case 'JS_ELEMENT_ACCESS': - return false - default: - return true - } - }) - } - for (const child of elementChildren) { - const childPath = EP.appendToPath(elementMetadata.elementPath, child.uid) - addPathToTree(childPath) + if (isJSXElementLike(element)) { + let elementChildren: Array + if (isFeatureEnabled('Condensed Navigator Entries')) { + elementChildren = element.children + } else { + elementChildren = element.children.filter((child) => { + switch (child.type) { + case 'JSX_TEXT_BLOCK': + case 'ATTRIBUTE_OTHER_JAVASCRIPT': + case 'JSX_MAP_EXPRESSION': + case 'JS_IDENTIFIER': + case 'JS_PROPERTY_ACCESS': + case 'JS_ELEMENT_ACCESS': + return false + default: + return true } - } - break - default: - break + }) + } + for (const child of elementChildren) { + const childPath = EP.appendToPath(elementMetadata.elementPath, child.uid) + addPathToTree(childPath) + } } }) } @@ -153,7 +151,7 @@ function getMissingParentPaths( for (const path of paths) { let parent = EP.parentPath(path) while (parent.parts.length > 0) { - if (!(EP.toString(parent) in metadata)) { + if (metadata[EP.toString(parent)] == null) { missingParentPaths.add(parent) } parent = EP.parentPath(parent) @@ -168,7 +166,7 @@ function getReorderedPaths( missingParents: ElementPath[], ): ElementPath[] { const pathsToBeReordered = original.filter((path) => { - // Omit elements that have a missing parent. + // Omit elements that have a missing ancestor. return !missingParents.some((parentPath) => EP.isDescendantOf(path, parentPath)) }) From e0d58e3454b17e8ac270e32e64fc9e3cca0b0a55 Mon Sep 17 00:00:00 2001 From: Sean Parsons Date: Wed, 11 Sep 2024 12:29:54 +0100 Subject: [PATCH 6/6] chore(pr) Tweaks from feedback. --- editor/src/core/shared/element-path-tree.ts | 11 +++- editor/src/core/shared/element-path.ts | 63 ++++++++++++--------- 2 files changed, 47 insertions(+), 27 deletions(-) diff --git a/editor/src/core/shared/element-path-tree.ts b/editor/src/core/shared/element-path-tree.ts index 0046924801dc..55d36be8332f 100644 --- a/editor/src/core/shared/element-path-tree.ts +++ b/editor/src/core/shared/element-path-tree.ts @@ -10,6 +10,7 @@ import { isJSXFragment, } from './element-template' import type { ElementPath } from './project-file-types' +import { assertNever } from './utils' export interface ElementPathTree { path: ElementPath @@ -129,8 +130,16 @@ function buildTree_MUTATE( case 'JS_PROPERTY_ACCESS': case 'JS_ELEMENT_ACCESS': return false - default: + case 'JSX_ELEMENT': + case 'JSX_CONDITIONAL_EXPRESSION': + case 'JSX_FRAGMENT': + case 'ATTRIBUTE_FUNCTION_CALL': + case 'ATTRIBUTE_NESTED_ARRAY': + case 'ATTRIBUTE_NESTED_OBJECT': + case 'ATTRIBUTE_VALUE': return true + default: + assertNever(child) } }) } diff --git a/editor/src/core/shared/element-path.ts b/editor/src/core/shared/element-path.ts index 17dfeac9da7f..a63ee8c85a88 100644 --- a/editor/src/core/shared/element-path.ts +++ b/editor/src/core/shared/element-path.ts @@ -642,37 +642,48 @@ export function findAmongAncestorsOfPath( } export function isDescendantOf(target: ElementPath, maybeAncestor: ElementPath): boolean { - if (target.parts.length >= maybeAncestor.parts.length) { - for (let index = 0; index < maybeAncestor.parts.length - 1; index++) { - const maybeAncestorPart = maybeAncestor.parts[index] - const targetPart = target.parts[index] - if (!elementPathPartsEqual(maybeAncestorPart, targetPart)) { - return false - } + // If the target has less parts than the potential ancestor, it's by default + // higher up the hierarchy. + if (target.parts.length < maybeAncestor.parts.length) { + return false + } + // Check if any of the shared (by index) parts are different, if any are that + // means these two are on different branches of the hierarchy. + for (let index = 0; index < maybeAncestor.parts.length - 1; index++) { + const maybeAncestorPart = maybeAncestor.parts[index] + const targetPart = target.parts[index] + if (!elementPathPartsEqual(maybeAncestorPart, targetPart)) { + return false } - const lastTargetPart = target.parts[maybeAncestor.parts.length - 1] - const lastAncestorPart = maybeAncestor.parts[maybeAncestor.parts.length - 1] - if (lastTargetPart.length >= lastAncestorPart.length) { - for (let partIndex = 0; partIndex < lastAncestorPart.length; partIndex++) { - const part = lastAncestorPart[partIndex] - if (lastTargetPart[partIndex] !== part) { - return false - } - } - if ( - target.parts.length === maybeAncestor.parts.length && - lastTargetPart.length === lastAncestorPart.length - ) { - // This caters for the case where the paths are the same. - return false - } - return true - } else { + } + // As the rest of the parts are the same, we can check the last part to see + // if the target has less parts as that puts it higher up the hierarchy than the + // potential ancestor. + const lastTargetPart = target.parts[maybeAncestor.parts.length - 1] + const lastAncestorPart = maybeAncestor.parts[maybeAncestor.parts.length - 1] + if (lastTargetPart.length < lastAncestorPart.length) { + return false + } + // Check the elements of the last part to see if there are any differences + // as that means they're on a different branch. Limiting it to the length of + // the potential ancestor. + for (let partIndex = 0; partIndex < lastAncestorPart.length; partIndex++) { + const part = lastAncestorPart[partIndex] + if (lastTargetPart[partIndex] !== part) { return false } - } else { + } + // If the lengths of the parts and the lengths of the last parts are the same, + // since so far we've determined those to all match, then these are identical paths. + if ( + target.parts.length === maybeAncestor.parts.length && + lastTargetPart.length === lastAncestorPart.length + ) { return false } + + // Fallback meaning this is a descendant. + return true } export function getAncestorsForLastPart(path: ElementPath): ElementPath[] {