From de1b3214bed3aeda9435999b2df02c763dc0eb04 Mon Sep 17 00:00:00 2001 From: jschanker Date: Tue, 13 Jul 2021 16:56:42 -0400 Subject: [PATCH] Enforce connection preconditions for setParent (#4999) * Fix error conditions for setParent #4989 * Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list. * Commented out error for when calling `a.setParent(null)` if a is connected to superior block. * Adjusted comment to reflect that blocks were no longer being disconnected in this method. * Also changed == to === per #4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`). * Fix error conditions for setParent #4989 * Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list. * Error now thrown when calling `a.setParent(null)` if a is connected to a superior block. * Adjusted comment to reflect that blocks were no longer being disconnected in this method. * Also changed == to === per #4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`). * Fix error conditions for setParent #4989 * Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list. * Commented out error for when calling `a.setParent(null)` if a is connected to superior block. * Adjusted comment to reflect that blocks were no longer being disconnected in this method. * Also changed == to === per google#4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`). * Fixed lint errors. * Fix error conditions for setParent google#4989 * Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list. * Commented out error for when calling `a.setParent(null)` if a is connected to superior block. * Adjusted comment to reflect that blocks were no longer being disconnected in this method. * Also changed == to === per google#4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`). * Fixed lint errors. * Adjusted comment. * Removed unnecessary set to null/added tests * One is failing (commented out), will investigate later * Lint fix * Removed failing test that correctly fails * Update comments to conform to style guide Capitalize first letter, period at end --- core/block.js | 30 ++++++++----- tests/mocha/block_test.js | 95 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 11 deletions(-) diff --git a/core/block.js b/core/block.js index b5229edc5..cfeaddebd 100644 --- a/core/block.js +++ b/core/block.js @@ -742,25 +742,33 @@ Blockly.Block.prototype.getChildren = function(ordered) { * @package */ Blockly.Block.prototype.setParent = function(newParent) { - if (newParent == this.parentBlock_) { + if (newParent === this.parentBlock_) { return; } + + // Check that block is connected to new parent if new parent is not null and + // that block is not connected to superior one if new parent is null. + var connection = this.previousConnection || this.outputConnection; + var isConnected = !!(connection && connection.targetBlock()); + + if (isConnected && newParent && connection.targetBlock() !== newParent) { + throw Error('Block connected to superior one that is not new parent.'); + } else if (!isConnected && newParent) { + throw Error('Block not connected to new parent.'); + } else if (isConnected && !newParent) { + throw Error('Cannot set parent to null while block is still connected to' + + ' superior block.'); + } + if (this.parentBlock_) { // Remove this block from the old parent's child list. Blockly.utils.arrayRemove(this.parentBlock_.childBlocks_, this); - // Disconnect from superior blocks. - if (this.previousConnection && this.previousConnection.isConnected()) { - throw Error('Still connected to previous block.'); - } - if (this.outputConnection && this.outputConnection.isConnected()) { - throw Error('Still connected to parent block.'); - } - this.parentBlock_ = null; // This block hasn't actually moved on-screen, so there's no need to update - // its connection locations. + // its connection locations. } else { - // Remove this block from the workspace's list of top-most blocks. + // New parent must be non-null so remove this block from the workspace's + // list of top-most blocks. this.workspace.removeTopBlock(this); } diff --git a/tests/mocha/block_test.js b/tests/mocha/block_test.js index cf602a386..7c841a134 100644 --- a/tests/mocha/block_test.js +++ b/tests/mocha/block_test.js @@ -890,6 +890,96 @@ suite('Blocks', function() { chai.assert.equal(this.getNext().length, 6); }); }); + suite('Setting Parent Block', function() { + setup(function() { + this.printBlock = Blockly.Xml.domToBlock(Blockly.Xml.textToDom( + '' + + ' ' + + ' ' + + ' ' + + ' ' + + ' ' + + ' ' + + ' ' + + ' ' + + ' ' + + '' + ), this.workspace); + this.textJoinBlock = this.printBlock.getInputTargetBlock('TEXT'); + this.textBlock = this.textJoinBlock.getInputTargetBlock('ADD0'); + }); + + function assertBlockIsOnlyChild(parent, child, inputName) { + chai.assert.equal(parent.getChildren().length, 1); + chai.assert.equal(parent.getInputTargetBlock(inputName), child); + chai.assert.equal(child.getParent(), parent); + } + function assertNonParentAndOrphan(nonParent, orphan, inputName) { + chai.assert.equal(nonParent.getChildren().length, 0); + chai.assert.isNull(nonParent.getInputTargetBlock('TEXT')); + chai.assert.isNull(orphan.getParent()); + } + function assertOriginalSetup() { + assertBlockIsOnlyChild(this.printBlock, this.textJoinBlock, 'TEXT'); + assertBlockIsOnlyChild(this.textJoinBlock, this.textBlock, 'ADD0'); + } + + test('Setting to connected parent', function() { + chai.assert.doesNotThrow(this.textJoinBlock.setParent + .bind(this.textJoinBlock, this.printBlock)); + assertOriginalSetup.call(this); + }); + test('Setting to new parent after connecting to it', function() { + this.textJoinBlock.outputConnection.disconnect(); + this.textBlock.outputConnection + .connect(this.printBlock.getInput('TEXT').connection); + chai.assert.doesNotThrow(this.textBlock.setParent + .bind(this.textBlock, this.printBlock)); + assertBlockIsOnlyChild(this.printBlock, this.textBlock, 'TEXT'); + }); + test('Setting to new parent while connected to other block', function() { + // Setting to grandparent with no available input connection. + chai.assert.throws(this.textBlock.setParent + .bind(this.textBlock, this.printBlock)); + this.textJoinBlock.outputConnection.disconnect(); + // Setting to block with available input connection. + chai.assert.throws(this.textBlock.setParent + .bind(this.textBlock, this.printBlock)); + assertNonParentAndOrphan(this.printBlock, this.textJoinBlock, 'TEXT'); + assertBlockIsOnlyChild(this.textJoinBlock, this.textBlock, 'ADD0'); + }); + test('Setting to same parent after disconnecting from it', function() { + this.textJoinBlock.outputConnection.disconnect(); + chai.assert.throws(this.textJoinBlock.setParent + .bind(this.textJoinBlock, this.printBlock)); + assertNonParentAndOrphan(this.printBlock, this.textJoinBlock, 'TEXT'); + }); + test('Setting to new parent when orphan', function() { + this.textBlock.outputConnection.disconnect(); + // When new parent has no available input connection. + chai.assert.throws(this.textBlock.setParent + .bind(this.textBlock, this.printBlock)); + this.textJoinBlock.outputConnection.disconnect(); + // When new parent has available input connection. + chai.assert.throws(this.textBlock.setParent + .bind(this.textBlock, this.printBlock)); + + assertNonParentAndOrphan(this.printBlock, this.textJoinBlock, 'TEXT'); + assertNonParentAndOrphan(this.printBlock, this.textBlock, 'TEXT'); + assertNonParentAndOrphan(this.textJoinBlock, this.textBlock, 'ADD0'); + }); + test('Setting parent to null after disconnecting', function() { + this.textBlock.outputConnection.disconnect(); + chai.assert.doesNotThrow(this.textBlock.setParent + .bind(this.textBlock, null)); + assertNonParentAndOrphan(this.textJoinBlock, this.textBlock, 'ADD0'); + }); + test('Setting parent to null without disconnecting', function() { + chai.assert.throws(this.textBlock.setParent + .bind(this.textBlock, null)); + assertOriginalSetup.call(this); + }); + }); suite('Remove Connections Programmatically', function() { test('Output', function() { var block = createRenderedBlock(this.workspace, 'row_block'); @@ -1106,11 +1196,16 @@ suite('Blocks', function() { }); suite('Getting/Setting Field (Values)', function() { setup(function() { + this.workspace = Blockly.inject('blocklyDiv'); this.block = Blockly.Xml.domToBlock(Blockly.Xml.textToDom( 'test' ), this.workspace); }); + teardown(function() { + workspaceTeardown.call(this, this.workspace); + }); + test('Getting Field', function() { chai.assert.instanceOf(this.block.getField('TEXT'), Blockly.Field); });