From b33285c634984d9270ab05e35c94f1abe3963899 Mon Sep 17 00:00:00 2001
From: somebody1234 <ehern.lee@gmail.com>
Date: Fri, 24 Nov 2023 20:49:15 +1000
Subject: [PATCH] Fix auto layout (#8345)

So it turns out auto-layout was broken if there were existing, already positioned nodes

# Important Notes
None
---
 app/gui2/src/components/ComponentBrowser.vue  |  2 -
 app/gui2/src/components/GraphEditor.vue       | 70 +++++++-------
 .../graph/__tests__/graphDatabase.test.ts     |  8 +-
 app/gui2/src/stores/graph/graphDatabase.ts    | 56 +----------
 app/gui2/src/stores/graph/index.ts            | 95 +++++++++++--------
 app/gui2/src/util/ast/extended.ts             |  8 ++
 6 files changed, 101 insertions(+), 138 deletions(-)

diff --git a/app/gui2/src/components/ComponentBrowser.vue b/app/gui2/src/components/ComponentBrowser.vue
index f73b965d123..494958a262f 100644
--- a/app/gui2/src/components/ComponentBrowser.vue
+++ b/app/gui2/src/components/ComponentBrowser.vue
@@ -45,9 +45,7 @@ const emit = defineEmits<{
 function getInitialContent(): string {
   if (props.sourceNode == null) return props.initialContent
   const sourceNode = props.sourceNode
-  console.log(sourceNode)
   const sourceNodeName = graphStore.db.getNodeMainOutputPortIdentifier(sourceNode)
-  console.log(sourceNodeName)
   const sourceNodeNameWithDot = sourceNodeName ? sourceNodeName + '.' : ''
   return sourceNodeNameWithDot + props.initialContent
 }
diff --git a/app/gui2/src/components/GraphEditor.vue b/app/gui2/src/components/GraphEditor.vue
index 4ec79c41c6a..f48c039da25 100644
--- a/app/gui2/src/components/GraphEditor.vue
+++ b/app/gui2/src/components/GraphEditor.vue
@@ -288,37 +288,37 @@ async function handleFileDrop(event: DragEvent) {
   }
 }
 
-function onComponentBrowserCommit(content: string) {
-  if (content != null && graphStore.editedNodeInfo != null) {
-    // We finish editing a node.
-    graphStore.setNodeContent(graphStore.editedNodeInfo.id, content)
-  } else if (content != null) {
-    // We finish creating a new node.
-    const nodePosition = componentBrowserPosition.value
-    graphStore.createNode(nodePosition.sub(COMPONENT_BROWSER_TO_NODE_OFFSET), content)
-  }
+function resetComponentBrowserState() {
   componentBrowserVisible.value = false
   graphStore.editedNodeInfo = undefined
   interaction.setCurrent(undefined)
 }
 
-function onComponentBrowserCancel() {
-  componentBrowserVisible.value = false
-  graphStore.editedNodeInfo = undefined
-  interaction.setCurrent(undefined)
+function onComponentBrowserCommit(content: string) {
+  if (content != null) {
+    if (graphStore.editedNodeInfo) {
+      // We finish editing a node.
+      graphStore.setNodeContent(graphStore.editedNodeInfo.id, content)
+    } else {
+      // We finish creating a new node.
+      const nodePosition = componentBrowserPosition.value
+      graphStore.createNode(nodePosition.sub(COMPONENT_BROWSER_TO_NODE_OFFSET), content)
+    }
+  }
+  resetComponentBrowserState()
 }
 
+const onComponentBrowserCancel = resetComponentBrowserState
+
 function getNodeContent(id: ExprId): string {
-  const node = graphStore.db.nodeIdToNode.get(id)
-  if (node == null) return ''
-  return node.rootSpan.repr()
+  return graphStore.db.nodeIdToNode.get(id)?.rootSpan.repr() ?? ''
 }
 
 // Watch the `editedNode` in the graph store
 watch(
   () => graphStore.editedNodeInfo,
   (editedInfo) => {
-    if (editedInfo != null) {
+    if (editedInfo) {
       componentBrowserPosition.value = targetComponentBrowserPosition()
       componentBrowserInputContent.value = getNodeContent(editedInfo.id)
       componentBrowserVisible.value = true
@@ -328,16 +328,16 @@ watch(
   },
 )
 
-const breadcrumbs = computed(() => {
-  return projectStore.executionContext.desiredStack.map((frame) => {
+const breadcrumbs = computed(() =>
+  projectStore.executionContext.desiredStack.map((frame) => {
     switch (frame.type) {
       case 'ExplicitCall':
         return frame.methodPointer.name
       case 'LocalCall':
         return frame.expressionId
     }
-  })
-})
+  }),
+)
 
 // === Clipboard ===
 
@@ -358,7 +358,7 @@ interface CopiedNode {
 function copyNodeContent() {
   const id = nodeSelection.selected.values().next().value
   const node = graphStore.db.nodeIdToNode.get(id)
-  if (node == null) return
+  if (!node) return
   const content = node.rootSpan.repr()
   const metadata = projectStore.module?.getNodeMetadata(id) ?? undefined
   const copiedNode: CopiedNode = { expression: content, metadata }
@@ -392,24 +392,23 @@ async function retrieveDataFromClipboard(): Promise<ClipboardData | undefined> {
 /// Read the clipboard and if it contains valid data, create a node from the content.
 async function readNodeFromClipboard() {
   let clipboardData = await retrieveDataFromClipboard()
-  if (clipboardData == undefined) {
+  if (!clipboardData) {
     console.warn('No valid data in clipboard.')
     return
   }
   const copiedNode = clipboardData.nodes[0]
-  if (copiedNode == undefined) {
+  if (!copiedNode) {
     console.warn('No valid node in clipboard.')
     return
   }
-  if (copiedNode.expression != null) {
-    graphStore.createNode(
-      graphNavigator.sceneMousePos ?? Vec2.Zero,
-      copiedNode.expression,
-      copiedNode.metadata,
-    )
-  } else {
+  if (copiedNode.expression == null) {
     console.warn('No valid expression in clipboard.')
   }
+  graphStore.createNode(
+    graphNavigator.sceneMousePos ?? Vec2.Zero,
+    copiedNode.expression,
+    copiedNode.metadata,
+  )
 }
 
 function handleNodeOutputPortDoubleClick(id: ExprId) {
@@ -428,15 +427,14 @@ function handleNodeOutputPortDoubleClick(id: ExprId) {
 </script>
 
 <template>
-  <!-- eslint-disable vue/attributes-order -->
   <div
     ref="viewportNode"
     class="GraphEditor"
     :class="{ draggingEdge: graphStore.unconnectedEdge != null }"
     :style="groupColors"
-    @click="graphBindingsHandler"
     v-on.="graphNavigator.events"
     v-on..="nodeSelection.events"
+    @click="graphBindingsHandler"
     @dragover.prevent
     @drop.prevent="handleFileDrop($event)"
   >
@@ -451,12 +449,12 @@ function handleNodeOutputPortDoubleClick(id: ExprId) {
       ref="componentBrowser"
       :navigator="graphNavigator"
       :position="componentBrowserPosition"
-      @accepted="onComponentBrowserCommit"
-      @closed="onComponentBrowserCancel"
-      @canceled="onComponentBrowserCancel"
       :initialContent="componentBrowserInputContent"
       :initialCaretPosition="graphStore.editedNodeInfo?.range ?? [0, 0]"
       :sourceNode="componentBrowserSourceNode"
+      @accepted="onComponentBrowserCommit"
+      @closed="onComponentBrowserCancel"
+      @canceled="onComponentBrowserCancel"
     />
     <TopBar
       v-model:mode="projectStore.executionMode"
diff --git a/app/gui2/src/stores/graph/__tests__/graphDatabase.test.ts b/app/gui2/src/stores/graph/__tests__/graphDatabase.test.ts
index 7713741df01..c152e2a38cc 100644
--- a/app/gui2/src/stores/graph/__tests__/graphDatabase.test.ts
+++ b/app/gui2/src/stores/graph/__tests__/graphDatabase.test.ts
@@ -38,13 +38,7 @@ test('Reading graph from definition', () => {
     else return undefined
   })
   assert(func?.isTree(Ast.Tree.Type.Function))
-  db.readFunctionAst(
-    func,
-    (_) => {
-      return { x: 0.0, y: 0.0, vis: null }
-    },
-    (_) => 100.0,
-  )
+  db.readFunctionAst(func, (_) => ({ x: 0.0, y: 0.0, vis: null }))
 
   expect(Array.from(db.nodeIdToNode.keys())).toEqual([id04, id08])
   expect(db.getExpressionNodeId(id04)).toBe(id04)
diff --git a/app/gui2/src/stores/graph/graphDatabase.ts b/app/gui2/src/stores/graph/graphDatabase.ts
index cdd510c1e31..d8dda681cc5 100644
--- a/app/gui2/src/stores/graph/graphDatabase.ts
+++ b/app/gui2/src/stores/graph/graphDatabase.ts
@@ -1,4 +1,3 @@
-import { nonDictatedPlacement } from '@/components/ComponentBrowser/placement'
 import { SuggestionDb, groupColorStyle, type Group } from '@/stores/suggestionDatabase'
 import type { SuggestionEntry } from '@/stores/suggestionDatabase/entry'
 import { byteArraysEqual, tryGetIndex } from '@/util/array'
@@ -8,10 +7,7 @@ import { colorFromString } from '@/util/colors'
 import { ComputedValueRegistry, type ExpressionInfo } from '@/util/computedValueRegistry'
 import { MappedKeyMap, MappedSet } from '@/util/containers'
 import { ReactiveDb, ReactiveIndex, ReactiveMapping } from '@/util/database/reactiveDb'
-import { getTextWidth } from '@/util/measurement'
 import type { Opt } from '@/util/opt'
-import { Rect } from '@/util/rect'
-import theme from '@/util/theme.json'
 import { Vec2 } from '@/util/vec2'
 import * as set from 'lib0/set'
 import type { MethodCall } from 'shared/languageServerTypes'
@@ -243,15 +239,8 @@ export class GraphDb {
   readFunctionAst(
     functionAst: AstExtended<Ast.Tree.Function>,
     getMeta: (id: ExprId) => NodeMetadata | undefined,
-    getWidth: (node: Node) => number = (node: Node) =>
-      // FIXME [ao]: This should take into account the width of all widgets. Probably
-      //   the better solution is to layout nodes once rendered (and all sizes are known).
-      getTextWidth(node.rootSpan.repr(), '11.5px', '"M PLUS 1", sans-serif') * 1.2,
   ) {
     const currentNodeIds = new Set<ExprId>()
-    const nodeRectMap = new Map<ExprId, Rect>()
-    let numberOfUnpositionedNodes = 0
-    let maxUnpositionedNodeWidth = 0
     if (functionAst) {
       for (const nodeAst of functionAst.visit(getFunctionNodeExpressions)) {
         const newNode = nodeFromAst(nodeAst)
@@ -272,20 +261,8 @@ export class GraphDb {
             node.rootSpan = newNode.rootSpan
           }
         }
-        if (!nodeMeta) {
-          numberOfUnpositionedNodes += 1
-          maxUnpositionedNodeWidth = Math.max(maxUnpositionedNodeWidth, getWidth(node ?? newNode))
-        } else {
+        if (nodeMeta) {
           this.assignUpdatedMetadata(node ?? newNode, nodeMeta)
-          nodeRectMap.set(
-            nodeId,
-            Rect.FromBounds(
-              nodeMeta.x,
-              nodeMeta.y,
-              nodeMeta.x + getWidth(node ?? newNode),
-              nodeMeta.y + theme.node.height,
-            ),
-          )
         }
       }
     }
@@ -295,37 +272,8 @@ export class GraphDb {
         this.nodeIdToNode.delete(nodeId)
       }
     }
+
     this.bindings.readFunctionAst(functionAst)
-
-    const nodeRects = [...nodeRectMap.values()]
-    const rectsHeight =
-      numberOfUnpositionedNodes * (theme.node.height + theme.node.vertical_gap) -
-      theme.node.vertical_gap
-    const { position: rectsPosition } = nonDictatedPlacement(
-      new Vec2(maxUnpositionedNodeWidth, rectsHeight),
-      {
-        nodeRects,
-        // The rest of the properties should not matter.
-        selectedNodeRects: [],
-        screenBounds: Rect.Zero,
-        mousePosition: Vec2.Zero,
-      },
-    )
-    let nodeIndex = 0
-    for (const nodeId of this.nodeIdToNode.keys()) {
-      const meta = getMeta(nodeId)
-      if (meta) continue
-      const node = this.nodeIdToNode.get(nodeId)!
-      const size = new Vec2(getWidth(node), theme.node.height)
-      const position = new Vec2(
-        rectsPosition.x,
-        rectsPosition.y + (theme.node.height + theme.node.vertical_gap) * nodeIndex,
-      )
-      nodeRects.push(new Rect(position, size))
-      node.position = new Vec2(position.x, position.y)
-
-      nodeIndex += 1
-    }
   }
 
   assignUpdatedMetadata(node: Node, meta: NodeMetadata) {
diff --git a/app/gui2/src/stores/graph/index.ts b/app/gui2/src/stores/graph/index.ts
index 44501d72df4..83c1ad48c5e 100644
--- a/app/gui2/src/stores/graph/index.ts
+++ b/app/gui2/src/stores/graph/index.ts
@@ -1,3 +1,4 @@
+import { nonDictatedPlacement } from '@/components/ComponentBrowser/placement'
 import { GraphDb } from '@/stores/graph/graphDatabase'
 import { useProjectStore } from '@/stores/project'
 import { useSuggestionDbStore } from '@/stores/suggestionDatabase'
@@ -5,7 +6,7 @@ import { DEFAULT_VISUALIZATION_IDENTIFIER } from '@/stores/visualization'
 import { Ast, AstExtended, childrenAstNodes, findAstWithRange, readAstSpan } from '@/util/ast'
 import { useObserveYjs } from '@/util/crdt'
 import type { Opt } from '@/util/opt'
-import type { Rect } from '@/util/rect'
+import { Rect } from '@/util/rect'
 import { Vec2 } from '@/util/vec2'
 import { defineStore } from 'pinia'
 import type { StackItem } from 'shared/languageServerTypes'
@@ -74,12 +75,12 @@ export const useGraphStore = defineStore('graph', () => {
 
   watch(text, (value) => {
     textContent.value = value?.toString() ?? ''
-    if (value != null) updateState()
+    if (value) updateState()
   })
 
   function updateState() {
     const module = proj.module
-    if (module == null) return
+    if (!module) return
     module.transact(() => {
       const idMap = module.getIdMap()
       const meta = module.doc.metadata
@@ -88,16 +89,16 @@ export const useGraphStore = defineStore('graph', () => {
       const ast = AstExtended.parse(textContentLocal, idMap)
       const updatedMap = idMap.finishAndSynchronize()
 
-      const methodAst = ast.isTree()
-        ? ast.tryMap((tree) =>
-            getExecutedMethodAst(
-              tree,
-              textContentLocal,
-              proj.executionContext.getStackTop(),
-              updatedMap,
-            ),
-          )
-        : undefined
+      const methodAst =
+        ast.isTree() &&
+        ast.tryMap((tree) =>
+          getExecutedMethodAst(
+            tree,
+            textContentLocal,
+            proj.executionContext.getStackTop(),
+            updatedMap,
+          ),
+        )
       if (methodAst) {
         db.readFunctionAst(methodAst, (id) => meta.get(id))
       }
@@ -110,7 +111,7 @@ export const useGraphStore = defineStore('graph', () => {
       if (op.action === 'update' || op.action === 'add') {
         const data = meta.get(id)
         const node = db.nodeIdToNode.get(id as ExprId)
-        if (data != null && node != null) {
+        if (data && node) {
           db.assignUpdatedMetadata(node, data)
         }
       }
@@ -118,11 +119,10 @@ export const useGraphStore = defineStore('graph', () => {
   })
 
   function generateUniqueIdent() {
-    let ident: string
-    do {
-      ident = randomString()
-    } while (db.identifierUsed(ident))
-    return ident
+    for (;;) {
+      const ident = randomIdent()
+      if (!db.identifierUsed(ident)) return ident
+    }
   }
 
   const edges = computed(() => {
@@ -134,7 +134,7 @@ export const useGraphStore = defineStore('graph', () => {
         edges.push({ source, target })
       }
     }
-    if (unconnectedEdge.value != null) {
+    if (unconnectedEdge.value) {
       edges.push({
         source: unconnectedEdge.value.source,
         target: unconnectedEdge.value.target,
@@ -146,14 +146,17 @@ export const useGraphStore = defineStore('graph', () => {
   function createEdgeFromOutput(source: ExprId) {
     unconnectedEdge.value = { source }
   }
+
   function disconnectSource(edge: Edge) {
-    if (edge.target != null)
+    if (edge.target)
       unconnectedEdge.value = { target: edge.target, disconnectedEdgeTarget: edge.target }
   }
+
   function disconnectTarget(edge: Edge) {
-    if (edge.source != null && edge.target != null)
+    if (edge.source && edge.target)
       unconnectedEdge.value = { source: edge.source, disconnectedEdgeTarget: edge.target }
   }
+
   function clearUnconnected() {
     unconnectedEdge.value = undefined
   }
@@ -164,7 +167,7 @@ export const useGraphStore = defineStore('graph', () => {
     metadata: NodeMetadata | undefined = undefined,
   ): Opt<ExprId> {
     const mod = proj.module
-    if (mod == null) return
+    if (!mod) return
     const meta = metadata ?? {
       x: position.x,
       y: -position.y,
@@ -186,13 +189,13 @@ export const useGraphStore = defineStore('graph', () => {
 
   function deleteNode(id: ExprId) {
     const node = db.nodeIdToNode.get(id)
-    if (node == null) return
+    if (!node) return
     proj.module?.deleteExpression(node.outerExprId)
   }
 
   function setNodeContent(id: ExprId, content: string) {
     const node = db.nodeIdToNode.get(id)
-    if (node == null) return
+    if (!node) return
     setExpressionContent(node.rootSpan.astId, content)
   }
 
@@ -210,13 +213,13 @@ export const useGraphStore = defineStore('graph', () => {
 
   function replaceNodeSubexpression(nodeId: ExprId, range: ContentRange, content: string) {
     const node = db.nodeIdToNode.get(nodeId)
-    if (node == null) return
+    if (!node) return
     proj.module?.replaceExpressionContent(node.rootSpan.astId, content, range)
   }
 
   function setNodePosition(nodeId: ExprId, position: Vec2) {
     const node = db.nodeIdToNode.get(nodeId)
-    if (node == null) return
+    if (!node) return
     proj.module?.updateNodeMetadata(nodeId, { x: position.x, y: -position.y })
   }
 
@@ -240,31 +243,45 @@ export const useGraphStore = defineStore('graph', () => {
 
   function setNodeVisualizationId(nodeId: ExprId, vis: Opt<VisualizationIdentifier>) {
     const node = db.nodeIdToNode.get(nodeId)
-    if (node == null) return
+    if (!node) return
     proj.module?.updateNodeMetadata(nodeId, { vis: normalizeVisMetadata(vis, node.vis?.visible) })
   }
 
   function setNodeVisualizationVisible(nodeId: ExprId, visible: boolean) {
     const node = db.nodeIdToNode.get(nodeId)
-    if (node == null) return
+    if (!node) return
     proj.module?.updateNodeMetadata(nodeId, { vis: normalizeVisMetadata(node.vis, visible) })
   }
 
   function updateNodeRect(id: ExprId, rect: Rect) {
-    nodeRects.set(id, rect)
+    if (rect.pos.equals(Vec2.Zero) && !metadata.value?.has(id)) {
+      const { position } = nonDictatedPlacement(rect.size, {
+        nodeRects: [...nodeRects.entries()]
+          .filter(([id]) => db.nodeIdToNode.get(id))
+          .map(([, rect]) => rect),
+        // The rest of the properties should not matter.
+        selectedNodeRects: [],
+        screenBounds: Rect.Zero,
+        mousePosition: Vec2.Zero,
+      })
+      metadata.value?.set(id, { x: position.x, y: -position.y, vis: null })
+      nodeRects.set(id, new Rect(position, rect.size))
+    } else {
+      nodeRects.set(id, rect)
+    }
   }
 
   function updateExprRect(id: ExprId, rect: Rect | undefined) {
     const current = exprRects.get(id)
     if (rect) {
-      if (current == null || !current.equals(rect)) exprRects.set(id, rect)
+      if (!current || !current.equals(rect)) exprRects.set(id, rect)
     } else {
-      if (current != null) exprRects.delete(id)
+      if (current) exprRects.delete(id)
     }
   }
 
   function setEditedNode(id: ExprId | null, cursorPosition: number | null) {
-    if (id == null) {
+    if (!id) {
       editedNodeInfo.value = undefined
       return
     }
@@ -272,7 +289,7 @@ export const useGraphStore = defineStore('graph', () => {
       console.warn('setEditedNode: cursorPosition is null')
       return
     }
-    const range = [cursorPosition, cursorPosition] as ContentRange
+    const range: ContentRange = [cursorPosition, cursorPosition]
     editedNodeInfo.value = { id, range }
   }
 
@@ -304,7 +321,7 @@ export const useGraphStore = defineStore('graph', () => {
   }
 })
 
-function randomString() {
+function randomIdent() {
   return 'operator' + Math.round(Math.random() * 100000)
 }
 
@@ -326,7 +343,7 @@ function getExecutedMethodAst(
   code: string,
   executionStackTop: StackItem,
   updatedIdMap: Y.Map<Uint8Array>,
-): Opt<Ast.Tree.Function> {
+): Ast.Tree.Function | undefined {
   switch (executionStackTop.type) {
     case 'ExplicitCall': {
       // Assume that the provided AST matches the module in the method pointer. There is no way to
@@ -338,7 +355,7 @@ function getExecutedMethodAst(
     case 'LocalCall': {
       const exprId = executionStackTop.expressionId
       const range = lookupIdRange(updatedIdMap, exprId)
-      if (range == null) return
+      if (!range) return
       const node = findAstWithRange(ast, range)
       if (node?.type === Ast.Tree.Type.Function) return node
     }
@@ -348,7 +365,7 @@ function getExecutedMethodAst(
 function lookupIdRange(updatedIdMap: Y.Map<Uint8Array>, id: ExprId): [number, number] | undefined {
   const doc = updatedIdMap.doc!
   const rangeBuffer = updatedIdMap.get(id)
-  if (rangeBuffer == null) return
+  if (!rangeBuffer) return
   const decoded = decodeRange(rangeBuffer)
   const index = Y.createAbsolutePositionFromRelativePosition(decoded[0], doc)?.index
   const endIndex = Y.createAbsolutePositionFromRelativePosition(decoded[1], doc)?.index
@@ -360,7 +377,7 @@ function findModuleMethod(
   moduleAst: Ast.Tree,
   code: string,
   methodName: string,
-): Opt<Ast.Tree.Function> {
+): Ast.Tree.Function | undefined {
   for (const node of childrenAstNodes(moduleAst)) {
     if (node.type === Ast.Tree.Type.Function && readAstSpan(node.name, code) === methodName)
       return node
diff --git a/app/gui2/src/util/ast/extended.ts b/app/gui2/src/util/ast/extended.ts
index 9cdf48633bf..d71457ef0c1 100644
--- a/app/gui2/src/util/ast/extended.ts
+++ b/app/gui2/src/util/ast/extended.ts
@@ -150,6 +150,14 @@ export class AstExtended<T extends Tree | Token = Tree | Token, HasIdMap extends
     }
   }
 
+  /**
+   * Recursively visit AST nodes in depth-first order. The children of a node will be skipped when
+   * `visit` callback returns `false`.
+   *
+   * @param node Root node of the tree to walk. It will be visited first.
+   * @param visit Callback that is called for each node. If it returns `false`, the children of that
+   * node will be skipped, and the walk will continue to the next sibling.
+   */
   visitRecursive(visitor: (t: AstExtended<Tree | Token, HasIdMap>) => boolean) {
     visitGenerator(this.walkRecursive(), visitor)
   }