From e24f3cef9bcb89bad4fb3a0deb59118b558140cb Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Tue, 14 Jul 2020 16:57:29 -0600 Subject: [PATCH] Add isDragging to canConnectWithReason and delete newly unused code paths. --- blocks/logic.js | 10 ++-- core/block.js | 5 +- core/connection.js | 23 ++++---- core/connection_checks.js | 93 ++++++++----------------------- core/connection_db.js | 10 +++- core/keyboard_nav/navigation.js | 18 ++---- core/rendered_connection.js | 2 +- core/renderers/common/renderer.js | 2 +- tests/mocha/connection_test.js | 8 +-- 9 files changed, 63 insertions(+), 108 deletions(-) diff --git a/blocks/logic.js b/blocks/logic.js index 6cea9d1d4..bc46c82d6 100644 --- a/blocks/logic.js +++ b/blocks/logic.js @@ -543,9 +543,8 @@ Blockly.Constants.Logic.LOGIC_COMPARE_ONCHANGE_MIXIN = { var blockB = this.getInputTargetBlock('B'); // Disconnect blocks that existed prior to this change if they don't match. if (blockA && blockB && - //!this.workspace.connectionTypeChecker.checkType( - //blockA.outputConnection, blockB.outputConnection)) { - !blockA.outputConnection.checkType(blockB.outputConnection)) { + !this.workspace.connectionTypeChecker.doTypeChecks( + blockA.outputConnection, blockB.outputConnection)) { // Mismatch between two inputs. Revert the block connections, // bumping away the newly connected block(s). Blockly.Events.setGroup(e.group); @@ -613,9 +612,8 @@ Blockly.Constants.Logic.LOGIC_TERNARY_ONCHANGE_MIXIN = { for (var i = 0; i < 2; i++) { var block = (i == 1) ? blockA : blockB; if (block && - // !block.workspace.connectionTypeChecker.checkType( - // block.outputConnection, parentConnection)) { - !block.outputConnection.checkType(parentConnection)) { + !block.workspace.connectionTypeChecker.doTypeChecks( + block.outputConnection, parentConnection)) { // Ensure that any disconnections are grouped with the causing event. Blockly.Events.setGroup(e.group); if (parentConnection === this.prevParentConnection_) { diff --git a/core/block.js b/core/block.js index 56710b7a3..9aabe7edb 100644 --- a/core/block.js +++ b/core/block.js @@ -32,6 +32,7 @@ goog.require('Blockly.utils.string'); goog.require('Blockly.Workspace'); goog.requireType('Blockly.IASTNodeLocation'); +goog.requireType('Blockly.connectionTypeChecker'); /** @@ -457,7 +458,7 @@ Blockly.Block.prototype.unplugFromRow_ = function(opt_healStack) { childConnection.disconnect(); // Connect child to the parent if possible, otherwise bump away. if (this.workspace.connectionTypeChecker.canConnect( - childConnection, parentConnection, false, false)) { + childConnection, parentConnection, false)) { parentConnection.connect(childConnection); } else { childConnection.onFailedConnect(parentConnection); @@ -511,7 +512,7 @@ Blockly.Block.prototype.unplugFromStack_ = function(opt_healStack) { nextTarget.disconnect(); if (previousTarget && this.workspace.connectionTypeChecker.canConnect( - previousTarget, nextTarget, false, false)) { + previousTarget, nextTarget, false)) { // Attach the next statement to the previous statement. previousTarget.connect(nextTarget); } diff --git a/core/connection.js b/core/connection.js index d989611ac..92e578d11 100644 --- a/core/connection.js +++ b/core/connection.js @@ -149,8 +149,7 @@ Blockly.Connection.prototype.connect_ = function(childConnection) { } else { var typeChecker = orphanBlock.workspace.connectionTypeChecker; if (typeChecker.canConnect( - orphanBlock.previousConnection, newBlock.nextConnection, - false, false)) { + orphanBlock.previousConnection, newBlock.nextConnection, false)) { newBlock.nextConnection.connect(orphanBlock.previousConnection); orphanBlock = null; } @@ -253,7 +252,8 @@ Blockly.Connection.prototype.isConnected = function() { */ Blockly.Connection.prototype.canConnectWithReason = function(target) { // TODO: deprecation warning with date, plus tests. - return this.getConnectionTypeChecker().canConnectWithReason(this, target); + return this.getConnectionTypeChecker().canConnectWithReason( + this, target); }; /** @@ -268,7 +268,10 @@ Blockly.Connection.prototype.checkConnection = function(target) { // TODO: Add deprecation warning notices *and* add tests to make sure these // still work (for any blocks that use them). var checker = this.getConnectionTypeChecker(); - checker.canConnect(this, target, false, true); + var reason = !checker.canConnectWithReason(this, target, false); + if (reason != Blockly.Connection.CAN_CONNECT) { + throw new Error(checker.getErrorMessage(this, target, reason)); + } }; /** @@ -288,7 +291,7 @@ Blockly.Connection.prototype.getConnectionTypeChecker = function() { * @deprecated July 2020 */ Blockly.Connection.prototype.isConnectionAllowed = function(candidate) { - return this.getConnectionTypeChecker().canConnect(this, candidate, true, false); + return this.getConnectionTypeChecker().canConnect(this, candidate, true); }; /** @@ -312,8 +315,7 @@ Blockly.Connection.prototype.connect = function(otherConnection) { } var checker = this.getConnectionTypeChecker(); - // TODO (fenichel): Try to get rid of the extra parameter (shouldThrow). - if (checker.canConnect(this, otherConnection, false, true)) { + if (checker.canConnect(this, otherConnection, false)) { var eventGroup = Blockly.Events.getGroup(); if (!eventGroup) { Blockly.Events.setGroup(true); @@ -362,7 +364,7 @@ Blockly.Connection.singleConnection_ = function(block, orphanBlock) { var thisConnection = block.inputList[i].connection; var typeChecker = output.getConnectionTypeChecker(); if (thisConnection && thisConnection.type == Blockly.INPUT_VALUE && - typeChecker.canConnect(output, thisConnection, false, false)) { + typeChecker.canConnect(output, thisConnection, false)) { if (connection) { return null; // More than one connection. } @@ -492,7 +494,8 @@ Blockly.Connection.prototype.targetBlock = function() { * @return {boolean} True if the connections share a type. */ Blockly.Connection.prototype.checkType = function(otherConnection) { - return this.getConnectionTypeChecker().canConnect(this, otherConnection, false, false); + return this.getConnectionTypeChecker().canConnect(this, otherConnection, + false); }; /** @@ -518,7 +521,7 @@ Blockly.Connection.prototype.onCheckChanged_ = function() { // The new value type may not be compatible with the existing connection. if (this.isConnected() && (!this.targetConnection || !this.getConnectionTypeChecker().canConnect( - this, this.targetConnection, false, false))) { + this, this.targetConnection, false))) { var child = this.isSuperior() ? this.targetBlock() : this.sourceBlock_; child.unplug(); } diff --git a/core/connection_checks.js b/core/connection_checks.js index 1b456ddbd..8b52697da 100644 --- a/core/connection_checks.js +++ b/core/connection_checks.js @@ -18,68 +18,13 @@ Blockly.ConnectionTypeChecker = function() { * @param {Blockly.Connection} two Connection to check compatibility with. * @param {boolean} isDragging True if the connection is being made by dragging * a block. - * @param {boolean} shouldThrow Whether to throw an error when a connection is - * invalid. * @return {boolean} Whether the connection is legal. + * @public */ Blockly.ConnectionTypeChecker.prototype.canConnect = function(one, two, - isDragging, shouldThrow) { - if (this.passesSafetyChecks(one, two, shouldThrow)) { - var connOne = /** @type {!Blockly.Connection} **/ (one); - var connTwo = /** @type {!Blockly.Connection} **/ (two); - if (this.passesTypeChecks(connOne, connTwo, shouldThrow)) { - if (!isDragging || this.passesDragChecks(connOne, connTwo, shouldThrow)) { - return true; - } - } - } - return false; -}; - -/** - * Check that connecting the given connections is safe, meaning that it would - * not break any of Blockly's basic assumptions--no self connections, etc. - * @param {Blockly.Connection} one The first of the connections to check. - * @param {Blockly.Connection} two The second of the connections to check. - * @param {boolean} shouldThrow Whether to throw an error if the connection is - * unsafe. - * @return {boolean} Whether the connection is safe. - * @package - */ -Blockly.ConnectionTypeChecker.prototype.passesSafetyChecks = function(one, two, shouldThrow) { - var safety = this.doSafetyChecks_(one, two); - if (safety == Blockly.Connection.CAN_CONNECT) { - return true; - } - if (shouldThrow) { - throw Error(this.getErrorMessage_(safety, one, two)); - } - return false; -}; - - -Blockly.ConnectionTypeChecker.prototype.passesTypeChecks = function(one, two, - shouldThrow) { - if (this.doTypeChecks_(one, two)) { - return true; - } - if (shouldThrow) { - throw Error(this.getErrorMessage_( - Blockly.Connection.REASON_CHECKS_FAILED, one, two)); - } - return false; -}; - -Blockly.ConnectionTypeChecker.prototype.passesDragChecks = function(one, two, - shouldThrow) { - if (this.doDragChecks_(one, two)) { - return true; - } - if (shouldThrow) { - throw Error(this.getErrorMessage_( - Blockly.Connection.REASON_DRAG_CHECKS_FAILED, one, two)); - } - return false; + isDragging) { + return this.canConnectWithReason(one, two, isDragging) == + Blockly.Connection.CAN_CONNECT; }; /** @@ -87,20 +32,28 @@ Blockly.ConnectionTypeChecker.prototype.passesDragChecks = function(one, two, * connection. * @param {Blockly.Connection} one Connection to check compatibility with. * @param {Blockly.Connection} two Connection to check compatibility with. + * @param {boolean} isDragging [description] * @return {number} Blockly.Connection.CAN_CONNECT if the connection is legal, * an error code otherwise. + * @public */ -Blockly.ConnectionTypeChecker.prototype.canConnectWithReason = function(one, two) { - var safety = this.doSafetyChecks_(one, two); +Blockly.ConnectionTypeChecker.prototype.canConnectWithReason = function( + one, two, isDragging) { + var safety = this.doSafetyChecks(one, two); if (safety != Blockly.Connection.CAN_CONNECT) { return safety; } var connOne = /** @type {!Blockly.Connection} **/ (one); var connTwo = /** @type {!Blockly.Connection} **/ (two); - if (!this.doTypeChecks_(connOne, connTwo)) { + if (!this.doTypeChecks(connOne, connTwo)) { return Blockly.Connection.REASON_CHECKS_FAILED; } + + if (isDragging && this.doDragChecks(connOne, connTwo, false)) { + return Blockly.REASON_DRAG_CHECKS_FAILED; + } + return Blockly.Connection.CAN_CONNECT; }; @@ -111,9 +64,9 @@ Blockly.ConnectionTypeChecker.prototype.canConnectWithReason = function(one, two * @param {Blockly.Connection} two The second of the two connections being * checked. * @return {string} A developer-readable error string. - * @private + * @public */ -Blockly.ConnectionTypeChecker.prototype.getErrorMessage_ = function(errorCode, +Blockly.ConnectionTypeChecker.prototype.getErrorMessage = function(errorCode, one, two) { switch (errorCode) { case Blockly.Connection.REASON_SELF_CONNECTION: @@ -144,9 +97,9 @@ Blockly.ConnectionTypeChecker.prototype.getErrorMessage_ = function(errorCode, * @param {Blockly.Connection} one The first of the connections to check. * @param {Blockly.Connection} two The second of the connections to check. * @return {number} An enum with the reason this connection is safe or unsafe. - * @private + * @public */ -Blockly.ConnectionTypeChecker.prototype.doSafetyChecks_ = function(one, two) { +Blockly.ConnectionTypeChecker.prototype.doSafetyChecks = function(one, two) { if (!one || !two) { return Blockly.Connection.REASON_TARGET_NULL; } @@ -176,9 +129,9 @@ Blockly.ConnectionTypeChecker.prototype.doSafetyChecks_ = function(one, two) { * @param {!Blockly.Connection} one Connection to compare. * @param {!Blockly.Connection} two Connection to compare against. * @return {boolean} True if the connections share a type. - * @protected + * @public */ -Blockly.ConnectionTypeChecker.prototype.doTypeChecks_ = function(one, two) { +Blockly.ConnectionTypeChecker.prototype.doTypeChecks = function(one, two) { var checkArrayOne = one.getCheck(); var checkArrayTwo = two.getCheck(); @@ -201,9 +154,9 @@ Blockly.ConnectionTypeChecker.prototype.doTypeChecks_ = function(one, two) { * @param {!Blockly.Connection} one Connection to compare. * @param {!Blockly.Connection} two Connection to compare against. * @return {boolean} True if the connections share a type. - * @protected + * @public */ -Blockly.ConnectionTypeChecker.prototype.doDragChecks_ = function(one, two) { +Blockly.ConnectionTypeChecker.prototype.doDragChecks = function(one, two) { // Don't consider insertion markers. if (two.getSourceBlock().isInsertionMarker()) { return false; diff --git a/core/connection_db.js b/core/connection_db.js index ff31704a2..c45163509 100644 --- a/core/connection_db.js +++ b/core/connection_db.js @@ -23,6 +23,9 @@ goog.requireType('Blockly.ConnectionTypeChecker'); * Database of connections. * Connections are stored in order of their vertical component. This way * connections in an area may be looked up quickly using a binary search. + * @param {!Blockly.ConnectionTypeChecker} typeChecker The workspace's + * connection type checker, used to decide if connections are valid during a + * drag. * @constructor */ Blockly.ConnectionDB = function(typeChecker) { @@ -246,7 +249,7 @@ Blockly.ConnectionDB.prototype.searchForClosest = function(conn, maxRadius, temp = this.connections_[pointerMin]; curDistance = temp.distanceFrom(conn); if (curDistance <= bestRadius && - this.typeChecker_.canConnect(conn, temp, true, false)) { + this.typeChecker_.canConnect(conn, temp, true)) { bestConnection = temp; bestRadius = curDistance; } @@ -259,7 +262,7 @@ Blockly.ConnectionDB.prototype.searchForClosest = function(conn, maxRadius, temp = this.connections_[pointerMax]; curDistance = temp.distanceFrom(conn); if (curDistance <= bestRadius && - this.typeChecker_.canConnect(conn, temp, true, false)) { + this.typeChecker_.canConnect(conn, temp, true)) { bestConnection = temp; bestRadius = curDistance; } @@ -276,6 +279,9 @@ Blockly.ConnectionDB.prototype.searchForClosest = function(conn, maxRadius, /** * Initialize a set of connection DBs for a workspace. + * @param {!Blockly.ConnectionTypeChecker} typeChecker The workspace's + * connection type checker, used to decide if connections are valid during a + * drag. * @return {!Array.} Array of databases. */ Blockly.ConnectionDB.init = function(typeChecker) { diff --git a/core/keyboard_nav/navigation.js b/core/keyboard_nav/navigation.js index 985a854bf..33b68cfc4 100644 --- a/core/keyboard_nav/navigation.js +++ b/core/keyboard_nav/navigation.js @@ -413,7 +413,7 @@ Blockly.navigation.moveAndConnect_ = function(movingConnection, destConnection) var checker = movingConnection.getConnectionTypeChecker(); - if (checker.canConnect(movingConnection, destConnection, false, false)) { + if (checker.canConnect(movingConnection, destConnection, false)) { Blockly.navigation.disconnectChild_(movingConnection, destConnection); if (!destConnection.isSuperior()) { @@ -501,17 +501,11 @@ Blockly.navigation.connect_ = function(movingConnection, destConnection) { } else if (Blockly.navigation.moveAndConnect_(movingConnection, destConnection)){ return true; } else { - try { - // TODO (fenichel): Don't rely on canConnect throwing an error. - // TODO (fenichel): Fix associated tests. - var checker = movingConnection.getConnectionTypeChecker(); - checker.canConnect(movingConnection, destConnection, false, true); - } - catch (e) { - // If nothing worked there should be an error. - // Report the error from the original connections. - Blockly.navigation.warn_('Connection failed with error: ' + e); - } + var checker = movingConnection.getConnectionTypeChecker(); + var reason = checker.canConnectWithReason( + movingConnection, destConnection, false); + Blockly.navigation.warn_('Connection failed with error: ' + + checker.getErrorMessage(movingConnection, destConnection, reason)); return false; } }; diff --git a/core/rendered_connection.js b/core/rendered_connection.js index ea8e2868e..e450db3dd 100644 --- a/core/rendered_connection.js +++ b/core/rendered_connection.js @@ -553,7 +553,7 @@ Blockly.RenderedConnection.prototype.onCheckChanged_ = function() { // The new value type may not be compatible with the existing connection. if (this.isConnected() && (!this.targetConnection || !this.getConnectionTypeChecker().canConnect( - this, this.targetConnection, false, false))) { + this, this.targetConnection, false))) { var child = this.isSuperior() ? this.targetBlock() : this.sourceBlock_; child.unplug(); // Bump away. diff --git a/core/renderers/common/renderer.js b/core/renderers/common/renderer.js index f0b158c0e..2807ec8c5 100644 --- a/core/renderers/common/renderer.js +++ b/core/renderers/common/renderer.js @@ -255,7 +255,7 @@ Blockly.blockRendering.Renderer.prototype.orphanCanConnectAtEnd = return false; } return orphanConnection.getConnectionTypeChecker().canConnect( - lastConnection, orphanConnection, false, false); + lastConnection, orphanConnection, false); }; /** diff --git a/tests/mocha/connection_test.js b/tests/mocha/connection_test.js index dd8627943..f105bcbe2 100644 --- a/tests/mocha/connection_test.js +++ b/tests/mocha/connection_test.js @@ -8,7 +8,7 @@ suite('Connection type checker', function() { suiteSetup(function() { this.checker = new Blockly.ConnectionTypeChecker(); }); - suite('Can Connect With Reason', function() { + suite('Safety checks', function() { function assertReasonHelper(checker, one, two, reason) { chai.assert.equal(checker.canConnectWithReason(one, two), reason); // Order should not matter. @@ -229,9 +229,9 @@ suite('Connection type checker', function() { this.con2 = new Blockly.Connection({}, Blockly.NEXT_STATEMENT); }); function assertCheckTypes(checker, one, two) { - chai.assert.isTrue(checker.passesTypeChecks(one, two)); + chai.assert.isTrue(checker.doTypeChecks(one, two)); // Order should not matter. - chai.assert.isTrue(checker.passesTypeChecks(one, two)); + chai.assert.isTrue(checker.doTypeChecks(one, two)); } test('No Types', function() { assertCheckTypes(this.checker, this.con1, this.con2); @@ -258,7 +258,7 @@ suite('Connection type checker', function() { test('No Compatible Types', function() { this.con1.setCheck('type1'); this.con2.setCheck('type2'); - chai.assert.isFalse(this.checker.passesTypeChecks(this.con1, this.con2)); + chai.assert.isFalse(this.checker.doTypeChecks(this.con1, this.con2)); }); }); });