fix: Don't clober event group when renaming vars (#6829)

* fix: Don't clober event group when renaming vars

Also audit all existing event group commands and tweak a few of them where I think there's a potential issue.

core/block.ts

@@ -289,9 +289,7 @@ export class Block implements IASTNodeLocation, IDeletable {
         eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_CREATE))(this));
       }
     } finally {
-      if (!existingGroup) {
-        eventUtils.setGroup(false);
-      }
+      eventUtils.setGroup(existingGroup);
       // In case init threw, recordUndo flag should still be reset.
       eventUtils.setRecordUndo(initialUndoFlag);
     }

core/bump_objects.ts

@@ -105,7 +105,7 @@ export function bumpIntoBoundsHandler(workspace: WorkspaceSvg):
         return;
       }
       // Handle undo.
-      const oldGroup = eventUtils.getGroup();
+      const existingGroup = eventUtils.getGroup() || false;
       eventUtils.setGroup(e.group);
 
       const wasBumped = bumpObjectIntoBounds(
@@ -116,9 +116,7 @@ export function bumpIntoBoundsHandler(workspace: WorkspaceSvg):
             'Moved object in bounds but there was no' +
             ' event group. This may break undo.');
       }
-      if (oldGroup !== null) {
-        eventUtils.setGroup(oldGroup);
-      }
+      eventUtils.setGroup(existingGroup);
     } else if (e.type === eventUtils.VIEWPORT_CHANGE) {
       const viewportEvent = (e as ViewportChange);
       if (viewportEvent.scale && viewportEvent.oldScale &&

core/connection.ts

@@ -223,8 +223,8 @@ export class Connection implements IASTNodeLocationWithBlock {
 
     const checker = this.getConnectionChecker();
     if (checker.canConnect(this, otherConnection, false)) {
-      const eventGroup = eventUtils.getGroup();
-      if (!eventGroup) {
+      const existingGroup = eventUtils.getGroup();
+      if (!existingGroup) {
         eventUtils.setGroup(true);
       }
       // Determine which block is superior (higher in the source stack).
@@ -235,9 +235,7 @@ export class Connection implements IASTNodeLocationWithBlock {
         // Inferior block.
         otherConnection.connect_(this);
       }
-      if (!eventGroup) {
-        eventUtils.setGroup(false);
-      }
+      eventUtils.setGroup(existingGroup);
     }
 
     return this.isConnected();
@@ -265,8 +263,10 @@ export class Connection implements IASTNodeLocationWithBlock {
       throw Error('Source connection not connected.');
     }
 
-    const eventGroup = eventUtils.getGroup();
-    if (!eventGroup) eventUtils.setGroup(true);
+    const existingGroup = eventUtils.getGroup();
+    if (!existingGroup) {
+      eventUtils.setGroup(true);
+    }
 
     let event;
     if (eventUtils.isEnabled()) {
@@ -289,7 +289,7 @@ export class Connection implements IASTNodeLocationWithBlock {
       parentConnection.respawnShadow_();
     }
 
-    if (!eventGroup) eventUtils.setGroup(false);
+    eventUtils.setGroup(existingGroup);
   }
 
   /**

core/contextmenu_items.ts

@@ -20,7 +20,6 @@ import * as Events from './events/events.js';
 import * as eventUtils from './events/utils.js';
 import {inputTypes} from './input_types.js';
 import {Msg} from './msg.js';
-import * as idGenerator from './utils/idgenerator.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
 
 
@@ -230,13 +229,18 @@ function getDeletableBlocks_(workspace: WorkspaceSvg): BlockSvg[] {
 /**
  * Deletes the given blocks. Used to delete all blocks in the workspace.
  *
- * @param deleteList list of blocks to delete.
- * @param eventGroup event group ID with which all delete events should be
- *     associated.
+ * @param deleteList List of blocks to delete.
+ * @param eventGroup Event group ID with which all delete events should be
+ *     associated.  If not specified, create a new group.
  */
-function deleteNext_(deleteList: BlockSvg[], eventGroup: string) {
+function deleteNext_(deleteList: BlockSvg[], eventGroup?: string) {
   const DELAY = 10;
-  eventUtils.setGroup(eventGroup);
+  if (eventGroup) {
+    eventUtils.setGroup(eventGroup);
+  } else {
+    eventUtils.setGroup(true);
+    eventGroup = eventUtils.getGroup();
+  }
   const block = deleteList.shift();
   if (block) {
     if (!block.isDeadOrDying()) {
@@ -277,16 +281,15 @@ export function registerDeleteAll() {
       }
       scope.workspace.cancelCurrentGesture();
       const deletableBlocks = getDeletableBlocks_(scope.workspace);
-      const eventGroup = idGenerator.genUid();
       if (deletableBlocks.length < 2) {
-        deleteNext_(deletableBlocks, eventGroup);
+        deleteNext_(deletableBlocks);
       } else {
         dialog.confirm(
             Msg['DELETE_ALL_BLOCKS'].replace(
                 '%1', String(deletableBlocks.length)),
             function(ok) {
               if (ok) {
-                deleteNext_(deletableBlocks, eventGroup);
+                deleteNext_(deletableBlocks);
               }
             });
       }
@@ -455,14 +458,12 @@ export function registerDisable() {
     },
     callback(scope: Scope) {
       const block = scope.block;
-      const group = eventUtils.getGroup();
-      if (!group) {
+      const existingGroup = eventUtils.getGroup();
+      if (!existingGroup) {
         eventUtils.setGroup(true);
       }
       block!.setEnabled(!block!.isEnabled());
-      if (!group) {
-        eventUtils.setGroup(false);
-      }
+      eventUtils.setGroup(existingGroup);
     },
     scopeType: ContextMenuRegistry.ScopeType.BLOCK,
     id: 'blockDisable',

core/rendered_connection.ts

@@ -452,7 +452,7 @@ export class RenderedConnection extends Connection {
   }
 
   /**
-   * Behavior after a connection attempt fails.
+   * Behaviour after a connection attempt fails.
    * Bumps this connection away from the other connection. Called when an
    * attempted connection fails.
    *

core/variable_map.ts

@@ -69,7 +69,10 @@ export class VariableMap {
     const type = variable.type;
     const conflictVar = this.getVariable(newName, type);
     const blocks = this.workspace.getAllBlocks(false);
-    eventUtils.setGroup(true);
+    const existingGroup = eventUtils.getGroup();
+    if (!existingGroup) {
+      eventUtils.setGroup(true);
+    }
     try {
       // The IDs may match if the rename is a simple case change (name1 ->
       // Name1).
@@ -80,7 +83,7 @@ export class VariableMap {
             variable, newName, conflictVar, blocks);
       }
     } finally {
-      eventUtils.setGroup(false);
+      eventUtils.setGroup(existingGroup);
     }
   }
 
@@ -284,9 +287,7 @@ export class VariableMap {
       }
       this.deleteVariable(variable);
     } finally {
-      if (!existingGroup) {
-        eventUtils.setGroup(false);
-      }
+      eventUtils.setGroup(existingGroup);
     }
   }
   /* End functions for variable deletion. */

core/workspace.ts

@@ -359,9 +359,7 @@ export class Workspace implements IASTNodeLocation {
       while (this.topComments.length) {
         this.topComments[this.topComments.length - 1].dispose();
       }
-      if (!existingGroup) {
-        eventUtils.setGroup(false);
-      }
+      eventUtils.setGroup(existingGroup);
       this.variableMap.clear();
       if (this.potentialVariableMap) {
         this.potentialVariableMap.clear();

core/workspace_comment.ts

@@ -307,9 +307,7 @@ export class WorkspaceComment {
         eventUtils.fire(
             new (eventUtils.get(eventUtils.COMMENT_CREATE))(comment));
       } finally {
-        if (!existingGroup) {
-          eventUtils.setGroup(false);
-        }
+        eventUtils.setGroup(existingGroup);
       }
     }
   }

core/workspace_svg.ts

@@ -1366,21 +1366,22 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg {
     if (!existingGroup) {
       eventUtils.setGroup(true);
     }
-
     let pastedThing;
-    // Checks if this is JSON. JSON has a type property, while elements don't.
-    if (state['type']) {
-      pastedThing = this.pasteBlock_(null, state as blocks.State);
-    } else {
-      const xmlBlock = state as Element;
-      if (xmlBlock.tagName.toLowerCase() === 'comment') {
-        pastedThing = this.pasteWorkspaceComment_(xmlBlock);
+    try {
+      // Checks if this is JSON. JSON has a type property, while elements don't.
+      if (state['type']) {
+        pastedThing = this.pasteBlock_(null, state as blocks.State);
       } else {
-        pastedThing = this.pasteBlock_(xmlBlock, null);
+        const xmlBlock = state as Element;
+        if (xmlBlock.tagName.toLowerCase() === 'comment') {
+          pastedThing = this.pasteWorkspaceComment_(xmlBlock);
+        } else {
+          pastedThing = this.pasteBlock_(xmlBlock, null);
+        }
       }
+    } finally {
+      eventUtils.setGroup(existingGroup);
     }
-
-    eventUtils.setGroup(existingGroup);
     return pastedThing;
   }
 

core/xml.ts

@@ -463,9 +463,7 @@ export function domToWorkspace(xml: Element, workspace: Workspace): string[] {
       }
     }
   } finally {
-    if (!existingGroup) {
-      eventUtils.setGroup(false);
-    }
+    eventUtils.setGroup(existingGroup);
     dom.stopTextWidthCache();
   }
   // Re-enable workspace resizing.