From c825e60813d850ccd453294990fe6888bc7c2e4e Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 29 Nov 2017 14:37:34 -0800 Subject: [PATCH 01/26] Allow variables of different types to share the same name. --- blocks/procedures.js | 18 +++++ core/block.js | 21 ++++++ core/variable_map.js | 37 +++++---- core/variables.js | 18 ++--- core/workspace.js | 55 ++++++++------ core/xml.js | 13 +++- tests/jsunit/variable_map_test.js | 36 +++++++-- tests/jsunit/workspace_test.js | 95 +++++++++++++++--------- tests/jsunit/workspace_undo_redo_test.js | 26 ++++--- tests/jsunit/xml_test.js | 6 +- 10 files changed, 214 insertions(+), 111 deletions(-) diff --git a/blocks/procedures.js b/blocks/procedures.js index 194de664c..72154eeb4 100644 --- a/blocks/procedures.js +++ b/blocks/procedures.js @@ -259,6 +259,23 @@ Blockly.Blocks['procedures_defnoreturn'] = { getVars: function() { return this.arguments_; }, + /** + * Return all variables referenced by this block. + * @return {!Array.} List of variable names. + * @this Blockly.Block + */ + getVarModels: function() { + var vars = []; + for (var i = 0, argName; argName = this.arguments_[i]; i++) { + // TODO (#1199): When we switch to tracking variables by ID, + // update this. + var model = this.workspace.getVariable(argName, ''); + if (model) { + vars.push(model); + } + } + return vars; + }, /** * Notification that a variable is renaming. * If the name matches one of this block's variables, rename it. @@ -376,6 +393,7 @@ Blockly.Blocks['procedures_defreturn'] = { return [this.getFieldValue('NAME'), this.arguments_, true]; }, getVars: Blockly.Blocks['procedures_defnoreturn'].getVars, + getVarModels: Blockly.Blocks['procedures_defnoreturn'].getVarModels, renameVar: Blockly.Blocks['procedures_defnoreturn'].renameVar, customContextMenu: Blockly.Blocks['procedures_defnoreturn'].customContextMenu, callType_: 'procedures_callreturn' diff --git a/core/block.js b/core/block.js index 11b97f1e6..570670822 100644 --- a/core/block.js +++ b/core/block.js @@ -686,6 +686,27 @@ Blockly.Block.prototype.getVars = function() { return vars; }; +/** + * Return all variables referenced by this block. + * @return {!Array.} List of variable models. + */ +Blockly.Block.prototype.getVarModels = function() { + var vars = []; + for (var i = 0, input; input = this.inputList[i]; i++) { + for (var j = 0, field; field = input.fieldRow[j]; j++) { + if (field instanceof Blockly.FieldVariable) { + // TODO (#1199): When we switch to tracking variables by ID, + // update this. + var model = this.workspace.getVariable(field.getValue(), ''); + if (model) { + vars.push(model); + } + } + } + } + return vars; +}; + /** * Notification that a variable is renaming. * If the name matches one of this block's variables, rename it. diff --git a/core/variable_map.js b/core/variable_map.js index fbf71151b..6af990a97 100644 --- a/core/variable_map.js +++ b/core/variable_map.js @@ -62,15 +62,15 @@ Blockly.VariableMap.prototype.clear = function() { * Rename the given variable by updating its name in the variable map. * @param {Blockly.VariableModel} variable Variable to rename. * @param {string} newName New variable name. + * @param {string=} opt_type The type of the variable to create if variable was + * null. */ -Blockly.VariableMap.prototype.renameVariable = function(variable, newName) { - var newVariable = this.getVariable(newName); +Blockly.VariableMap.prototype.renameVariable = function(variable, newName, + opt_type) { + var type = variable ? variable.type : (opt_type || ''); + var newVariable = this.getVariable(newName, type); var variableIndex = -1; var newVariableIndex = -1; - var type = ''; - if (variable || newVariable) { - type = (variable || newVariable).type; - } var variableList = this.getVariablesOfType(type); if (variable) { @@ -116,19 +116,14 @@ Blockly.VariableMap.prototype.renameVariable = function(variable, newName) { */ Blockly.VariableMap.prototype.createVariable = function(name, opt_type, opt_id) { - var variable = this.getVariable(name); + var variable = this.getVariable(name, opt_type); if (variable) { - if (opt_type && variable.type != opt_type) { - throw Error('Variable "' + name + '" is already in use and its type is "' - + variable.type + '" which conflicts with the passed in ' + - 'type, "' + opt_type + '".'); - } if (opt_id && variable.getId() != opt_id) { throw Error('Variable "' + name + '" is already in use and its id is "' + variable.getId() + '" which conflicts with the passed in ' + 'id, "' + opt_id + '".'); } - // The variable already exists and has the same ID and type. + // The variable already exists and has the same ID. return variable; } if (opt_id && this.getVariableById(opt_id)) { @@ -164,17 +159,19 @@ Blockly.VariableMap.prototype.deleteVariable = function(variable) { }; /** - * Find the variable by the given name and return it. Return null if it is not - * found. + * Find the variable by the given name and type and return it. Return null if + * it is not found. * @param {string} name The name to check for. + * @param {string=} opt_type The type of the variable. If not provided it + * defaults to the empty string, which is a specific type. * @return {Blockly.VariableModel} The variable with the given name, or null if * it was not found. */ -Blockly.VariableMap.prototype.getVariable = function(name) { - var keys = Object.keys(this.variableMap_); - for (var i = 0; i < keys.length; i++ ) { - var key = keys[i]; - for (var j = 0, variable; variable = this.variableMap_[key][j]; j++) { +Blockly.VariableMap.prototype.getVariable = function(name, opt_type) { + var type = opt_type || ''; + var list = this.variableMap_[type]; + if (list) { + for (var j = 0, variable; variable = list[j]; j++) { if (Blockly.Names.equals(variable.name, name)) { return variable; } diff --git a/core/variables.js b/core/variables.js index be9dcb681..f2367fd13 100644 --- a/core/variables.js +++ b/core/variables.js @@ -64,6 +64,7 @@ Blockly.Variables.allUsedVariables = function(root) { var variableHash = Object.create(null); // Iterate through every block and add each variable to the hash. for (var x = 0; x < blocks.length; x++) { + // TODO (#1199) Switch to IDs. var blockVariables = blocks[x].getVars(); if (blockVariables) { for (var y = 0; y < blockVariables.length; y++) { @@ -241,7 +242,7 @@ Blockly.Variables.createVariable = function(workspace, opt_callback, opt_type) { Blockly.Variables.promptName(Blockly.Msg.NEW_VARIABLE_TITLE, defaultName, function(text) { if (text) { - if (workspace.getVariable(text)) { + if (workspace.getVariable(text, opt_type)) { Blockly.alert(Blockly.Msg.VARIABLE_ALREADY_EXISTS.replace('%1', text.toLowerCase()), function() { @@ -281,18 +282,9 @@ Blockly.Variables.renameVariable = function(workspace, variable, Blockly.Msg.RENAME_VARIABLE_TITLE.replace('%1', variable.name), defaultName, function(newName) { if (newName) { - var newVariable = workspace.getVariable(newName); - if (newVariable && newVariable.type != variable.type) { - Blockly.alert(Blockly.Msg.VARIABLE_ALREADY_EXISTS_FOR_ANOTHER_TYPE.replace('%1', - newName.toLowerCase()).replace('%2', newVariable.type), - function() { - promptAndCheckWithAlert(newName); // Recurse - }); - } else { - workspace.renameVariable(variable.name, newName); - if (opt_callback) { - opt_callback(newName); - } + workspace.renameVariable(variable.name, newName); + if (opt_callback) { + opt_callback(newName); } } else { // User canceled prompt without a value. diff --git a/core/workspace.js b/core/workspace.js index f3c6ca4a4..c506fec02 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -239,19 +239,15 @@ Blockly.Workspace.prototype.updateVariableStore = function(clear) { * variable to rename with the given variable. * @param {?Blockly.VariableModel} variable Variable to rename. * @param {string} newName New variable name. + * @param {string=} opt_type The type of the variable to create if variable was + * null. */ Blockly.Workspace.prototype.renameVariableInternal_ = function( - variable, newName) { - var newVariable = this.getVariable(newName); + variable, newName, opt_type) { + var type = variable ? variable.type : (opt_type || ''); + var newVariable = this.getVariable(newName, type); var oldCase; - // If they are different types, throw an error. - if (variable && newVariable && variable.type != newVariable.type) { - throw Error('Variable "' + variable.name + '" is type "' + variable.type + - '" and variable "' + newName + '" is type "' + newVariable.type + - '". Both must be the same type.'); - } - // Find if newVariable case is different. if (newVariable && newVariable.name != newName) { oldCase = newVariable.name; @@ -266,7 +262,7 @@ Blockly.Workspace.prototype.renameVariableInternal_ = function( blocks[i].renameVar(oldCase, newName); } } - this.variableMap_.renameVariable(variable, newName); + this.variableMap_.renameVariable(variable, newName, type); Blockly.Events.setGroup(false); }; @@ -276,11 +272,15 @@ Blockly.Workspace.prototype.renameVariableInternal_ = function( * variable to rename with the given name. * @param {string} oldName Variable to rename. * @param {string} newName New variable name. + * @param {string=} opt_type The type of the variable. If not provided it + * defaults to the empty string, which is a specific type. */ -Blockly.Workspace.prototype.renameVariable = function(oldName, newName) { +Blockly.Workspace.prototype.renameVariable = function(oldName, newName, + opt_type) { + var type = opt_type || ''; // Warning: Prefer to use renameVariableById. - var variable = this.getVariable(oldName); - this.renameVariableInternal_(variable, newName); + var variable = this.getVariable(oldName, type); + this.renameVariableInternal_(variable, newName, opt_type); }; /** @@ -312,17 +312,25 @@ Blockly.Workspace.prototype.createVariable = function(name, opt_type, opt_id) { /** * Find all the uses of a named variable. * @param {string} name Name of variable. + * @param {string=} opt_type The type of the variable. If not provided it + * defaults to the empty string, which is a specific type. * @return {!Array.} Array of block usages. */ -Blockly.Workspace.prototype.getVariableUses = function(name) { +Blockly.Workspace.prototype.getVariableUses = function(name, opt_type) { + var type = opt_type || ''; var uses = []; var blocks = this.getAllBlocks(); // Iterate through every block and check the name. for (var i = 0; i < blocks.length; i++) { - var blockVariables = blocks[i].getVars(); + var blockVariables = blocks[i].getVarModels(); if (blockVariables) { for (var j = 0; j < blockVariables.length; j++) { - var varName = blockVariables[j]; + var varModel = blockVariables[j]; + var varName = varModel.name; + // Skip variables of the wrong type. + if (varModel.type != type) { + continue; + } // Variable name may be null if the block is only half-built. if (varName && name && Blockly.Names.equals(varName, name)) { uses.push(blocks[i]); @@ -337,10 +345,13 @@ Blockly.Workspace.prototype.getVariableUses = function(name) { * Delete a variable by the passed in name and all of its uses from this * workspace. May prompt the user for confirmation. * @param {string} name Name of variable to delete. + * @param {string=} opt_type The type of the variable. If not provided it + * defaults to the empty string, which is a specific type. */ -Blockly.Workspace.prototype.deleteVariable = function(name) { +Blockly.Workspace.prototype.deleteVariable = function(name, opt_type) { + var type = opt_type || ''; // Check whether this variable is a function parameter before deleting. - var uses = this.getVariableUses(name); + var uses = this.getVariableUses(name, type); for (var i = 0, block; block = uses[i]; i++) { if (block.type == 'procedures_defnoreturn' || block.type == 'procedures_defreturn') { @@ -354,7 +365,7 @@ Blockly.Workspace.prototype.deleteVariable = function(name) { } var workspace = this; - var variable = workspace.getVariable(name); + var variable = workspace.getVariable(name, type); if (uses.length > 1) { // Confirm before deleting multiple blocks. Blockly.confirm( @@ -423,10 +434,12 @@ Blockly.Workspace.prototype.variableIndexOf = function( * Find the variable by the given name and return it. Return null if it is not * found. * @param {!string} name The name to check for. + * @param {string=} opt_type The type of the variable. If not provided it + * defaults to the empty string, which is a specific type. * @return {?Blockly.VariableModel} the variable with the given name. */ -Blockly.Workspace.prototype.getVariable = function(name) { - return this.variableMap_.getVariable(name); +Blockly.Workspace.prototype.getVariable = function(name, opt_type) { + return this.variableMap_.getVariable(name, opt_type); }; /** diff --git a/core/xml.js b/core/xml.js index bf0d9e321..343cf8f04 100644 --- a/core/xml.js +++ b/core/xml.js @@ -713,16 +713,23 @@ Blockly.Xml.domToField_ = function(block, fieldName, xml) { return; } + var workspace = block.workspace; var text = xml.textContent; if (field instanceof Blockly.FieldVariable) { // TODO (#1199): When we change setValue and getValue to // interact with IDs instead of names, update this so that we get // the variable based on ID instead of textContent. var type = xml.getAttribute('variabletype') || ''; - var variable = block.workspace.getVariable(text); + // TODO: Consider using a different name (varID?) because this is the + // node's ID. + var id = xml.id; + if (id) { + var variable = workspace.getVariableById(id); + } else { + var variable = workspace.getVariable(text, type); + } if (!variable) { - variable = block.workspace.createVariable(text, type, - xml.getAttribute('id')); + variable = workspace.createVariable(text, type, id); } if (type != null && type !== variable.type) { throw Error('Serialized variable type with id \'' + diff --git a/tests/jsunit/variable_map_test.js b/tests/jsunit/variable_map_test.js index b2c0ccb95..568ce0b0d 100644 --- a/tests/jsunit/variable_map_test.js +++ b/tests/jsunit/variable_map_test.js @@ -43,18 +43,24 @@ function variableMapTest_tearDown() { variable_map = null; } -function test_getVariable_Trivial() { +function test_getVariable_ByNameAndType() { variableMapTest_setUp(); var var_1 = variable_map.createVariable('name1', 'type1', 'id1'); var var_2 = variable_map.createVariable('name2', 'type1', 'id2'); var var_3 = variable_map.createVariable('name3', 'type2', 'id3'); - var result_1 = variable_map.getVariable('name1'); - var result_2 = variable_map.getVariable('name2'); - var result_3 = variable_map.getVariable('name3'); + var result_1 = variable_map.getVariable('name1', 'type1'); + var result_2 = variable_map.getVariable('name2', 'type1'); + var result_3 = variable_map.getVariable('name3', 'type2'); + // Searching by name + type is correct. assertEquals(var_1, result_1); assertEquals(var_2, result_2); assertEquals(var_3, result_3); + + // Searching only by name defaults to the '' type. + assertNull(variable_map.getVariable('name1')); + assertNull(variable_map.getVariable('name2')); + assertNull(variable_map.getVariable('name3')); variableMapTest_tearDown(); } @@ -105,7 +111,7 @@ function test_createVariableAlreadyExists() { var varMapLength = variable_map.variableMap_[keys[0]].length; assertEquals(1, varMapLength); - variable_map.createVariable('name1'); + variable_map.createVariable('name1', 'type1'); checkVariableValues(variable_map, 'name1', 'type1', 'id1'); // Check that the size of the variableMap_ did not change. keys = Object.keys(variable_map.variableMap_); @@ -115,6 +121,26 @@ function test_createVariableAlreadyExists() { variableMapTest_tearDown(); } +function test_createVariableNameAlreadyExists() { + // Expect that when a variable with the same name but a different type already + // exists, the new variable is created. + variableMapTest_setUp(); + variable_map.createVariable('name1', 'type1', 'id1'); + + // Assert there is only one variable in the variable_map. + var keys = Object.keys(variable_map.variableMap_); + assertEquals(1, keys.length); + var varMapLength = variable_map.variableMap_[keys[0]].length; + assertEquals(1, varMapLength); + + variable_map.createVariable('name1', 'type2', 'id2'); + checkVariableValues(variable_map, 'name1', 'type1', 'id1'); + checkVariableValues(variable_map, 'name1', 'type2', 'id2'); + // Check that the size of the variableMap_ did change. + keys = Object.keys(variable_map.variableMap_); + assertEquals(2, keys.length); + variableMapTest_tearDown(); +} function test_createVariableNullAndUndefinedType() { variableMapTest_setUp(); variable_map.createVariable('name1', null, 'id1'); diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index a96a34ac4..59a7fa902 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -158,17 +158,19 @@ function test_getBlockById() { function test_deleteVariable_InternalTrivial() { workspaceTest_setUp(); - var var_1 = workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. + var var_1 = workspace.createVariable('name1', '', 'id1'); + workspace.createVariable('name2', '', 'id2'); createMockBlock('name1'); createMockBlock('name1'); createMockBlock('name2'); workspace.deleteVariableInternal_(var_1); - var variable = workspace.getVariable('name1'); + var variable = workspace.getVariable('name1', ''); var block_var_name = workspace.topBlocks_[0].getVars()[0]; assertNull(variable); - checkVariableValues(workspace, 'name2', 'type2', 'id2'); + checkVariableValues(workspace, 'name2', '', 'id2'); assertEquals('name2', block_var_name); workspaceTest_tearDown(); } @@ -207,15 +209,18 @@ function test_updateVariableStore_NameNotInvariableMap_NoClear() { function test_updateVariableStore_ClearAndAllInUse() { workspaceTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); + workspace.createVariable('name1', '', 'id1'); + workspace.createVariable('name2', '', 'id2'); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. + // TODO (#1199): get rid of updateVariableStore if possible. setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', [workspace], [['name1', 'name2']]); try { workspace.updateVariableStore(true); - checkVariableValues(workspace, 'name1', 'type1', 'id1'); - checkVariableValues(workspace, 'name2', 'type2', 'id2'); + checkVariableValues(workspace, 'name1', '', 'id1'); + checkVariableValues(workspace, 'name2', '', 'id2'); } finally { workspaceTest_tearDown(); } @@ -223,15 +228,18 @@ function test_updateVariableStore_ClearAndAllInUse() { function test_updateVariableStore_ClearAndOneInUse() { workspaceTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); + workspace.createVariable('name1', '', 'id1'); + workspace.createVariable('name2', '', 'id2'); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. + // TODO (#1199): get rid of updateVariableStore if possible. setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', [workspace], [['name1']]); try { workspace.updateVariableStore(true); - checkVariableValues(workspace, 'name1', 'type1', 'id1'); - var variabe = workspace.getVariable('name2'); + checkVariableValues(workspace, 'name1', '', 'id1'); + var variabe = workspace.getVariable('name2', ''); assertNull(variable); } finally { workspaceTest_tearDown(); @@ -302,7 +310,7 @@ function test_renameVariable_NoBlocks() { try { workspace.renameVariable(oldName, newName); checkVariableValues(workspace, 'name2', '', '1'); - var variable = workspace.getVariable(oldName); + var variable = workspace.getVariable(oldName, ''); assertNull(variable); } finally { workspaceTest_tearDown(); @@ -325,12 +333,14 @@ function test_renameVariable_OnlyOldNameBlockExists() { workspaceTest_setUp(); var oldName = 'name1'; var newName = 'name2'; - workspace.createVariable(oldName, 'type1', 'id1'); + workspace.createVariable(oldName, '', 'id1'); createMockBlock(oldName); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. workspace.renameVariable(oldName, newName); - checkVariableValues(workspace, newName, 'type1', 'id1'); - var variable = workspace.getVariable(oldName); + checkVariableValues(workspace, newName, '', 'id1'); + var variable = workspace.getVariable(oldName, ''); var block_var_name = workspace.topBlocks_[0].getVars()[0]; assertNull(variable); assertEquals(newName, block_var_name); @@ -343,13 +353,15 @@ function test_renameVariable_TwoVariablesSameType() { workspaceTest_setUp(); var oldName = 'name1'; var newName = 'name2'; - workspace.createVariable(oldName, 'type1', 'id1'); - workspace.createVariable(newName, 'type1', 'id2'); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. + workspace.createVariable(oldName, '', 'id1'); + workspace.createVariable(newName, '', 'id2'); createMockBlock(oldName); createMockBlock(newName); workspace.renameVariable(oldName, newName); - checkVariableValues(workspace, newName, 'type1', 'id2'); + checkVariableValues(workspace, newName, '', 'id2'); var variable = workspace.getVariable(oldName); var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; @@ -389,12 +401,14 @@ function test_renameVariable_OldCase() { workspaceTest_setUp(); var oldCase = 'Name1'; var newName = 'name1'; - workspace.createVariable(oldCase, 'type1', 'id1'); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. + workspace.createVariable(oldCase, '', 'id1'); createMockBlock(oldCase); workspace.renameVariable(oldCase, newName); - checkVariableValues(workspace, newName, 'type1', 'id1'); - var result_oldCase = workspace.getVariable(oldCase).name; + checkVariableValues(workspace, newName, '', 'id1'); + var result_oldCase = workspace.getVariable(oldCase, '').name; assertNotEquals(oldCase, result_oldCase); workspaceTest_tearDown(); } @@ -405,14 +419,16 @@ function test_renameVariable_TwoVariablesAndOldCase() { var oldName = 'name1'; var oldCase = 'Name2'; var newName = 'name2'; - workspace.createVariable(oldName, 'type1', 'id1'); - workspace.createVariable(oldCase, 'type1', 'id2'); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. + workspace.createVariable(oldName, '', 'id1'); + workspace.createVariable(oldCase, '', 'id2'); createMockBlock(oldName); createMockBlock(oldCase); workspace.renameVariable(oldName, newName); - checkVariableValues(workspace, newName, 'type1', 'id2'); + checkVariableValues(workspace, newName, '', 'id2'); var variable = workspace.getVariable(oldName); var result_oldCase = workspace.getVariable(oldCase).name; var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; @@ -432,13 +448,15 @@ function test_renameVariableById_TwoVariablesSameType() { workspaceTest_setUp(); var oldName = 'name1'; var newName = 'name2'; - workspace.createVariable(oldName, 'type1', 'id1'); - workspace.createVariable(newName, 'type1', 'id2'); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. + workspace.createVariable(oldName, '', 'id1'); + workspace.createVariable(newName, '', 'id2'); createMockBlock(oldName); createMockBlock(newName); workspace.renameVariableById('id1', newName); - checkVariableValues(workspace, newName, 'type1', 'id2'); + checkVariableValues(workspace, newName, '', 'id2'); var variable = workspace.getVariable(oldName); var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; @@ -450,14 +468,16 @@ function test_renameVariableById_TwoVariablesSameType() { function test_deleteVariable_Trivial() { workspaceTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type1', 'id2'); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. + workspace.createVariable('name1', '', 'id1'); + workspace.createVariable('name2', '', 'id2'); createMockBlock('name1'); createMockBlock('name2'); - workspace.deleteVariable('name1'); - checkVariableValues(workspace, 'name2', 'type1', 'id2'); - var variable = workspace.getVariable('name1'); + workspace.deleteVariable('name1', ''); + checkVariableValues(workspace, 'name2', '', 'id2'); + var variable = workspace.getVariable('name1', ''); var block_var_name = workspace.topBlocks_[0].getVars()[0]; assertNull(variable); assertEquals('name2', block_var_name); @@ -466,14 +486,15 @@ function test_deleteVariable_Trivial() { function test_deleteVariableById_Trivial() { workspaceTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type1', 'id2'); + // TODO (#1199): Make a version of this test that uses different types. + workspace.createVariable('name1', '', 'id1'); + workspace.createVariable('name2', '', 'id2'); createMockBlock('name1'); createMockBlock('name2'); workspace.deleteVariableById('id1'); - checkVariableValues(workspace, 'name2', 'type1', 'id2'); - var variable = workspace.getVariable('name1'); + checkVariableValues(workspace, 'name2', '', 'id2'); + var variable = workspace.getVariable('name1', ''); var block_var_name = workspace.topBlocks_[0].getVars()[0]; assertNull(variable); assertEquals('name2', block_var_name); diff --git a/tests/jsunit/workspace_undo_redo_test.js b/tests/jsunit/workspace_undo_redo_test.js index 33dc855d7..a8befe33c 100644 --- a/tests/jsunit/workspace_undo_redo_test.js +++ b/tests/jsunit/workspace_undo_redo_test.js @@ -146,8 +146,10 @@ function test_undoDeleteVariable_NoBlocks() { function test_undoDeleteVariable_WithBlocks() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. + workspace.createVariable('name1', '', 'id1'); + workspace.createVariable('name2', '', 'id2'); createMockBlock('name1'); createMockBlock('name2'); workspace.deleteVariableById('id1'); @@ -156,13 +158,13 @@ function test_undoDeleteVariable_WithBlocks() { workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name2'); assertNull(workspace.getVariableById('id1')); - checkVariableValues(workspace, 'name2', 'type2', 'id2'); + checkVariableValues(workspace, 'name2', '', 'id2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name2'); undoRedoTest_checkBlockVariableName(1, 'name1'); - checkVariableValues(workspace, 'name1', 'type1', 'id1'); - checkVariableValues(workspace, 'name2', 'type2', 'id2'); + checkVariableValues(workspace, 'name1', '', 'id1'); + checkVariableValues(workspace, 'name2', '', 'id2'); undoRedoTest_tearDown(); } @@ -190,8 +192,10 @@ function test_redoAndUndoDeleteVariable_NoBlocks() { function test_redoAndUndoDeleteVariable_WithBlocks() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. + workspace.createVariable('name1', '', 'id1'); + workspace.createVariable('name2', '', 'id2'); createMockBlock('name1'); createMockBlock('name2'); workspace.deleteVariableById('id1'); @@ -210,7 +214,7 @@ function test_redoAndUndoDeleteVariable_WithBlocks() { // Expect that variable 'id2' is recreated undoRedoTest_checkBlockVariableName(0, 'name2'); assertNull(workspace.getVariableById('id1')); - checkVariableValues(workspace, 'name2', 'type2', 'id2'); + checkVariableValues(workspace, 'name2', '', 'id2'); undoRedoTest_tearDown(); } @@ -241,7 +245,9 @@ function test_redoAndUndoDeleteVariableTwice_NoBlocks() { function test_redoAndUndoDeleteVariableTwice_WithBlocks() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. + workspace.createVariable('name1', '', 'id1'); createMockBlock('name1'); workspace.deleteVariableById('id1'); workspace.deleteVariableById('id1'); @@ -255,7 +261,7 @@ function test_redoAndUndoDeleteVariableTwice_WithBlocks() { // undo delete workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); - checkVariableValues(workspace, 'name1', 'type1', 'id1'); + checkVariableValues(workspace, 'name1', '', 'id1'); // redo delete workspace.undo(true); diff --git a/tests/jsunit/xml_test.js b/tests/jsunit/xml_test.js index 1a6dbce3d..0c2af7441 100644 --- a/tests/jsunit/xml_test.js +++ b/tests/jsunit/xml_test.js @@ -285,11 +285,13 @@ function test_appendDomToWorkspace() { function test_blockToDom_fieldToDom_trivial() { xmlTest_setUpWithMockBlocks(); - workspace.createVariable('name1', 'type1', 'id1'); + // TODO (#1199): make a similar test where the variable is given a non-empty + // type. + workspace.createVariable('name1', '', 'id1'); var block = new Blockly.Block(workspace, 'field_variable_test_block'); block.inputList[0].fieldRow[0].setValue('name1'); var resultFieldDom = Blockly.Xml.blockToDom(block).childNodes[0]; - xmlTest_checkVariableFieldDomValues(resultFieldDom, 'VAR', 'type1', 'id1', + xmlTest_checkVariableFieldDomValues(resultFieldDom, 'VAR', '', 'id1', 'name1'); xmlTest_tearDownWithMockBlocks(); } From 731d2735c0c29c9f1f13c88074adb036c63084b5 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 29 Nov 2017 15:22:19 -0800 Subject: [PATCH 02/26] Add TODOs and fix return type --- blocks/procedures.js | 2 +- core/workspace.js | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/blocks/procedures.js b/blocks/procedures.js index 72154eeb4..0b5866188 100644 --- a/blocks/procedures.js +++ b/blocks/procedures.js @@ -261,7 +261,7 @@ Blockly.Blocks['procedures_defnoreturn'] = { }, /** * Return all variables referenced by this block. - * @return {!Array.} List of variable names. + * @return {!Array.} List of variable models. * @this Blockly.Block */ getVarModels: function() { diff --git a/core/workspace.js b/core/workspace.js index c506fec02..9196333da 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -270,6 +270,7 @@ Blockly.Workspace.prototype.renameVariableInternal_ = function( /** * Rename a variable by updating its name in the variable map. Identify the * variable to rename with the given name. + * TODO (#1199): Possibly delete this function. * @param {string} oldName Variable to rename. * @param {string} newName New variable name. * @param {string=} opt_type The type of the variable. If not provided it @@ -311,6 +312,7 @@ Blockly.Workspace.prototype.createVariable = function(name, opt_type, opt_id) { /** * Find all the uses of a named variable. + * TODO (#1199): Possibly delete this function. * @param {string} name Name of variable. * @param {string=} opt_type The type of the variable. If not provided it * defaults to the empty string, which is a specific type. @@ -344,6 +346,7 @@ Blockly.Workspace.prototype.getVariableUses = function(name, opt_type) { /** * Delete a variable by the passed in name and all of its uses from this * workspace. May prompt the user for confirmation. + * TODO (#1199): Possibly delete this function. * @param {string} name Name of variable to delete. * @param {string=} opt_type The type of the variable. If not provided it * defaults to the empty string, which is a specific type. @@ -433,6 +436,7 @@ Blockly.Workspace.prototype.variableIndexOf = function( /** * Find the variable by the given name and return it. Return null if it is not * found. + * TODO (#1199): Possibly delete this function. * @param {!string} name The name to check for. * @param {string=} opt_type The type of the variable. If not provided it * defaults to the empty string, which is a specific type. From 745bed5ac3b3335dc147c3a2ca127a5e9b69ef59 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 30 Nov 2017 16:30:58 -0800 Subject: [PATCH 03/26] Can now create a variable with the button in the flyout; drag a block with a variable out of the flyout; handle default variable names; and import and export variables --- core/field_variable.js | 140 +++++++++++++++++++++++++++++++++++------ core/variables.js | 22 ++++--- core/workspace.js | 2 + core/xml.js | 85 ++++++++++++++++--------- 4 files changed, 192 insertions(+), 57 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index 007ca67f6..4b4b1c427 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -47,13 +47,45 @@ goog.require('goog.string'); * @constructor */ Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes) { - Blockly.FieldVariable.superClass_.constructor.call(this, - Blockly.FieldVariable.dropdownCreate, opt_validator); - this.setValue(varname || ''); + // Don't call the FieldDropdown constructor. It'll try too hard. + this.menuGenerator_ = Blockly.FieldVariable.dropdownCreate; + this.size_ = new goog.math.Size(0, Blockly.BlockSvg.MIN_BLOCK_Y); + this.setValidator(opt_validator); + //this.setValue(varname || ''); + // TODO: Add opt_default_type to match default value. If not set, ''. + this.defaultVariableName = (varname || ''); + this.defaultType_ = ''; this.variableTypes = opt_variableTypes; }; goog.inherits(Blockly.FieldVariable, Blockly.FieldDropdown); +Blockly.FieldVariable.getOrCreateVariable = function(workspace, text, type, + id) { + var potentialVariableMap = workspace.isFlyout ? + workspace.targetWorkspace.potentialVariableMap_ : null; + if (id) { + var variable = workspace.getVariableById(id); + if (!variable && potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + } else { + var variable = workspace.getVariable(text, type); + if (!variable && potentialVariableMap) { + variable = potentialVariableMap.getVariable(text, type); + } + } + // Didn't find the variable. + if (!variable) { + if (potentialVariableMap) { + variable = potentialVariableMap.createVariable(text, type, id); + } else { + variable = workspace.createVariable(text, type, id); + } + } + + return variable; +}; + /** * Install this dropdown on a block. */ @@ -69,20 +101,39 @@ Blockly.FieldVariable.prototype.init = function() { }; Blockly.FieldVariable.prototype.initModel = function() { - if (!this.getValue()) { - // Variables without names get uniquely named for this workspace. - var workspace = - this.sourceBlock_.isInFlyout ? - this.sourceBlock_.workspace.targetWorkspace : - this.sourceBlock_.workspace; - this.setValue(Blockly.Variables.generateUniqueName(workspace)); - } - // If the selected variable doesn't exist yet, create it. - // For instance, some blocks in the toolbox have variable dropdowns filled - // in by default. - if (!this.sourceBlock_.isInFlyout) { - this.sourceBlock_.workspace.createVariable(this.getValue()); + // this.workspace_ = this.sourceBlock_.isInFlyout ? + // this.sourceBlock_.workspace.targetWorkspace : + // this.sourceBlock_.workspace; + // // TODO: Describe how the potential variable map is different from the variable + // // map; use getters. + // this.variableMap_ = this.sourceBlock_.isInFlyout ? + // this.workspace_.potentialVariableMap_ : this.workspace_.variableMap_; + // var name = this.defaultValue; + // if (!name) { + // // Variables without names get uniquely named for this workspace. + // name = Blockly.Variables.generateUniqueName(this.workspace_); + // } + // // If the selected variable doesn't exist yet, create it. + // // For instance, some blocks in the toolbox have variable dropdowns filled + // // in by default. + + // var variable = this.variableMap_.getVariable(name, this.defaultType_); + // if (!variable) { + // variable = this.variableMap_.createVariable(name, this.defaultType_); + // } + if (this.variable_) { + return; // Initialization already happened. } + this.workspace_ = this.sourceBlock_.workspace; + var variable = Blockly.FieldVariable.getOrCreateVariable( + this.workspace_, this.defaultVariableName, this.defaultType_, null); + this.setValue(variable.getId()); +}; + +Blockly.FieldVariable.dispose = function() { + Blockly.FieldVariable.superClass_.dispose.call(this); + this.workspace_ = null; + this.variableMap_ = null; }; /** @@ -101,14 +152,20 @@ Blockly.FieldVariable.prototype.setSourceBlock = function(block) { * @return {string} Current text. */ Blockly.FieldVariable.prototype.getValue = function() { - return this.getText(); + //return this.getText(); + return this.variable_ ? this.variable_.getId() : ''; +}; + +Blockly.FieldVariable.prototype.getText = function() { + //return this.getText(); + return this.variable_ ? this.variable_.name : ''; }; /** - * Set the variable name. + * Set the variable name. (DEPRECATED) * @param {string} value New text. */ -Blockly.FieldVariable.prototype.setValue = function(value) { +Blockly.FieldVariable.prototype.oldSetValue = function(value) { var newValue = value; var newText = value; @@ -131,6 +188,47 @@ Blockly.FieldVariable.prototype.setValue = function(value) { this.setText(newText); }; +/** + * Set the variable ID. + * @param {string} id New variable ID, which must reference an existing + * variable. + */ +Blockly.FieldVariable.prototype.setValue = function(id) { + var workspace = this.sourceBlock_.workspace; + //var variable = this.variableMap_.getVariableById(id); + var potentialVariableMap = workspace.isFlyout ? + workspace.targetWorkspace.potentialVariableMap_ : null; + var variable = workspace.getVariableById(id); + if (!variable && potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + if (!variable) { + throw new Error('Variable id doesn\'t point to a real variable! ID was ' + + id); + } + // Type checks! + var type = variable.type; + if (!this.typeIsAllowed_(type)) { + throw new Error('Variable type doesn\'t match this field! Type was ' + + type); + } + this.variable_ = variable; + this.setText(variable.name); +}; + +Blockly.FieldVariable.prototype.typeIsAllowed_ = function(type) { + var typeList = this.getVariableTypes_(); + if (!typeList) { + return true; // If it's null, all types are valid. + } + for (var i = 0; i < typeList.length; i++) { + if (type == typeList[i]) { + return true; + } + } + return false; +}; + /** * Return a list of variable types to include in the dropdown. * @return {!Array.} Array of variable types. @@ -138,6 +236,8 @@ Blockly.FieldVariable.prototype.setValue = function(value) { * @private */ Blockly.FieldVariable.prototype.getVariableTypes_ = function() { + // TODO: Why does this happen every time, instead of once when the workspace + // is set? Do we expect the variable types to change that much? var variableTypes = this.variableTypes; if (variableTypes === null || variableTypes === undefined) { // If variableTypes is null, return all variable types. @@ -234,7 +334,7 @@ Blockly.FieldVariable.prototype.onItemSelected = function(menu, menuItem) { return; } else if (id == Blockly.DELETE_VARIABLE_ID) { // Delete variable. - workspace.deleteVariable(this.getText()); + workspace.deleteVariableById(this.variable_.getId()); return; } diff --git a/core/variables.js b/core/variables.js index f2367fd13..4788595eb 100644 --- a/core/variables.js +++ b/core/variables.js @@ -330,12 +330,20 @@ Blockly.Variables.promptName = function(promptText, defaultText, callback) { Blockly.Variables.generateVariableFieldXml_ = function(variableModel) { // The variable name may be user input, so it may contain characters that need // to be escaped to create valid XML. - var element = goog.dom.createDom('field'); - element.setAttribute('name', 'VAR'); - element.setAttribute('variabletype', variableModel.type); - element.setAttribute('id', variableModel.getId()); - element.textContent = variableModel.name; + var typeString = variableModel.type; + if (typeString == '') { + typeString = '\'\''; + } + var text = '' + variableModel.name + ''; + return text; + // var element = goog.dom.createDom('field'); + // element.setAttribute('name', 'VAR'); + // element.setAttribute('variabletype', variableModel.type); + // element.setAttribute('id', variableModel.getId()); + // element.textContent = variableModel.name; - var xmlString = Blockly.Xml.domToText(element); - return xmlString; + // var xmlString = Blockly.Xml.domToText(element); + // return xmlString; }; diff --git a/core/workspace.js b/core/workspace.js index 9196333da..41ed1f915 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -84,6 +84,8 @@ Blockly.Workspace = function(opt_options) { * @private */ this.variableMap_ = new Blockly.VariableMap(this); + + this.potentialVariableMap_ = new Blockly.VariableMap(this); }; /** diff --git a/core/xml.js b/core/xml.js index 343cf8f04..a88de9a6e 100644 --- a/core/xml.js +++ b/core/xml.js @@ -86,6 +86,28 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) { return element; }; +Blockly.Xml.fieldToDomVariable_ = function(field, workspace) { + var potentialVariableMap = workspace.isFlyout ? + workspace.targetWorkspace.potentialVariableMap_ : null; + // Ugh that's not true at all. + var id = field.getValue(); + var variable = workspace.getVariableById(id); + if (!variable && potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + if (variable) { + var container = goog.dom.createDom('field', null, variable.name); + container.setAttribute('name', field.name); + container.setAttribute('id', variable.getId()); + container.setAttribute('variabletype', variable.type); + return container; + } else { + // something went wrong? + console.log('no variable in fieldtodom'); + return null; + } +}; + /** * Encode a field as XML. * @param {!Blockly.Field} field The field to encode. @@ -96,16 +118,13 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) { */ Blockly.Xml.fieldToDom_ = function(field, workspace) { if (field.name && field.EDITABLE) { - var container = goog.dom.createDom('field', null, field.getValue()); - container.setAttribute('name', field.name); if (field instanceof Blockly.FieldVariable) { - var variable = workspace.getVariable(field.getValue()); - if (variable) { - container.setAttribute('id', variable.getId()); - container.setAttribute('variabletype', variable.type); - } + return Blockly.Xml.fieldToDomVariable_(field, workspace); + } else { + var container = goog.dom.createDom('field', null, field.getValue()); + container.setAttribute('name', field.name); + return container; } - return container; } return null; }; @@ -698,6 +717,31 @@ Blockly.Xml.domToBlockHeadless_ = function(xmlBlock, workspace) { return block; }; +Blockly.Xml.domToFieldVariable_ = function(workspace, xml, text, field) { + // TODO (#1199): When we change setValue and getValue to + // interact with IDs instead of names, update this so that we get + // the variable based on ID instead of textContent. + var type = xml.getAttribute('variabletype') || ''; + if (type == '\'\'') { + type = ''; + } + // TODO: Consider using a different name (var_id?) because this is the + // node's ID. + var id = xml.id; + var variable = Blockly.FieldVariable.getOrCreateVariable(workspace, text, + type, id); + + // This should never happen :) + if (type != null && type !== variable.type) { + throw Error('Serialized variable type with id \'' + + variable.getId() + '\' had type ' + variable.type + ', and ' + + 'does not match variable field that references it: ' + + Blockly.Xml.domToText(xml) + '.'); + } + + field.setValue(variable.getId()); +}; + /** * Decode an XML field tag and set the value of that field on the given block. * @param {!Blockly.Block} block The block that is currently being deserialized. @@ -716,29 +760,10 @@ Blockly.Xml.domToField_ = function(block, fieldName, xml) { var workspace = block.workspace; var text = xml.textContent; if (field instanceof Blockly.FieldVariable) { - // TODO (#1199): When we change setValue and getValue to - // interact with IDs instead of names, update this so that we get - // the variable based on ID instead of textContent. - var type = xml.getAttribute('variabletype') || ''; - // TODO: Consider using a different name (varID?) because this is the - // node's ID. - var id = xml.id; - if (id) { - var variable = workspace.getVariableById(id); - } else { - var variable = workspace.getVariable(text, type); - } - if (!variable) { - variable = workspace.createVariable(text, type, id); - } - if (type != null && type !== variable.type) { - throw Error('Serialized variable type with id \'' + - variable.getId() + '\' had type ' + variable.type + ', and ' + - 'does not match variable field that references it: ' + - Blockly.Xml.domToText(xml) + '.'); - } + Blockly.Xml.domToFieldVariable_(workspace, xml, text, field); + } else { + field.setValue(text); } - field.setValue(text); }; /** From 08e065a75bc5d7eb3a3b6559f2a1cdb42e54e47e Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 30 Nov 2017 17:12:39 -0800 Subject: [PATCH 04/26] Fix renaming --- core/block.js | 18 ++++++++++++++++ core/field_variable.js | 47 ++++++++---------------------------------- core/variables.js | 4 ++-- core/workspace.js | 16 +++++++++++--- 4 files changed, 42 insertions(+), 43 deletions(-) diff --git a/core/block.js b/core/block.js index 570670822..c16129a39 100644 --- a/core/block.js +++ b/core/block.js @@ -724,6 +724,24 @@ Blockly.Block.prototype.renameVar = function(oldName, newName) { } }; +/** + * Notification that a variable is renaming. + * If the name matches one of this block's variables, rename it. + * @param {string} oldId ID of variable to rename. + * @param {string} newId ID of new variable. May be the same as oldId, but with + * an updated name. + */ +Blockly.Block.prototype.renameVarById = function(oldId, newId) { + for (var i = 0, input; input = this.inputList[i]; i++) { + for (var j = 0, field; field = input.fieldRow[j]; j++) { + if (field instanceof Blockly.FieldVariable && + oldId == field.getValue()) { + field.setValue(newId); + } + } + } +}; + /** * Returns the language-neutral value from the field of a block. * @param {string} name The name of the field. diff --git a/core/field_variable.js b/core/field_variable.js index 4b4b1c427..0a793ec1f 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -161,33 +161,6 @@ Blockly.FieldVariable.prototype.getText = function() { return this.variable_ ? this.variable_.name : ''; }; -/** - * Set the variable name. (DEPRECATED) - * @param {string} value New text. - */ -Blockly.FieldVariable.prototype.oldSetValue = function(value) { - var newValue = value; - var newText = value; - - if (this.sourceBlock_) { - var variable = this.sourceBlock_.workspace.getVariableById(value); - if (variable) { - newText = variable.name; - } - // TODO(marisaleung): Remove name lookup after converting all Field Variable - // instances to use ID instead of name. - else if (variable = this.sourceBlock_.workspace.getVariable(value)) { - newValue = variable.getId(); - } - if (Blockly.Events.isEnabled()) { - Blockly.Events.fire(new Blockly.Events.BlockChange( - this.sourceBlock_, 'field', this.name, this.value_, newValue)); - } - } - this.value_ = newValue; - this.setText(newText); -}; - /** * Set the variable ID. * @param {string} id New variable ID, which must reference an existing @@ -319,17 +292,13 @@ Blockly.FieldVariable.prototype.onItemSelected = function(menu, menuItem) { var id = menuItem.getValue(); // TODO(marisaleung): change setValue() to take in an ID as the parameter. // Then remove itemText. - var itemText; if (this.sourceBlock_ && this.sourceBlock_.workspace) { var workspace = this.sourceBlock_.workspace; var variable = workspace.getVariableById(id); // If the item selected is a variable, set itemText to the variable name. - if (variable) { - itemText = variable.name; - } else if (id == Blockly.RENAME_VARIABLE_ID) { + if (id == Blockly.RENAME_VARIABLE_ID) { // Rename variable. - var currentName = this.getText(); - variable = workspace.getVariable(currentName); + variable = this.variable_; Blockly.Variables.renameVariable(workspace, variable); return; } else if (id == Blockly.DELETE_VARIABLE_ID) { @@ -338,10 +307,12 @@ Blockly.FieldVariable.prototype.onItemSelected = function(menu, menuItem) { return; } - // Call any validation function, and allow it to override. - itemText = this.callValidator(itemText); - } - if (itemText !== null) { - this.setValue(itemText); + // TODO: Call any validation function, and allow it to override. + // For now it's unclear whether the validator should act on the id. + //var validatedId = this.callValidator(variable.getId()); } + // if (variable.getId() !== null) { + // this.setValue(validatedId); + // } + this.setValue(id); }; diff --git a/core/variables.js b/core/variables.js index 4788595eb..26dc0b452 100644 --- a/core/variables.js +++ b/core/variables.js @@ -269,7 +269,7 @@ Blockly.Variables.createVariable = function(workspace, opt_callback, opt_type) { * Rename a variable with the given workspace, variableType, and oldName. * @param {!Blockly.Workspace} workspace The workspace on which to rename the * variable. - * @param {?Blockly.VariableModel} variable Variable to rename. + * @param {Blockly.VariableModel} variable Variable to rename. * @param {function(?string=)=} opt_callback A callback. It will * be passed an acceptable new variable name, or null if change is to be * aborted (cancel button), or undefined if an existing variable was chosen. @@ -282,7 +282,7 @@ Blockly.Variables.renameVariable = function(workspace, variable, Blockly.Msg.RENAME_VARIABLE_TITLE.replace('%1', variable.name), defaultName, function(newName) { if (newName) { - workspace.renameVariable(variable.name, newName); + workspace.renameVariableById(variable.getId(), newName); if (opt_callback) { opt_callback(newName); } diff --git a/core/workspace.js b/core/workspace.js index 41ed1f915..a8f7eccfa 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -248,6 +248,13 @@ Blockly.Workspace.prototype.renameVariableInternal_ = function( variable, newName, opt_type) { var type = variable ? variable.type : (opt_type || ''); var newVariable = this.getVariable(newName, type); + + // If a variable previously existed with the new name, we will coalesce the + // variables and use the ID of the existing variable with the new name. + // Otherwise, we change the current variable's name but not its ID. + var oldId = variable.getId(); + var newId = newVariable ? newVariable.getId() : oldId; + var oldCase; // Find if newVariable case is different. @@ -257,14 +264,17 @@ Blockly.Workspace.prototype.renameVariableInternal_ = function( Blockly.Events.setGroup(true); var blocks = this.getAllBlocks(); + this.variableMap_.renameVariable(variable, newName, type); + + // Iterate through every block and update name. for (var i = 0; i < blocks.length; i++) { - blocks[i].renameVar(variable.name, newName); + blocks[i].renameVarById(oldId, newId); if (oldCase) { - blocks[i].renameVar(oldCase, newName); + blocks[i].renameVarById(newId, newId); + //blocks[i].renameVar(oldCase, newName); } } - this.variableMap_.renameVariable(variable, newName, type); Blockly.Events.setGroup(false); }; From 0516310f6654a87eae25154ae9c8bcc5e205e98f Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 30 Nov 2017 17:21:12 -0800 Subject: [PATCH 05/26] Delete by id --- core/block.js | 2 +- core/workspace.js | 25 ++++++++++++++++++++++++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/core/block.js b/core/block.js index c16129a39..58bc311a4 100644 --- a/core/block.js +++ b/core/block.js @@ -697,7 +697,7 @@ Blockly.Block.prototype.getVarModels = function() { if (field instanceof Blockly.FieldVariable) { // TODO (#1199): When we switch to tracking variables by ID, // update this. - var model = this.workspace.getVariable(field.getValue(), ''); + var model = this.workspace.getVariableById(field.getValue()); if (model) { vars.push(model); } diff --git a/core/workspace.js b/core/workspace.js index a8f7eccfa..6055dc4d0 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -355,6 +355,29 @@ Blockly.Workspace.prototype.getVariableUses = function(name, opt_type) { return uses; }; +/** + * Find all the uses of a named variable. + * TODO (#1199): Possibly delete this function. + * @param {string} id ID of the variable to find. + * @return {!Array.} Array of block usages. + */ +Blockly.Workspace.prototype.getVariableUsesById = function(id) { + var uses = []; + var blocks = this.getAllBlocks(); + // Iterate through every block and check the name. + for (var i = 0; i < blocks.length; i++) { + var blockVariables = blocks[i].getVarModels(); + if (blockVariables) { + for (var j = 0; j < blockVariables.length; j++) { + if (blockVariables[j].getId() == id) { + uses.push(blocks[i]); + } + } + } + } + return uses; +}; + /** * Delete a variable by the passed in name and all of its uses from this * workspace. May prompt the user for confirmation. @@ -419,7 +442,7 @@ Blockly.Workspace.prototype.deleteVariableById = function(id) { * @private */ Blockly.Workspace.prototype.deleteVariableInternal_ = function(variable) { - var uses = this.getVariableUses(variable.name); + var uses = this.getVariableUsesById(variable.getId()); Blockly.Events.setGroup(true); for (var i = 0; i < uses.length; i++) { uses[i].dispose(true, false); From c541a544c6ff095adcce99cee0ee9a024502f623 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Fri, 1 Dec 2017 14:04:10 -0800 Subject: [PATCH 06/26] Fix tests in field_variable_test.js --- core/field_variable.js | 20 +--- tests/jsunit/field_variable_test.js | 163 +++++++++++----------------- 2 files changed, 68 insertions(+), 115 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index 0a793ec1f..73fccd58f 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -236,10 +236,12 @@ Blockly.FieldVariable.prototype.getVariableTypes_ = function() { * @this {Blockly.FieldVariable} */ Blockly.FieldVariable.dropdownCreate = function() { + if (!this.variable_) { + throw new Error('Tried to call dropdownCreate on a variable field with no' + + ' variable selected.'); + } var variableModelList = []; var name = this.getText(); - // Don't create a new variable if there is nothing selected. - var createSelectedVariable = name ? true : false; var workspace = null; if (this.sourceBlock_) { workspace = this.sourceBlock_.workspace; @@ -254,20 +256,9 @@ Blockly.FieldVariable.dropdownCreate = function() { var variables = workspace.getVariablesOfType(variableType); variableModelList = variableModelList.concat(variables); } - for (var i = 0; i < variableModelList.length; i++) { - if (createSelectedVariable && - goog.string.caseInsensitiveEquals(variableModelList[i].name, name)) { - createSelectedVariable = false; - break; - } - } - } - // Ensure that the currently selected variable is an option. - if (createSelectedVariable && workspace) { - var newVar = workspace.createVariable(name); - variableModelList.push(newVar); } variableModelList.sort(Blockly.VariableModel.compareByName); + var options = []; for (var i = 0; i < variableModelList.length; i++) { // Set the UUID as the internal representation of the variable. @@ -278,6 +269,7 @@ Blockly.FieldVariable.dropdownCreate = function() { options.push([Blockly.Msg.DELETE_VARIABLE.replace('%1', name), Blockly.DELETE_VARIABLE_ID]); } + return options; }; diff --git a/tests/jsunit/field_variable_test.js b/tests/jsunit/field_variable_test.js index 0ef296273..0cf1f11f8 100644 --- a/tests/jsunit/field_variable_test.js +++ b/tests/jsunit/field_variable_test.js @@ -44,10 +44,20 @@ function fieldVariable_mockBlock(workspace) { return {'workspace': workspace, 'isShadow': function(){return false;}}; } +function fieldVariable_createAndInitField(workspace) { + var fieldVariable = new Blockly.FieldVariable('name1'); + var mockBlock = fieldVariable_mockBlock(workspace); + fieldVariable.setSourceBlock(mockBlock); + // No view to initialize, but the model still needs work. + fieldVariable.initModel(); + return fieldVariable; +} + function test_fieldVariable_Constructor() { workspace = new Blockly.Workspace(); var fieldVariable = new Blockly.FieldVariable('name1'); - assertEquals('name1', fieldVariable.getText()); + // The field does not have a variable until after init() is called. + assertEquals('', fieldVariable.getText()); workspace.dispose(); } @@ -55,52 +65,41 @@ function test_fieldVariable_setValueMatchId() { // Expect the fieldVariable value to be set to variable name fieldVariableTestWithMocks_setUp(); workspace.createVariable('name2', null, 'id2'); - var fieldVariable = new Blockly.FieldVariable('name1'); - var mockBlock = fieldVariable_mockBlock(workspace); - fieldVariable.setSourceBlock(mockBlock); + + var fieldVariable = fieldVariable_createAndInitField(workspace); + var event = new Blockly.Events.BlockChange( - mockBlock, 'field', undefined, 'name1', 'id2'); + fieldVariable.sourceBlock_, 'field', undefined, 'name1', 'id2'); setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null); fieldVariable.setValue('id2'); assertEquals('name2', fieldVariable.getText()); - assertEquals('id2', fieldVariable.value_); - fieldVariableTestWithMocks_tearDown(); -} - -function test_fieldVariable_setValueMatchName() { - // Expect the fieldVariable value to be set to variable name - fieldVariableTestWithMocks_setUp(); - workspace.createVariable('name2', null, 'id2'); - var fieldVariable = new Blockly.FieldVariable('name1'); - var mockBlock = fieldVariable_mockBlock(workspace); - fieldVariable.setSourceBlock(mockBlock); - var event = new Blockly.Events.BlockChange( - mockBlock, 'field', undefined, 'name1', 'id2'); - setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null); - - fieldVariable.setValue('name2'); - assertEquals('name2', fieldVariable.getText()); - assertEquals('id2', fieldVariable.value_); + assertEquals('id2', fieldVariable.getValue()); fieldVariableTestWithMocks_tearDown(); } function test_fieldVariable_setValueNoVariable() { - // Expect the fieldVariable value to be set to the passed in string. No error - // should be thrown. fieldVariableTestWithMocks_setUp(); - var fieldVariable = new Blockly.FieldVariable('name1'); - var mockBlock = {'workspace': workspace, - 'isShadow': function(){return false;}}; - fieldVariable.setSourceBlock(mockBlock); + + var fieldVariable = fieldVariable_createAndInitField(workspace); + var mockBlock = fieldVariable.sourceBlock_; + mockBlock.isShadow = function() { + return false; + }; + var event = new Blockly.Events.BlockChange( mockBlock, 'field', undefined, 'name1', 'id1'); setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null); - fieldVariable.setValue('id1'); - assertEquals('id1', fieldVariable.getText()); - assertEquals('id1', fieldVariable.value_); - fieldVariableTestWithMocks_tearDown(); + try { + fieldVariable.setValue('id1'); + // Calling setValue with a variable that doesn't exist throws an error. + fail(); + } catch (e) { + // expected + } finally { + fieldVariableTestWithMocks_tearDown(); + } } function test_fieldVariable_dropdownCreateVariablesExist() { @@ -108,12 +107,12 @@ function test_fieldVariable_dropdownCreateVariablesExist() { workspace = new Blockly.Workspace(); workspace.createVariable('name1', '', 'id1'); workspace.createVariable('name2', '', 'id2'); + + var fieldVariable = fieldVariable_createAndInitField(workspace); + var result_options = Blockly.FieldVariable.dropdownCreate.call( - { - 'sourceBlock_': {'workspace': workspace}, - 'getText': function(){return 'name1';}, - 'getVariableTypes_': function(){return [''];} - }); + fieldVariable); + assertEquals(result_options.length, 3); isEqualArrays(result_options[0], ['name1', 'id1']); isEqualArrays(result_options[1], ['name2', 'id2']); @@ -121,77 +120,30 @@ function test_fieldVariable_dropdownCreateVariablesExist() { workspace.dispose(); } -function test_fieldVariable_dropdownCreateVariablesExist() { - // Expect that the dropdown options will contain the variables that exist. - workspace = new Blockly.Workspace(); - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - var result_options = Blockly.FieldVariable.dropdownCreate.call( - { - 'sourceBlock_': {'workspace': workspace}, - 'getText': function(){return 'name1';}, - 'getVariableTypes_': function(){return [''];} - }); - assertEquals(result_options.length, 3); - isEqualArrays(result_options[0], ['name1', 'id1']); - isEqualArrays(result_options[1], ['name2', 'id2']); - - workspace.dispose(); -} - -function test_fieldVariable_dropdownVariableAndTypeDoesNotExist() { - // Expect a variable will be created for the selected option. Expect the - // workspace variable map to contain the new variable once. +function test_fieldVariable_setValueNull() { + // This should no longer create a variable for the selected option. fieldVariableTestWithMocks_setUp(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['id1', null]); - var result_options = Blockly.FieldVariable.dropdownCreate.call( - { - 'sourceBlock_': {'workspace': workspace}, - 'getText': function(){return 'name1';}, - 'getVariableTypes_': function(){return [''];} - }); + var fieldVariable = fieldVariable_createAndInitField(workspace); + try { + fieldVariable.setValue(null); + fail(); + } catch (e) { + // expected + } finally { + fieldVariableTestWithMocks_tearDown(); + } - // Check the options. - assertEquals(2, result_options.length); - isEqualArrays(result_options[0], ['name1', 'id1']); - // Check the variable map. - assertEquals(1, workspace.getAllVariables().length); - checkVariableValues(workspace, 'name1', '', 'id1'); - - fieldVariableTestWithMocks_tearDown(); -} - -function test_fieldVariable_dropdownVariableDoesNotExistTypeDoes() { - // Expect a variable will be created for the selected option. Expect the - // workspace variable map to contain the new variable once. - fieldVariableTestWithMocks_setUp(); - workspace.createVariable('name1', '', 'id1'); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['id2', null]); - - var result_options = Blockly.FieldVariable.dropdownCreate.call( - { - 'sourceBlock_': {'workspace': workspace}, - 'getText': function(){return 'name2';}, - 'getVariableTypes_': function(){return [''];} - }); - - assertEquals(3, result_options.length); - isEqualArrays(result_options[0], ['name1', 'id1']); - isEqualArrays(result_options[1], ['name2', 'id2']); - assertEquals(2, workspace.variableMap_.getAllVariables().length); - checkVariableValues(workspace, 'name1', '', 'id1'); - checkVariableValues(workspace, 'name2', '', 'id2'); - - fieldVariableTestWithMocks_tearDown(); } function test_fieldVariable_getVariableTypes_undefinedVariableTypes() { // Expect that since variableTypes is undefined, only type empty string - // will be returned. + // will be returned (regardless of what types are available on the workspace). workspace = new Blockly.Workspace(); workspace.createVariable('name1', 'type1'); workspace.createVariable('name2', 'type2'); + var fieldVariable = new Blockly.FieldVariable('name1'); var resultTypes = fieldVariable.getVariableTypes_(); isEqualArrays(resultTypes, ['']); @@ -199,12 +151,14 @@ function test_fieldVariable_getVariableTypes_undefinedVariableTypes() { } function test_fieldVariable_getVariableTypes_givenVariableTypes() { - // Expect that since variableTypes is undefined, only type empty string - // will be returned. + // Expect that since variableTypes is defined, it will be the return value, + // regardless of what types are available on the workspace. workspace = new Blockly.Workspace(); workspace.createVariable('name1', 'type1'); workspace.createVariable('name2', 'type2'); - var fieldVariable = new Blockly.FieldVariable('name1', null, ['type1', 'type2']); + + var fieldVariable = new Blockly.FieldVariable( + 'name1', null, ['type1', 'type2']); var resultTypes = fieldVariable.getVariableTypes_(); isEqualArrays(resultTypes, ['type1', 'type2']); workspace.dispose(); @@ -212,13 +166,17 @@ function test_fieldVariable_getVariableTypes_givenVariableTypes() { function test_fieldVariable_getVariableTypes_nullVariableTypes() { // Expect all variable types to be returned. + // The variable does not need to be initialized to do this--it just needs a + // pointer to the workspace. workspace = new Blockly.Workspace(); workspace.createVariable('name1', 'type1'); workspace.createVariable('name2', 'type2'); + var fieldVariable = new Blockly.FieldVariable('name1'); var mockBlock = fieldVariable_mockBlock(workspace); fieldVariable.setSourceBlock(mockBlock); fieldVariable.variableTypes = null; + var resultTypes = fieldVariable.getVariableTypes_(); isEqualArrays(resultTypes, ['type1', 'type2']); workspace.dispose(); @@ -229,12 +187,15 @@ function test_fieldVariable_getVariableTypes_emptyListVariableTypes() { workspace = new Blockly.Workspace(); workspace.createVariable('name1', 'type1'); workspace.createVariable('name2', 'type2'); + var fieldVariable = new Blockly.FieldVariable('name1'); var mockBlock = fieldVariable_mockBlock(workspace); fieldVariable.setSourceBlock(mockBlock); fieldVariable.variableTypes = []; + try { fieldVariable.getVariableTypes_(); + fail(); } catch (e) { //expected } finally { From 5145ad6918c21b3f3924b0bcd30801f23ca72573 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Fri, 1 Dec 2017 14:05:37 -0800 Subject: [PATCH 07/26] Fix variable_map_test and variable_model_test --- core/xml.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/core/xml.js b/core/xml.js index a88de9a6e..f71c3b52a 100644 --- a/core/xml.js +++ b/core/xml.js @@ -87,13 +87,17 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) { }; Blockly.Xml.fieldToDomVariable_ = function(field, workspace) { - var potentialVariableMap = workspace.isFlyout ? - workspace.targetWorkspace.potentialVariableMap_ : null; // Ugh that's not true at all. var id = field.getValue(); var variable = workspace.getVariableById(id); - if (!variable && potentialVariableMap) { - variable = potentialVariableMap.getVariableById(id); + if (!variable) { + if (workspace.isFlyout && workspace.targetWorkspace) { + var potentialVariableMap = + workspace.targetWorkspace.potentialVariableMap_; + if (potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + } } if (variable) { var container = goog.dom.createDom('field', null, variable.name); From 62737bc233a3365a622d3a00427581bb25128e38 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Fri, 1 Dec 2017 16:21:19 -0800 Subject: [PATCH 08/26] Update tests in workspace_test. Get rid of renameVariable --- core/field_variable.js | 14 +- core/workspace.js | 27 +--- tests/jsunit/workspace_test.js | 284 +++++++++++++++------------------ 3 files changed, 147 insertions(+), 178 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index 73fccd58f..9bff99214 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -168,13 +168,17 @@ Blockly.FieldVariable.prototype.getText = function() { */ Blockly.FieldVariable.prototype.setValue = function(id) { var workspace = this.sourceBlock_.workspace; - //var variable = this.variableMap_.getVariableById(id); - var potentialVariableMap = workspace.isFlyout ? - workspace.targetWorkspace.potentialVariableMap_ : null; var variable = workspace.getVariableById(id); - if (!variable && potentialVariableMap) { - variable = potentialVariableMap.getVariableById(id); + if (!variable) { + if (workspace.isFlyout && workspace.targetWorkspace) { + var potentialVariableMap = + workspace.targetWorkspace.potentialVariableMap_; + if (potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + } } + if (!variable) { throw new Error('Variable id doesn\'t point to a real variable! ID was ' + id); diff --git a/core/workspace.js b/core/workspace.js index 6055dc4d0..3919190bd 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -241,12 +241,10 @@ Blockly.Workspace.prototype.updateVariableStore = function(clear) { * variable to rename with the given variable. * @param {?Blockly.VariableModel} variable Variable to rename. * @param {string} newName New variable name. - * @param {string=} opt_type The type of the variable to create if variable was - * null. */ Blockly.Workspace.prototype.renameVariableInternal_ = function( - variable, newName, opt_type) { - var type = variable ? variable.type : (opt_type || ''); + variable, newName) { + var type = variable ? variable.type : ''; var newVariable = this.getVariable(newName, type); // If a variable previously existed with the new name, we will coalesce the @@ -278,24 +276,6 @@ Blockly.Workspace.prototype.renameVariableInternal_ = function( Blockly.Events.setGroup(false); }; - -/** - * Rename a variable by updating its name in the variable map. Identify the - * variable to rename with the given name. - * TODO (#1199): Possibly delete this function. - * @param {string} oldName Variable to rename. - * @param {string} newName New variable name. - * @param {string=} opt_type The type of the variable. If not provided it - * defaults to the empty string, which is a specific type. - */ -Blockly.Workspace.prototype.renameVariable = function(oldName, newName, - opt_type) { - var type = opt_type || ''; - // Warning: Prefer to use renameVariableById. - var variable = this.getVariable(oldName, type); - this.renameVariableInternal_(variable, newName, opt_type); -}; - /** * Rename a variable by updating its name in the variable map. Identify the * variable to rename with the given ID. @@ -304,6 +284,9 @@ Blockly.Workspace.prototype.renameVariable = function(oldName, newName, */ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { var variable = this.getVariableById(id); + if (!variable) { + throw new Error('Tried to rename a variable that didn\'t exist. ID: ' + id); + } this.renameVariableInternal_(variable, newName); }; diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index 59a7fa902..d39eac1e4 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -31,6 +31,7 @@ Blockly.defineBlocksWithJsonArray([{ { "type": "field_variable", "name": "VAR", + "variableTypes": ["", "type1", "type2"] } ] }]); @@ -50,9 +51,12 @@ function workspaceTest_tearDown() { * @param {?string} variable_name The string to put into the variable field. * @return {!Blockly.Block} The created block. */ -function createMockBlock(variable_name) { +function createMockBlock(variable_id) { + // Turn off events to avoid testing XML at the same time. + Blockly.Events.disable(); var block = new Blockly.Block(workspace, 'get_var_block'); - block.inputList[0].fieldRow[0].setValue(variable_name); + block.inputList[0].fieldRow[0].setValue(variable_id); + Blockly.Events.enable(); return block; } @@ -158,19 +162,17 @@ function test_getBlockById() { function test_deleteVariable_InternalTrivial() { workspaceTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - var var_1 = workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name1'); - createMockBlock('name2'); + var var_1 = workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + createMockBlock('id1'); + createMockBlock('id1'); + createMockBlock('id2'); workspace.deleteVariableInternal_(var_1); - var variable = workspace.getVariable('name1', ''); - var block_var_name = workspace.topBlocks_[0].getVars()[0]; + var variable = workspace.getVariableById('id1'); + var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; assertNull(variable); - checkVariableValues(workspace, 'name2', '', 'id2'); + checkVariableValues(workspace, 'name2', 'type2', 'id2'); assertEquals('name2', block_var_name); workspaceTest_tearDown(); } @@ -239,7 +241,7 @@ function test_updateVariableStore_ClearAndOneInUse() { try { workspace.updateVariableStore(true); checkVariableValues(workspace, 'name1', '', 'id1'); - var variabe = workspace.getVariable('name2', ''); + var variable = workspace.getVariable('name2', ''); assertNull(variable); } finally { workspaceTest_tearDown(); @@ -249,16 +251,21 @@ function test_updateVariableStore_ClearAndOneInUse() { function test_addTopBlock_TrivialFlyoutIsTrue() { workspaceTest_setUp(); workspace.isFlyout = true; - var block = createMockBlock(); - workspace.removeTopBlock(block); - setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', [block], - [['name1']]); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); + var targetWorkspace = new Blockly.Workspace(); + workspace.targetWorkspace = targetWorkspace; + targetWorkspace.createVariable('name1', '', '1'); + + // Flyout.init usually does this binding. + workspace.getVariableById = + targetWorkspace.getVariableById.bind(targetWorkspace); try { + var block = createMockBlock('1'); + workspace.removeTopBlock(block); workspace.addTopBlock(block); checkVariableValues(workspace, 'name1', '', '1'); } finally { + targetWorkspace.dispose(); workspaceTest_tearDown(); } } @@ -297,52 +304,41 @@ function test_clear_NoVariables() { } } -function test_renameVariable_NoBlocks() { - // Expect 'renameVariable' to create new variable with newName. +function test_renameVariable_NoReference() { + // Test renaming a variable in the simplest case: when no blocks refer to it. workspaceTest_setUp(); + var id = 'id1'; + var type = 'type1'; var oldName = 'name1'; var newName = 'name2'; - // Mocked setGroup to ensure only one call to the mocked genUid. - setUpMockMethod(mockControl_, Blockly.Events, 'setGroup', [true, false], - null); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); + workspace.createVariable(oldName, type, id); try { - workspace.renameVariable(oldName, newName); - checkVariableValues(workspace, 'name2', '', '1'); - var variable = workspace.getVariable(oldName, ''); - assertNull(variable); + workspace.renameVariableById(id, newName); + checkVariableValues(workspace, newName, type, id); + // Renaming should not have created a new variable. + assertEquals(1, workspace.getAllVariables().length); } finally { workspaceTest_tearDown(); } } -function test_renameVariable_SameNameNoBlocks() { - // Expect 'renameVariable' to create new variable with newName. - workspaceTest_setUp(); - var name = 'name1'; - workspace.createVariable(name, 'type1', 'id1'); - - workspace.renameVariable(name, name); - checkVariableValues(workspace, name, 'type1', 'id1'); - workspaceTest_tearDown(); -} - -function test_renameVariable_OnlyOldNameBlockExists() { +function test_renameVariable_ReferenceExists() { + // Test renaming a variable when a reference to it exists. // Expect 'renameVariable' to change oldName variable name to newName. workspaceTest_setUp(); + var id = 'id1'; + var type = 'type1'; var oldName = 'name1'; var newName = 'name2'; - workspace.createVariable(oldName, '', 'id1'); - createMockBlock(oldName); + workspace.createVariable(oldName, type, id); + createMockBlock(id); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.renameVariable(oldName, newName); - checkVariableValues(workspace, newName, '', 'id1'); - var variable = workspace.getVariable(oldName, ''); - var block_var_name = workspace.topBlocks_[0].getVars()[0]; - assertNull(variable); + workspace.renameVariableById(id, newName); + checkVariableValues(workspace, newName, type, id); + // Renaming should not have created a new variable. + assertEquals(1, workspace.getAllVariables().length); + var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; assertEquals(newName, block_var_name); workspaceTest_tearDown(); } @@ -351,151 +347,137 @@ function test_renameVariable_TwoVariablesSameType() { // Expect 'renameVariable' to change oldName variable name to newName. // Expect oldName block name to change to newName workspaceTest_setUp(); + var id1 = 'id1'; + var id2 = 'id2'; + var type = 'type1'; + var oldName = 'name1'; var newName = 'name2'; - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable(oldName, '', 'id1'); - workspace.createVariable(newName, '', 'id2'); - createMockBlock(oldName); - createMockBlock(newName); + // Create two variables of the same type. + workspace.createVariable(oldName, type, id1); + workspace.createVariable(newName, type, id2); + // Create blocks to refer to both of them. + createMockBlock(id1); + createMockBlock(id2); - workspace.renameVariable(oldName, newName); - checkVariableValues(workspace, newName, '', 'id2'); - var variable = workspace.getVariable(oldName); - var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; - var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; + workspace.renameVariableById(id1, newName); + checkVariableValues(workspace, newName, type, id2); + // The old variable should have been deleted. + var variable = workspace.getVariableById(id1); assertNull(variable); + + // There should only be one variable left. + assertEquals(1, workspace.getAllVariables().length); + + // References should have the correct names. + var block_var_name_1 = workspace.topBlocks_[0].getVarModels()[0].name; + var block_var_name_2 = workspace.topBlocks_[1].getVarModels()[0].name; assertEquals(newName, block_var_name_1); assertEquals(newName, block_var_name_2); + workspaceTest_tearDown(); } function test_renameVariable_TwoVariablesDifferentType() { - // Expect triggered error because of different types + // Expect the rename to succeed, because variables with different types are + // allowed to have the same name. workspaceTest_setUp(); + var id1 = 'id1'; + var id2 = 'id2'; + var type1 = 'type1'; + var type2 = 'type2'; + var oldName = 'name1'; var newName = 'name2'; - workspace.createVariable(oldName, 'type1', 'id1'); - workspace.createVariable(newName, 'type2', 'id2'); - createMockBlock(oldName); - createMockBlock(newName); + // Create two variables of different types. + workspace.createVariable(oldName, type1, id1); + workspace.createVariable(newName, type2, id2); + // Create blocks to refer to both of them. + createMockBlock(id1); + createMockBlock(id2); - try { - workspace.renameVariable(oldName, newName); - fail(); - } catch (e) { - // expected - } - checkVariableValues(workspace, oldName, 'type1', 'id1'); - checkVariableValues(workspace, newName, 'type2', 'id2'); - var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; - var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; - assertEquals(oldName, block_var_name_1); + workspace.renameVariableById(id1, newName); + + checkVariableValues(workspace, newName, type1, id1); + checkVariableValues(workspace, newName, type2, id2); + + // References shoul have the correct names. + var block_var_name_1 = workspace.topBlocks_[0].getVarModels()[0].name; + var block_var_name_2 = workspace.topBlocks_[1].getVarModels()[0].name; + assertEquals(newName, block_var_name_1); assertEquals(newName, block_var_name_2); + workspaceTest_tearDown(); } function test_renameVariable_OldCase() { - // Expect triggered error because of different types + // Rename a variable with a single reference. Update only the capitalization. workspaceTest_setUp(); var oldCase = 'Name1'; var newName = 'name1'; - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable(oldCase, '', 'id1'); - createMockBlock(oldCase); + var type = 'type1'; + var id = 'id1'; + workspace.createVariable(oldCase, type, id); + createMockBlock(id); - workspace.renameVariable(oldCase, newName); - checkVariableValues(workspace, newName, '', 'id1'); - var result_oldCase = workspace.getVariable(oldCase, '').name; - assertNotEquals(oldCase, result_oldCase); + workspace.renameVariableById(id, newName); + checkVariableValues(workspace, newName, type, id); + var variable = workspace.getVariableById(id); + assertNotEquals(oldCase, id.name); workspaceTest_tearDown(); } function test_renameVariable_TwoVariablesAndOldCase() { - // Expect triggered error because of different types + // Test renaming a variable to an in-use name, but with different + // capitalization. The new capitalization should apply everywhere. + + // TODO (fenichel): What about different capitalization but also different + // types? workspaceTest_setUp(); var oldName = 'name1'; var oldCase = 'Name2'; var newName = 'name2'; - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable(oldName, '', 'id1'); - workspace.createVariable(oldCase, '', 'id2'); - createMockBlock(oldName); - createMockBlock(oldCase); - workspace.renameVariable(oldName, newName); + var id1 = 'id1'; + var id2 = 'id2'; - checkVariableValues(workspace, newName, '', 'id2'); - var variable = workspace.getVariable(oldName); - var result_oldCase = workspace.getVariable(oldCase).name; - var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; - var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; + var type = 'type1'; + + workspace.createVariable(oldName, type, id1); + workspace.createVariable(oldCase, type, id2); + createMockBlock(id1); + createMockBlock(id2); + + workspace.renameVariableById(id1, newName); + + checkVariableValues(workspace, newName, type, id2); + + // The old variable should have been deleted. + var variable = workspace.getVariableById(id1); assertNull(variable); - assertNotEquals(oldCase, result_oldCase); + + // There should only be one variable left. + assertEquals(1, workspace.getAllVariables().length); + + // Blocks should now use the new capitalization. + var block_var_name_1 = workspace.topBlocks_[0].getVarModels()[0].name; + var block_var_name_2 = workspace.topBlocks_[1].getVarModels()[0].name; assertEquals(newName, block_var_name_1); assertEquals(newName, block_var_name_2); workspaceTest_tearDown(); } -// Extra testing not required for renameVariableById. It calls renameVariable -// and that has extensive testing. -function test_renameVariableById_TwoVariablesSameType() { - // Expect 'renameVariableById' to change oldName variable name to newName. - // Expect oldName block name to change to newName - workspaceTest_setUp(); - var oldName = 'name1'; - var newName = 'name2'; - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable(oldName, '', 'id1'); - workspace.createVariable(newName, '', 'id2'); - createMockBlock(oldName); - createMockBlock(newName); - - workspace.renameVariableById('id1', newName); - checkVariableValues(workspace, newName, '', 'id2'); - var variable = workspace.getVariable(oldName); - var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; - var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; - assertNull(variable); - assertEquals(newName, block_var_name_1); - assertEquals(newName, block_var_name_2); - workspaceTest_tearDown(); -} - -function test_deleteVariable_Trivial() { - workspaceTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name2'); - - workspace.deleteVariable('name1', ''); - checkVariableValues(workspace, 'name2', '', 'id2'); - var variable = workspace.getVariable('name1', ''); - var block_var_name = workspace.topBlocks_[0].getVars()[0]; - assertNull(variable); - assertEquals('name2', block_var_name); - workspaceTest_tearDown(); -} - function test_deleteVariableById_Trivial() { workspaceTest_setUp(); - // TODO (#1199): Make a version of this test that uses different types. - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name2'); + workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + createMockBlock('id1'); + createMockBlock('id2'); workspace.deleteVariableById('id1'); - checkVariableValues(workspace, 'name2', '', 'id2'); - var variable = workspace.getVariable('name1', ''); - var block_var_name = workspace.topBlocks_[0].getVars()[0]; + checkVariableValues(workspace, 'name2', 'type2', 'id2'); + var variable = workspace.getVariableById('id1'); + var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; assertNull(variable); assertEquals('name2', block_var_name); workspaceTest_tearDown(); From a7b98f7479f28c88cec59cd641e940392e1e75f6 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Fri, 1 Dec 2017 16:22:52 -0800 Subject: [PATCH 09/26] Move code from renameVariableInternal to renameVariableById --- core/workspace.js | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/core/workspace.js b/core/workspace.js index 3919190bd..2c4205158 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -238,13 +238,17 @@ Blockly.Workspace.prototype.updateVariableStore = function(clear) { /** * Rename a variable by updating its name in the variable map. Identify the - * variable to rename with the given variable. - * @param {?Blockly.VariableModel} variable Variable to rename. + * variable to rename with the given ID. + * @param {string} id ID of the variable to rename. * @param {string} newName New variable name. */ -Blockly.Workspace.prototype.renameVariableInternal_ = function( - variable, newName) { - var type = variable ? variable.type : ''; +Blockly.Workspace.prototype.renameVariableById = function(id, newName) { + var variable = this.getVariableById(id); + if (!variable) { + throw new Error('Tried to rename a variable that didn\'t exist. ID: ' + id); + } + + var type = variable.type; var newVariable = this.getVariable(newName, type); // If a variable previously existed with the new name, we will coalesce the @@ -264,32 +268,16 @@ Blockly.Workspace.prototype.renameVariableInternal_ = function( var blocks = this.getAllBlocks(); this.variableMap_.renameVariable(variable, newName, type); - // Iterate through every block and update name. for (var i = 0; i < blocks.length; i++) { blocks[i].renameVarById(oldId, newId); if (oldCase) { blocks[i].renameVarById(newId, newId); - //blocks[i].renameVar(oldCase, newName); } } Blockly.Events.setGroup(false); }; -/** - * Rename a variable by updating its name in the variable map. Identify the - * variable to rename with the given ID. - * @param {string} id ID of the variable to rename. - * @param {string} newName New variable name. - */ -Blockly.Workspace.prototype.renameVariableById = function(id, newName) { - var variable = this.getVariableById(id); - if (!variable) { - throw new Error('Tried to rename a variable that didn\'t exist. ID: ' + id); - } - this.renameVariableInternal_(variable, newName); -}; - /** * Create a variable with a given name, optional type, and optional ID. * @param {!string} name The name of the variable. This must be unique across From f3f3f34fc2cd44a7de201c3a68148f37eaba51b2 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Mon, 4 Dec 2017 14:19:19 -0800 Subject: [PATCH 10/26] All but XML tests now pass --- core/field_variable.js | 7 ++ core/workspace.js | 3 +- tests/jsunit/field_variable_test.js | 7 +- tests/jsunit/index.html | 4 +- tests/jsunit/procedures_test.js | 11 +- tests/jsunit/test_utilities.js | 19 +++ tests/jsunit/workspace_test.js | 37 ++---- tests/jsunit/workspace_undo_redo_test.js | 143 ++++++++++------------- tests/jsunit/xml_test.js | 6 +- 9 files changed, 118 insertions(+), 119 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index 9bff99214..0b9238f02 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -167,6 +167,8 @@ Blockly.FieldVariable.prototype.getText = function() { * variable. */ Blockly.FieldVariable.prototype.setValue = function(id) { + // TODO: Handle undo a change to go back to the default value. + // TODO: Handle null ID, which means "use default". var workspace = this.sourceBlock_.workspace; var variable = workspace.getVariableById(id); if (!variable) { @@ -189,6 +191,11 @@ Blockly.FieldVariable.prototype.setValue = function(id) { throw new Error('Variable type doesn\'t match this field! Type was ' + type); } + if (this.sourceBlock_ && Blockly.Events.isEnabled()) { + var oldValue = this.variable_ ? this.variable_.getId() : null; + Blockly.Events.fire(new Blockly.Events.BlockChange( + this.sourceBlock_, 'field', this.name, oldValue, variable.getId())); + } this.variable_ = variable; this.setText(variable.name); }; diff --git a/core/workspace.js b/core/workspace.js index 2c4205158..6fed60405 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -266,7 +266,6 @@ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { Blockly.Events.setGroup(true); var blocks = this.getAllBlocks(); - this.variableMap_.renameVariable(variable, newName, type); // Iterate through every block and update name. for (var i = 0; i < blocks.length; i++) { @@ -275,6 +274,8 @@ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { blocks[i].renameVarById(newId, newId); } } + + this.variableMap_.renameVariable(variable, newName, type); Blockly.Events.setGroup(false); }; diff --git a/tests/jsunit/field_variable_test.js b/tests/jsunit/field_variable_test.js index 0cf1f11f8..6d3ef9d66 100644 --- a/tests/jsunit/field_variable_test.js +++ b/tests/jsunit/field_variable_test.js @@ -68,8 +68,9 @@ function test_fieldVariable_setValueMatchId() { var fieldVariable = fieldVariable_createAndInitField(workspace); + var oldId = fieldVariable.getValue(); var event = new Blockly.Events.BlockChange( - fieldVariable.sourceBlock_, 'field', undefined, 'name1', 'id2'); + fieldVariable.sourceBlock_, 'field', undefined, oldId, 'id2'); setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null); fieldVariable.setValue('id2'); @@ -87,10 +88,6 @@ function test_fieldVariable_setValueNoVariable() { return false; }; - var event = new Blockly.Events.BlockChange( - mockBlock, 'field', undefined, 'name1', 'id1'); - setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null); - try { fieldVariable.setValue('id1'); // Calling setValue with a variable that doesn't exist throws an error. diff --git a/tests/jsunit/index.html b/tests/jsunit/index.html index 500f2cb37..96424df44 100644 --- a/tests/jsunit/index.html +++ b/tests/jsunit/index.html @@ -8,7 +8,7 @@ - + diff --git a/tests/jsunit/procedures_test.js b/tests/jsunit/procedures_test.js index 0284f3978..f993af892 100644 --- a/tests/jsunit/procedures_test.js +++ b/tests/jsunit/procedures_test.js @@ -37,14 +37,15 @@ function proceduresTest_setUpWithMockBlocks() { 'message0': '%1', 'args0': [ { + // TODO: Why is this a variable? It shouldn't need to be. 'type': 'field_variable', 'name': 'NAME', 'variable': 'item' } - ], + ] }]); Blockly.Blocks['procedure_mock_block'].getProcedureDef = function() { - return [this.getFieldValue('NAME'), [], false]; + return [this.getField('NAME').getText(), [], false]; }; } @@ -63,8 +64,9 @@ function test_isNameUsed_NoBlocks() { function test_isNameUsed_False() { proceduresTest_setUpWithMockBlocks(); + workspace.createVariable('name2', '', 'id2'); var block = new Blockly.Block(workspace, 'procedure_mock_block'); - block.setFieldValue('name2', 'NAME'); + block.setFieldValue('id2', 'NAME'); var result = Blockly.Procedures.isNameUsed('name1', workspace); assertFalse(result); @@ -73,8 +75,9 @@ function test_isNameUsed_False() { function test_isNameUsed_True() { proceduresTest_setUpWithMockBlocks(); + workspace.createVariable('name1', '', 'id1'); var block = new Blockly.Block(workspace, 'procedure_mock_block'); - block.setFieldValue('name1', 'NAME'); + block.setFieldValue('id1', 'NAME'); var result = Blockly.Procedures.isNameUsed('name1', workspace); assertTrue(result); diff --git a/tests/jsunit/test_utilities.js b/tests/jsunit/test_utilities.js index 6a80a3549..d7f6085bf 100644 --- a/tests/jsunit/test_utilities.js +++ b/tests/jsunit/test_utilities.js @@ -89,3 +89,22 @@ function checkVariableValues(container, name, type, id) { assertEquals(type, variable.type); assertEquals(id, variable.getId()); } + +/** + * Create a test get_var_block. + * Will fail if get_var_block isn't defined. + * TODO (fenichel): Rename to createMockVarBlock. + * @param {!string} variable_id The id of the variable to reference. + * @return {!Blockly.Block} The created block. + */ +function createMockBlock(variable_id) { + if (!Blockly.Blocks['get_var_block']) { + fail(); + } + // Turn off events to avoid testing XML at the same time. + Blockly.Events.disable(); + var block = new Blockly.Block(workspace, 'get_var_block'); + block.inputList[0].fieldRow[0].setValue(variable_id); + Blockly.Events.enable(); + return block; +} diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index d39eac1e4..6e4d960d4 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -24,42 +24,29 @@ goog.require('goog.testing.MockControl'); var workspace; var mockControl_; -Blockly.defineBlocksWithJsonArray([{ - "type": "get_var_block", - "message0": "%1", - "args0": [ - { - "type": "field_variable", - "name": "VAR", - "variableTypes": ["", "type1", "type2"] - } - ] -}]); function workspaceTest_setUp() { + Blockly.defineBlocksWithJsonArray([{ + "type": "get_var_block", + "message0": "%1", + "args0": [ + { + "type": "field_variable", + "name": "VAR", + "variableTypes": ["", "type1", "type2"] + } + ] + }]); workspace = new Blockly.Workspace(); mockControl_ = new goog.testing.MockControl(); } function workspaceTest_tearDown() { + delete Blockly.Blocks['get_var_block']; mockControl_.$tearDown(); workspace.dispose(); } -/** - * Create a test get_var_block. - * @param {?string} variable_name The string to put into the variable field. - * @return {!Blockly.Block} The created block. - */ -function createMockBlock(variable_id) { - // Turn off events to avoid testing XML at the same time. - Blockly.Events.disable(); - var block = new Blockly.Block(workspace, 'get_var_block'); - block.inputList[0].fieldRow[0].setValue(variable_id); - Blockly.Events.enable(); - return block; -} - function test_emptyWorkspace() { workspaceTest_setUp(); try { diff --git a/tests/jsunit/workspace_undo_redo_test.js b/tests/jsunit/workspace_undo_redo_test.js index a8befe33c..092dbb4a8 100644 --- a/tests/jsunit/workspace_undo_redo_test.js +++ b/tests/jsunit/workspace_undo_redo_test.js @@ -33,16 +33,6 @@ goog.require('goog.testing.MockControl'); var workspace; var mockControl_; var savedFireFunc = Blockly.Events.fire; -Blockly.defineBlocksWithJsonArray([{ - "type": "get_var_block", - "message0": "%1", - "args0": [ - { - "type": "field_variable", - "name": "VAR", - } - ] -}]); function temporary_fireEvent(event) { if (!Blockly.Events.isEnabled()) { @@ -53,28 +43,29 @@ function temporary_fireEvent(event) { } function undoRedoTest_setUp() { + Blockly.defineBlocksWithJsonArray([{ + "type": "get_var_block", + "message0": "%1", + "args0": [ + { + "type": "field_variable", + "name": "VAR", + "variableTypes": ["", "type1", "type2"] + } + ] + }]); workspace = new Blockly.Workspace(); mockControl_ = new goog.testing.MockControl(); Blockly.Events.fire = temporary_fireEvent; } function undoRedoTest_tearDown() { + delete Blockly.Blocks['get_var_block']; mockControl_.$tearDown(); workspace.dispose(); Blockly.Events.fire = savedFireFunc; } -/** - * Create a test get_var_block. - * @param {string} variableName The string to put into the variable field. - * @return {!Blockly.Block} The created block. - */ -function createMockBlock(variableName) { - var block = new Blockly.Block(workspace, 'get_var_block'); - block.inputList[0].fieldRow[0].setValue(variableName); - return block; -} - /** * Check that the top block with the given index contains a variable with * the given name. @@ -82,7 +73,7 @@ function createMockBlock(variableName) { * @param {string} name The expected name of the variable in the block. */ function undoRedoTest_checkBlockVariableName(blockIndex, name) { - var blockVarName = workspace.topBlocks_[blockIndex].getVars()[0]; + var blockVarName = workspace.topBlocks_[blockIndex].getVarModels()[0].name; assertEquals(name, blockVarName); } @@ -146,25 +137,23 @@ function test_undoDeleteVariable_NoBlocks() { function test_undoDeleteVariable_WithBlocks() { undoRedoTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name2'); + workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + createMockBlock('id1'); + createMockBlock('id2'); workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name2'); assertNull(workspace.getVariableById('id1')); - checkVariableValues(workspace, 'name2', '', 'id2'); + checkVariableValues(workspace, 'name2', 'type2', 'id2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name2'); undoRedoTest_checkBlockVariableName(1, 'name1'); - checkVariableValues(workspace, 'name1', '', 'id1'); - checkVariableValues(workspace, 'name2', '', 'id2'); + checkVariableValues(workspace, 'name1', 'type1', 'id1'); + checkVariableValues(workspace, 'name2', 'type2', 'id2'); undoRedoTest_tearDown(); } @@ -192,12 +181,10 @@ function test_redoAndUndoDeleteVariable_NoBlocks() { function test_redoAndUndoDeleteVariable_WithBlocks() { undoRedoTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name2'); + workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + createMockBlock('id1'); + createMockBlock('id2'); workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); @@ -214,7 +201,7 @@ function test_redoAndUndoDeleteVariable_WithBlocks() { // Expect that variable 'id2' is recreated undoRedoTest_checkBlockVariableName(0, 'name2'); assertNull(workspace.getVariableById('id1')); - checkVariableValues(workspace, 'name2', '', 'id2'); + checkVariableValues(workspace, 'name2', 'type2', 'id2'); undoRedoTest_tearDown(); } @@ -245,12 +232,11 @@ function test_redoAndUndoDeleteVariableTwice_NoBlocks() { function test_redoAndUndoDeleteVariableTwice_WithBlocks() { undoRedoTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable('name1', '', 'id1'); - createMockBlock('name1'); - workspace.deleteVariableById('id1'); - workspace.deleteVariableById('id1'); + var id = 'id1'; + workspace.createVariable('name1', 'type1', id); + createMockBlock(id); + workspace.deleteVariableById(id); + workspace.deleteVariableById(id); // Check the undoStack only recorded one delete event. var undoStack = workspace.undoStack_; @@ -261,45 +247,45 @@ function test_redoAndUndoDeleteVariableTwice_WithBlocks() { // undo delete workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); - checkVariableValues(workspace, 'name1', '', 'id1'); + checkVariableValues(workspace, 'name1', 'type1', id); // redo delete workspace.undo(true); assertEquals(0, workspace.topBlocks_.length); - assertNull(workspace.getVariableById('id1')); + assertNull(workspace.getVariableById(id)); // redo delete, nothing should happen workspace.undo(true); assertEquals(0, workspace.topBlocks_.length); - assertNull(workspace.getVariableById('id1')); + assertNull(workspace.getVariableById(id)); undoRedoTest_tearDown(); } -function test_undoRedoRenameVariable_NeitherVariableExists() { - // Expect that a variable with the name, 'name2', and the generated UUID, - // 'id2', to be created when rename is called. Undo removes this variable - // and redo recreates it. - undoRedoTest_setUp(); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, - ['rename_group', 'id2', 'delete_group']); - workspace.renameVariable('name1', 'name2'); +// TODO: Decide if this needs to be possible. +// function test_undoRedoRenameVariable_NeitherVariableExists() { +// // Expect that a variable with the name, 'name2', and the generated UUID, +// // 'id2', to be created when rename is called. Undo removes this variable +// // and redo recreates it. +// undoRedoTest_setUp(); +// setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, +// ['rename_group', 'id2', 'delete_group']); +// workspace.renameVariable('name1', 'name2'); - workspace.undo(); - assertNull(workspace.getVariableById('id2')); +// workspace.undo(); +// assertNull(workspace.getVariableById('id2')); - workspace.undo(true); - checkVariableValues(workspace, 'name2', '', 'id2'); - undoRedoTest_tearDown(); -} +// workspace.undo(true); +// checkVariableValues(workspace, 'name2', '', 'id2'); +// undoRedoTest_tearDown(); +// } function test_undoRedoRenameVariable_OneExists_NoBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); - workspace.renameVariable('name1', 'name2'); + workspace.renameVariableById('id1', 'name2'); workspace.undo(); checkVariableValues(workspace, 'name1', '', 'id1'); - assertNull(workspace.getVariable('name2')); workspace.undo(true); checkVariableValues(workspace, 'name2', '', 'id1'); @@ -309,13 +295,12 @@ function test_undoRedoRenameVariable_OneExists_NoBlocks() { function test_undoRedoRenameVariable_OneExists_WithBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); - createMockBlock('name1'); - workspace.renameVariable('name1', 'name2'); + createMockBlock('id1'); + workspace.renameVariableById('id1', 'name2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); checkVariableValues(workspace, 'name1', '', 'id1'); - assertNull(workspace.getVariable('name2')); workspace.undo(true); checkVariableValues(workspace, 'name2', '', 'id1'); @@ -326,7 +311,7 @@ function test_undoRedoRenameVariable_OneExists_WithBlocks() { function test_undoRedoRenameVariable_BothExist_NoBlocks() { undoRedoTest_setUp(); createTwoVarsEmptyType(); - workspace.renameVariable('name1', 'name2'); + workspace.renameVariableById('id1', 'name2'); workspace.undo(); checkVariableValues(workspace, 'name1', '', 'id1'); @@ -334,16 +319,16 @@ function test_undoRedoRenameVariable_BothExist_NoBlocks() { workspace.undo(true); checkVariableValues(workspace, 'name2', '', 'id2'); - assertNull(workspace.getVariable('name1')); + assertNull(workspace.getVariableById('id1')); undoRedoTest_tearDown(); } function test_undoRedoRenameVariable_BothExist_WithBlocks() { undoRedoTest_setUp(); createTwoVarsEmptyType(); - createMockBlock('name1'); - createMockBlock('name2'); - workspace.renameVariable('name1', 'name2'); + createMockBlock('id1'); + createMockBlock('id2'); + workspace.renameVariableById('id1', 'name2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); @@ -360,7 +345,7 @@ function test_undoRedoRenameVariable_BothExist_WithBlocks() { function test_undoRedoRenameVariable_BothExistCaseChange_NoBlocks() { undoRedoTest_setUp(); createTwoVarsEmptyType(); - workspace.renameVariable('name1', 'Name2'); + workspace.renameVariableById('id1', 'Name2'); workspace.undo(); checkVariableValues(workspace, 'name1', '', 'id1'); @@ -375,9 +360,9 @@ function test_undoRedoRenameVariable_BothExistCaseChange_NoBlocks() { function test_undoRedoRenameVariable_BothExistCaseChange_WithBlocks() { undoRedoTest_setUp(); createTwoVarsEmptyType(); - createMockBlock('name1'); - createMockBlock('name2'); - workspace.renameVariable('name1', 'Name2'); + createMockBlock('id1'); + createMockBlock('id2'); + workspace.renameVariableById('id1', 'Name2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); @@ -387,7 +372,7 @@ function test_undoRedoRenameVariable_BothExistCaseChange_WithBlocks() { workspace.undo(true); checkVariableValues(workspace, 'Name2', '', 'id2'); - assertNull(workspace.getVariable('name1')); + assertNull(workspace.getVariableById('id1')); undoRedoTest_checkBlockVariableName(0, 'Name2'); undoRedoTest_checkBlockVariableName(1, 'Name2'); undoRedoTest_tearDown(); @@ -396,7 +381,7 @@ function test_undoRedoRenameVariable_BothExistCaseChange_WithBlocks() { function test_undoRedoRenameVariable_OnlyCaseChange_NoBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); - workspace.renameVariable('name1', 'Name1'); + workspace.renameVariableById('id1', 'Name1'); workspace.undo(); checkVariableValues(workspace, 'name1', '', 'id1'); @@ -409,8 +394,8 @@ function test_undoRedoRenameVariable_OnlyCaseChange_NoBlocks() { function test_undoRedoRenameVariable_OnlyCaseChange_WithBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); - createMockBlock('name1'); - workspace.renameVariable('name1', 'Name1'); + createMockBlock('id1'); + workspace.renameVariableById('id1', 'Name1'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); diff --git a/tests/jsunit/xml_test.js b/tests/jsunit/xml_test.js index 0c2af7441..f8ebe19fb 100644 --- a/tests/jsunit/xml_test.js +++ b/tests/jsunit/xml_test.js @@ -286,10 +286,10 @@ function test_appendDomToWorkspace() { function test_blockToDom_fieldToDom_trivial() { xmlTest_setUpWithMockBlocks(); // TODO (#1199): make a similar test where the variable is given a non-empty - // type. + // type.f workspace.createVariable('name1', '', 'id1'); var block = new Blockly.Block(workspace, 'field_variable_test_block'); - block.inputList[0].fieldRow[0].setValue('name1'); + block.inputList[0].fieldRow[0].setValue('id1'); var resultFieldDom = Blockly.Xml.blockToDom(block).childNodes[0]; xmlTest_checkVariableFieldDomValues(resultFieldDom, 'VAR', '', 'id1', 'name1'); @@ -301,7 +301,7 @@ function test_blockToDom_fieldToDom_defaultCase() { setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1', '1']); workspace.createVariable('name1'); var block = new Blockly.Block(workspace, 'field_variable_test_block'); - block.inputList[0].fieldRow[0].setValue('name1'); + block.inputList[0].fieldRow[0].setValue('1'); var resultFieldDom = Blockly.Xml.blockToDom(block).childNodes[0]; // Expect type is '' and id is '1' since we don't specify type and id. xmlTest_checkVariableFieldDomValues(resultFieldDom, 'VAR', '', '1', 'name1'); From 2bb66aa9aa72ac26e53e22a0226c97dd1fc0ba20 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Mon, 4 Dec 2017 14:29:16 -0800 Subject: [PATCH 11/26] All tests pass --- core/variables.js | 8 -------- core/xml.js | 2 -- tests/jsunit/index.html | 4 ++-- tests/jsunit/xml_test.js | 18 ++++++++---------- 4 files changed, 10 insertions(+), 22 deletions(-) diff --git a/core/variables.js b/core/variables.js index 26dc0b452..d43ae7bb2 100644 --- a/core/variables.js +++ b/core/variables.js @@ -338,12 +338,4 @@ Blockly.Variables.generateVariableFieldXml_ = function(variableModel) { '" variabletype="' + typeString + '">' + variableModel.name + ''; return text; - // var element = goog.dom.createDom('field'); - // element.setAttribute('name', 'VAR'); - // element.setAttribute('variabletype', variableModel.type); - // element.setAttribute('id', variableModel.getId()); - // element.textContent = variableModel.name; - - // var xmlString = Blockly.Xml.domToText(element); - // return xmlString; }; diff --git a/core/xml.js b/core/xml.js index f71c3b52a..9946d3832 100644 --- a/core/xml.js +++ b/core/xml.js @@ -87,7 +87,6 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) { }; Blockly.Xml.fieldToDomVariable_ = function(field, workspace) { - // Ugh that's not true at all. var id = field.getValue(); var variable = workspace.getVariableById(id); if (!variable) { @@ -420,7 +419,6 @@ Blockly.Xml.domToWorkspace = function(xml, workspace) { } Blockly.Field.stopCache(); } - workspace.updateVariableStore(false); // Re-enable workspace resizing. if (workspace.setResizesEnabled) { workspace.setResizesEnabled(true); diff --git a/tests/jsunit/index.html b/tests/jsunit/index.html index 96424df44..619e4143b 100644 --- a/tests/jsunit/index.html +++ b/tests/jsunit/index.html @@ -8,7 +8,7 @@ - + diff --git a/tests/jsunit/xml_test.js b/tests/jsunit/xml_test.js index f8ebe19fb..a2f929e51 100644 --- a/tests/jsunit/xml_test.js +++ b/tests/jsunit/xml_test.js @@ -345,14 +345,14 @@ function test_variablesToDom_oneVariable() { function test_variablesToDom_twoVariables_oneBlock() { xmlTest_setUpWithMockBlocks(); - workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name1', '', 'id1'); workspace.createVariable('name2', 'type2', 'id2'); var block = new Blockly.Block(workspace, 'field_variable_test_block'); - block.inputList[0].fieldRow[0].setValue('name1'); + block.inputList[0].fieldRow[0].setValue('id1'); var resultDom = Blockly.Xml.variablesToDom(workspace.getAllVariables()); assertEquals(2, resultDom.children.length); - xmlTest_checkVariableDomValues(resultDom.children[0], 'type1', 'id1', + xmlTest_checkVariableDomValues(resultDom.children[0], '', 'id1', 'name1'); xmlTest_checkVariableDomValues(resultDom.children[1], 'type2', 'id2', 'name2'); @@ -380,14 +380,12 @@ function test_variableFieldXml_caseSensitive() { } }; - var generatedXml = Blockly.Variables.generateVariableFieldXml_(mockVariableModel); - // The field contains this XML tag as a result of how we're generating this - // XML. This is not desirable, but the goal of this test is to make sure - // we're preserving case-sensitivity. - var xmlns = 'xmlns="http://www.w3.org/1999/xhtml"'; + var generatedXml = + Blockly.Variables.generateVariableFieldXml_(mockVariableModel); var goldenXml = - '' + name + ''; + '>' + name + ''; assertEquals(goldenXml, generatedXml); } From 7df7750b195bc5280a12b0f1d8f26c75da047f08 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Mon, 4 Dec 2017 15:46:44 -0800 Subject: [PATCH 12/26] Make unique variable names in the flyout --- core/field_variable.js | 9 ++++++++- core/flyout_base.js | 4 ++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/core/field_variable.js b/core/field_variable.js index 0b9238f02..67604d2a5 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -63,6 +63,7 @@ Blockly.FieldVariable.getOrCreateVariable = function(workspace, text, type, id) { var potentialVariableMap = workspace.isFlyout ? workspace.targetWorkspace.potentialVariableMap_ : null; + if (id) { var variable = workspace.getVariableById(id); if (!variable && potentialVariableMap) { @@ -76,6 +77,12 @@ Blockly.FieldVariable.getOrCreateVariable = function(workspace, text, type, } // Didn't find the variable. if (!variable) { + if (!text) { + var ws = workspace.isFlyout ? workspace.targetWorkspace : workspace; + // Variables without names get uniquely named for this workspace. + text = Blockly.Variables.generateUniqueName(ws); + } + if (potentialVariableMap) { variable = potentialVariableMap.createVariable(text, type, id); } else { @@ -126,7 +133,7 @@ Blockly.FieldVariable.prototype.initModel = function() { } this.workspace_ = this.sourceBlock_.workspace; var variable = Blockly.FieldVariable.getOrCreateVariable( - this.workspace_, this.defaultVariableName, this.defaultType_, null); + this.workspace_, name, this.defaultType_, null); this.setValue(variable.getId()); }; diff --git a/core/flyout_base.js b/core/flyout_base.js index c56ea6253..2e3a33b2c 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -415,6 +415,7 @@ Blockly.Flyout.prototype.hide = function() { this.workspace_.removeChangeListener(this.reflowWrapper_); this.reflowWrapper_ = null; } + // Do NOT delete the blocks here. Wait until Flyout.show. // https://neil.fraser.name/news/2014/08/09/ }; @@ -542,6 +543,9 @@ Blockly.Flyout.prototype.clearOldBlocks_ = function() { button.dispose(); } this.buttons_.length = 0; + + // Clear potential variables from the previous showing. + this.targetWorkspace_.potentialVariableMap_.clear(); }; /** From f7438d9c95fc2a38d7dc4b448f708db5da76d080 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Mon, 4 Dec 2017 15:52:53 -0800 Subject: [PATCH 13/26] Fix default variable name --- core/field_variable.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/field_variable.js b/core/field_variable.js index 67604d2a5..d6d011dbc 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -133,7 +133,7 @@ Blockly.FieldVariable.prototype.initModel = function() { } this.workspace_ = this.sourceBlock_.workspace; var variable = Blockly.FieldVariable.getOrCreateVariable( - this.workspace_, name, this.defaultType_, null); + this.workspace_, this.defaultVariableName, this.defaultType_, null); this.setValue(variable.getId()); }; From 9e0d9081335a1457f3e8c0500d008b78ddc9e480 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Tue, 5 Dec 2017 13:08:01 -0800 Subject: [PATCH 14/26] Move getOrCreateVariable to variables.js --- blocks/procedures.js | 4 ++ core/field_variable.js | 109 +++++++++++---------------------------- core/flyout_base.js | 1 - core/variables.js | 113 +++++++++++++++++++++++++++++++++++++++-- core/workspace.js | 23 +++++++++ core/xml.js | 4 +- 6 files changed, 168 insertions(+), 86 deletions(-) diff --git a/blocks/procedures.js b/blocks/procedures.js index 0b5866188..5fc29d302 100644 --- a/blocks/procedures.js +++ b/blocks/procedures.js @@ -266,6 +266,10 @@ Blockly.Blocks['procedures_defnoreturn'] = { */ getVarModels: function() { var vars = []; + // Not fully initialized. + if (!this.arguments_) { + return vars; + } for (var i = 0, argName; argName = this.arguments_[i]; i++) { // TODO (#1199): When we switch to tracking variables by ID, // update this. diff --git a/core/field_variable.js b/core/field_variable.js index d6d011dbc..14e2b1fbf 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -47,11 +47,11 @@ goog.require('goog.string'); * @constructor */ Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes) { - // Don't call the FieldDropdown constructor. It'll try too hard. + // 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); - //this.setValue(varname || ''); // TODO: Add opt_default_type to match default value. If not set, ''. this.defaultVariableName = (varname || ''); this.defaultType_ = ''; @@ -59,42 +59,11 @@ Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes) { }; goog.inherits(Blockly.FieldVariable, Blockly.FieldDropdown); -Blockly.FieldVariable.getOrCreateVariable = function(workspace, text, type, - id) { - var potentialVariableMap = workspace.isFlyout ? - workspace.targetWorkspace.potentialVariableMap_ : null; - - if (id) { - var variable = workspace.getVariableById(id); - if (!variable && potentialVariableMap) { - variable = potentialVariableMap.getVariableById(id); - } - } else { - var variable = workspace.getVariable(text, type); - if (!variable && potentialVariableMap) { - variable = potentialVariableMap.getVariable(text, type); - } - } - // Didn't find the variable. - if (!variable) { - if (!text) { - var ws = workspace.isFlyout ? workspace.targetWorkspace : workspace; - // Variables without names get uniquely named for this workspace. - text = Blockly.Variables.generateUniqueName(ws); - } - - if (potentialVariableMap) { - variable = potentialVariableMap.createVariable(text, type, id); - } else { - variable = workspace.createVariable(text, type, id); - } - } - - return variable; -}; /** - * Install this dropdown on a block. + * Initialize everything needed to render this field. This includes making sure + * that the field's value is valid. + * @public */ Blockly.FieldVariable.prototype.init = function() { if (this.fieldGroup_) { @@ -107,36 +76,26 @@ Blockly.FieldVariable.prototype.init = function() { this.initModel(); }; +/** + * Initialize the model for this field if it has not already been initialized. + * If the value has not been set to a variable by the first render, we make up a + * variable rather than let the value be invalid. + * @package + */ Blockly.FieldVariable.prototype.initModel = function() { - // this.workspace_ = this.sourceBlock_.isInFlyout ? - // this.sourceBlock_.workspace.targetWorkspace : - // this.sourceBlock_.workspace; - // // TODO: Describe how the potential variable map is different from the variable - // // map; use getters. - // this.variableMap_ = this.sourceBlock_.isInFlyout ? - // this.workspace_.potentialVariableMap_ : this.workspace_.variableMap_; - // var name = this.defaultValue; - // if (!name) { - // // Variables without names get uniquely named for this workspace. - // name = Blockly.Variables.generateUniqueName(this.workspace_); - // } - // // If the selected variable doesn't exist yet, create it. - // // For instance, some blocks in the toolbox have variable dropdowns filled - // // in by default. - - // var variable = this.variableMap_.getVariable(name, this.defaultType_); - // if (!variable) { - // variable = this.variableMap_.createVariable(name, this.defaultType_); - // } if (this.variable_) { return; // Initialization already happened. } this.workspace_ = this.sourceBlock_.workspace; - var variable = Blockly.FieldVariable.getOrCreateVariable( - this.workspace_, this.defaultVariableName, this.defaultType_, null); + var variable = Blockly.Variables.getOrCreateVariable( + this.workspace_, null, this.defaultVariableName, this.defaultType_); this.setValue(variable.getId()); }; +/** + * Dispose of this field. + * @public + */ Blockly.FieldVariable.dispose = function() { Blockly.FieldVariable.superClass_.dispose.call(this); this.workspace_ = null; @@ -154,15 +113,17 @@ Blockly.FieldVariable.prototype.setSourceBlock = function(block) { }; /** - * Get the variable's name (use a variableDB to convert into a real name). - * Unline a regular dropdown, variables are literal and have no neutral value. - * @return {string} Current text. + * Get the variable's ID. + * @return {string} Current variable's ID. */ Blockly.FieldVariable.prototype.getValue = function() { - //return this.getText(); return this.variable_ ? this.variable_.getId() : ''; }; +/** + * Get the text from this field. + * @return {string} Current text. + */ Blockly.FieldVariable.prototype.getText = function() { //return this.getText(); return this.variable_ ? this.variable_.name : ''; @@ -177,16 +138,7 @@ Blockly.FieldVariable.prototype.setValue = function(id) { // TODO: Handle undo a change to go back to the default value. // TODO: Handle null ID, which means "use default". var workspace = this.sourceBlock_.workspace; - var variable = workspace.getVariableById(id); - if (!variable) { - if (workspace.isFlyout && workspace.targetWorkspace) { - var potentialVariableMap = - workspace.targetWorkspace.potentialVariableMap_; - if (potentialVariableMap) { - variable = potentialVariableMap.getVariableById(id); - } - } - } + var variable = Blockly.Variables.getVariable(workspace, id); if (!variable) { throw new Error('Variable id doesn\'t point to a real variable! ID was ' + @@ -207,6 +159,12 @@ Blockly.FieldVariable.prototype.setValue = function(id) { this.setText(variable.name); }; +/** + * Check whether the given variable type is allowed on this field. + * @param {string} type The type to check. + * @return {boolean} True if the type is in the list of allowed types. + * @private + */ Blockly.FieldVariable.prototype.typeIsAllowed_ = function(type) { var typeList = this.getVariableTypes_(); if (!typeList) { @@ -300,16 +258,11 @@ Blockly.FieldVariable.dropdownCreate = function() { */ Blockly.FieldVariable.prototype.onItemSelected = function(menu, menuItem) { var id = menuItem.getValue(); - // TODO(marisaleung): change setValue() to take in an ID as the parameter. - // Then remove itemText. if (this.sourceBlock_ && this.sourceBlock_.workspace) { var workspace = this.sourceBlock_.workspace; - var variable = workspace.getVariableById(id); - // If the item selected is a variable, set itemText to the variable name. if (id == Blockly.RENAME_VARIABLE_ID) { // Rename variable. - variable = this.variable_; - Blockly.Variables.renameVariable(workspace, variable); + Blockly.Variables.renameVariable(workspace, this.variable_); return; } else if (id == Blockly.DELETE_VARIABLE_ID) { // Delete variable. diff --git a/core/flyout_base.js b/core/flyout_base.js index 2e3a33b2c..8b1421e39 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -415,7 +415,6 @@ Blockly.Flyout.prototype.hide = function() { this.workspace_.removeChangeListener(this.reflowWrapper_); this.reflowWrapper_ = null; } - // Do NOT delete the blocks here. Wait until Flyout.show. // https://neil.fraser.name/news/2014/08/09/ }; diff --git a/core/variables.js b/core/variables.js index d43ae7bb2..4d0167efa 100644 --- a/core/variables.js +++ b/core/variables.js @@ -65,13 +65,13 @@ Blockly.Variables.allUsedVariables = function(root) { // Iterate through every block and add each variable to the hash. for (var x = 0; x < blocks.length; x++) { // TODO (#1199) Switch to IDs. - var blockVariables = blocks[x].getVars(); + var blockVariables = blocks[x].getVarModels(); if (blockVariables) { for (var y = 0; y < blockVariables.length; y++) { - var varName = blockVariables[y]; - // Variable name may be null if the block is only half-built. - if (varName) { - variableHash[varName.toLowerCase()] = varName; + var variable = blockVariables[y]; + // Variable ID may be null if the block is only half-built. + if (variable.getId()) { + variableHash[variable.name.toLowerCase()] = variable.name; } } } @@ -339,3 +339,106 @@ Blockly.Variables.generateVariableFieldXml_ = function(variableModel) { '">' + variableModel.name + ''; return text; }; + +/** + * Helper function to look up or create a variable on the given workspace. + * If no variable exists, creates and returns it. + * @param {!Blockly.Workspace} workspace The workspace to search for the + * variable. It may be a flyout workspace or main workspace. + * @param {string} id The ID to use to look up or create the variable, or null. + * @param {string} name The string to use to look up or create the variable, + * @param {string} type The type to use to look up or create the variable. + * @return {!Blockly.VariableModel} The variable corresponding to the given ID + * or name + type combination. + * @package + */ +Blockly.Variables.getOrCreateVariable = function(workspace, id, name, type) { + var variable = Blockly.Variables.getVariable(workspace, id, name, type); + if (!variable) { + variable = Blockly.Variables.createVariable_(workspace, id, name, type); + } + return variable; +}; + +/** + * Look up a variable on the given workspace. + * Always looks in the main workspace before looking in the flyout workspace. + * Always prefers lookup by ID to lookup by name + type. + * @param {!Blockly.Workspace} workspace The workspace to search for the + * variable. It may be a flyout workspace or main workspace. + * @param {string} id The ID to use to look up the variable, or null. + * @param {string=} opt_name The string to use to look up the variable. Only + * used if lookup by ID fails. + * @param {string=} opt_type The type to use to look up the variable. Only used + * if lookup by ID fails. + * @return {?Blockly.VariableModel} The variable corresponding to the given ID + * or name + type combination, or null if not found. + * @private + */ +Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) { + var potentialVariableMap = + Blockly.Variables.getPotentialVariableMap_(workspace); + // Try to just get the variable, by ID if possible. + if (id) { + // Look in the real variable map before checking the potential variable map. + var variable = workspace.getVariableById(id); + if (!variable && potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + } else if (opt_name && (opt_type != undefined)){ + // Otherwise look up by name and type. + var variable = workspace.getVariable(opt_name, opt_type); + if (!variable && potentialVariableMap) { + variable = potentialVariableMap.getVariable(opt_name, opt_type); + } + } + return variable; +}; + +/** + * Helper function to create a variable on the given workspace. + * @param {!Blockly.Workspace} workspace The workspace in which to create the + * variable. It may be a flyout workspace or main workspace. + * @param {string} id The ID to use to create the variable, or null. + * @param {string} name The string to use to create the variable. + * @param {string} type The type to use to create the variable. + * @return {!Blockly.VariableModel} The variable corresponding to the given ID + * or name + type combination. + * @private + */ +Blockly.Variables.createVariable_ = function(workspace, id, name, type) { + var potentialVariableMap = + Blockly.Variables.getPotentialVariableMap_(workspace); + // Variables without names get uniquely named for this workspace. + if (!name) { + var ws = workspace.isFlyout ? workspace.targetWorkspace : workspace; + name = Blockly.Variables.generateUniqueName(ws); + } + + // Create a potential variable if in the flyout. + if (potentialVariableMap) { + var variable = potentialVariableMap.createVariable(name, type, id); + } else { // In the main workspace, create a real variable. + var variable = workspace.createVariable(name, type, id); + } + return variable; +}; + +/** + * Blocks in the flyout can refer to variables that don't exist in the + * workspace. For instance, the "get item in list" block refers to an "item" + * variable regardless of whether the variable has been created yet. + * A FieldVariable must always refer to a Blockly.VariableModel. We reconcile + * these by tracking "potential" variables in the flyout. These variables + * become real when references to them are dragged into the main workspace. + * @param {!Blockly.Workspace} workspace The workspace to get the potential + * variable map from. + * @return {?Blockly.VariableMap} The potential variable map for the given + * workspace, or null if it was not a flyout workspace. + * @private + */ +Blockly.Variables.getPotentialVariableMap_ = function(workspace) { + return workspace.isFlyout && workspace.targetWorkspace ? + workspace.targetWorkspace.getPotentialVariableMap() : null; + +}; diff --git a/core/workspace.js b/core/workspace.js index 6fed60405..03477519f 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -85,6 +85,16 @@ Blockly.Workspace = function(opt_options) { */ this.variableMap_ = new Blockly.VariableMap(this); + /** + * Blocks in the flyout can refer to variables that don't exist in the + * workspace. For instance, the "get item in list" block refers to an "item" + * variable regardless of whether the variable has been created yet. + * A FieldVariable must always refer to a Blockly.VariableModel. We reconcile + * these by tracking "potential" variables in the flyout. These variables + * become real when references to them are dragged into the main workspace. + * @type {!Blockly.VariableMap} + * @private + */ this.potentialVariableMap_ = new Blockly.VariableMap(this); }; @@ -199,6 +209,7 @@ Blockly.Workspace.prototype.clear = function() { Blockly.Events.setGroup(false); } this.variableMap_.clear(); + this.potentialVariableMap_.clear(); }; /** @@ -628,6 +639,18 @@ Blockly.Workspace.prototype.getAllVariables = function() { return this.variableMap_.getAllVariables(); }; +/** + * Return the variable map that contains "potential" variables. These exist in + * the flyout but not in the workspace. + * TODO: Decide if this can be stored on the flyout workspace instead of the + * main workspace. + * @return {?Blockly.VariableMap} The potential variable map. + * @package + */ +Blockly.Workspace.prototype.getPotentialVariableMap = function() { + return this.potentialVariableMap_; +}; + /** * Database of all workspaces. * @private diff --git a/core/xml.js b/core/xml.js index 9946d3832..f9a0106c1 100644 --- a/core/xml.js +++ b/core/xml.js @@ -730,8 +730,8 @@ Blockly.Xml.domToFieldVariable_ = function(workspace, xml, text, field) { // TODO: Consider using a different name (var_id?) because this is the // node's ID. var id = xml.id; - var variable = Blockly.FieldVariable.getOrCreateVariable(workspace, text, - type, id); + var variable = + Blockly.Variables.getOrCreateVariable(workspace, id, text, type); // This should never happen :) if (type != null && type !== variable.type) { From 72e4be9b4ef618c9b0494d28fea5e6e12258f8a3 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Tue, 5 Dec 2017 13:44:08 -0800 Subject: [PATCH 15/26] Get rid of workspace.deleteVariable --- blocks/procedures.js | 5 ++- core/field_variable.js | 1 - core/variable_map.js | 8 ++-- core/variables.js | 32 +++++++------- core/workspace.js | 81 ++++++++++++++-------------------- tests/jsunit/workspace_test.js | 4 +- 6 files changed, 59 insertions(+), 72 deletions(-) diff --git a/blocks/procedures.js b/blocks/procedures.js index 5fc29d302..d8fc6f30a 100644 --- a/blocks/procedures.js +++ b/blocks/procedures.js @@ -467,12 +467,13 @@ Blockly.Blocks['procedures_mutatorarg'] = { if (source && source.workspace && source.workspace.options && source.workspace.options.parentWorkspace) { var workspace = source.workspace.options.parentWorkspace; - var variable = workspace.getVariable(newText); + var variableType = ''; + var variable = workspace.getVariable(newText, variableType); // If there is a case change, rename the variable. if (variable && variable.name !== newText) { workspace.renameVariableById(variable.getId(), newText); } else { - workspace.createVariable(newText); + workspace.createVariable(newText, variableType); } } } diff --git a/core/field_variable.js b/core/field_variable.js index 14e2b1fbf..5276d1d25 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -125,7 +125,6 @@ Blockly.FieldVariable.prototype.getValue = function() { * @return {string} Current text. */ Blockly.FieldVariable.prototype.getText = function() { - //return this.getText(); return this.variable_ ? this.variable_.name : ''; }; diff --git a/core/variable_map.js b/core/variable_map.js index 6af990a97..3a51fc450 100644 --- a/core/variable_map.js +++ b/core/variable_map.js @@ -62,12 +62,10 @@ Blockly.VariableMap.prototype.clear = function() { * Rename the given variable by updating its name in the variable map. * @param {Blockly.VariableModel} variable Variable to rename. * @param {string} newName New variable name. - * @param {string=} opt_type The type of the variable to create if variable was - * null. + * @package */ -Blockly.VariableMap.prototype.renameVariable = function(variable, newName, - opt_type) { - var type = variable ? variable.type : (opt_type || ''); +Blockly.VariableMap.prototype.renameVariable = function(variable, newName) { + var type = variable.type; var newVariable = this.getVariable(newName, type); var variableIndex = -1; var newVariableIndex = -1; diff --git a/core/variables.js b/core/variables.js index 4d0167efa..c6e3462da 100644 --- a/core/variables.js +++ b/core/variables.js @@ -64,7 +64,6 @@ Blockly.Variables.allUsedVariables = function(root) { var variableHash = Object.create(null); // Iterate through every block and add each variable to the hash. for (var x = 0; x < blocks.length; x++) { - // TODO (#1199) Switch to IDs. var blockVariables = blocks[x].getVarModels(); if (blockVariables) { for (var y = 0; y < blockVariables.length; y++) { @@ -275,24 +274,25 @@ Blockly.Variables.createVariable = function(workspace, opt_callback, opt_type) { * aborted (cancel button), or undefined if an existing variable was chosen. */ Blockly.Variables.renameVariable = function(workspace, variable, - opt_callback) { + opt_callback) { // This function needs to be named so it can be called recursively. var promptAndCheckWithAlert = function(defaultName) { - Blockly.Variables.promptName( - Blockly.Msg.RENAME_VARIABLE_TITLE.replace('%1', variable.name), defaultName, - function(newName) { - if (newName) { - workspace.renameVariableById(variable.getId(), newName); - if (opt_callback) { - opt_callback(newName); + var promptText = + Blockly.Msg.RENAME_VARIABLE_TITLE.replace('%1', variable.name); + Blockly.Variables.promptName(promptText, defaultName, + function(newName) { + if (newName) { + workspace.renameVariableById(variable.getId(), newName); + if (opt_callback) { + opt_callback(newName); + } + } else { + // User canceled prompt without a value. + if (opt_callback) { + opt_callback(null); + } } - } else { - // User canceled prompt without a value. - if (opt_callback) { - opt_callback(null); - } - } - }); + }); }; promptAndCheckWithAlert(''); }; diff --git a/core/workspace.js b/core/workspace.js index 03477519f..fda910f43 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -286,7 +286,7 @@ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { } } - this.variableMap_.renameVariable(variable, newName, type); + this.variableMap_.renameVariable(variable, newName); Blockly.Events.setGroup(false); }; @@ -361,49 +361,6 @@ Blockly.Workspace.prototype.getVariableUsesById = function(id) { return uses; }; -/** - * Delete a variable by the passed in name and all of its uses from this - * workspace. May prompt the user for confirmation. - * TODO (#1199): Possibly delete this function. - * @param {string} name Name of variable to delete. - * @param {string=} opt_type The type of the variable. If not provided it - * defaults to the empty string, which is a specific type. - */ -Blockly.Workspace.prototype.deleteVariable = function(name, opt_type) { - var type = opt_type || ''; - // Check whether this variable is a function parameter before deleting. - var uses = this.getVariableUses(name, type); - for (var i = 0, block; block = uses[i]; i++) { - if (block.type == 'procedures_defnoreturn' || - block.type == 'procedures_defreturn') { - var procedureName = block.getFieldValue('NAME'); - Blockly.alert( - Blockly.Msg.CANNOT_DELETE_VARIABLE_PROCEDURE. - replace('%1', name). - replace('%2', procedureName)); - return; - } - } - - var workspace = this; - var variable = workspace.getVariable(name, type); - if (uses.length > 1) { - // Confirm before deleting multiple blocks. - Blockly.confirm( - Blockly.Msg.DELETE_VARIABLE_CONFIRMATION.replace('%1', - String(uses.length)). - replace('%2', name), - function(ok) { - if (ok) { - workspace.deleteVariableInternal_(variable); - } - }); - } else { - // No confirmation necessary for a single block. - this.deleteVariableInternal_(variable); - } -}; - /** * Delete a variables by the passed in ID and all of its uses from this * workspace. May prompt the user for confirmation. @@ -412,7 +369,37 @@ Blockly.Workspace.prototype.deleteVariable = function(name, opt_type) { Blockly.Workspace.prototype.deleteVariableById = function(id) { var variable = this.getVariableById(id); if (variable) { - this.deleteVariableInternal_(variable); + // Check whether this variable is a function parameter before deleting. + var variableName = variable.name; + var uses = this.getVariableUsesById(id); + for (var i = 0, block; block = uses[i]; i++) { + if (block.type == 'procedures_defnoreturn' || + block.type == 'procedures_defreturn') { + var procedureName = block.getFieldValue('NAME'); + var deleteText = Blockly.Msg.CANNOT_DELETE_VARIABLE_PROCEDURE. + replace('%1', variableName). + replace('%2', procedureName); + Blockly.alert(deleteText); + return; + } + } + + var workspace = this; + if (uses.length > 1) { + // Confirm before deleting multiple blocks. + var confirmText = Blockly.Msg.DELETE_VARIABLE_CONFIRMATION. + replace('%1', String(uses.length)). + replace('%2', variableName); + Blockly.confirm(confirmText, + function(ok) { + if (ok) { + workspace.deleteVariableInternal_(variable, uses); + } + }); + } else { + // No confirmation necessary for a single block. + this.deleteVariableInternal_(variable, uses); + } } else { console.warn("Can't delete non-existent variable: " + id); } @@ -422,10 +409,10 @@ Blockly.Workspace.prototype.deleteVariableById = function(id) { * Deletes a variable and all of its uses from this workspace without asking the * user for confirmation. * @param {!Blockly.VariableModel} variable Variable to delete. + * @param {!Array.} uses An array of uses of the variable. * @private */ -Blockly.Workspace.prototype.deleteVariableInternal_ = function(variable) { - var uses = this.getVariableUsesById(variable.getId()); +Blockly.Workspace.prototype.deleteVariableInternal_ = function(variable, uses) { Blockly.Events.setGroup(true); for (var i = 0; i < uses.length; i++) { uses[i].dispose(true, false); diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index 6e4d960d4..c97700f3e 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -155,7 +155,9 @@ function test_deleteVariable_InternalTrivial() { createMockBlock('id1'); createMockBlock('id2'); - workspace.deleteVariableInternal_(var_1); + var uses = workspace.getVariableUsesById(var_1.getId()); + workspace.deleteVariableInternal_(var_1, uses); + var variable = workspace.getVariableById('id1'); var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; assertNull(variable); From 3b91de7bb483e3236aabcae9174887bab7308732 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Tue, 5 Dec 2017 13:52:01 -0800 Subject: [PATCH 16/26] Remove ws.updateVariableStore and tests, and ws.getVariableUses --- core/workspace.js | 69 ---------------------------------- tests/jsunit/workspace_test.js | 69 ---------------------------------- 2 files changed, 138 deletions(-) diff --git a/core/workspace.js b/core/workspace.js index fda910f43..bcda8d927 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -212,41 +212,6 @@ Blockly.Workspace.prototype.clear = function() { this.potentialVariableMap_.clear(); }; -/** - * Walk the workspace and update the map of variables to only contain ones in - * use on the workspace. Use when loading new workspaces from disk. - * @param {boolean} clear True if the old variable map should be cleared. - */ -Blockly.Workspace.prototype.updateVariableStore = function(clear) { - // TODO: Sort - if (this.isFlyout) { - return; - } - var variableNames = Blockly.Variables.allUsedVariables(this); - var varList = []; - for (var i = 0, name; name = variableNames[i]; i++) { - // Get variable model with the used variable name. - var tempVar = this.getVariable(name); - if (tempVar) { - varList.push({'name': tempVar.name, 'type': tempVar.type, - 'id': tempVar.getId()}); - } else { - varList.push({'name': name, 'type': null, 'id': null}); - // TODO(marisaleung): Use variable.type and variable.getId() once variable - // instances are storing more than just name. - } - } - if (clear) { - this.variableMap_.clear(); - } - // Update the list in place so that the flyout's references stay correct. - for (var i = 0, varDict; varDict = varList[i]; i++) { - if (!this.getVariable(varDict.name)) { - this.createVariable(varDict.name, varDict.type, varDict.id); - } - } -}; - /** * Rename a variable by updating its name in the variable map. Identify the * variable to rename with the given ID. @@ -307,40 +272,6 @@ Blockly.Workspace.prototype.createVariable = function(name, opt_type, opt_id) { /** * Find all the uses of a named variable. - * TODO (#1199): Possibly delete this function. - * @param {string} name Name of variable. - * @param {string=} opt_type The type of the variable. If not provided it - * defaults to the empty string, which is a specific type. - * @return {!Array.} Array of block usages. - */ -Blockly.Workspace.prototype.getVariableUses = function(name, opt_type) { - var type = opt_type || ''; - var uses = []; - var blocks = this.getAllBlocks(); - // Iterate through every block and check the name. - for (var i = 0; i < blocks.length; i++) { - var blockVariables = blocks[i].getVarModels(); - if (blockVariables) { - for (var j = 0; j < blockVariables.length; j++) { - var varModel = blockVariables[j]; - var varName = varModel.name; - // Skip variables of the wrong type. - if (varModel.type != type) { - continue; - } - // Variable name may be null if the block is only half-built. - if (varName && name && Blockly.Names.equals(varName, name)) { - uses.push(blocks[i]); - } - } - } - } - return uses; -}; - -/** - * Find all the uses of a named variable. - * TODO (#1199): Possibly delete this function. * @param {string} id ID of the variable to find. * @return {!Array.} Array of block usages. */ diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index c97700f3e..999aa81f8 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -168,75 +168,6 @@ function test_deleteVariable_InternalTrivial() { // TODO(marisaleung): Test the alert for deleting a variable that is a procedure. -function test_updateVariableStore_TrivialNoClear() { - workspaceTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); - setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', - [workspace], [['name1', 'name2']]); - - try { - workspace.updateVariableStore(); - checkVariableValues(workspace, 'name1', 'type1', 'id1'); - checkVariableValues(workspace, 'name2', 'type2', 'id2'); - } finally { - workspaceTest_tearDown(); - } -} - -function test_updateVariableStore_NameNotInvariableMap_NoClear() { - workspaceTest_setUp(); - setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', - [workspace], [['name1']]); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); - - try { - workspace.updateVariableStore(); - checkVariableValues(workspace, 'name1', '', '1'); - } finally { - workspaceTest_tearDown(); - } -} - -function test_updateVariableStore_ClearAndAllInUse() { - workspaceTest_setUp(); - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - // TODO (#1199): get rid of updateVariableStore if possible. - setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', - [workspace], [['name1', 'name2']]); - - try { - workspace.updateVariableStore(true); - checkVariableValues(workspace, 'name1', '', 'id1'); - checkVariableValues(workspace, 'name2', '', 'id2'); - } finally { - workspaceTest_tearDown(); - } -} - -function test_updateVariableStore_ClearAndOneInUse() { - workspaceTest_setUp(); - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - // TODO (#1199): get rid of updateVariableStore if possible. - setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', - [workspace], [['name1']]); - - try { - workspace.updateVariableStore(true); - checkVariableValues(workspace, 'name1', '', 'id1'); - var variable = workspace.getVariable('name2', ''); - assertNull(variable); - } finally { - workspaceTest_tearDown(); - } -} - function test_addTopBlock_TrivialFlyoutIsTrue() { workspaceTest_setUp(); workspace.isFlyout = true; From 96e814deff5e11ba944992bd4d860b4ab0a9d619 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 6 Dec 2017 14:41:56 -0800 Subject: [PATCH 17/26] Fix events for variable renaming --- core/block.js | 18 ++++++++ core/variable_map.js | 97 ++++++++++++++++++++++++++++++-------------- core/workspace.js | 37 ++++------------- core/xml.js | 23 +++++++---- 4 files changed, 108 insertions(+), 67 deletions(-) diff --git a/core/block.js b/core/block.js index 58bc311a4..d55b508c6 100644 --- a/core/block.js +++ b/core/block.js @@ -709,6 +709,7 @@ Blockly.Block.prototype.getVarModels = function() { /** * Notification that a variable is renaming. + * TODO (fenichel): consider deleting this. * If the name matches one of this block's variables, rename it. * @param {string} oldName Previous name of variable. * @param {string} newName Renamed variable. @@ -724,6 +725,23 @@ Blockly.Block.prototype.renameVar = function(oldName, newName) { } }; +/** + * Notification that a variable is renaming but keeping the same ID. If the + * variable is in use on this block, rerender to show the new name. + * @param {!Blockly.VariableModel} variable The variable being renamed. + * @public + */ +Blockly.Block.prototype.updateVarName = function(variable) { + for (var i = 0, input; input = this.inputList[i]; i++) { + for (var j = 0, field; field = input.fieldRow[j]; j++) { + if (field instanceof Blockly.FieldVariable && + variable.getId() == field.getValue()) { + field.setText(variable.name); + } + } + } +}; + /** * Notification that a variable is renaming. * If the name matches one of this block's variables, rename it. diff --git a/core/variable_map.js b/core/variable_map.js index 3a51fc450..c9a02aa43 100644 --- a/core/variable_map.js +++ b/core/variable_map.js @@ -60,45 +60,80 @@ Blockly.VariableMap.prototype.clear = function() { /** * Rename the given variable by updating its name in the variable map. - * @param {Blockly.VariableModel} variable Variable to rename. + * @param {!Blockly.VariableModel} variable Variable to rename. * @param {string} newName New variable name. * @package */ Blockly.VariableMap.prototype.renameVariable = function(variable, newName) { var type = variable.type; - var newVariable = this.getVariable(newName, type); - var variableIndex = -1; - var newVariableIndex = -1; + var conflictVar = this.getVariable(newName, type); + // The IDs may match if the rename is a simple case change (name1 -> Name1). + if (!conflictVar || conflictVar.getId() == variable.getId()) { + this.renameVariableNoConflict_(variable, newName); + } else { + this.renameVariableWithConflict_(variable, newName, conflictVar); + } +}; +/** + * Update the name of the given variable and refresh all references to it. + * The new name must not conflict with any existing variable names. + * @param {!Blockly.VariableModel} variable Variable to rename. + * @param {string} newName New variable name. + * @private + */ +Blockly.VariableMap.prototype.renameVariableNoConflict_ = function(variable, newName) { + Blockly.Events.fire(new Blockly.Events.VarRename(variable, newName)); + variable.name = newName; + var blocks = this.workspace.getAllBlocks(); + for (var i = 0; i < blocks.length; i++) { + blocks[i].updateVarName(variable); + } +}; + +/** + * Update the name of the given variable to the same name as an existing + * variable. The two variables are coalesced into a single variable with the ID + * of the existing variable that was already using newName. + * Refresh all references to the variable. + * @param {!Blockly.VariableModel} variable Variable to rename. + * @param {string} newName New variable name. + * @param {!Blockly.VariableModel} conflictVar The variable that was already + * using newName. + * @private + */ +Blockly.VariableMap.prototype.renameVariableWithConflict_ = function(variable, + newName, conflictVar) { + var type = variable.type; + + Blockly.Events.setGroup(true); + Blockly.Events.fire(new Blockly.Events.VarRename(conflictVar, newName)); + var oldCase = conflictVar.name; + conflictVar.name = newName; + + // These blocks refer to the same variable but the case may have changed. + // No change events should be fired here. + var blocks = this.workspace.getAllBlocks(); + if (newName != oldCase) { + for (var i = 0; i < blocks.length; i++) { + blocks[i].updateVarName(conflictVar); + } + } + + // These blocks now refer to a different variable. + // These will fire change events. + for (var i = 0; i < blocks.length; i++) { + blocks[i].renameVarById(variable.getId(), conflictVar.getId()); + } + + // Finally delete the original variable, which is now unreferenced. + Blockly.Events.fire(new Blockly.Events.VarDelete(variable)); + // And remove it from the list. var variableList = this.getVariablesOfType(type); - if (variable) { - variableIndex = variableList.indexOf(variable); - } - if (newVariable) { // see if I can get rid of newVariable dependency - newVariableIndex = variableList.indexOf(newVariable); - } + var variableIndex = variableList.indexOf(variable); + this.variableMap_[type].splice(variableIndex, 1); - if (variableIndex == -1 && newVariableIndex == -1) { - this.createVariable(newName, ''); - console.log('Tried to rename an non-existent variable.'); - } else if (variableIndex == newVariableIndex || - variableIndex != -1 && newVariableIndex == -1) { - // Only changing case, or renaming to a completely novel name. - var variableToRename = this.variableMap_[type][variableIndex]; - Blockly.Events.fire(new Blockly.Events.VarRename(variableToRename, - newName)); - variableToRename.name = newName; - } else if (variableIndex != -1 && newVariableIndex != -1) { - // Renaming one existing variable to another existing variable. - // The case might have changed, so we update the destination ID. - var variableToRename = this.variableMap_[type][newVariableIndex]; - Blockly.Events.fire(new Blockly.Events.VarRename(variableToRename, - newName)); - var variableToDelete = this.variableMap_[type][variableIndex]; - Blockly.Events.fire(new Blockly.Events.VarDelete(variableToDelete)); - variableToRename.name = newName; - this.variableMap_[type].splice(variableIndex, 1); - } + Blockly.Events.setGroup(false); }; /** diff --git a/core/workspace.js b/core/workspace.js index bcda8d927..698f523eb 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -224,35 +224,7 @@ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { throw new Error('Tried to rename a variable that didn\'t exist. ID: ' + id); } - var type = variable.type; - var newVariable = this.getVariable(newName, type); - - // If a variable previously existed with the new name, we will coalesce the - // variables and use the ID of the existing variable with the new name. - // Otherwise, we change the current variable's name but not its ID. - var oldId = variable.getId(); - var newId = newVariable ? newVariable.getId() : oldId; - - var oldCase; - - // Find if newVariable case is different. - if (newVariable && newVariable.name != newName) { - oldCase = newVariable.name; - } - - Blockly.Events.setGroup(true); - var blocks = this.getAllBlocks(); - - // Iterate through every block and update name. - for (var i = 0; i < blocks.length; i++) { - blocks[i].renameVarById(oldId, newId); - if (oldCase) { - blocks[i].renameVarById(newId, newId); - } - } - this.variableMap_.renameVariable(variable, newName); - Blockly.Events.setGroup(false); }; /** @@ -569,6 +541,15 @@ Blockly.Workspace.prototype.getPotentialVariableMap = function() { return this.potentialVariableMap_; }; +/** + * Return the map of all variables on the workspace. + * @return {?Blockly.VariableMap} The variable map. + * @package + */ +Blockly.Workspace.prototype.getVariableMap = function() { + return this.variableMap_; +}; + /** * Database of all workspaces. * @private diff --git a/core/xml.js b/core/xml.js index f9a0106c1..e1eaaa3a3 100644 --- a/core/xml.js +++ b/core/xml.js @@ -106,7 +106,7 @@ Blockly.Xml.fieldToDomVariable_ = function(field, workspace) { return container; } else { // something went wrong? - console.log('no variable in fieldtodom'); + console.warn('no variable in fieldtodom'); return null; } }; @@ -500,6 +500,7 @@ Blockly.Xml.domToBlock = function(xmlBlock, workspace) { try { var topBlock = Blockly.Xml.domToBlockHeadless_(xmlBlock, workspace); if (workspace.rendered) { + // TODO (fenichel): Otherwise call initModel? // Hide connections to speed up assembly. topBlock.setConnectionsHidden(true); // Generate list of all blocks. @@ -719,19 +720,25 @@ Blockly.Xml.domToBlockHeadless_ = function(xmlBlock, workspace) { return block; }; +/** + * Decode an XML variable field tag and set the value of that field. + * @param {!Blockly.Workspace} workspace The workspace that is currently being + * deserialized. + * @param {!Element} xml The field tag to decode. + * @param {string} text The text content of the XML tag. + * @param {!Blockly.FieldVariable} field The field on which the value will be + * set. + * @private + */ Blockly.Xml.domToFieldVariable_ = function(workspace, xml, text, field) { - // TODO (#1199): When we change setValue and getValue to - // interact with IDs instead of names, update this so that we get - // the variable based on ID instead of textContent. var type = xml.getAttribute('variabletype') || ''; + // TODO (fenichel): Does this need to be explicit or not? if (type == '\'\'') { type = ''; } - // TODO: Consider using a different name (var_id?) because this is the - // node's ID. - var id = xml.id; + var variable = - Blockly.Variables.getOrCreateVariable(workspace, id, text, type); + Blockly.Variables.getOrCreateVariable(workspace, xml.id, text, type); // This should never happen :) if (type != null && type !== variable.type) { From e7e66fb7b7fe9ea5b15a20bf23e1e422252a0560 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 6 Dec 2017 15:49:20 -0800 Subject: [PATCH 18/26] Tests --- blocks/procedures.js | 2 +- core/block.js | 7 +--- tests/jsunit/test_utilities.js | 14 +++++++ tests/jsunit/workspace_test.js | 52 ++++++++---------------- tests/jsunit/workspace_undo_redo_test.js | 51 ++++++++--------------- 5 files changed, 49 insertions(+), 77 deletions(-) diff --git a/blocks/procedures.js b/blocks/procedures.js index d8fc6f30a..a6800f934 100644 --- a/blocks/procedures.js +++ b/blocks/procedures.js @@ -271,7 +271,7 @@ Blockly.Blocks['procedures_defnoreturn'] = { return vars; } for (var i = 0, argName; argName = this.arguments_[i]; i++) { - // TODO (#1199): When we switch to tracking variables by ID, + // TODO (#1494): When we switch to tracking procedure arguments by ID, // update this. var model = this.workspace.getVariable(argName, ''); if (model) { diff --git a/core/block.js b/core/block.js index d55b508c6..5080c3b37 100644 --- a/core/block.js +++ b/core/block.js @@ -695,12 +695,7 @@ Blockly.Block.prototype.getVarModels = function() { for (var i = 0, input; input = this.inputList[i]; i++) { for (var j = 0, field; field = input.fieldRow[j]; j++) { if (field instanceof Blockly.FieldVariable) { - // TODO (#1199): When we switch to tracking variables by ID, - // update this. - var model = this.workspace.getVariableById(field.getValue()); - if (model) { - vars.push(model); - } + vars.push(this.workspace.getVariableById(field.getValue())); } } } diff --git a/tests/jsunit/test_utilities.js b/tests/jsunit/test_utilities.js index d7f6085bf..aa5840d9e 100644 --- a/tests/jsunit/test_utilities.js +++ b/tests/jsunit/test_utilities.js @@ -108,3 +108,17 @@ function createMockBlock(variable_id) { Blockly.Events.enable(); return block; } + +function createTwoVariablesAndBlocks(workspace) { + // Create two variables of different types. + workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + // Create blocks to refer to both of them. + createMockBlock('id1'); + createMockBlock('id2'); +} + +function createVariableAndBlock(workspace) { + workspace.createVariable('name1', 'type1', 'id1'); + createMockBlock('id1'); +} diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index 999aa81f8..e991106a3 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -247,15 +247,12 @@ function test_renameVariable_ReferenceExists() { // Test renaming a variable when a reference to it exists. // Expect 'renameVariable' to change oldName variable name to newName. workspaceTest_setUp(); - var id = 'id1'; - var type = 'type1'; - var oldName = 'name1'; var newName = 'name2'; - workspace.createVariable(oldName, type, id); - createMockBlock(id); - workspace.renameVariableById(id, newName); - checkVariableValues(workspace, newName, type, id); + createVariableAndBlock(workspace); + + workspace.renameVariableById('id1', newName); + checkVariableValues(workspace, newName, 'type1', 'id1'); // Renaming should not have created a new variable. assertEquals(1, workspace.getAllVariables().length); var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; @@ -302,24 +299,13 @@ function test_renameVariable_TwoVariablesDifferentType() { // Expect the rename to succeed, because variables with different types are // allowed to have the same name. workspaceTest_setUp(); - var id1 = 'id1'; - var id2 = 'id2'; - var type1 = 'type1'; - var type2 = 'type2'; + createTwoVariablesAndBlocks(workspace); - var oldName = 'name1'; var newName = 'name2'; - // Create two variables of different types. - workspace.createVariable(oldName, type1, id1); - workspace.createVariable(newName, type2, id2); - // Create blocks to refer to both of them. - createMockBlock(id1); - createMockBlock(id2); + workspace.renameVariableById('id1', newName); - workspace.renameVariableById(id1, newName); - - checkVariableValues(workspace, newName, type1, id1); - checkVariableValues(workspace, newName, type2, id2); + checkVariableValues(workspace, newName, 'type1', 'id1'); + checkVariableValues(workspace, newName, 'type2', 'id2'); // References shoul have the correct names. var block_var_name_1 = workspace.topBlocks_[0].getVarModels()[0].name; @@ -333,17 +319,14 @@ function test_renameVariable_TwoVariablesDifferentType() { function test_renameVariable_OldCase() { // Rename a variable with a single reference. Update only the capitalization. workspaceTest_setUp(); - var oldCase = 'Name1'; - var newName = 'name1'; - var type = 'type1'; - var id = 'id1'; - workspace.createVariable(oldCase, type, id); - createMockBlock(id); + var newName = 'Name1'; - workspace.renameVariableById(id, newName); - checkVariableValues(workspace, newName, type, id); - var variable = workspace.getVariableById(id); - assertNotEquals(oldCase, id.name); + createVariableAndBlock(workspace); + + workspace.renameVariableById('id1', newName); + checkVariableValues(workspace, newName, 'type1', 'id1'); + var variable = workspace.getVariableById('id1'); + assertNotEquals('name1', variable.name); workspaceTest_tearDown(); } @@ -389,10 +372,7 @@ function test_renameVariable_TwoVariablesAndOldCase() { function test_deleteVariableById_Trivial() { workspaceTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); - createMockBlock('id1'); - createMockBlock('id2'); + createTwoVariablesAndBlocks(workspace); workspace.deleteVariableById('id1'); checkVariableValues(workspace, 'name2', 'type2', 'id2'); diff --git a/tests/jsunit/workspace_undo_redo_test.js b/tests/jsunit/workspace_undo_redo_test.js index 092dbb4a8..f014c8ba2 100644 --- a/tests/jsunit/workspace_undo_redo_test.js +++ b/tests/jsunit/workspace_undo_redo_test.js @@ -82,10 +82,14 @@ function createTwoVarsEmptyType() { workspace.createVariable('name2', '', 'id2'); } -function test_undoCreateVariable_Trivial() { - undoRedoTest_setUp(); +function createTwoVarsDifferentTypes() { workspace.createVariable('name1', 'type1', 'id1'); workspace.createVariable('name2', 'type2', 'id2'); +} + +function test_undoCreateVariable_Trivial() { + undoRedoTest_setUp(); + createTwoVarsDifferentTypes(); workspace.undo(); checkVariableValues(workspace, 'name1', 'type1', 'id1'); @@ -98,8 +102,7 @@ function test_undoCreateVariable_Trivial() { function test_redoAndUndoCreateVariable_Trivial() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); + createTwoVarsDifferentTypes(); workspace.undo(); workspace.undo(true); @@ -120,8 +123,7 @@ function test_redoAndUndoCreateVariable_Trivial() { function test_undoDeleteVariable_NoBlocks() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); + createTwoVarsDifferentTypes(); workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); @@ -137,10 +139,9 @@ function test_undoDeleteVariable_NoBlocks() { function test_undoDeleteVariable_WithBlocks() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); - createMockBlock('id1'); - createMockBlock('id2'); + + createTwoVariablesAndBlocks(workspace); + workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); @@ -159,8 +160,9 @@ function test_undoDeleteVariable_WithBlocks() { function test_redoAndUndoDeleteVariable_NoBlocks() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); + + createTwoVarsDifferentTypes(); + workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); @@ -181,10 +183,9 @@ function test_redoAndUndoDeleteVariable_NoBlocks() { function test_redoAndUndoDeleteVariable_WithBlocks() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); - createMockBlock('id1'); - createMockBlock('id2'); + + createTwoVariablesAndBlocks(workspace); + workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); @@ -261,24 +262,6 @@ function test_redoAndUndoDeleteVariableTwice_WithBlocks() { undoRedoTest_tearDown(); } -// TODO: Decide if this needs to be possible. -// function test_undoRedoRenameVariable_NeitherVariableExists() { -// // Expect that a variable with the name, 'name2', and the generated UUID, -// // 'id2', to be created when rename is called. Undo removes this variable -// // and redo recreates it. -// undoRedoTest_setUp(); -// setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, -// ['rename_group', 'id2', 'delete_group']); -// workspace.renameVariable('name1', 'name2'); - -// workspace.undo(); -// assertNull(workspace.getVariableById('id2')); - -// workspace.undo(true); -// checkVariableValues(workspace, 'name2', '', 'id2'); -// undoRedoTest_tearDown(); -// } - function test_undoRedoRenameVariable_OneExists_NoBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); From 464b2769a8b129a93c48339b64c877f9041d2ad7 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 6 Dec 2017 15:56:18 -0800 Subject: [PATCH 19/26] Tests --- tests/jsunit/index.html | 2 +- tests/jsunit/procedures_test.js | 1 - tests/jsunit/test_utilities.js | 18 ++++++++++++++++++ tests/jsunit/workspace_test.js | 14 ++------------ tests/jsunit/workspace_undo_redo_test.js | 14 ++------------ 5 files changed, 23 insertions(+), 26 deletions(-) diff --git a/tests/jsunit/index.html b/tests/jsunit/index.html index 619e4143b..500f2cb37 100644 --- a/tests/jsunit/index.html +++ b/tests/jsunit/index.html @@ -25,9 +25,9 @@ + - diff --git a/tests/jsunit/procedures_test.js b/tests/jsunit/procedures_test.js index f993af892..9d1468af2 100644 --- a/tests/jsunit/procedures_test.js +++ b/tests/jsunit/procedures_test.js @@ -37,7 +37,6 @@ function proceduresTest_setUpWithMockBlocks() { 'message0': '%1', 'args0': [ { - // TODO: Why is this a variable? It shouldn't need to be. 'type': 'field_variable', 'name': 'NAME', 'variable': 'item' diff --git a/tests/jsunit/test_utilities.js b/tests/jsunit/test_utilities.js index aa5840d9e..39e63eeea 100644 --- a/tests/jsunit/test_utilities.js +++ b/tests/jsunit/test_utilities.js @@ -122,3 +122,21 @@ function createVariableAndBlock(workspace) { workspace.createVariable('name1', 'type1', 'id1'); createMockBlock('id1'); } + +function defineGetVarBlock() { + Blockly.defineBlocksWithJsonArray([{ + "type": "get_var_block", + "message0": "%1", + "args0": [ + { + "type": "field_variable", + "name": "VAR", + "variableTypes": ["", "type1", "type2"] + } + ] + }]); +} + +function undefineGetVarBlock() { + delete Blockly.Blocks['get_var_block']; +} diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index e991106a3..20421d711 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -26,23 +26,13 @@ var workspace; var mockControl_; function workspaceTest_setUp() { - Blockly.defineBlocksWithJsonArray([{ - "type": "get_var_block", - "message0": "%1", - "args0": [ - { - "type": "field_variable", - "name": "VAR", - "variableTypes": ["", "type1", "type2"] - } - ] - }]); + defineGetVarBlock(); workspace = new Blockly.Workspace(); mockControl_ = new goog.testing.MockControl(); } function workspaceTest_tearDown() { - delete Blockly.Blocks['get_var_block']; + undefineGetVarBlock(); mockControl_.$tearDown(); workspace.dispose(); } diff --git a/tests/jsunit/workspace_undo_redo_test.js b/tests/jsunit/workspace_undo_redo_test.js index f014c8ba2..aa960d9f0 100644 --- a/tests/jsunit/workspace_undo_redo_test.js +++ b/tests/jsunit/workspace_undo_redo_test.js @@ -43,24 +43,14 @@ function temporary_fireEvent(event) { } function undoRedoTest_setUp() { - Blockly.defineBlocksWithJsonArray([{ - "type": "get_var_block", - "message0": "%1", - "args0": [ - { - "type": "field_variable", - "name": "VAR", - "variableTypes": ["", "type1", "type2"] - } - ] - }]); + defineGetVarBlock(); workspace = new Blockly.Workspace(); mockControl_ = new goog.testing.MockControl(); Blockly.Events.fire = temporary_fireEvent; } function undoRedoTest_tearDown() { - delete Blockly.Blocks['get_var_block']; + undefineGetVarBlock(); mockControl_.$tearDown(); workspace.dispose(); Blockly.Events.fire = savedFireFunc; From 1491950394815d05358f87b76d026e2a2fad6ab0 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 6 Dec 2017 16:07:21 -0800 Subject: [PATCH 20/26] Fix checkmarks when the variable dropdown is open. --- core/field_variable.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/field_variable.js b/core/field_variable.js index 5276d1d25..35b73cb51 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -56,6 +56,7 @@ Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes) { this.defaultVariableName = (varname || ''); this.defaultType_ = ''; this.variableTypes = opt_variableTypes; + this.value_ = null; }; goog.inherits(Blockly.FieldVariable, Blockly.FieldDropdown); @@ -155,6 +156,7 @@ Blockly.FieldVariable.prototype.setValue = function(id) { this.sourceBlock_, 'field', this.name, oldValue, variable.getId())); } this.variable_ = variable; + this.value_ = id; this.setText(variable.name); }; From 676253ec32d21dfde84bf723922c3a7dc1836686 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 6 Dec 2017 16:41:17 -0800 Subject: [PATCH 21/26] Fix missing events for variable creation --- core/flyout_base.js | 30 ++---------------------------- core/variables.js | 29 ++++++++++++++++++++++++++++- core/xml.js | 8 ++++++++ 3 files changed, 38 insertions(+), 29 deletions(-) diff --git a/core/flyout_base.js b/core/flyout_base.js index 8b1421e39..dacab6169 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -599,33 +599,6 @@ Blockly.Flyout.prototype.onMouseDown_ = function(e) { } }; -/** - * Helper function to get the list of variables that have been added to the - * workspace after adding a new block, using the given list of variables that - * were in the workspace before the new block was added. - * @param {!Array.} originalVariables The array of - * variables that existed in the workspace before adding the new block. - * @return {!Array.} The new array of variables that were - * freshly added to the workspace after creating the new block, or [] if no - * new variables were added to the workspace. - * @private - */ -Blockly.Flyout.prototype.getAddedVariables_ = function(originalVariables) { - var allCurrentVariables = this.targetWorkspace_.getAllVariables(); - var addedVariables = []; - if (originalVariables.length != allCurrentVariables.length) { - for (var i = 0; i < allCurrentVariables.length; i++) { - var variable = allCurrentVariables[i]; - // For any variable that is present in allCurrentVariables but not - // present in originalVariables, add the variable to addedVariables. - if (!originalVariables.includes(variable)) { - addedVariables.push(variable); - } - } - } - return addedVariables; -}; - /** * Create a copy of this block on the workspace. * @param {!Blockly.BlockSvg} originalBlock The block to copy from the flyout. @@ -646,7 +619,8 @@ Blockly.Flyout.prototype.createBlock = function(originalBlock) { Blockly.Events.enable(); } - var newVariables = this.getAddedVariables_(variablesBeforeCreation); + var newVariables = Blockly.Variables.getAddedVariables(this.targetWorkspace_, + variablesBeforeCreation); if (Blockly.Events.isEnabled()) { Blockly.Events.setGroup(true); diff --git a/core/variables.js b/core/variables.js index c6e3462da..562068660 100644 --- a/core/variables.js +++ b/core/variables.js @@ -440,5 +440,32 @@ Blockly.Variables.createVariable_ = function(workspace, id, name, type) { Blockly.Variables.getPotentialVariableMap_ = function(workspace) { return workspace.isFlyout && workspace.targetWorkspace ? workspace.targetWorkspace.getPotentialVariableMap() : null; - +}; + +/** + * Helper function to get the list of variables that have been added to the + * workspace after adding a new block, using the given list of variables that + * were in the workspace before the new block was added. + * @param {!Blockly.Workspace} workspace The workspace to inspect. + * @param {!Array.} originalVariables The array of + * variables that existed in the workspace before adding the new block. + * @return {!Array.} The new array of variables that were + * freshly added to the workspace after creating the new block, or [] if no + * new variables were added to the workspace. + * @package + */ +Blockly.Variables.getAddedVariables = function(workspace, originalVariables) { + var allCurrentVariables = workspace.getAllVariables(); + var addedVariables = []; + if (originalVariables.length != allCurrentVariables.length) { + for (var i = 0; i < allCurrentVariables.length; i++) { + var variable = allCurrentVariables[i]; + // For any variable that is present in allCurrentVariables but not + // present in originalVariables, add the variable to addedVariables. + if (!originalVariables.includes(variable)) { + addedVariables.push(variable); + } + } + } + return addedVariables; }; diff --git a/core/xml.js b/core/xml.js index e1eaaa3a3..db8121131 100644 --- a/core/xml.js +++ b/core/xml.js @@ -497,6 +497,7 @@ Blockly.Xml.domToBlock = function(xmlBlock, workspace) { } // Create top-level block. Blockly.Events.disable(); + var variablesBeforeCreation = workspace.getAllVariables(); try { var topBlock = Blockly.Xml.domToBlockHeadless_(xmlBlock, workspace); if (workspace.rendered) { @@ -529,6 +530,13 @@ Blockly.Xml.domToBlock = function(xmlBlock, workspace) { } if (Blockly.Events.isEnabled()) { Blockly.Events.fire(new Blockly.Events.BlockCreate(topBlock)); + var newVariables = Blockly.Variables.getAddedVariables(workspace, + variablesBeforeCreation); + // Fire a VarCreate event for each (if any) new variable created. + for(var i = 0; i < newVariables.length; i++) { + var thisVariable = newVariables[i]; + Blockly.Events.fire(new Blockly.Events.VarCreate(thisVariable)); + } } return topBlock; }; From f0b4b4402c6f8e57dc40dcc607c9b11013649784 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 6 Dec 2017 16:57:46 -0800 Subject: [PATCH 22/26] Avoid spurious rename event --- core/variable_map.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/variable_map.js b/core/variable_map.js index c9a02aa43..2dc70f8ff 100644 --- a/core/variable_map.js +++ b/core/variable_map.js @@ -107,14 +107,16 @@ Blockly.VariableMap.prototype.renameVariableWithConflict_ = function(variable, var type = variable.type; Blockly.Events.setGroup(true); - Blockly.Events.fire(new Blockly.Events.VarRename(conflictVar, newName)); var oldCase = conflictVar.name; - conflictVar.name = newName; - // These blocks refer to the same variable but the case may have changed. - // No change events should be fired here. var blocks = this.workspace.getAllBlocks(); if (newName != oldCase) { + // Only fire a rename event if the case changed. + Blockly.Events.fire(new Blockly.Events.VarRename(conflictVar, newName)); + conflictVar.name = newName; + + // These blocks refer to the same variable but the case changed. + // No change events should be fired here. for (var i = 0; i < blocks.length; i++) { blocks[i].updateVarName(conflictVar); } From 9f4b52a83429aa8d5ce33b28b2d140de9445f656 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 7 Dec 2017 10:43:12 -0800 Subject: [PATCH 23/26] Clean up TODOs and move potential variable map to the flyout workspace --- core/block.js | 2 +- core/field_variable.js | 4 +--- core/flyout_base.js | 2 +- core/variables.js | 24 ++---------------------- core/workspace.js | 4 +--- core/xml.js | 3 +-- 6 files changed, 7 insertions(+), 32 deletions(-) diff --git a/core/block.js b/core/block.js index 5080c3b37..1c20108b7 100644 --- a/core/block.js +++ b/core/block.js @@ -704,7 +704,7 @@ Blockly.Block.prototype.getVarModels = function() { /** * Notification that a variable is renaming. - * TODO (fenichel): consider deleting this. + * TODO (#1498): consider deleting this. * If the name matches one of this block's variables, rename it. * @param {string} oldName Previous name of variable. * @param {string} newName Renamed variable. diff --git a/core/field_variable.js b/core/field_variable.js index 35b73cb51..262947c4d 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -52,7 +52,7 @@ Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes) { this.menuGenerator_ = Blockly.FieldVariable.dropdownCreate; this.size_ = new goog.math.Size(0, Blockly.BlockSvg.MIN_BLOCK_Y); this.setValidator(opt_validator); - // TODO: Add opt_default_type to match default value. If not set, ''. + // TODO (#1499): Add opt_default_type to match default value. If not set, ''. this.defaultVariableName = (varname || ''); this.defaultType_ = ''; this.variableTypes = opt_variableTypes; @@ -135,8 +135,6 @@ Blockly.FieldVariable.prototype.getText = function() { * variable. */ Blockly.FieldVariable.prototype.setValue = function(id) { - // TODO: Handle undo a change to go back to the default value. - // TODO: Handle null ID, which means "use default". var workspace = this.sourceBlock_.workspace; var variable = Blockly.Variables.getVariable(workspace, id); diff --git a/core/flyout_base.js b/core/flyout_base.js index dacab6169..29ca24d18 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -544,7 +544,7 @@ Blockly.Flyout.prototype.clearOldBlocks_ = function() { this.buttons_.length = 0; // Clear potential variables from the previous showing. - this.targetWorkspace_.potentialVariableMap_.clear(); + this.workspace_.getPotentialVariableMap().clear(); }; /** diff --git a/core/variables.js b/core/variables.js index 562068660..676223c24 100644 --- a/core/variables.js +++ b/core/variables.js @@ -376,8 +376,7 @@ Blockly.Variables.getOrCreateVariable = function(workspace, id, name, type) { * @private */ Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) { - var potentialVariableMap = - Blockly.Variables.getPotentialVariableMap_(workspace); + var potentialVariableMap = workspace.getPotentialVariableMap(); // Try to just get the variable, by ID if possible. if (id) { // Look in the real variable map before checking the potential variable map. @@ -407,8 +406,7 @@ Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) { * @private */ Blockly.Variables.createVariable_ = function(workspace, id, name, type) { - var potentialVariableMap = - Blockly.Variables.getPotentialVariableMap_(workspace); + var potentialVariableMap = workspace.getPotentialVariableMap(); // Variables without names get uniquely named for this workspace. if (!name) { var ws = workspace.isFlyout ? workspace.targetWorkspace : workspace; @@ -424,24 +422,6 @@ Blockly.Variables.createVariable_ = function(workspace, id, name, type) { return variable; }; -/** - * Blocks in the flyout can refer to variables that don't exist in the - * workspace. For instance, the "get item in list" block refers to an "item" - * variable regardless of whether the variable has been created yet. - * A FieldVariable must always refer to a Blockly.VariableModel. We reconcile - * these by tracking "potential" variables in the flyout. These variables - * become real when references to them are dragged into the main workspace. - * @param {!Blockly.Workspace} workspace The workspace to get the potential - * variable map from. - * @return {?Blockly.VariableMap} The potential variable map for the given - * workspace, or null if it was not a flyout workspace. - * @private - */ -Blockly.Variables.getPotentialVariableMap_ = function(workspace) { - return workspace.isFlyout && workspace.targetWorkspace ? - workspace.targetWorkspace.getPotentialVariableMap() : null; -}; - /** * Helper function to get the list of variables that have been added to the * workspace after adding a new block, using the given list of variables that diff --git a/core/workspace.js b/core/workspace.js index 698f523eb..6505560ce 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -532,13 +532,11 @@ Blockly.Workspace.prototype.getAllVariables = function() { /** * Return the variable map that contains "potential" variables. These exist in * the flyout but not in the workspace. - * TODO: Decide if this can be stored on the flyout workspace instead of the - * main workspace. * @return {?Blockly.VariableMap} The potential variable map. * @package */ Blockly.Workspace.prototype.getPotentialVariableMap = function() { - return this.potentialVariableMap_; + return this.isFlyout ? this.potentialVariableMap_ : null; }; /** diff --git a/core/xml.js b/core/xml.js index db8121131..ec1da0e18 100644 --- a/core/xml.js +++ b/core/xml.js @@ -91,8 +91,7 @@ Blockly.Xml.fieldToDomVariable_ = function(field, workspace) { var variable = workspace.getVariableById(id); if (!variable) { if (workspace.isFlyout && workspace.targetWorkspace) { - var potentialVariableMap = - workspace.targetWorkspace.potentialVariableMap_; + var potentialVariableMap = workspace.getPotentialVariableMap(); if (potentialVariableMap) { variable = potentialVariableMap.getVariableById(id); } From 9d71c972cb1f49eb1d4d03657fbf4ed74bffafeb Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 7 Dec 2017 13:12:20 -0800 Subject: [PATCH 24/26] Respond to review comments in variable_map --- core/field_variable.js | 2 ++ core/variable_map.js | 32 ++++++++++++++------------------ 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index 262947c4d..bc9132412 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -135,6 +135,8 @@ Blockly.FieldVariable.prototype.getText = function() { * variable. */ Blockly.FieldVariable.prototype.setValue = function(id) { + // What do I do when id is null? That happens when undoing a change event + // for the first time the value was set. var workspace = this.sourceBlock_.workspace; var variable = Blockly.Variables.getVariable(workspace, id); diff --git a/core/variable_map.js b/core/variable_map.js index 2dc70f8ff..5081db09a 100644 --- a/core/variable_map.js +++ b/core/variable_map.js @@ -67,12 +67,15 @@ Blockly.VariableMap.prototype.clear = function() { Blockly.VariableMap.prototype.renameVariable = function(variable, newName) { var type = variable.type; var conflictVar = this.getVariable(newName, type); + var blocks = this.workspace.getAllBlocks(); + Blockly.Events.setGroup(true); // The IDs may match if the rename is a simple case change (name1 -> Name1). if (!conflictVar || conflictVar.getId() == variable.getId()) { - this.renameVariableNoConflict_(variable, newName); + this.renameVariableAndUses_(variable, newName, blocks); } else { - this.renameVariableWithConflict_(variable, newName, conflictVar); + this.renameVariableWithConflict_(variable, newName, conflictVar, blocks); } + Blockly.Events.setGroup(false); }; /** @@ -80,12 +83,14 @@ Blockly.VariableMap.prototype.renameVariable = function(variable, newName) { * The new name must not conflict with any existing variable names. * @param {!Blockly.VariableModel} variable Variable to rename. * @param {string} newName New variable name. + * @param {!Array.} blocks The list of all blocks in the + * workspace. * @private */ -Blockly.VariableMap.prototype.renameVariableNoConflict_ = function(variable, newName) { +Blockly.VariableMap.prototype.renameVariableAndUses_ = function(variable, + newName, blocks) { Blockly.Events.fire(new Blockly.Events.VarRename(variable, newName)); variable.name = newName; - var blocks = this.workspace.getAllBlocks(); for (var i = 0; i < blocks.length; i++) { blocks[i].updateVarName(variable); } @@ -100,26 +105,18 @@ Blockly.VariableMap.prototype.renameVariableNoConflict_ = function(variable, new * @param {string} newName New variable name. * @param {!Blockly.VariableModel} conflictVar The variable that was already * using newName. + * @param {!Array.} blocks The list of all blocks in the + * workspace. * @private */ Blockly.VariableMap.prototype.renameVariableWithConflict_ = function(variable, - newName, conflictVar) { + newName, conflictVar, blocks) { var type = variable.type; - - Blockly.Events.setGroup(true); var oldCase = conflictVar.name; - var blocks = this.workspace.getAllBlocks(); if (newName != oldCase) { - // Only fire a rename event if the case changed. - Blockly.Events.fire(new Blockly.Events.VarRename(conflictVar, newName)); - conflictVar.name = newName; - - // These blocks refer to the same variable but the case changed. - // No change events should be fired here. - for (var i = 0; i < blocks.length; i++) { - blocks[i].updateVarName(conflictVar); - } + // Simple rename to change the case and update references. + this.renameVariableAndUses_(conflictVar, newName, blocks); } // These blocks now refer to a different variable. @@ -135,7 +132,6 @@ Blockly.VariableMap.prototype.renameVariableWithConflict_ = function(variable, var variableIndex = variableList.indexOf(variable); this.variableMap_[type].splice(variableIndex, 1); - Blockly.Events.setGroup(false); }; /** From 05351569c5d25e0c04bfe0ac63d0871f4719f18a Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 7 Dec 2017 16:08:00 -0800 Subject: [PATCH 25/26] Fix html escaping and flyouts opening --- core/block.js | 7 ++++++- core/variables.js | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/core/block.js b/core/block.js index 1c20108b7..c6f400619 100644 --- a/core/block.js +++ b/core/block.js @@ -695,7 +695,12 @@ Blockly.Block.prototype.getVarModels = function() { for (var i = 0, input; input = this.inputList[i]; i++) { for (var j = 0, field; field = input.fieldRow[j]; j++) { if (field instanceof Blockly.FieldVariable) { - vars.push(this.workspace.getVariableById(field.getValue())); + var model = this.workspace.getVariableById(field.getValue()); + // Check if the variable actually exists (and isn't just a potential + // variable). + if (model) { + vars.push(model); + } } } } diff --git a/core/variables.js b/core/variables.js index 676223c24..b8d40028b 100644 --- a/core/variables.js +++ b/core/variables.js @@ -336,7 +336,7 @@ Blockly.Variables.generateVariableFieldXml_ = function(variableModel) { } var text = '' + variableModel.name + ''; + '">' + goog.string.htmlEscape(variableModel.name) + ''; return text; }; From db9a9f51821d928b065a75de7a27e328f4c56861 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Fri, 15 Dec 2017 15:31:41 -0800 Subject: [PATCH 26/26] Move code from the variable to the variable map. Fixes #1512. --- core/flyout_base.js | 24 +-------- core/variable_map.js | 106 +++++++++++++++++++++++++++++++++++++ core/workspace.js | 123 ++++++++++++------------------------------- 3 files changed, 141 insertions(+), 112 deletions(-) diff --git a/core/flyout_base.js b/core/flyout_base.js index 6ee69aa30..07f4a9096 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -249,29 +249,7 @@ Blockly.Flyout.prototype.init = function(targetWorkspace) { this.targetWorkspace_.getGesture.bind(this.targetWorkspace_); // Get variables from the main workspace rather than the target workspace. - this.workspace_.getVariable = - this.targetWorkspace_.getVariable.bind(this.targetWorkspace_); - - this.workspace_.getVariableById = - this.targetWorkspace_.getVariableById.bind(this.targetWorkspace_); - - this.workspace_.getVariablesOfType = - this.targetWorkspace_.getVariablesOfType.bind(this.targetWorkspace_); - - this.workspace_.deleteVariable = - this.targetWorkspace_.deleteVariable.bind(this.targetWorkspace_); - - this.workspace_.deleteVariableById = - this.targetWorkspace_.deleteVariableById.bind(this.targetWorkspace_); - - this.workspace_.renameVariable = - this.targetWorkspace_.renameVariable.bind(this.targetWorkspace_); - - this.workspace_.renameVariableById = - this.targetWorkspace_.renameVariableById.bind(this.targetWorkspace_); - - this.workspace_.getVariableTypes = - this.targetWorkspace_.getVariableTypes.bind(this.targetWorkspace_); + this.workspace_.variableMap_ = this.targetWorkspace_.getVariableMap(); }; /** diff --git a/core/variable_map.js b/core/variable_map.js index 0995d4fdd..40f150b20 100644 --- a/core/variable_map.js +++ b/core/variable_map.js @@ -58,6 +58,8 @@ Blockly.VariableMap.prototype.clear = function() { this.variableMap_ = new Object(null); }; +/* Begin functions for renaming variables. */ + /** * Rename the given variable by updating its name in the variable map. * @param {!Blockly.VariableModel} variable Variable to rename. @@ -78,6 +80,21 @@ Blockly.VariableMap.prototype.renameVariable = function(variable, newName) { Blockly.Events.setGroup(false); }; +/** + * Rename a variable by updating its name in the variable map. Identify the + * variable to rename with the given ID. + * @param {string} id ID of the variable to rename. + * @param {string} newName New variable name. + */ +Blockly.VariableMap.prototype.renameVariableById = function(id, newName) { + var variable = this.getVariableById(id); + if (!variable) { + throw new Error('Tried to rename a variable that didn\'t exist. ID: ' + id); + } + + this.renameVariable(variable, newName); +}; + /** * Update the name of the given variable and refresh all references to it. * The new name must not conflict with any existing variable names. @@ -134,6 +151,8 @@ Blockly.VariableMap.prototype.renameVariableWithConflict_ = function(variable, }; +/* End functions for renaming variabless. */ + /** * Create a variable with a given name, optional type, and optional ID. * @param {string} name The name of the variable. This must be unique across @@ -174,6 +193,8 @@ Blockly.VariableMap.prototype.createVariable = function(name, return variable; }; +/* Begin functions for variable deletion. */ + /** * Delete a variable. * @param {!Blockly.VariableModel} variable Variable to delete. @@ -189,6 +210,69 @@ Blockly.VariableMap.prototype.deleteVariable = function(variable) { } }; +/** + * Delete a variables by the passed in ID and all of its uses from this + * workspace. May prompt the user for confirmation. + * @param {string} id ID of variable to delete. + */ +Blockly.VariableMap.prototype.deleteVariableById = function(id) { + var variable = this.getVariableById(id); + if (variable) { + // Check whether this variable is a function parameter before deleting. + var variableName = variable.name; + var uses = this.getVariableUsesById(id); + for (var i = 0, block; block = uses[i]; i++) { + if (block.type == 'procedures_defnoreturn' || + block.type == 'procedures_defreturn') { + var procedureName = block.getFieldValue('NAME'); + var deleteText = Blockly.Msg.CANNOT_DELETE_VARIABLE_PROCEDURE. + replace('%1', variableName). + replace('%2', procedureName); + Blockly.alert(deleteText); + return; + } + } + + var map = this; + if (uses.length > 1) { + // Confirm before deleting multiple blocks. + var confirmText = Blockly.Msg.DELETE_VARIABLE_CONFIRMATION. + replace('%1', String(uses.length)). + replace('%2', variableName); + Blockly.confirm(confirmText, + function(ok) { + if (ok) { + map.deleteVariableInternal_(variable, uses); + } + }); + } else { + // No confirmation necessary for a single block. + map.deleteVariableInternal_(variable, uses); + } + } else { + console.warn("Can't delete non-existent variable: " + id); + } +}; + +/** + * Deletes a variable and all of its uses from this workspace without asking the + * user for confirmation. + * @param {!Blockly.VariableModel} variable Variable to delete. + * @param {!Array.} uses An array of uses of the variable. + * @private + */ +Blockly.VariableMap.prototype.deleteVariableInternal_ = function(variable, + uses) { + Blockly.Events.setGroup(true); + for (var i = 0; i < uses.length; i++) { + uses[i].dispose(true, false); + } + this.deleteVariable(variable); + Blockly.Events.setGroup(false); +}; + +/* End functions for variable deletion. */ + /** * Find the variable by the given name and type and return it. Return null if * it is not found. @@ -276,3 +360,25 @@ Blockly.VariableMap.prototype.getAllVariables = function() { } return all_variables; }; + +/** + * Find all the uses of a named variable. + * @param {string} id ID of the variable to find. + * @return {!Array.} Array of block usages. + */ +Blockly.VariableMap.prototype.getVariableUsesById = function(id) { + var uses = []; + var blocks = this.workspace.getAllBlocks(); + // Iterate through every block and check the name. + for (var i = 0; i < blocks.length; i++) { + var blockVariables = blocks[i].getVarModels(); + if (blockVariables) { + for (var j = 0; j < blockVariables.length; j++) { + if (blockVariables[j].getId() == id) { + uses.push(blocks[i]); + } + } + } + } + return uses; +}; diff --git a/core/workspace.js b/core/workspace.js index 6505560ce..29dfb860d 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -212,6 +212,7 @@ Blockly.Workspace.prototype.clear = function() { this.potentialVariableMap_.clear(); }; +/* Begin functions that are just pass-throughs to the variable map. */ /** * Rename a variable by updating its name in the variable map. Identify the * variable to rename with the given ID. @@ -219,12 +220,7 @@ Blockly.Workspace.prototype.clear = function() { * @param {string} newName New variable name. */ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { - var variable = this.getVariableById(id); - if (!variable) { - throw new Error('Tried to rename a variable that didn\'t exist. ID: ' + id); - } - - this.variableMap_.renameVariable(variable, newName); + this.variableMap_.renameVariableById(id, newName); }; /** @@ -248,20 +244,7 @@ Blockly.Workspace.prototype.createVariable = function(name, opt_type, opt_id) { * @return {!Array.} Array of block usages. */ Blockly.Workspace.prototype.getVariableUsesById = function(id) { - var uses = []; - var blocks = this.getAllBlocks(); - // Iterate through every block and check the name. - for (var i = 0; i < blocks.length; i++) { - var blockVariables = blocks[i].getVarModels(); - if (blockVariables) { - for (var j = 0; j < blockVariables.length; j++) { - if (blockVariables[j].getId() == id) { - uses.push(blocks[i]); - } - } - } - } - return uses; + return this.variableMap_.getVariableUsesById(id); }; /** @@ -270,42 +253,7 @@ Blockly.Workspace.prototype.getVariableUsesById = function(id) { * @param {string} id ID of variable to delete. */ Blockly.Workspace.prototype.deleteVariableById = function(id) { - var variable = this.getVariableById(id); - if (variable) { - // Check whether this variable is a function parameter before deleting. - var variableName = variable.name; - var uses = this.getVariableUsesById(id); - for (var i = 0, block; block = uses[i]; i++) { - if (block.type == 'procedures_defnoreturn' || - block.type == 'procedures_defreturn') { - var procedureName = block.getFieldValue('NAME'); - var deleteText = Blockly.Msg.CANNOT_DELETE_VARIABLE_PROCEDURE. - replace('%1', variableName). - replace('%2', procedureName); - Blockly.alert(deleteText); - return; - } - } - - var workspace = this; - if (uses.length > 1) { - // Confirm before deleting multiple blocks. - var confirmText = Blockly.Msg.DELETE_VARIABLE_CONFIRMATION. - replace('%1', String(uses.length)). - replace('%2', variableName); - Blockly.confirm(confirmText, - function(ok) { - if (ok) { - workspace.deleteVariableInternal_(variable, uses); - } - }); - } else { - // No confirmation necessary for a single block. - this.deleteVariableInternal_(variable, uses); - } - } else { - console.warn("Can't delete non-existent variable: " + id); - } + this.variableMap_.deleteVariableById(id); }; /** @@ -316,12 +264,7 @@ Blockly.Workspace.prototype.deleteVariableById = function(id) { * @private */ Blockly.Workspace.prototype.deleteVariableInternal_ = function(variable, uses) { - Blockly.Events.setGroup(true); - for (var i = 0; i < uses.length; i++) { - uses[i].dispose(true, false); - } - this.variableMap_.deleteVariable(variable); - Blockly.Events.setGroup(false); + this.variableMap_.deleteVariableInternal_(variable, uses); }; /** @@ -364,6 +307,35 @@ Blockly.Workspace.prototype.getVariableById = function(id) { return this.variableMap_.getVariableById(id); }; +/** + * Find the variable with the specified type. If type is null, return list of + * variables with empty string type. + * @param {?string} type Type of the variables to find. + * @return {Array.} The sought after variables of the + * passed in type. An empty array if none are found. + */ +Blockly.Workspace.prototype.getVariablesOfType = function(type) { + return this.variableMap_.getVariablesOfType(type); +}; + +/** + * Return all variable types. + * @return {!Array.} List of variable types. + */ +Blockly.Workspace.prototype.getVariableTypes = function() { + return this.variableMap_.getVariableTypes(); +}; + +/** + * Return all variables of all types. + * @return {!Array.} List of variable models. + */ +Blockly.Workspace.prototype.getAllVariables = function() { + return this.variableMap_.getAllVariables(); +}; + +/* End functions that are just pass-throughs to the variable map. */ + /** * Returns the horizontal offset of the workspace. * Intended for LTR/RTL compatibility in XML. @@ -502,33 +474,6 @@ Blockly.Workspace.prototype.allInputsFilled = function(opt_shadowBlocksAreFilled return true; }; -/** - * Find the variable with the specified type. If type is null, return list of - * variables with empty string type. - * @param {?string} type Type of the variables to find. - * @return {Array.} The sought after variables of the - * passed in type. An empty array if none are found. - */ -Blockly.Workspace.prototype.getVariablesOfType = function(type) { - return this.variableMap_.getVariablesOfType(type); -}; - -/** - * Return all variable types. - * @return {!Array.} List of variable types. - */ -Blockly.Workspace.prototype.getVariableTypes = function() { - return this.variableMap_.getVariableTypes(); -}; - -/** - * Return all variables of all types. - * @return {!Array.} List of variable models. - */ -Blockly.Workspace.prototype.getAllVariables = function() { - return this.variableMap_.getAllVariables(); -}; - /** * Return the variable map that contains "potential" variables. These exist in * the flyout but not in the workspace.