feat!: allow blocks to receive their own delete events (#6337)

BeksOmega · web-flow · commit e9920a54e0b5 · 2022-08-16T15:49:40.000-07:00
* feat!: allow blocks to receive their own delete events

* fix: move block tests back into main directory

* chore: add a test for disposing of callers

* chore: add test for delete being received

* chore: add comment about why we have to run the clock twice

* chore: fix whitespace

* chore: fix whitespace

* chore: fix imports in tests

* chore: bump mocha timeout

* chore: bump timeout again?

* chore: eliminate the possibility that tests are actually timing out

* chore: change timeout back

* chore: remove tests that might be the problematic ones

* chore: attempt enabling delete event test

* chore: enable lists tests

* chore: try ternary test as well

* chore: actually add block test files

* chore: enable remaining tests
diff --git a/blocks/procedures.js b/blocks/procedures.js
@@ -980,7 +980,7 @@ const PROCEDURE_CALL_COMMON = {
         Xml.domToWorkspace(xml, this.workspace);
         Events.setGroup(false);
       }
-    } else if (event.type === Events.BLOCK_DELETE) {
+    } else if (event.type === Events.BLOCK_DELETE && event.blockId != this.id) {
       // Look for the case where a procedure definition has been deleted,
       // leaving this block (a procedure call) orphaned.  In this case, delete
       // the orphan.
diff --git a/core/block.ts b/core/block.ts
@@ -316,18 +316,18 @@ export class Block implements IASTNodeLocation, IDeletable {
    */
   dispose(healStack: boolean) {
     if (this.disposed) {
-      // Already deleted.
       return;
     }
-    // Terminate onchange event calls.
-    if (this.onchangeWrapper_) {
-      this.workspace.removeChangeListener(this.onchangeWrapper_);
-    }
 
     this.unplug(healStack);
     if (eventUtils.isEnabled()) {
       eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_DELETE))!(this));
     }
+
+    if (this.onchangeWrapper_) {
+      this.workspace.removeChangeListener(this.onchangeWrapper_);
+    }
+
     eventUtils.disable();
 
     try {
diff --git a/tests/mocha/block_delete_event_test.js b/tests/mocha/block_delete_event_test.js
@@ -0,0 +1,39 @@
+/**
+ * @license
+ * Copyright 2021 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+goog.declareModuleId('Blockly.test.blockDeleteEvent');
+
+import * as eventUtils from '../../build/src/core/events/utils.js';
+import {sharedTestSetup, sharedTestTeardown} from './test_helpers/setup_teardown.js';
+
+suite('Block Delete Event', function() {
+  setup(function() {
+    sharedTestSetup.call(this);
+    this.workspace = new Blockly.Workspace();
+  });
+
+  teardown(function() {
+    sharedTestTeardown.call(this);
+  });
+
+  suite('Receiving', function() {
+    test('blocks receive their own delete events', function() {
+      Blockly.Blocks['test'] = {
+        onchange: function(e) {},
+      };
+      // Need to stub the definition, because the property on the definition is
+      // what gets registered as an event listener.
+      const spy = sinon.spy(Blockly.Blocks['test'], 'onchange');
+      const testBlock = this.workspace.newBlock('test');
+
+      testBlock.dispose();
+
+      const deleteClass = eventUtils.get(eventUtils.BLOCK_DELETE);
+      chai.assert.isTrue(spy.calledOnce);
+      chai.assert.isTrue(spy.getCall(0).args[0] instanceof deleteClass);
+    });
+  });
+});
diff --git a/tests/mocha/blocks/lists_test.js b/tests/mocha/blocks/lists_test.js
@@ -8,7 +8,7 @@ goog.declareModuleId('Blockly.test.lists');
 
 import {runSerializationTestSuite} from '../test_helpers/serialization.js';
 import {sharedTestSetup, sharedTestTeardown} from '../test_helpers/setup_teardown.js';
-import {ConnectionType} from '../../build/src/core/connection_type.js';
+import {ConnectionType} from '../../../build/src/core/connection_type.js';
 import {defineStatementBlock} from '../test_helpers/block_definitions.js';
 
 
diff --git a/tests/mocha/blocks/logic_ternary_test.js b/tests/mocha/blocks/logic_ternary_test.js
@@ -6,7 +6,7 @@
 
 goog.declareModuleId('Blockly.test.logicTernary');
 
-import * as eventUtils from '../../build/src/core/events/utils.js';
+import * as eventUtils from '../../../build/src/core/events/utils.js';
 import {runSerializationTestSuite} from '../test_helpers/serialization.js';
 import {sharedTestSetup, sharedTestTeardown} from '../test_helpers/setup_teardown.js';
 
diff --git a/tests/mocha/blocks/procedures_test.js b/tests/mocha/blocks/procedures_test.js
@@ -6,12 +6,11 @@
 
 goog.declareModuleId('Blockly.test.procedures');
 
-import * as Blockly from '../../build/src/core/blockly.js';
+import * as Blockly from '../../../build/src/core/blockly.js';
 import {assertCallBlockStructure, assertDefBlockStructure, createProcDefBlock, createProcCallBlock} from '../test_helpers/procedures.js';
 import {runSerializationTestSuite} from '../test_helpers/serialization.js';
 import {createGenUidStubWithReturns, sharedTestSetup, sharedTestTeardown, workspaceTeardown} from '../test_helpers/setup_teardown.js';
 
-
 suite('Procedures', function() {
   setup(function() {
     sharedTestSetup.call(this);
@@ -1210,3 +1209,43 @@ suite('Procedures', function() {
     });
   });
 });
+
+suite('Procedures, dont auto fire events', function() {
+  setup(function() {
+    sharedTestSetup.call(this, {fireEventsNow: false});
+    this.workspace = new Blockly.Workspace();
+  });
+  teardown(function() {
+    sharedTestTeardown.call(this);
+  });
+
+  const testSuites = [
+    {title: 'procedures_defreturn', hasReturn: true,
+      defType: 'procedures_defreturn', callType: 'procedures_callreturn'},
+    {title: 'procedures_defnoreturn', hasReturn: false,
+      defType: 'procedures_defnoreturn', callType: 'procedures_callnoreturn'},
+  ];
+
+  testSuites.forEach((testSuite) => {
+    suite(testSuite.title, function() {
+      suite('Disposal', function() {
+        test('callers are disposed when definitions are disposed', function() {
+          this.defBlock = new Blockly.Block(this.workspace, testSuite.defType);
+          this.defBlock.setFieldValue('proc name', 'NAME');
+          this.callerBlock = new Blockly.Block(
+              this.workspace, testSuite.callType);
+          this.callerBlock.setFieldValue('proc name', 'NAME');
+
+          // Run the clock now so that the create events get fired. If we fire
+          // it after disposing, a new procedure def will get created when
+          // the caller create event is heard.
+          this.clock.runAll();
+          this.defBlock.dispose();
+          this.clock.runAll();
+
+          chai.assert.isTrue(this.callerBlock.disposed);
+        });
+      });
+    });
+  });
+});
diff --git a/tests/mocha/index.html b/tests/mocha/index.html
@@ -19,8 +19,8 @@
     <div id="failureCount" style="display:none" tests_failed="unset"></div>
     <div id="failureMessages" style="display:none"></div>
     <!-- Load mocha et al. before bootstrapping so that we can safely
-	 goog.require() the test modules that make calls to (e.g.)
-	 suite() at the top level. -->
+      goog.require() the test modules that make calls to (e.g.)
+      suite() at the top level. -->
     <script src="../../node_modules/chai/chai.js"></script>
     <script src="../../node_modules/mocha/mocha.js"></script>
     <script src="../../node_modules/sinon/pkg/sinon.js"></script>
@@ -31,90 +31,91 @@
       });
 
       var BLOCKLY_BOOTSTRAP_OPTIONS = {
-	loadCompressed: false,
-	depsFiles: ['build/deps.js', 'build/deps.mocha.js'],
-	requires: [
-	  // Blockly modules needed by tests.
-	  'Blockly',
-	  'Blockly.libraryBlocks',
-	  'Blockly.Dart',
-	  'Blockly.Dart.texts',
-	  'Blockly.JavaScript',
-	  'Blockly.JavaScript.texts',
-	  'Blockly.Lua',
-	  'Blockly.Lua.texts',
-	  'Blockly.PHP',
-	  'Blockly.PHP.texts',
-	  'Blockly.Python',
-	  'Blockly.Python.texts',
+        loadCompressed: false,
+        depsFiles: ['build/deps.js', 'build/deps.mocha.js'],
+        requires: [
+          // Blockly modules needed by tests.
+          'Blockly',
+          'Blockly.libraryBlocks',
+          'Blockly.Dart',
+          'Blockly.Dart.texts',
+          'Blockly.JavaScript',
+          'Blockly.JavaScript.texts',
+          'Blockly.Lua',
+          'Blockly.Lua.texts',
+          'Blockly.PHP',
+          'Blockly.PHP.texts',
+          'Blockly.Python',
+          'Blockly.Python.texts',
 
-	  // Test modules.
-	  'Blockly.test.astNode',
-	  'Blockly.test.blockChangeEvent',
-	  'Blockly.test.blockCreateEvent',
-	  'Blockly.test.blockJson',
-	  'Blockly.test.blocks',
-	  'Blockly.test.comments',
-	  'Blockly.test.commentDeserialization',
-	  'Blockly.test.connectionChecker',
-	  'Blockly.test.connectionDb',
-	  'Blockly.test.connection',
-	  'Blockly.test.contextMenuItem',
-	  'Blockly.test.cursor',
-	  'Blockly.test.dropdown',
-	  'Blockly.test.event',
-	  'Blockly.test.extensions',
-	  'Blockly.test.fieldAngle',
-	  'Blockly.test.fieldCheckbox',
-	  'Blockly.test.fieldColour',
-	  'Blockly.test.fieldDropdown',
-	  'Blockly.test.fieldImage',
-	  'Blockly.test.fieldLabelSerialization',
-	  'Blockly.test.fieldLabel',
-	  'Blockly.test.fieldMultiline',
-	  'Blockly.test.fieldNumber',
-	  'Blockly.test.fieldRegistry',
-	  'Blockly.test.fieldTest',
-	  'Blockly.test.fieldTextInput',
-	  'Blockly.test.fieldVariable',
-	  'Blockly.test.flyout',
-	  'Blockly.test.generator',
-	  'Blockly.test.gesture',
-	  'Blockly.test.input',
-	  'Blockly.test.insertionMarker',
-	  'Blockly.test.jsoDeserialization',
-	  'Blockly.test.jsoSerialization',
-	  'Blockly.test.json',
-	  'Blockly.test.keydown',
-	  'Blockly.test.lists',
-	  'Blockly.test.logicTernary',
-	  'Blockly.test.metrics',
-	  'Blockly.test.mutator',
-	  'Blockly.test.names',
-	  'Blockly.test.procedures',
-	  'Blockly.test.registry',
-	  'Blockly.test.serialization',
-	  'Blockly.test.shortcutRegistry',
-	  'Blockly.test.theme',
-	  'Blockly.test.toolbox',
-	  'Blockly.test.tooltip',
-	  'Blockly.test.trashcan',
-	  'Blockly.test.utils',
-	  'Blockly.test.variableMap',
-	  'Blockly.test.variableModel',
-	  'Blockly.test.variables',
-	  'Blockly.test.widgetDiv',
-	  'Blockly.test.workspaceComment',
-	  'Blockly.test.workspaceSvg',
-	  'Blockly.test.workspace',
-	  'Blockly.test.xml',
-	  'Blockly.test.zoomControls',
-	],
-	additionalScripts: [
-	  'msg/messages.js',
-	  'tests/playgrounds/screenshot.js',
-	  'node_modules/@blockly/dev-tools/dist/index.js',
-	],
+          // Test modules.
+          'Blockly.test.astNode',
+          'Blockly.test.blockChangeEvent',
+          'Blockly.test.blockDeleteEvent',
+          'Blockly.test.blockCreateEvent',
+          'Blockly.test.blockJson',
+          'Blockly.test.blocks',
+          'Blockly.test.comments',
+          'Blockly.test.commentDeserialization',
+          'Blockly.test.connectionChecker',
+          'Blockly.test.connectionDb',
+          'Blockly.test.connection',
+          'Blockly.test.contextMenuItem',
+          'Blockly.test.cursor',
+          'Blockly.test.dropdown',
+          'Blockly.test.event',
+          'Blockly.test.extensions',
+          'Blockly.test.fieldAngle',
+          'Blockly.test.fieldCheckbox',
+          'Blockly.test.fieldColour',
+          'Blockly.test.fieldDropdown',
+          'Blockly.test.fieldImage',
+          'Blockly.test.fieldLabelSerialization',
+          'Blockly.test.fieldLabel',
+          'Blockly.test.fieldMultiline',
+          'Blockly.test.fieldNumber',
+          'Blockly.test.fieldRegistry',
+          'Blockly.test.fieldTest',
+          'Blockly.test.fieldTextInput',
+          'Blockly.test.fieldVariable',
+          'Blockly.test.flyout',
+          'Blockly.test.generator',
+          'Blockly.test.gesture',
+          'Blockly.test.input',
+          'Blockly.test.insertionMarker',
+          'Blockly.test.jsoDeserialization',
+          'Blockly.test.jsoSerialization',
+          'Blockly.test.json',
+          'Blockly.test.keydown',
+          'Blockly.test.lists',
+          'Blockly.test.logicTernary',
+          'Blockly.test.metrics',
+          'Blockly.test.mutator',
+          'Blockly.test.names',
+          'Blockly.test.procedures',
+          'Blockly.test.registry',
+          'Blockly.test.serialization',
+          'Blockly.test.shortcutRegistry',
+          'Blockly.test.theme',
+          'Blockly.test.toolbox',
+          'Blockly.test.tooltip',
+          'Blockly.test.trashcan',
+          'Blockly.test.utils',
+          'Blockly.test.variableMap',
+          'Blockly.test.variableModel',
+          'Blockly.test.variables',
+          'Blockly.test.widgetDiv',
+          'Blockly.test.workspaceComment',
+          'Blockly.test.workspaceSvg',
+          'Blockly.test.workspace',
+          'Blockly.test.xml',
+          'Blockly.test.zoomControls',
+        ],
+        additionalScripts: [
+          'msg/messages.js',
+          'tests/playgrounds/screenshot.js',
+          'node_modules/@blockly/dev-tools/dist/index.js',
+        ],
       }
     </script>
     <script src="../bootstrap.js"></script>

Original file line number	Diff line number	Diff line change
`@@ -980,7 +980,7 @@ const PROCEDURE_CALL_COMMON = {`
`980`	`980`	`Xml.domToWorkspace(xml, this.workspace);`
`981`	`981`	`Events.setGroup(false);`
`982`	`982`	`}`
`983`		`- } else if (event.type === Events.BLOCK_DELETE) {`
	`983`	`+ } else if (event.type === Events.BLOCK_DELETE && event.blockId != this.id) {`
`984`	`984`	`// Look for the case where a procedure definition has been deleted,`
`985`	`985`	`// leaving this block (a procedure call) orphaned. In this case, delete`
`986`	`986`	`// the orphan.`