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) {