From 7314bdfb3a5fa5026ffe055c928e0f772ad627f2 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 17 Jan 2018 16:50:14 -0800 Subject: [PATCH 1/2] Fix #1499 and add some validation --- core/block.js | 3 +- core/field_variable.js | 51 +++++++++++++++++++++++++---- tests/jsunit/field_variable_test.js | 37 ++++++++++++++++++++- 3 files changed, 82 insertions(+), 9 deletions(-) diff --git a/core/block.js b/core/block.js index 536bed2ff..4783c0dea 100644 --- a/core/block.js +++ b/core/block.js @@ -1340,7 +1340,8 @@ Blockly.Block.newFieldTextInputFromJson_ = function(options) { Blockly.Block.newFieldVariableFromJson_ = function(options) { var varname = Blockly.utils.replaceMessageReferences(options['variable']); var variableTypes = options['variableTypes']; - return new Blockly.FieldVariable(varname, null, variableTypes); + var defaultType = options['defaultType']; + return new Blockly.FieldVariable(varname, null, variableTypes, defaultType); }; diff --git a/core/field_variable.js b/core/field_variable.js index 53883e56a..c9ff00046 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -30,7 +30,6 @@ goog.require('Blockly.FieldDropdown'); goog.require('Blockly.Msg'); goog.require('Blockly.VariableModel'); goog.require('Blockly.Variables'); -goog.require('Blockly.VariableModel'); goog.require('goog.asserts'); goog.require('goog.string'); @@ -43,24 +42,25 @@ goog.require('goog.string'); * option is selected. Its sole argument is the new option value. * @param {Array.=} opt_variableTypes A list of the types of variables * to include in the dropdown. + * @param {string=} opt_defaultType The type of variable to create if this + * field's value is not explicitly set. Defaults to ''. * @extends {Blockly.FieldDropdown} * @constructor */ -Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes) { +Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes, + opt_defaultType) { // The FieldDropdown constructor would call setValue, which might create a // spurious variable. Just do the relevant parts of the constructor. this.menuGenerator_ = Blockly.FieldVariable.dropdownCreate; this.size_ = new goog.math.Size(0, Blockly.BlockSvg.MIN_BLOCK_Y); this.setValidator(opt_validator); - // TODO (#1499): Add opt_default_type to match default value. If not set, ''. this.defaultVariableName = (varname || ''); - this.defaultType_ = ''; - this.variableTypes = opt_variableTypes; + + this.setTypes_(opt_variableTypes, opt_defaultType); this.value_ = null; }; goog.inherits(Blockly.FieldVariable, Blockly.FieldDropdown); - /** * Initialize everything needed to render this field. This includes making sure * that the field's value is valid. @@ -207,7 +207,7 @@ Blockly.FieldVariable.prototype.typeIsAllowed_ = function(type) { Blockly.FieldVariable.prototype.getVariableTypes_ = function() { // TODO (#1513): Try to avoid calling this every time the field is edited. var variableTypes = this.variableTypes; - if (variableTypes === null || variableTypes === undefined) { + if (variableTypes === null) { // If variableTypes is null, return all variable types. if (this.sourceBlock_) { var workspace = this.sourceBlock_.workspace; @@ -224,6 +224,43 @@ Blockly.FieldVariable.prototype.getVariableTypes_ = function() { return variableTypes; }; +/** + * Parse the optional arguments representing the allowed variable types and the + * default variable type. + * @param {Array.=} opt_variableTypes A list of the types of variables + * to include in the dropdown. If null or undefined, variables of all types + * will be displayed in the dropdown. + * @param {string=} opt_defaultType The type of the variable to create if this + * field's value is not explicitly set. Defaults to ''. + * @private + */ +Blockly.FieldVariable.prototype.setTypes_ = function(opt_variableTypes, + opt_defaultType) { + // If you expected that the default type would be the same as the only entry + // in the variable types array, tell the Blockly team by commenting on #1499. + this.defaultType_ = opt_defaultType || ''; + // Set the allowable variable types. Null means all types on the workspace. + if (opt_variableTypes == null || opt_variableTypes == undefined) { + this.variableTypes = null; + } else if (Array.isArray(opt_variableTypes)) { + this.variableTypes = opt_variableTypes; + // Make sure the default type is valid. + var isInArray = false; + for (var i = 0; i < this.variableTypes.length; i++) { + if (this.variableTypes[i] == this.defaultType_) { + isInArray = true; + } + } + if (!isInArray) { + throw new Error('Invalid default type \'' + this.defaultType_ + '\' in ' + + 'the definition of a FieldVariable'); + } + } else { + throw new Error('\'variableTypes\' was not an array in the definition of ' + + 'a FieldVariable'); + } +}; + /** * Return a sorted list of variable names for variable dropdown menus. * Include a special option at the end for creating a new variable name. diff --git a/tests/jsunit/field_variable_test.js b/tests/jsunit/field_variable_test.js index bc36b7d69..3f8785cb9 100644 --- a/tests/jsunit/field_variable_test.js +++ b/tests/jsunit/field_variable_test.js @@ -155,9 +155,10 @@ function test_fieldVariable_getVariableTypes_givenVariableTypes() { workspace.createVariable('name2', 'type2'); var fieldVariable = new Blockly.FieldVariable( - 'name1', null, ['type1', 'type2']); + 'name1', null, ['type1', 'type2'], 'type1'); var resultTypes = fieldVariable.getVariableTypes_(); isEqualArrays(resultTypes, ['type1', 'type2']); + assertEquals('Default type was wrong', 'type1', fieldVariable.defaultType_); workspace.dispose(); } @@ -200,3 +201,37 @@ function test_fieldVariable_getVariableTypes_emptyListVariableTypes() { workspace.dispose(); } } + +function test_fieldVariable_defaultType_exists() { + var fieldVariable = new Blockly.FieldVariable(null, null, ['b'], 'b'); + assertEquals('The variable field\'s default type should be "b"', + 'b', fieldVariable.defaultType_); +} + +function test_fieldVariable_noDefaultType() { + var fieldVariable = new Blockly.FieldVariable(null); + assertEquals('The variable field\'s default type should be the empty string', + '', fieldVariable.defaultType_); + assertNull('The variable field\'s allowed types should be null', + fieldVariable.variableTypes); +} + +function test_fieldVariable_defaultTypeMismatch() { + try { + var fieldVariable = new Blockly.FieldVariable(null, null, ['a'], 'b'); + fail('Variable field creation should have failed due to an invalid ' + + 'default type'); + } catch (e) { + // expected + } +} + +function test_fieldVariable_defaultTypeMismatch_empty() { + try { + var fieldVariable = new Blockly.FieldVariable(null, null, ['a']); + fail('Variable field creation should have failed due to an invalid ' + + 'default type'); + } catch (e) { + // expected + } +} From 06ae9ade8e0ad23c7e0ca0138b7e7300768e5d57 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Fri, 19 Jan 2018 15:49:39 -0800 Subject: [PATCH 2/2] Don't update the field until all checks pass --- core/field_variable.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index c9ff00046..6ad79f0c9 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -238,27 +238,30 @@ Blockly.FieldVariable.prototype.setTypes_ = function(opt_variableTypes, opt_defaultType) { // If you expected that the default type would be the same as the only entry // in the variable types array, tell the Blockly team by commenting on #1499. - this.defaultType_ = opt_defaultType || ''; + var defaultType = opt_defaultType || ''; // Set the allowable variable types. Null means all types on the workspace. if (opt_variableTypes == null || opt_variableTypes == undefined) { - this.variableTypes = null; + var variableTypes = null; } else if (Array.isArray(opt_variableTypes)) { - this.variableTypes = opt_variableTypes; + var variableTypes = opt_variableTypes; // Make sure the default type is valid. var isInArray = false; - for (var i = 0; i < this.variableTypes.length; i++) { - if (this.variableTypes[i] == this.defaultType_) { + for (var i = 0; i < variableTypes.length; i++) { + if (variableTypes[i] == defaultType) { isInArray = true; } } if (!isInArray) { - throw new Error('Invalid default type \'' + this.defaultType_ + '\' in ' + + throw new Error('Invalid default type \'' + defaultType + '\' in ' + 'the definition of a FieldVariable'); } } else { throw new Error('\'variableTypes\' was not an array in the definition of ' + 'a FieldVariable'); } + // Only update the field once all checks pass. + this.defaultType_ = defaultType; + this.variableTypes = variableTypes; }; /**