Merge pull request #2770 from BeksOmega/fixes/DisposalConnections

Updated How Connections Dispose of Themselves

core/block.js

@@ -226,6 +226,13 @@ Blockly.Block.obtain = function(workspace, prototypeName) {
  */
 Blockly.Block.prototype.data = null;
 
+/**
+ * Has this block been disposed of?
+ * @type {boolean}
+ * @package
+ */
+Blockly.Block.prototype.disposed = false;
+
 /**
  * Colour of the block as HSV hue value (0-360)
  * This may be null if the block colour was not set via a hue number.
@@ -316,15 +323,12 @@ Blockly.Block.prototype.dispose = function(healStack) {
     this.inputList.length = 0;
     // Dispose of any remaining connections (next/previous/output).
     var connections = this.getConnections_(true);
-    for (var i = 0; i < connections.length; i++) {
-      var connection = connections[i];
-      if (connection.isConnected()) {
-        connection.disconnect();
-      }
-      connections[i].dispose();
+    for (var i = 0, connection; connection = connections[i]; i++) {
+      connection.dispose();
     }
   } finally {
     Blockly.Events.enable();
+    this.disposed = true;
   }
 };
 
@@ -1759,17 +1763,6 @@ Blockly.Block.prototype.moveNumberedInputBefore = function(
 Blockly.Block.prototype.removeInput = function(name, opt_quiet) {
   for (var i = 0, input; input = this.inputList[i]; i++) {
     if (input.name == name) {
-      if (input.connection && input.connection.isConnected()) {
-        input.connection.setShadowDom(null);
-        var block = input.connection.targetBlock();
-        if (block.isShadow()) {
-          // Destroy any attached shadow block.
-          block.dispose();
-        } else {
-          // Disconnect any attached normal block.
-          block.unplug();
-        }
-      }
       input.dispose();
       this.inputList.splice(i, 1);
       return;

core/connection.js

@@ -71,6 +71,13 @@ Blockly.Connection.REASON_SHADOW_PARENT = 6;
  */
 Blockly.Connection.prototype.targetConnection = null;
 
+/**
+ * Has this connection been disposed of?
+ * @type {boolean}
+ * @package
+ */
+Blockly.Connection.prototype.disposed = false;
+
 /**
  * List of compatible value types.  Null if all types are compatible.
  * @type {Array}
@@ -233,15 +240,30 @@ Blockly.Connection.prototype.connect_ = function(childConnection) {
 };
 
 /**
- * Sever all links to this connection (not including from the source object).
+ * Dispose of this connection. Deal with connected blocks and remove this
+ * connection from the database.
+ * @package
  */
 Blockly.Connection.prototype.dispose = function() {
+
+  // isConnected returns true for shadows and non-shadows.
   if (this.isConnected()) {
-    throw Error('Disconnect connection before disposing of it.');
+    this.setShadowDom(null);
+    var targetBlock = this.targetBlock();
+    if (targetBlock.isShadow()) {
+      // Destroy the attached shadow block & its children.
+      targetBlock.dispose();
+    } else {
+      // Disconnect the attached normal block.
+      targetBlock.unplug();
+    }
   }
+
   if (this.inDB_) {
     this.db_.removeConnection_(this);
   }
+
+  this.disposed = true;
 };
 
 /**

core/field.js

@@ -149,6 +149,7 @@ Blockly.Field.X_PADDING = 10;
  * @type {[type]}
  */
 Blockly.Field.DEFAULT_TEXT_OFFSET = Blockly.Field.X_PADDING / 2;
+
 /**
  * Name of field.  Unique within each block.
  * Static labels are usually unnamed.
@@ -156,6 +157,13 @@ Blockly.Field.DEFAULT_TEXT_OFFSET = Blockly.Field.X_PADDING / 2;
  */
 Blockly.Field.prototype.name = undefined;
 
+/**
+ * Has this field been disposed of?
+ * @type {boolean}
+ * @package
+ */
+Blockly.Field.prototype.disposed = false;
+
 /**
  * Maximum characters of text to display before adding an ellipsis.
  * @type {number}

tests/mocha/block_test.js

@@ -1,27 +1,32 @@
-
+/**
+ * @license
+ * Visual Blocks Editor
+ *
+ * Copyright 2019 Google Inc.
+ * https://developers.google.com/blockly/
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
 
 suite('Blocks', function() {
+  setup(function() {
+    this.workspace = new Blockly.Workspace();
+  });
+  teardown(function() {
+    this.workspace.dispose();
+  });
 
-  suite('Unplug', function() {
-    function assertUnpluggedNoheal(blocks) {
-      // A has nothing connected to it.
-      assertEquals(0, blocks.A.getChildren().length);
-      // B and C are still connected.
-      assertEquals(blocks.B, blocks.C.getParent());
-      // B is the top of its stack.
-      assertNull(blocks.B.getParent());
-    }
-
-    function assertUnpluggedHealed(blocks) {
-      // A and C are connected.
-      assertEquals(1, blocks.A.getChildren().length);
-      assertEquals(blocks.A, blocks.C.getParent());
-      // B has nothing connected to it.
-      assertEquals(0, blocks.B.getChildren().length);
-      // B is the top of its stack.
-      assertNull(blocks.B.getParent());
-    }
-
+  suite('Connection Management', function() {
     setup(function() {
       Blockly.defineBlocksWithJsonArray([{
         "type": "stack_block",
@@ -40,136 +45,375 @@ suite('Blocks', function() {
         ],
         "output": null
       }]);
-
-      this.workspace = new Blockly.Workspace();
     });
-
     teardown(function() {
       delete Blockly.Blocks['stack_block'];
       delete Blockly.Blocks['row_block'];
-
-      this.workspace.dispose();
     });
 
-    suite('Row', function() {
-      setup(function() {
-        var blockA = this.workspace.newBlock('row_block');
-        var blockB = this.workspace.newBlock('row_block');
-        var blockC = this.workspace.newBlock('row_block');
-
-        blockA.inputList[0].connection.connect(blockB.outputConnection);
-        blockB.inputList[0].connection.connect(blockC.outputConnection);
-
-        assertEquals(blockB, blockC.getParent());
-
-        this.blocks = {
-          A: blockA,
-          B: blockB,
-          C: blockC
-        };
-      });
-
-      test('Don\'t heal', function() {
-        this.blocks.B.unplug(false);
-        assertUnpluggedNoheal(this.blocks);
-      });
-
-      test('Heal', function() {
-        this.blocks.B.unplug(true);
-        // Each block has only one input, and the types work.
-        assertUnpluggedHealed(this.blocks);
-      });
-
-      test('Heal with bad checks', function() {
-        var blocks = this.blocks;
-
-        // A and C can't connect, but both can connect to B.
-        blocks.A.inputList[0].connection.setCheck('type1');
-        blocks.C.outputConnection.setCheck('type2');
-
-        // Each block has only one input, but the types don't work.
-        blocks.B.unplug(true);
-        assertUnpluggedNoheal(blocks);
-      });
-
-      test('Parent has multiple inputs', function() {
-        var blocks = this.blocks;
-        // Add extra input to parent
-        blocks.A.appendValueInput("INPUT").setCheck(null);
-        blocks.B.unplug(true);
-        assertUnpluggedHealed(blocks);
-      });
-
-      test('Middle block has multiple inputs', function() {
-        var blocks = this.blocks;
-        // Add extra input to middle block
-        blocks.B.appendValueInput("INPUT").setCheck(null);
-        blocks.B.unplug(true);
-        assertUnpluggedHealed(blocks);
-      });
-
-      test('Child block has multiple inputs', function() {
-        var blocks = this.blocks;
-        // Add extra input to child block
-        blocks.C.appendValueInput("INPUT").setCheck(null);
-        // Child block input count doesn't matter.
-        blocks.B.unplug(true);
-        assertUnpluggedHealed(blocks);
-      });
-    });
-
-
-    suite('Stack', function() {
-      setup(function() {
-        var blockA = this.workspace.newBlock('stack_block');
-        var blockB = this.workspace.newBlock('stack_block');
-        var blockC = this.workspace.newBlock('stack_block');
-
-        blockA.nextConnection.connect(blockB.previousConnection);
-        blockB.nextConnection.connect(blockC.previousConnection);
-
-        assertEquals(blockB, blockC.getParent());
-
-        this.blocks = {
-          A: blockA,
-          B: blockB,
-          C: blockC
-        };
-      });
-
-      test('Don\'t heal', function() {
-        this.blocks.B.unplug();
-        assertUnpluggedNoheal(this.blocks);
-      });
-
-      test('Heal', function() {
-        this.blocks.B.unplug(true);
-        assertUnpluggedHealed(this.blocks);
-      });
-
-      test('Heal with bad checks', function() {
-        var blocks = this.blocks;
-        // A and C can't connect, but both can connect to B.
-        blocks.A.nextConnection.setCheck('type1');
-        blocks.C.previousConnection.setCheck('type2');
-
-        // The types don't work.
-        blocks.B.unplug(true);
-
-        // Stack blocks unplug before checking whether the types match.
-        // TODO (#1994): Check types before unplugging.
+    suite('Unplug', function() {
+      function assertUnpluggedNoheal(blocks) {
         // A has nothing connected to it.
         assertEquals(0, blocks.A.getChildren().length);
-        // B has nothing connected to it.
-        assertEquals(0, blocks.B.getChildren().length);
-        // C has nothing connected to it.
-        assertEquals(0, blocks.C.getChildren().length);
-        // A is the top of its stack.
-        assertNull(blocks.A.getParent());
+        // B and C are still connected.
+        assertEquals(blocks.B, blocks.C.getParent());
         // B is the top of its stack.
         assertNull(blocks.B.getParent());
-        // C is the top of its stack.
-        assertNull(blocks.C.getParent());
+      }
+      function assertUnpluggedHealed(blocks) {
+        // A and C are connected.
+        assertEquals(1, blocks.A.getChildren().length);
+        assertEquals(blocks.A, blocks.C.getParent());
+        // B has nothing connected to it.
+        assertEquals(0, blocks.B.getChildren().length);
+        // B is the top of its stack.
+        assertNull(blocks.B.getParent());
+      }
+
+      suite('Row', function() {
+        setup(function() {
+          var blockA = this.workspace.newBlock('row_block');
+          var blockB = this.workspace.newBlock('row_block');
+          var blockC = this.workspace.newBlock('row_block');
+
+          blockA.inputList[0].connection.connect(blockB.outputConnection);
+          blockB.inputList[0].connection.connect(blockC.outputConnection);
+
+          assertEquals(blockB, blockC.getParent());
+
+          this.blocks = {
+            A: blockA,
+            B: blockB,
+            C: blockC
+          };
+        });
+
+        test('Don\'t heal', function() {
+          this.blocks.B.unplug(false);
+          assertUnpluggedNoheal(this.blocks);
+        });
+        test('Heal', function() {
+          this.blocks.B.unplug(true);
+          // Each block has only one input, and the types work.
+          assertUnpluggedHealed(this.blocks);
+        });
+        test('Heal with bad checks', function() {
+          var blocks = this.blocks;
+
+          // A and C can't connect, but both can connect to B.
+          blocks.A.inputList[0].connection.setCheck('type1');
+          blocks.C.outputConnection.setCheck('type2');
+
+          // Each block has only one input, but the types don't work.
+          blocks.B.unplug(true);
+          assertUnpluggedNoheal(blocks);
+        });
+        test('A has multiple inputs', function() {
+          var blocks = this.blocks;
+          // Add extra input to parent
+          blocks.A.appendValueInput("INPUT").setCheck(null);
+          blocks.B.unplug(true);
+          assertUnpluggedHealed(blocks);
+        });
+        test('B has multiple inputs', function() {
+          var blocks = this.blocks;
+          // Add extra input to middle block
+          blocks.B.appendValueInput("INPUT").setCheck(null);
+          blocks.B.unplug(true);
+          assertUnpluggedHealed(blocks);
+        });
+        test('C has multiple inputs', function() {
+          var blocks = this.blocks;
+          // Add extra input to child block
+          blocks.C.appendValueInput("INPUT").setCheck(null);
+          // Child block input count doesn't matter.
+          blocks.B.unplug(true);
+          assertUnpluggedHealed(blocks);
+        });
+        test('C is Shadow', function() {
+          var blocks = this.blocks;
+          blocks.C.setShadow(true);
+          blocks.B.unplug(true);
+          // Even though we're asking to heal, it will appear as if it has not
+          // healed because shadows always stay with the parent.
+          assertUnpluggedNoheal(blocks);
+        });
+      });
+      suite('Stack', function() {
+        setup(function() {
+          var blockA = this.workspace.newBlock('stack_block');
+          var blockB = this.workspace.newBlock('stack_block');
+          var blockC = this.workspace.newBlock('stack_block');
+
+          blockA.nextConnection.connect(blockB.previousConnection);
+          blockB.nextConnection.connect(blockC.previousConnection);
+
+          assertEquals(blockB, blockC.getParent());
+
+          this.blocks = {
+            A: blockA,
+            B: blockB,
+            C: blockC
+          };
+        });
+
+        test('Don\'t heal', function() {
+          this.blocks.B.unplug();
+          assertUnpluggedNoheal(this.blocks);
+        });
+        test('Heal', function() {
+          this.blocks.B.unplug(true);
+          assertUnpluggedHealed(this.blocks);
+        });
+        test.skip('Heal with bad checks', function() {
+          var blocks = this.blocks;
+          // A and C can't connect, but both can connect to B.
+          blocks.A.nextConnection.setCheck('type1');
+          blocks.C.previousConnection.setCheck('type2');
+
+          // The types don't work.
+          blocks.B.unplug(true);
+
+          // TODO (#1994): Check types before unplugging. Currently
+          //  everything disconnects, when C should stick with B
+          assertUnpluggedNoheal();
+        });
+        test('C is Shadow', function() {
+          var blocks = this.blocks;
+          blocks.C.setShadow(true);
+          blocks.B.unplug(true);
+          // Even though we're asking to heal, it will appear as if it has not
+          // healed because shadows always stay with the parent.
+          assertUnpluggedNoheal(blocks);
+        });
+      });
+    });
+    suite('Dispose', function() {
+      function assertDisposedNoheal(blocks) {
+        chai.assert.isFalse(blocks.A.disposed);
+        // A has nothing connected to it.
+        chai.assert.equal(0, blocks.A.getChildren().length);
+        // B is disposed.
+        chai.assert.isTrue(blocks.B.disposed);
+        // And C is disposed.
+        chai.assert.isTrue(blocks.C.disposed);
+      }
+      function assertDisposedHealed(blocks) {
+        chai.assert.isFalse(blocks.A.disposed);
+        chai.assert.isFalse(blocks.C.disposed);
+        // A and C are connected.
+        assertEquals(1, blocks.A.getChildren().length);
+        assertEquals(blocks.A, blocks.C.getParent());
+        // B is disposed.
+        chai.assert.isTrue(blocks.B.disposed);
+      }
+
+      suite('Row', function() {
+        setup(function() {
+          var blockA = this.workspace.newBlock('row_block');
+          var blockB = this.workspace.newBlock('row_block');
+          var blockC = this.workspace.newBlock('row_block');
+
+          blockA.inputList[0].connection.connect(blockB.outputConnection);
+          blockB.inputList[0].connection.connect(blockC.outputConnection);
+
+          assertEquals(blockB, blockC.getParent());
+
+          this.blocks = {
+            A: blockA,
+            B: blockB,
+            C: blockC
+          };
+        });
+
+        test('Don\'t heal', function() {
+          this.blocks.B.dispose(false);
+          assertDisposedNoheal(this.blocks);
+        });
+        test('Heal', function() {
+          this.blocks.B.dispose(true);
+          // Each block has only one input, and the types work.
+          assertDisposedHealed(this.blocks);
+        });
+        test('Heal with bad checks', function() {
+          var blocks = this.blocks;
+
+          // A and C can't connect, but both can connect to B.
+          blocks.A.inputList[0].connection.setCheck('type1');
+          blocks.C.outputConnection.setCheck('type2');
+
+          // Each block has only one input, but the types don't work.
+          blocks.B.dispose(true);
+          assertDisposedNoheal(blocks);
+        });
+        test('A has multiple inputs', function() {
+          var blocks = this.blocks;
+          // Add extra input to parent
+          blocks.A.appendValueInput("INPUT").setCheck(null);
+          blocks.B.dispose(true);
+          assertDisposedHealed(blocks);
+        });
+        test('B has multiple inputs', function() {
+          var blocks = this.blocks;
+          // Add extra input to middle block
+          blocks.B.appendValueInput("INPUT").setCheck(null);
+          blocks.B.dispose(true);
+          assertDisposedHealed(blocks);
+        });
+        test('C has multiple inputs', function() {
+          var blocks = this.blocks;
+          // Add extra input to child block
+          blocks.C.appendValueInput("INPUT").setCheck(null);
+          // Child block input count doesn't matter.
+          blocks.B.dispose(true);
+          assertDisposedHealed(blocks);
+        });
+        test('C is Shadow', function() {
+          var blocks = this.blocks;
+          blocks.C.setShadow(true);
+          blocks.B.dispose(true);
+          // Even though we're asking to heal, it will appear as if it has not
+          // healed because shadows always get destroyed.
+          assertDisposedNoheal(blocks);
+        });
+      });
+      suite('Stack', function() {
+        setup(function() {
+          var blockA = this.workspace.newBlock('stack_block');
+          var blockB = this.workspace.newBlock('stack_block');
+          var blockC = this.workspace.newBlock('stack_block');
+
+          blockA.nextConnection.connect(blockB.previousConnection);
+          blockB.nextConnection.connect(blockC.previousConnection);
+
+          assertEquals(blockB, blockC.getParent());
+
+          this.blocks = {
+            A: blockA,
+            B: blockB,
+            C: blockC
+          };
+        });
+
+        test('Don\'t heal', function() {
+          this.blocks.B.dispose();
+          assertDisposedNoheal(this.blocks);
+        });
+        test('Heal', function() {
+          this.blocks.B.dispose(true);
+          assertDisposedHealed(this.blocks);
+        });
+        test.skip('Heal with bad checks', function() {
+          var blocks = this.blocks;
+          // A and C can't connect, but both can connect to B.
+          blocks.A.nextConnection.setCheck('type1');
+          blocks.C.previousConnection.setCheck('type2');
+
+          // The types don't work.
+          blocks.B.dispose(true);
+
+          // TODO (#1994): Check types before unplugging. Current C gets
+          //  left behind when it should get disposed with B.
+          assertDisposedNoheal(blocks);
+        });
+        test('C is Shadow', function() {
+          var blocks = this.blocks;
+          blocks.C.setShadow(true);
+          blocks.B.dispose(true);
+          // Even though we're asking to heal, it will appear as if it has not
+          // healed because shadows always get destroyed.
+          assertDisposedNoheal(blocks);
+        });
+      });
+    });
+    suite('Remove Input', function() {
+      setup(function() {
+        Blockly.defineBlocksWithJsonArray([
+          {
+            "type": "value_block",
+            "message0": "%1",
+            "args0": [
+              {
+                "type": "input_value",
+                "name": "VALUE"
+              }
+            ]
+          },
+          {
+            "type": "statement_block",
+            "message0": "%1",
+            "args0": [
+              {
+                "type": "input_statement",
+                "name": "STATEMENT"
+              }
+            ]
+          },
+        ]);
+      });
+      teardown(function() {
+        delete Blockly.Blocks['value_block'];
+        delete Blockly.Blocks['statement_block'];
+      });
+
+      suite('Value', function() {
+        setup(function() {
+          this.blockA = this.workspace.newBlock('value_block');
+        });
+
+        test('No Connected', function() {
+          this.blockA.removeInput('VALUE');
+          chai.assert.isNull(this.blockA.getInput('VALUE'));
+        });
+        test('Block Connected', function() {
+          var blockB = this.workspace.newBlock('row_block');
+          this.blockA.getInput('VALUE').connection
+              .connect(blockB.outputConnection);
+
+          this.blockA.removeInput('VALUE');
+          chai.assert.isFalse(blockB.disposed);
+          chai.assert.equal(this.blockA.getChildren().length, 0);
+        });
+        test('Shadow Connected', function() {
+          var blockB = this.workspace.newBlock('row_block');
+          blockB.setShadow(true);
+          this.blockA.getInput('VALUE').connection
+              .connect(blockB.outputConnection);
+
+          this.blockA.removeInput('VALUE');
+          chai.assert.isTrue(blockB.disposed);
+          chai.assert.equal(this.blockA.getChildren().length, 0);
+        });
+      });
+      suite('Statement', function() {
+        setup(function() {
+          this.blockA = this.workspace.newBlock('statement_block');
+        });
+
+        test('No Connected', function() {
+          this.blockA.removeInput('STATEMENT');
+          chai.assert.isNull(this.blockA.getInput('STATEMENT'));
+        });
+        test('Block Connected', function() {
+          var blockB = this.workspace.newBlock('stack_block');
+          this.blockA.getInput('STATEMENT').connection
+              .connect(blockB.previousConnection);
+
+          this.blockA.removeInput('STATEMENT');
+          chai.assert.isFalse(blockB.disposed);
+          chai.assert.equal(this.blockA.getChildren().length, 0);
+        });
+        test('Shadow Connected', function() {
+          var blockB = this.workspace.newBlock('stack_block');
+          blockB.setShadow(true);
+          this.blockA.getInput('STATEMENT').connection
+              .connect(blockB.previousConnection);
+
+          this.blockA.removeInput('STATEMENT');
+          console.log(blockB.disposed, blockB);
+          chai.assert.isTrue(blockB.disposed);
+          chai.assert.equal(this.blockA.getChildren().length, 0);
+        });
       });
     });
   });

tests/mocha/connection_test.js

@@ -1,4 +1,22 @@
-
+/**
+ * @license
+ * Visual Blocks Editor
+ *
+ * Copyright 2019 Google Inc.
+ * https://developers.google.com/blockly/
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
 
 suite('Connections', function() {