From b65491e6564196e26aa744e4c37fb8af4f69e98c Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Fri, 3 Aug 2018 13:54:29 -0700 Subject: [PATCH 1/3] Heal rows when unplugging, and add tests --- core/block.js | 109 ++++++++++++++---- tests/jsunit/block_test.js | 221 ++++++++++++++++++++++++++----------- 2 files changed, 244 insertions(+), 86 deletions(-) diff --git a/core/block.js b/core/block.js index f5727690e..d3193fb74 100644 --- a/core/block.js +++ b/core/block.js @@ -314,27 +314,98 @@ Blockly.Block.prototype.initModel = function() { */ Blockly.Block.prototype.unplug = function(opt_healStack) { if (this.outputConnection) { - if (this.outputConnection.isConnected()) { - // Disconnect from any superior block. - this.outputConnection.disconnect(); - } + this.unplugFromRow_(opt_healStack); } else if (this.previousConnection) { - var previousTarget = null; - if (this.previousConnection.isConnected()) { - // Remember the connection that any next statements need to connect to. - previousTarget = this.previousConnection.targetConnection; - // Detach this block from the parent's tree. - this.previousConnection.disconnect(); - } - var nextBlock = this.getNextBlock(); - if (opt_healStack && nextBlock) { - // Disconnect the next statement. - var nextTarget = this.nextConnection.targetConnection; - nextTarget.disconnect(); - if (previousTarget && previousTarget.checkType_(nextTarget)) { - // Attach the next statement to the previous statement. - previousTarget.connect(nextTarget); + this.unplugFromStack_(opt_healStack); + } +}; + +/** + * Unplug this block's output from an input on another block. Optionally + * reconnect the block's parent to the only child block, if possible. + * @param {boolean=} opt_healStack Disconnect right-side block and connect to + * left-side block. Defaults to false. + * @private + */ +Blockly.Block.prototype.unplugFromRow_ = function(opt_healStack) { + var parentConnection = null; + if (this.outputConnection.isConnected()) { + parentConnection = this.outputConnection.targetConnection; + // Disconnect from any superior block. + this.outputConnection.disconnect(); + } + + // Return early in obvious cases. + if (!parentConnection || !opt_healStack) { + return; + } + + var parentBlock = parentConnection.getSourceBlock(); + if (parentConnection != parentBlock.getOnlyValueConnection_()) { + // Too many or too few possible connections on the parent. + return; + } + + var thisConnection = this.getOnlyValueConnection_(); + if (!thisConnection || !thisConnection.isConnected()) { + // Too many or too few possible connections on this block, or there's + // nothing on the other side of this connection. + return; + } + + // Only disconnect the child if it's possible to move it to the parent. + var childConnection = thisConnection.targetConnection; + if (childConnection.checkType_(parentConnection)) { + // Disconnect the child block. + childConnection.disconnect(); + parentConnection.connect(childConnection); + } +}; + +/** + * Returns the connection on the only value input on the block, or null if the + * number of value inputs is not one. + * @return {Blockly.Connection} The connection on the value input, or null. + * @private + */ +Blockly.Block.prototype.getOnlyValueConnection_ = function() { + var connection = false; + for (var i = 0; i < this.inputList.length; i++) { + var thisConnection = this.inputList[i].connection; + if (thisConnection && thisConnection.type == Blockly.INPUT_VALUE) { + if (connection) { + return null; // More than one value input found. } + connection = thisConnection; + } + } + return connection; +}; + +/** + * Unplug this statement block from its superior block. Optionally reconnect + * the block underneath with the block on top. + * @param {boolean=} opt_healStack Disconnect child statement and reconnect + * stack. Defaults to false. + * @private + */ +Blockly.Block.prototype.unplugFromStack_ = function(opt_healStack) { + var previousTarget = null; + if (this.previousConnection.isConnected()) { + // Remember the connection that any next statements need to connect to. + previousTarget = this.previousConnection.targetConnection; + // Detach this block from the parent's tree. + this.previousConnection.disconnect(); + } + var nextBlock = this.getNextBlock(); + if (opt_healStack && nextBlock) { + // Disconnect the next statement. + var nextTarget = this.nextConnection.targetConnection; + nextTarget.disconnect(); + // Shouldn't checkType happen earlier? + if (previousTarget && previousTarget.checkType_(nextTarget)) { + // Attach the next statement to the previous statement. + previousTarget.connect(nextTarget); } } }; diff --git a/tests/jsunit/block_test.js b/tests/jsunit/block_test.js index f2e5f6b8e..72d4931c5 100644 --- a/tests/jsunit/block_test.js +++ b/tests/jsunit/block_test.js @@ -61,26 +61,66 @@ function blockTest_tearDown() { workspace.dispose(); } +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()); +} + +function setUpRowBlocks() { + var blockA = workspace.newBlock('row_block'); + var blockB = workspace.newBlock('row_block'); + var blockC = workspace.newBlock('row_block'); + + blockA.inputList[0].connection.connect(blockB.outputConnection); + blockB.inputList[0].connection.connect(blockC.outputConnection); + + assertEquals(blockB, blockC.getParent()); + + return { + A: blockA, + B: blockB, + C: blockC + } +} + +function setUpStackBlocks() { + var blockA = workspace.newBlock('stack_block'); + var blockB = workspace.newBlock('stack_block'); + var blockC = workspace.newBlock('stack_block'); + + blockA.nextConnection.connect(blockB.previousConnection); + blockB.nextConnection.connect(blockC.previousConnection); + + assertEquals(blockB, blockC.getParent()); + + return { + A: blockA, + B: blockB, + C: blockC + } +} + + function test_block_stack_unplug_noheal() { blockTest_setUp(); try { - var blockA = workspace.newBlock('stack_block'); - var blockB = workspace.newBlock('stack_block'); - var blockC = workspace.newBlock('stack_block'); - - blockA.nextConnection.connect(blockB.previousConnection); - blockB.nextConnection.connect(blockC.previousConnection); - - assertEquals(blockB, blockC.getParent()); - - blockB.unplug(); - - // A has nothing connected to it. - assertEquals(0, blockA.getChildren().length); - // B and C are still connected. - assertEquals(blockB, blockC.getParent()); - // B is the top of its stack. - assertNull(blockB.getParent()); + var blocks = setUpStackBlocks(); + blocks.B.unplug(); + assertUnpluggedNoheal(blocks); } finally { blockTest_tearDown(); } @@ -89,24 +129,40 @@ function test_block_stack_unplug_noheal() { function test_block_stack_unplug_heal() { blockTest_setUp(); try { - var blockA = workspace.newBlock('stack_block'); - var blockB = workspace.newBlock('stack_block'); - var blockC = workspace.newBlock('stack_block'); + var blocks = setUpStackBlocks(); + blocks.B.unplug(true); + assertUnpluggedHealed(blocks); + } finally { + blockTest_tearDown(); + } +} - blockA.nextConnection.connect(blockB.previousConnection); - blockB.nextConnection.connect(blockC.previousConnection); +function test_block_stack_unplug_heal_bad_checks() { + blockTest_setUp(); + try { + var blocks = setUpStackBlocks(); - assertEquals(blockB, blockC.getParent()); + // A and C can't connect, but both can connect to B. + blocks.A.nextConnection.setCheck('type1'); + blocks.C.previousConnection.setCheck('type2'); - blockB.unplug(true); + // The types don't work. + blocks.B.unplug(true); - // A and C are connected. - assertEquals(1, blockA.getChildren().length); - assertEquals(blockA, blockC.getParent()); + // Stack blocks unplug before checking whether the types match. + // TODO (#1994): Check types before unplugging. + // A has nothing connected to it. + assertEquals(0, blocks.A.getChildren().length); // B has nothing connected to it. - assertEquals(0, blockB.getChildren().length); + 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 is the top of its stack. - assertNull(blockB.getParent()); + assertNull(blocks.B.getParent()); + // C is the top of its stack. + assertNull(blocks.C.getParent()); } finally { blockTest_tearDown(); } @@ -115,23 +171,9 @@ function test_block_stack_unplug_heal() { function test_block_row_unplug_noheal() { blockTest_setUp(); try { - var blockA = workspace.newBlock('row_block'); - var blockB = workspace.newBlock('row_block'); - var blockC = workspace.newBlock('row_block'); - - blockA.inputList[0].connection.connect(blockB.outputConnection); - blockB.inputList[0].connection.connect(blockC.outputConnection); - - assertEquals(blockB, blockC.getParent()); - - blockB.unplug(false); - - // A has nothing connected to it. - assertEquals(0, blockA.getChildren().length); - // B and C are still connected. - assertEquals(blockB, blockC.getParent()); - // B is the top of its stack. - assertNull(blockB.getParent()); + var blocks = setUpRowBlocks(); + blocks.B.unplug(false); + assertUnpluggedNoheal(blocks); } finally { blockTest_tearDown(); } @@ -140,27 +182,72 @@ function test_block_row_unplug_noheal() { function test_block_row_unplug_heal() { blockTest_setUp(); try { - var blockA = workspace.newBlock('row_block'); - var blockB = workspace.newBlock('row_block'); - var blockC = workspace.newBlock('row_block'); - - blockA.inputList[0].connection.connect(blockB.outputConnection); - blockB.inputList[0].connection.connect(blockC.outputConnection); - - assertEquals(blockB, blockC.getParent()); - - blockB.unplug(true); - - // Unplugging in a row doesn't heal the stack, regardless of the value of - // the healStack argument. These are the same asserts as - // test_block_row_unplug_noheal. - - // A has nothing connected to it. - assertEquals(0, blockA.getChildren().length); - // B and C are still connected. - assertEquals(blockB, blockC.getParent()); - // B is the top of its stack. - assertNull(blockB.getParent()); + var blocks = setUpRowBlocks(); + // Each block has only one input, and the types work. + blocks.B.unplug(true); + assertUnpluggedHealed(blocks); + } finally { + blockTest_tearDown(); + } +} + +function test_block_row_unplug_heal_bad_checks() { + blockTest_setUp(); + try { + var blocks = setUpRowBlocks(); + + // 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); + } finally { + blockTest_tearDown(); + } +} + +function test_block_row_unplug_multi_inputs_parent() { + blockTest_setUp(); + try { + var blocks = setUpRowBlocks(); + // Add extra input to parent + blocks.A.appendValueInput("INPUT").setCheck(null); + + // Parent block has multiple inputs. + blocks.B.unplug(true); + assertUnpluggedNoheal(blocks); + } finally { + blockTest_tearDown(); + } +} + +function test_block_row_unplug_multi_inputs_middle() { + blockTest_setUp(); + try { + var blocks = setUpRowBlocks(); + // Add extra input to middle block + blocks.B.appendValueInput("INPUT").setCheck(null); + + // Middle block has multiple inputs. + blocks.B.unplug(true); + assertUnpluggedNoheal(blocks); + } finally { + blockTest_tearDown(); + } +} + +function test_block_row_unplug_multi_inputs_child() { + blockTest_setUp(); + try { + var blocks = setUpRowBlocks(); + // 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); } finally { blockTest_tearDown(); } From a8d52903e274c7504040d2c54b8298698a5031c2 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Mon, 6 Aug 2018 10:29:12 -0700 Subject: [PATCH 2/3] update comment with todo --- core/block.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/block.js b/core/block.js index d3193fb74..4e7f65299 100644 --- a/core/block.js +++ b/core/block.js @@ -402,7 +402,7 @@ Blockly.Block.prototype.unplugFromStack_ = function(opt_healStack) { // Disconnect the next statement. var nextTarget = this.nextConnection.targetConnection; nextTarget.disconnect(); - // Shouldn't checkType happen earlier? + // TODO (#1994): Check types before unplugging. if (previousTarget && previousTarget.checkType_(nextTarget)) { // Attach the next statement to the previous statement. previousTarget.connect(nextTarget); From b89487e332986d72628b138cff36792f853e1521 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Mon, 6 Aug 2018 11:18:00 -0700 Subject: [PATCH 3/3] Ignore whether parent has multiple inputs --- core/block.js | 6 ------ tests/jsunit/block_test.js | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/core/block.js b/core/block.js index 4e7f65299..d73336697 100644 --- a/core/block.js +++ b/core/block.js @@ -340,12 +340,6 @@ Blockly.Block.prototype.unplugFromRow_ = function(opt_healStack) { return; } - var parentBlock = parentConnection.getSourceBlock(); - if (parentConnection != parentBlock.getOnlyValueConnection_()) { - // Too many or too few possible connections on the parent. - return; - } - var thisConnection = this.getOnlyValueConnection_(); if (!thisConnection || !thisConnection.isConnected()) { // Too many or too few possible connections on this block, or there's diff --git a/tests/jsunit/block_test.js b/tests/jsunit/block_test.js index 72d4931c5..d7efed435 100644 --- a/tests/jsunit/block_test.js +++ b/tests/jsunit/block_test.js @@ -217,7 +217,7 @@ function test_block_row_unplug_multi_inputs_parent() { // Parent block has multiple inputs. blocks.B.unplug(true); - assertUnpluggedNoheal(blocks); + assertUnpluggedHealed(blocks); } finally { blockTest_tearDown(); }