From f2e408b6fa6178b248729f5fb917195680eaf588 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Mon, 26 Sep 2022 12:37:36 -0700 Subject: [PATCH] fix: disposing of a workspace that has overwritten shadows (#6424) * fix: disposing of a workspace that has overwritten shadows * fix: try slightly different placement to fix tests * fix: add disposing parameter to guaruntee consistent behavior * chore: wrap properties in a isDeadOrDying method * chore: make disposing private --- core/block.ts | 25 ++++++++++++++++--- core/block_svg.ts | 24 ++++++++---------- core/connection.ts | 2 +- core/contextmenu_items.ts | 2 +- core/field.ts | 5 ++-- core/field_variable.ts | 6 ++--- core/keyboard_nav/ast_node.ts | 2 +- tests/mocha/event_test.js | 4 --- tests/mocha/test_helpers/block_definitions.js | 1 + 9 files changed, 42 insertions(+), 29 deletions(-) diff --git a/core/block.ts b/core/block.ts index a99565754..caff93df2 100644 --- a/core/block.ts +++ b/core/block.ts @@ -180,6 +180,11 @@ export class Block implements IASTNodeLocation, IDeletable { protected collapsed_ = false; protected outputShape_: number|null = null; + /** + * Is the current block currently in the process of being disposed? + */ + private disposing = false; + /** * A string representing the comment attached to this block. * @@ -316,7 +321,7 @@ export class Block implements IASTNodeLocation, IDeletable { * @suppress {checkTypes} */ dispose(healStack: boolean) { - if (this.disposed) { + if (this.isDeadOrDying()) { return; } @@ -338,6 +343,7 @@ export class Block implements IASTNodeLocation, IDeletable { this.workspace.removeTypedBlock(this); // Remove from block database. this.workspace.removeBlockById(this.id); + this.disposing = true; // First, dispose of all my children. for (let i = this.childBlocks_.length - 1; i >= 0; i--) { @@ -360,6 +366,16 @@ export class Block implements IASTNodeLocation, IDeletable { } } + /** + * Returns true if the block is either in the process of being disposed, or + * is disposed. + * + * @internal + */ + isDeadOrDying(): boolean { + return this.disposing || this.disposed; + } + /** * Call initModel on all fields on the block. * May be called more than once. @@ -772,7 +788,7 @@ export class Block implements IASTNodeLocation, IDeletable { * @returns True if deletable. */ isDeletable(): boolean { - return this.deletable_ && !this.isShadow_ && !this.disposed && + return this.deletable_ && !this.isShadow_ && !this.isDeadOrDying() && !this.workspace.options.readOnly; } @@ -791,7 +807,7 @@ export class Block implements IASTNodeLocation, IDeletable { * @returns True if movable. */ isMovable(): boolean { - return this.movable_ && !this.isShadow_ && !this.disposed && + return this.movable_ && !this.isShadow_ && !this.isDeadOrDying() && !this.workspace.options.readOnly; } @@ -865,7 +881,8 @@ export class Block implements IASTNodeLocation, IDeletable { * @returns True if editable. */ isEditable(): boolean { - return this.editable_ && !this.disposed && !this.workspace.options.readOnly; + return this.editable_ && !this.isDeadOrDying() && + !this.workspace.options.readOnly; } /** diff --git a/core/block_svg.ts b/core/block_svg.ts index 1da3bd32b..7486bf5b1 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -525,7 +525,7 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg, /** Snap this block to the nearest grid point. */ snapToGrid() { - if (this.disposed) { + if (this.isDeadOrDying()) { return; // Deleted block. } if (this.workspace.isDragging()) { @@ -861,8 +861,7 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg, * @suppress {checkTypes} */ override dispose(healStack?: boolean, animate?: boolean) { - if (this.disposed) { - // The block has already been deleted. + if (this.isDeadOrDying()) { return; } Tooltip.dispose(); @@ -1067,13 +1066,12 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg, if (this.workspace.isDragging()) { // Don't change the warning text during a drag. // Wait until the drag finishes. - this.warningTextDb.set( - id, setTimeout(() => { - if (!this.disposed) { // Check block wasn't deleted. - this.warningTextDb.delete(id); - this.setWarningText(text, id); - } - }, 100)); + this.warningTextDb.set(id, setTimeout(() => { + if (!this.isDeadOrDying()) { + this.warningTextDb.delete(id); + this.setWarningText(text, id); + } + }, 100)); return; } if (this.isInFlyout) { @@ -1541,11 +1539,11 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg, * connected should not coincidentally line up on screen. */ override bumpNeighbours() { - if (this.disposed) { - return; // Deleted block. + if (this.isDeadOrDying()) { + return; } if (this.workspace.isDragging()) { - return; // Don't bump blocks during a drag. + return; } const rootBlock = this.getRootBlock(); if (rootBlock.isInFlyout) { diff --git a/core/connection.ts b/core/connection.ts index 8abf5e89b..e495df85d 100644 --- a/core/connection.ts +++ b/core/connection.ts @@ -571,7 +571,7 @@ export class Connection implements IASTNodeLocationWithBlock { const parentBlock = this.getSourceBlock(); const shadowState = this.getShadowState(); const shadowDom = this.getShadowDom(); - if (parentBlock.disposed || !shadowState && !shadowDom) { + if (parentBlock.isDeadOrDying() || !shadowState && !shadowDom) { return null; } diff --git a/core/contextmenu_items.ts b/core/contextmenu_items.ts index 9e31f6caa..db87b2618 100644 --- a/core/contextmenu_items.ts +++ b/core/contextmenu_items.ts @@ -249,7 +249,7 @@ function deleteNext_(deleteList: BlockSvg[], eventGroup: string) { eventUtils.setGroup(eventGroup); const block = deleteList.shift(); if (block) { - if (!block.disposed) { + if (!block.isDeadOrDying()) { block.dispose(false, true); setTimeout(deleteNext_, DELAY, deleteList, eventGroup); } else { diff --git a/core/field.ts b/core/field.ts index de670e236..a72d46c01 100644 --- a/core/field.ts +++ b/core/field.ts @@ -258,7 +258,8 @@ export abstract class Field implements IASTNodeLocationSvg, * @returns The renderer constant provider. */ getConstants(): ConstantProvider|null { - if (!this.constants_ && this.sourceBlock_ && !this.sourceBlock_.disposed && + if (!this.constants_ && this.sourceBlock_ && + !this.sourceBlock_.isDeadOrDying() && this.sourceBlock_.workspace.rendered) { this.constants_ = (this.sourceBlock_.workspace as WorkspaceSvg) .getRenderer() @@ -1050,7 +1051,7 @@ export abstract class Field implements IASTNodeLocationSvg, * @param e Mouse down event. */ protected onMouseDown_(e: Event) { - if (!this.sourceBlock_ || this.sourceBlock_.disposed) { + if (!this.sourceBlock_ || this.sourceBlock_.isDeadOrDying()) { return; } const gesture = (this.sourceBlock_.workspace as WorkspaceSvg).getGesture(e); diff --git a/core/field_variable.ts b/core/field_variable.ts index 12382219f..2c31cf13a 100644 --- a/core/field_variable.ts +++ b/core/field_variable.ts @@ -377,7 +377,7 @@ export class FieldVariable extends FieldDropdown { let variableTypes = this.variableTypes; if (variableTypes === null) { // If variableTypes is null, return all variable types. - if (this.sourceBlock_ && !this.sourceBlock_.disposed) { + if (this.sourceBlock_ && !this.sourceBlock_.isDeadOrDying()) { return this.sourceBlock_.workspace.getVariableTypes(); } } @@ -456,7 +456,7 @@ export class FieldVariable extends FieldDropdown { protected override onItemSelected_(menu: Menu, menuItem: MenuItem) { const id = menuItem.getValue(); // Handle special cases. - if (this.sourceBlock_ && !this.sourceBlock_.disposed) { + if (this.sourceBlock_ && !this.sourceBlock_.isDeadOrDying()) { if (id === internalConstants.RENAME_VARIABLE_ID) { // Rename variable. Variables.renameVariable( @@ -515,7 +515,7 @@ export class FieldVariable extends FieldDropdown { } const name = this.getText(); let variableModelList: AnyDuringMigration[] = []; - if (this.sourceBlock_ && !this.sourceBlock_.disposed) { + if (this.sourceBlock_ && !this.sourceBlock_.isDeadOrDying()) { const variableTypes = this.getVariableTypes_(); // Get a copy of the list, so that adding rename and new variable options // doesn't modify the workspace's list. diff --git a/core/keyboard_nav/ast_node.ts b/core/keyboard_nav/ast_node.ts index 1f9f4a10d..24bf02803 100644 --- a/core/keyboard_nav/ast_node.ts +++ b/core/keyboard_nav/ast_node.ts @@ -275,7 +275,7 @@ export class ASTNode { // TODO(#6097): Use instanceof checks to exit early for values of // curLocation that don't make sense. const curLocationAsBlock = curLocation as Block; - if (!curLocationAsBlock || curLocationAsBlock.disposed) { + if (!curLocationAsBlock || curLocationAsBlock.isDeadOrDying()) { return null; } const curRoot = curLocationAsBlock.getRootBlock(); diff --git a/tests/mocha/event_test.js b/tests/mocha/event_test.js index 30bec99ed..ba3209cbf 100644 --- a/tests/mocha/event_test.js +++ b/tests/mocha/event_test.js @@ -997,10 +997,6 @@ suite('Events', function() { this.eventsFireSpy, 0, Blockly.Events.BlockDelete, {oldXml: expectedOldXml, group: ''}, workspaceSvg.id, expectedId); - assertNthCallEventArgEquals( - changeListenerSpy, 0, Blockly.Events.BlockDelete, - {oldXml: expectedOldXml, group: ''}, - workspaceSvg.id, expectedId); // Expect the workspace to not have a variable with ID 'test_block_id'. chai.assert.isNull(this.workspace.getVariableById(TEST_BLOCK_ID)); diff --git a/tests/mocha/test_helpers/block_definitions.js b/tests/mocha/test_helpers/block_definitions.js index 213075a74..7d05523c2 100644 --- a/tests/mocha/test_helpers/block_definitions.js +++ b/tests/mocha/test_helpers/block_definitions.js @@ -167,6 +167,7 @@ export function createTestBlock() { }, 'renameVarById': Blockly.Block.prototype.renameVarById, 'updateVarName': Blockly.Block.prototype.updateVarName, + 'isDeadOrDying': () => false, }; }