From 6f64d1801a3147dcd71b1aca12f54dbbb28e2c76 Mon Sep 17 00:00:00 2001 From: Neil Fraser Date: Fri, 17 Mar 2023 20:09:51 +0100 Subject: [PATCH] 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 | 4 +--- core/bump_objects.ts | 6 ++---- core/connection.ts | 16 ++++++++-------- core/contextmenu_items.ts | 29 +++++++++++++++-------------- core/rendered_connection.ts | 2 +- core/variable_map.ts | 11 ++++++----- core/workspace.ts | 4 +--- core/workspace_comment.ts | 4 +--- core/workspace_svg.ts | 23 ++++++++++++----------- core/xml.ts | 4 +--- 10 files changed, 48 insertions(+), 55 deletions(-) diff --git a/core/block.ts b/core/block.ts index c8932277f..33dc56af3 100644 --- a/core/block.ts +++ b/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); } diff --git a/core/bump_objects.ts b/core/bump_objects.ts index 301d65a96..873b007aa 100644 --- a/core/bump_objects.ts +++ b/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 && diff --git a/core/connection.ts b/core/connection.ts index d6e046b3b..ee7ab8b99 100644 --- a/core/connection.ts +++ b/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); } /** diff --git a/core/contextmenu_items.ts b/core/contextmenu_items.ts index 667e0896f..81c1003e0 100644 --- a/core/contextmenu_items.ts +++ b/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', diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index 6d7a8fc99..1e91778af 100644 --- a/core/rendered_connection.ts +++ b/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. * diff --git a/core/variable_map.ts b/core/variable_map.ts index 7ef809b53..722c51b12 100644 --- a/core/variable_map.ts +++ b/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. */ diff --git a/core/workspace.ts b/core/workspace.ts index 61cbb3fa7..04a03eb91 100644 --- a/core/workspace.ts +++ b/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(); diff --git a/core/workspace_comment.ts b/core/workspace_comment.ts index 6745a1bb2..88418da82 100644 --- a/core/workspace_comment.ts +++ b/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); } } } diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index d186772bc..7ac747c14 100644 --- a/core/workspace_svg.ts +++ b/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; } diff --git a/core/xml.ts b/core/xml.ts index 473ffdb51..bf54caf4a 100644 --- a/core/xml.ts +++ b/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.