Fix deletion-failure bug (#10888)

- Fix bug in deletion logic (the `syncModule` was passed to a function that tried to commit it as an edit) - Refactor APIs to avoid similar bugs (remove `direct` edit option; direct access to the sync module must be done by name)
enso-org · Aug 23, 2024 · dd1e4f5 · dd1e4f5
1 parent 582b7f9
commit dd1e4f5
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 35 deletions.
diff --git a/app/gui2/src/components/ColorPickerMenu.vue b/app/gui2/src/components/ColorPickerMenu.vue
@@ -24,7 +24,7 @@ const editedNodeInitialColors = new Map<NodeId, string | undefined>()
 
 function setColor(color: string | undefined) {
   currentColor.value = color
-  graphStore.transact(() => {
+  graphStore.batchEdits(() => {
     if (color) {
       for (const node of selection.selected) {
         if (!editedNodeInitialColors.has(node))

diff --git a/app/gui2/src/components/GraphEditor.vue b/app/gui2/src/components/GraphEditor.vue
@@ -333,7 +333,7 @@ const graphBindingsHandler = graphBindings.handler({
       selected,
       (id) => graphStore.db.nodeIdToNode.get(id)?.vis?.visible === true,
     )
-    graphStore.transact(() => {
+    graphStore.batchEdits(() => {
       for (const nodeId of selected) {
         graphStore.setNodeVisualization(nodeId, { visible: !allVisible })
       }

diff --git a/app/gui2/src/composables/astDocumentation.ts b/app/gui2/src/composables/astDocumentation.ts
@@ -11,12 +11,7 @@ export function useAstDocumentation(graphStore: GraphStore, ast: ToValue<Ast | u
         const astValue = toValue(ast)
         if (!astValue) return
         if (value.trimStart() !== '') {
-          graphStore.edit(
-            (edit) =>
-              edit.getVersion(astValue).getOrInitDocumentation().setDocumentationText(value),
-            true,
-            true,
-          )
+          graphStore.getMutable(astValue).getOrInitDocumentation().setDocumentationText(value)
         } else {
           // Remove the documentation node.
           const documented = astValue.documentingAncestor()

diff --git a/app/gui2/src/stores/graph/index.ts b/app/gui2/src/stores/graph/index.ts
@@ -300,23 +300,19 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
     }
 
     function deleteNodes(ids: Iterable<NodeId>) {
-      edit(
-        (edit) => {
-          for (const id of ids) {
-            const node = db.nodeIdToNode.get(id)
-            if (!node) continue
-            if (node.type !== 'component') continue
-            const usages = db.getNodeUsages(id)
-            for (const usage of usages) updatePortValue(edit, usage, undefined)
-            const outerExpr = edit.getVersion(node.outerExpr)
-            if (outerExpr) Ast.deleteFromParentBlock(outerExpr)
-            nodeRects.delete(id)
-            nodeHoverAnimations.delete(id)
-          }
-        },
-        true,
-        true,
-      )
+      edit((edit) => {
+        for (const id of ids) {
+          const node = db.nodeIdToNode.get(id)
+          if (!node) continue
+          if (node.type !== 'component') continue
+          const usages = db.getNodeUsages(id)
+          for (const usage of usages) updatePortValue(edit, usage, undefined)
+          const outerExpr = edit.getVersion(node.outerExpr)
+          if (outerExpr) Ast.deleteFromParentBlock(outerExpr)
+          nodeRects.delete(id)
+          nodeHoverAnimations.delete(id)
+        }
+      })
     }
 
     function setNodeContent(
@@ -346,10 +342,6 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
       })
     }
 
-    function transact(fn: () => void) {
-      syncModule.value!.transact(fn)
-    }
-
     const undoManager = {
       undo() {
         proj.module?.undoManager.undo()
@@ -542,11 +534,9 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
      *
      *  @param skipTreeRepair - If the edit is certain not to produce incorrect or non-canonical syntax, this may be set
      *  to `true` for better performance.
-     *  @param direct - Apply all changes directly to the synchronized module; they will be committed even if the callback
-     *  exits by throwing an exception.
      */
-    function edit<T>(f: (edit: MutableModule) => T, skipTreeRepair?: boolean, direct?: boolean): T {
-      const edit = direct ? syncModule.value : syncModule.value?.edit()
+    function edit<T>(f: (edit: MutableModule) => T, skipTreeRepair?: boolean): T {
+      const edit = syncModule.value?.edit()
       assert(edit != null)
       let result
       edit.transact(() => {
@@ -556,11 +546,17 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
           assert(root instanceof Ast.BodyBlock)
           Ast.repair(root, edit)
         }
-        if (!direct) syncModule.value!.applyEdit(edit)
       })
       return result!
     }
 
+    /** Obtain a version of the given `Ast` for direct mutation. The `ast` must exist in the current module.
+     *  This can be more efficient than creating and committing an edit, but skips tree-repair and cannot be aborted.
+     */
+    function getMutable<T extends Ast.Ast>(ast: T): Ast.Mutable<T> {
+      return syncModule.value!.getVersion(ast)
+    }
+
     function batchEdits(f: () => void, origin: Origin = defaultLocalOrigin) {
       assert(syncModule.value != null)
       syncModule.value.transact(f, origin)
@@ -691,7 +687,6 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
     })
 
     return proxyRefs({
-      transact,
       db: markRaw(db),
       mockExpressionUpdate,
       editedNodeInfo,
@@ -710,6 +705,7 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
       pickInCodeOrder,
       ensureCorrectNodeOrder,
       batchEdits,
+      getMutable,
       overrideNodeColor,
       getNodeColorOverride,
       setNodeContent,