Change lastConnectionInRow to getPlaceForOrphanedOutput - Take 2 (#4851)

* Change lastConnectionInRow to getPlaceForOrphanedOutput

* PR comments

core/connection.js

@@ -134,7 +134,7 @@ Blockly.Connection.prototype.connect_ = function(childConnection) {
       // Attempt to reattach the orphan at the end of the newly inserted
       // block.  Since this block may be a row, walk down to the end
       // or to the first (and only) shadow block.
-      var connection = Blockly.Connection.lastConnectionInRow(
+      var connection = Blockly.Connection.getConnectionForOrphanedOutput(
           childBlock, orphanBlock);
       if (connection) {
         orphanBlock.outputConnection.connect(connection);
@@ -372,30 +372,31 @@ Blockly.Connection.connectReciprocally_ = function(first, second) {
 };
 
 /**
- * Does the given block have one and only one connection point that will accept
- * an orphaned block?
+ * Returns the single connection on the block that will accept the orphaned
+ * block, if one can be found. If the block has multiple compatible connections
+ * (even if they are filled) this returns null. If the block has no compatible
+ * connections, this returns null.
  * @param {!Blockly.Block} block The superior block.
  * @param {!Blockly.Block} orphanBlock The inferior block.
  * @return {Blockly.Connection} The suitable connection point on 'block',
  *     or null.
  * @private
  */
-Blockly.Connection.singleConnection_ = function(block, orphanBlock) {
-  var connection = null;
+Blockly.Connection.getSingleConnection_ = function(block, orphanBlock) {
+  var foundConnection = null;
   var output = orphanBlock.outputConnection;
-  for (var i = 0; i < block.inputList.length; i++) {
-    var thisConnection = block.inputList[i].connection;
-    var typeChecker = output.getConnectionChecker();
-    if (thisConnection &&
-        thisConnection.type == Blockly.connectionTypes.INPUT_VALUE &&
-        typeChecker.canConnect(output, thisConnection, false)) {
-      if (connection) {
+  var typeChecker = output.getConnectionChecker();
+
+  for (var i = 0, input; (input = block.inputList[i]); i++) {
+    var connection = input.connection;
+    if (connection && typeChecker.canConnect(output, connection, false)) {
+      if (foundConnection) {
         return null;  // More than one connection.
       }
-      connection = thisConnection;
+      foundConnection = connection;
     }
   }
-  return connection;
+  return foundConnection;
 };
 
 /**
@@ -410,18 +411,19 @@ Blockly.Connection.singleConnection_ = function(block, orphanBlock) {
  *     of blocks, or null.
  * @package
  */
-Blockly.Connection.lastConnectionInRow = function(startBlock, orphanBlock) {
-  var newBlock = startBlock;
-  var connection;
-  while ((connection = Blockly.Connection.singleConnection_(
-      /** @type {!Blockly.Block} */ (newBlock), orphanBlock))) {
-    newBlock = connection.targetBlock();
-    if (!newBlock || newBlock.isShadow()) {
-      return connection;
-    }
-  }
-  return null;
-};
+Blockly.Connection.getConnectionForOrphanedOutput =
+    function(startBlock, orphanBlock) {
+      var newBlock = startBlock;
+      var connection;
+      while ((connection = Blockly.Connection.getSingleConnection_(
+          /** @type {!Blockly.Block} */ (newBlock), orphanBlock))) {
+        newBlock = connection.targetBlock();
+        if (!newBlock || newBlock.isShadow()) {
+          return connection;
+        }
+      }
+      return null;
+    };
 
 /**
  * Disconnect this connection.

core/renderers/common/renderer.js

@@ -248,19 +248,13 @@ Blockly.blockRendering.Renderer.prototype.orphanCanConnectAtEnd =
     function(topBlock, orphanBlock, localType) {
       var orphanConnection = null;
       var lastConnection = null;
-      if (localType ==
-          Blockly.connectionTypes
-              .OUTPUT_VALUE) {  // We are replacing an output.
+      if (localType == Blockly.connectionTypes.OUTPUT_VALUE) {
         orphanConnection = orphanBlock.outputConnection;
-        // TODO:  I don't think this function necessarily has the correct logic,
-        //  but for now it is being kept for behavioral backwards-compat.
-        lastConnection = Blockly.Connection
-            .lastConnectionInRow(
+        lastConnection =
+            Blockly.Connection.getConnectionForOrphanedOutput(
                 /** @type {!Blockly.Block} **/ (topBlock), orphanBlock);
-      } else {  // We are replacing a previous.
+      } else {
         orphanConnection = orphanBlock.previousConnection;
-        // TODO: This lives on the block while lastConnectionInRow lives on
-        //  on the connection. Something is fishy.
         lastConnection = topBlock.lastConnectionInStack();
       }
 

tests/mocha/connection_test.js

@@ -156,9 +156,6 @@ suite('Connection', function() {
 
         teardown(function() {
           workspaceTeardown.call(this, this.workspace);
-          delete Blockly.Blocks['stack_block'];
-          delete Blockly.Blocks['row_block'];
-          delete Blockly.Blocks['statement_block'];
         });
 
         suite('Add - No Block Connected', function() {
@@ -784,4 +781,327 @@ suite('Connection', function() {
       });
     });
   });
+
+  suite('getConnectionForOrphanedOutput', function() {
+    setup(function() {
+      this.workspace = new Blockly.Workspace();
+
+      Blockly.defineBlocksWithJsonArray([
+        {
+          'type': 'input',
+          'message0': '%1',
+          'args0': [
+            {
+              'type': 'input_value',
+              'name': 'INPUT',
+              'check': 'check'
+            }
+          ],
+        },
+        {
+          'type': 'output',
+          'message0': '',
+          'output': 'check',
+        },
+      ]);
+    });
+
+    teardown(function() {
+      workspaceTeardown.call(this, this.workspace);
+    });
+
+    suite('No available spots', function() {
+      setup(function() {
+        Blockly.defineBlocksWithJsonArray([
+          {
+            'type': 'output_and_statements',
+            'message0': '%1 %2',
+            'args0': [
+              {
+                'type': 'input_statement',
+                'name': 'INPUT',
+                'check': 'check'
+              },
+              {
+                'type': 'input_statement',
+                'name': 'INPUT2',
+                'check': 'check'
+              }
+            ],
+            'output': 'check',
+          },
+          {
+            'type': 'output_and_inputs',
+            'message0': '%1 %2',
+            'args0': [
+              {
+                'type': 'input_value',
+                'name': 'INPUT',
+                'check': 'check2'
+              },
+              {
+                'type': 'input_value',
+                'name': 'INPUT2',
+                'check': 'check2'
+              }
+            ],
+            'output': 'check',
+          },
+          {
+            'type': 'check_to_check2',
+            'message0': '%1',
+            'args0': [
+              {
+                'type': 'input_value',
+                'name': 'INPUT',
+                'check': 'check2'
+              },
+            ],
+            'output': 'check',
+          },
+          {
+            'type': 'check2_to_check',
+            'message0': '%1',
+            'args0': [
+              {
+                'type': 'input_value',
+                'name': 'CHECK2TOCHECKINPUT',
+                'check': 'check'
+              },
+            ],
+            'output': 'check2',
+          },
+        ]);
+      });
+
+      test('No connection', function() {
+        const parent = this.workspace.newBlock('input');
+        const oldChild = this.workspace.newBlock('output');
+        const newChild = this.workspace.newBlock('output');
+
+        parent.getInput('INPUT').connection.connect(oldChild.outputConnection);
+        chai.assert.notExists(
+            Blockly.Connection.getConnectionForOrphanedOutput(oldChild, newChild));
+      });
+
+      test('All statements', function() {
+        const parent = this.workspace.newBlock('input');
+        const oldChild = this.workspace.newBlock('output_and_statements');
+        const newChild = this.workspace.newBlock('output');
+
+        parent.getInput('INPUT').connection.connect(oldChild.outputConnection);
+        chai.assert.notExists(
+            Blockly.Connection.getConnectionForOrphanedOutput(oldChild, newChild));
+      });
+
+      test('Bad checks', function() {
+        const parent = this.workspace.newBlock('input');
+        const oldChild = this.workspace.newBlock('output_and_inputs');
+        const newChild = this.workspace.newBlock('output');
+
+        parent.getInput('INPUT').connection.connect(oldChild.outputConnection);
+        chai.assert.notExists(
+            Blockly.Connection.getConnectionForOrphanedOutput(oldChild, newChild));
+      });
+
+      test('Through different types', function() {
+        const parent = this.workspace.newBlock('input');
+        const oldChild = this.workspace.newBlock('check_to_check2');
+        const otherChild = this.workspace.newBlock('check2_to_check');
+        const newChild = this.workspace.newBlock('output');
+
+        parent.getInput('INPUT').connection
+            .connect(oldChild.outputConnection);
+        oldChild.getInput('INPUT').connection
+            .connect(otherChild.outputConnection);
+
+        chai.assert.notExists(
+            Blockly.Connection.getConnectionForOrphanedOutput(oldChild, newChild));
+      });
+    });
+
+    suite('Multiple available spots', function() {
+      setup(function() {
+        Blockly.defineBlocksWithJsonArray([
+          {
+            'type': 'multiple_inputs',
+            'message0': '%1 %2',
+            'args0': [
+              {
+                'type': 'input_value',
+                'name': 'INPUT',
+                'check': 'check'
+              },
+              {
+                'type': 'input_value',
+                'name': 'INPUT2',
+                'check': 'check'
+              },
+            ],
+            'output': 'check',
+          },
+          {
+            'type': 'single_input',
+            'message0': '%1',
+            'args0': [
+              {
+                'type': 'input_value',
+                'name': 'INPUT',
+                'check': 'check'
+              },
+            ],
+            'output': 'check',
+          },
+        ]);
+      });
+
+      suite('No shadows', function() {
+        test('Top block', function() {
+          const parent = this.workspace.newBlock('input');
+          const oldChild = this.workspace.newBlock('multiple_inputs');
+          const newChild = this.workspace.newBlock('output');
+
+          parent.getInput('INPUT').connection.connect(oldChild.outputConnection);
+          chai.assert.notExists(
+              Blockly.Connection.getConnectionForOrphanedOutput(oldChild, newChild));
+        });
+
+        test('Child blocks', function() {
+          const parent = this.workspace.newBlock('input');
+          const oldChild = this.workspace.newBlock('multiple_inputs');
+          const childX = this.workspace.newBlock('single_input');
+          const childY = this.workspace.newBlock('single_input');
+          const newChild = this.workspace.newBlock('output');
+
+          parent.getInput('INPUT').connection.connect(oldChild.outputConnection);
+          oldChild.getInput('INPUT').connection.connect(childX.outputConnection);
+          oldChild.getInput('INPUT2').connection.connect(childY.outputConnection);
+
+          chai.assert.notExists(
+              Blockly.Connection.getConnectionForOrphanedOutput(oldChild, newChild));
+        });
+
+        test('Spots filled', function() {
+          const parent = this.workspace.newBlock('input');
+          const oldChild = this.workspace.newBlock('multiple_inputs');
+          const otherChild = this.workspace.newBlock('output');
+          const newChild = this.workspace.newBlock('output');
+
+          parent.getInput('INPUT').connection
+              .connect(oldChild.outputConnection);
+          oldChild.getInput('INPUT').connection
+              .connect(otherChild.outputConnection);
+
+          chai.assert.notExists(
+              Blockly.Connection.getConnectionForOrphanedOutput(oldChild, newChild));
+        });
+      });
+
+      suite('Shadows', function() {
+        test('Top block', function() {
+          const parent = this.workspace.newBlock('input');
+          const oldChild = this.workspace.newBlock('multiple_inputs');
+          const newChild = this.workspace.newBlock('output');
+
+          parent.getInput('INPUT').connection.connect(oldChild.outputConnection);
+          oldChild.getInput('INPUT').connection.setShadowDom(
+              Blockly.Xml.textToDom('<xml><shadow type="output"/></xml>')
+                  .firstChild);
+          oldChild.getInput('INPUT2').connection.setShadowDom(
+              Blockly.Xml.textToDom('<xml><shadow type="output"/></xml>')
+                  .firstChild);
+
+          chai.assert.notExists(
+              Blockly.Connection.getConnectionForOrphanedOutput(oldChild, newChild));
+        });
+
+        test('Child blocks', function() {
+          const parent = this.workspace.newBlock('input');
+          const oldChild = this.workspace.newBlock('multiple_inputs');
+          const childX = this.workspace.newBlock('single_input');
+          const childY = this.workspace.newBlock('single_input');
+          const newChild = this.workspace.newBlock('output');
+
+          parent.getInput('INPUT').connection.connect(oldChild.outputConnection);
+          oldChild.getInput('INPUT').connection.connect(childX.outputConnection);
+          oldChild.getInput('INPUT2').connection.connect(childY.outputConnection);
+          childX.getInput('INPUT').connection.setShadowDom(
+              Blockly.Xml.textToDom('<xml><shadow type="output"/></xml>')
+                  .firstChild);
+          childY.getInput('INPUT').connection.setShadowDom(
+              Blockly.Xml.textToDom('<xml><shadow type="output"/></xml>')
+                  .firstChild);
+
+          chai.assert.notExists(
+              Blockly.Connection.getConnectionForOrphanedOutput(oldChild, newChild));
+        });
+
+        test('Spots filled', function() {
+          const parent = this.workspace.newBlock('input');
+          const oldChild = this.workspace.newBlock('multiple_inputs');
+          const otherChild = this.workspace.newBlock('output');
+          const newChild = this.workspace.newBlock('output');
+
+          parent.getInput('INPUT').connection
+              .connect(oldChild.outputConnection);
+          oldChild.getInput('INPUT').connection
+              .connect(otherChild.outputConnection);
+          oldChild.getInput('INPUT2').connection.setShadowDom(
+              Blockly.Xml.textToDom('<xml><shadow type="output"/></xml>')
+                  .firstChild);
+
+          chai.assert.notExists(
+              Blockly.Connection.getConnectionForOrphanedOutput(oldChild, newChild));
+        });
+      });
+    });
+
+    suite('Single available spot', function() {
+      setup(function() {
+        Blockly.defineBlocksWithJsonArray([
+          {
+            'type': 'single_input',
+            'message0': '%1',
+            'args0': [
+              {
+                'type': 'input_value',
+                'name': 'INPUT',
+                'check': 'check'
+              },
+            ],
+            'output': 'check',
+          },
+        ]);
+      });
+
+      test('No shadows', function() {
+        const parent = this.workspace.newBlock('input');
+        const oldChild = this.workspace.newBlock('single_input');
+        const newChild = this.workspace.newBlock('output');
+
+        parent.getInput('INPUT').connection.connect(oldChild.outputConnection);
+
+        const result = Blockly.Connection
+            .getConnectionForOrphanedOutput(oldChild, newChild);
+        chai.assert.exists(result);
+        chai.assert.equal(result.getParentInput().name, 'INPUT');
+      });
+
+      test('Shadows', function() {
+        const parent = this.workspace.newBlock('input');
+        const oldChild = this.workspace.newBlock('single_input');
+        const newChild = this.workspace.newBlock('output');
+
+        parent.getInput('INPUT').connection.connect(oldChild.outputConnection);
+        oldChild.getInput('INPUT').connection.setShadowDom(
+            Blockly.Xml.textToDom('<xml><shadow type="output"/></xml>')
+                .firstChild);
+
+        const result = Blockly.Connection
+            .getConnectionForOrphanedOutput(oldChild, newChild);
+        chai.assert.exists(result);
+        chai.assert.equal(result.getParentInput().name, 'INPUT');
+      });
+    });
+  });
 });