Merge pull request #1525 from rachel-fenichel/bugfix/variables_procedures

Use variable IDs for procedure arguments

blocks/procedures.js

@@ -59,6 +59,7 @@ Blockly.Blocks['procedures_defnoreturn'] = {
     this.setTooltip(Blockly.Msg.PROCEDURES_DEFNORETURN_TOOLTIP);
     this.setHelpUrl(Blockly.Msg.PROCEDURES_DEFNORETURN_HELPURL);
     this.arguments_ = [];
+    this.argumentVarModels_ = [];
     this.setStatements_(true);
     this.statementConnection_ = null;
   },
@@ -153,9 +154,14 @@ Blockly.Blocks['procedures_defnoreturn'] = {
    */
   domToMutation: function(xmlElement) {
     this.arguments_ = [];
+    this.argumentVarModels_ = [];
     for (var i = 0, childNode; childNode = xmlElement.childNodes[i]; i++) {
       if (childNode.nodeName.toLowerCase() == 'arg') {
-        this.arguments_.push(childNode.getAttribute('name'));
+        var varName = childNode.getAttribute('name');
+        this.arguments_.push(varName);
+        var variable = Blockly.Variables.getOrCreateVariable(this.workspace,
+            null, varName, '');
+        this.argumentVarModels_.push(variable);
       }
     }
     this.updateParams_();
@@ -206,9 +212,14 @@ Blockly.Blocks['procedures_defnoreturn'] = {
     // Parameter list.
     this.arguments_ = [];
     this.paramIds_ = [];
+    this.argumentVarModels_ = [];
     var paramBlock = containerBlock.getInputTargetBlock('STACK');
     while (paramBlock) {
-      this.arguments_.push(paramBlock.getFieldValue('NAME'));
+      var varName = paramBlock.getFieldValue('NAME');
+      this.arguments_.push(varName);
+      var variable = Blockly.Variables.getOrCreateVariable(this.workspace, null,
+          varName, '');
+      this.argumentVarModels_.push(variable);
       this.paramIds_.push(paramBlock.id);
       paramBlock = paramBlock.nextConnection &&
           paramBlock.nextConnection.targetBlock();
@@ -265,46 +276,67 @@ Blockly.Blocks['procedures_defnoreturn'] = {
    * @this Blockly.Block
    */
   getVarModels: function() {
-    var vars = [];
-    // Not fully initialized.
-    if (!this.arguments_) {
-      return vars;
-    }
-    for (var i = 0, argName; argName = this.arguments_[i]; i++) {
-      // TODO (#1494): When we switch to tracking procedure arguments by ID,
-      // update this.
-      var model = this.workspace.getVariable(argName, '');
-      if (model) {
-        vars.push(model);
-      }
-    }
-    return vars;
+    return this.argumentVarModels_;
   },
   /**
    * Notification that a variable is renaming.
    * If the name matches one of this block's variables, rename it.
-   * @param {string} oldName Previous name of variable.
-   * @param {string} newName Renamed variable.
+   * @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.  Guaranteed to be the same type as the old
+   *     variable.
    * @this Blockly.Block
    */
-  renameVar: function(oldName, newName) {
+  renameVarById: function(oldId, newId) {
+    var oldVariable = this.workspace.getVariableById(oldId);
+    if (oldVariable.type != '') {
+      // Procedure arguments always have the empty type.
+      return;
+    }
+    var oldName = oldVariable.name;
+    var newVar = this.workspace.getVariableById(newId);
+
     var change = false;
-    for (var i = 0; i < this.arguments_.length; i++) {
-      if (Blockly.Names.equals(oldName, this.arguments_[i])) {
+    for (var i = 0; i < this.argumentVarModels_.length; i++) {
+      if (this.argumentVarModels_[i].getId() == oldId) {
+        this.arguments_[i] = newVar.name;
+        this.argumentVarModels_[i] = newVar;
+        change = true;
+      }
+    }
+    if (change) {
+      this.displayRenamedVar_(oldName, newVar.name);
+    }
+  },
+  updateVarName: function(variable) {
+    var newName = variable.name;
+    var change = false;
+    for (var i = 0; i < this.argumentVarModels_.length; i++) {
+      if (this.argumentVarModels_[i].getId() == variable.getId()) {
+        var oldName = this.arguments_[i];
         this.arguments_[i] = newName;
         change = true;
       }
     }
     if (change) {
-      this.updateParams_();
-      // Update the mutator's variables if the mutator is open.
-      if (this.mutator.isVisible()) {
-        var blocks = this.mutator.workspace_.getAllBlocks();
-        for (var i = 0, block; block = blocks[i]; i++) {
-          if (block.type == 'procedures_mutatorarg' &&
-              Blockly.Names.equals(oldName, block.getFieldValue('NAME'))) {
-            block.setFieldValue(newName, 'NAME');
-          }
+      this.displayRenamedVar_(oldName, newName);
+    }
+  },
+  /**
+   * Update the display to reflect a newly renamed argument.
+   * @param {string} oldName The old display name of the argument.
+   * @param {string} newName The new display name of the argument.
+   * @private
+   */
+  displayRenamedVar_: function(oldName, newName) {
+    this.updateParams_();
+    // Update the mutator's variables if the mutator is open.
+    if (this.mutator.isVisible()) {
+      var blocks = this.mutator.workspace_.getAllBlocks();
+      for (var i = 0, block; block = blocks[i]; i++) {
+        if (block.type == 'procedures_mutatorarg' &&
+            Blockly.Names.equals(oldName, block.getFieldValue('NAME'))) {
+          block.setFieldValue(newName, 'NAME');
         }
       }
     }
@@ -376,6 +408,7 @@ Blockly.Blocks['procedures_defreturn'] = {
     this.setTooltip(Blockly.Msg.PROCEDURES_DEFRETURN_TOOLTIP);
     this.setHelpUrl(Blockly.Msg.PROCEDURES_DEFRETURN_HELPURL);
     this.arguments_ = [];
+    this.argumentVarModels_ = [];
     this.setStatements_(true);
     this.statementConnection_ = null;
   },
@@ -398,7 +431,9 @@ Blockly.Blocks['procedures_defreturn'] = {
   },
   getVars: Blockly.Blocks['procedures_defnoreturn'].getVars,
   getVarModels: Blockly.Blocks['procedures_defnoreturn'].getVarModels,
-  renameVar: Blockly.Blocks['procedures_defnoreturn'].renameVar,
+  renameVarById: Blockly.Blocks['procedures_defnoreturn'].renameVarById,
+  updateVarName: Blockly.Blocks['procedures_defnoreturn'].updateVarName,
+  displayRenamedVar_: Blockly.Blocks['procedures_defnoreturn'].displayRenamedVar_,
   customContextMenu: Blockly.Blocks['procedures_defnoreturn'].customContextMenu,
   callType_: 'procedures_callreturn'
 };
@@ -493,6 +528,7 @@ Blockly.Blocks['procedures_callnoreturn'] = {
     // Tooltip is set in renameProcedure.
     this.setHelpUrl(Blockly.Msg.PROCEDURES_CALLNORETURN_HELPURL);
     this.arguments_ = [];
+    this.argumentVarModels_ = [];
     this.quarkConnections_ = {};
     this.quarkIds_ = null;
   },
@@ -591,6 +627,14 @@ Blockly.Blocks['procedures_callnoreturn'] = {
     }
     // Rebuild the block's arguments.
     this.arguments_ = [].concat(paramNames);
+    // And rebuild the argument model list.
+    this.argumentVarModels_ = [];
+    for (var i = 0; i < this.arguments_.length; i++) {
+      var variable = Blockly.Variables.getOrCreateVariable(this.workspace, null,
+          this.arguments_[i], '');
+      this.argumentVarModels_.push(variable);
+    }
+
     this.updateShape_();
     this.quarkIds_ = paramIds;
     // Reconnect any child blocks.
@@ -695,18 +739,30 @@ Blockly.Blocks['procedures_callnoreturn'] = {
   /**
    * Notification that a variable is renaming.
    * If the name matches one of this block's variables, rename it.
-   * @param {string} oldName Previous name of variable.
-   * @param {string} newName Renamed variable.
+   * @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.  Guaranteed to be the same type as the old
+   *     variable.
    * @this Blockly.Block
    */
-  renameVar: function(oldName, newName) {
+  renameVarById: function(oldId, newId) {
+    var newVar = this.workspace.getVariableById(newId);
     for (var i = 0; i < this.arguments_.length; i++) {
-      if (Blockly.Names.equals(oldName, this.arguments_[i])) {
-        this.arguments_[i] = newName;
-        this.getField('ARGNAME' + i).setValue(newName);
+      if (oldId == this.argumentVarModels_[i].getId()) {
+        this.arguments_[i] = newVar.name;
+        this.argumentVarModels_[i] = newVar;
+        this.getField('ARGNAME' + i).setValue(newVar.name);
       }
     }
   },
+  /**
+   * Return all variables referenced by this block.
+   * @return {!Array.<!Blockly.VariableModel>} List of variable models.
+   * @this Blockly.Block
+   */
+  getVarModels: function() {
+    return this.argumentVarModels_;
+  },
   /**
    * Procedure calls cannot exist without the corresponding procedure
    * definition.  Enforce this link whenever an event is fired.
@@ -816,7 +872,7 @@ Blockly.Blocks['procedures_callreturn'] = {
   updateShape_: Blockly.Blocks['procedures_callnoreturn'].updateShape_,
   mutationToDom: Blockly.Blocks['procedures_callnoreturn'].mutationToDom,
   domToMutation: Blockly.Blocks['procedures_callnoreturn'].domToMutation,
-  renameVar: Blockly.Blocks['procedures_callnoreturn'].renameVar,
+  getVarModels: Blockly.Blocks['procedures_callnoreturn'].getVarModels,
   onchange: Blockly.Blocks['procedures_callnoreturn'].onchange,
   customContextMenu:
       Blockly.Blocks['procedures_callnoreturn'].customContextMenu,

core/field_variable.js

@@ -137,6 +137,18 @@ Blockly.FieldVariable.prototype.getText = function() {
   return this.variable_ ? this.variable_.name : '';
 };
 
+/**
+ * Get the variable model for the selected variable.
+ * Not guaranteed to be in the variable map on the workspace (e.g. if accessed
+ * after the variable has been deleted).
+ * @return {?Blockly.VariableModel} the selected variable, or null if none was
+ *     selected.
+ * @package
+ */
+Blockly.FieldVariable.prototype.getVariable = function() {
+  return this.variable_;
+};
+
 /**
  * Set the variable ID.
  * @param {string} id New variable ID, which must reference an existing

core/workspace.js

@@ -144,6 +144,8 @@ Blockly.Workspace.prototype.addTopBlock = function(block) {
   var variableNames = Blockly.Variables.allUsedVariables(block);
   for (var i = 0, name; name = variableNames[i]; i++) {
     if (!this.getVariable(name)) {
+      // TODO (fenichel): Is this still necessary?  Is allUsedVariables still
+      // necessary?
       this.createVariable(name);
     }
   }

core/xml.js

@@ -89,19 +89,20 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) {
 /**
  * Encode a variable field as XML.
  * @param {!Blockly.Field} field The field to encode.
- * @param {!Blockly.Workspace} workspace The workspace that the field is in.
  * @return {?Element} XML element, or null if the field did not need to be
  *     serialized.
  * @private
  */
-Blockly.Xml.fieldToDomVariable_ = function(field, workspace) {
+Blockly.Xml.fieldToDomVariable_ = function(field) {
   var id = field.getValue();
   // The field had not been initialized fully before being serialized.
   if (id == null) {
     field.initModel();
     id = field.getValue();
   }
-  var variable = Blockly.Variables.getVariable(workspace, id);
+  // Get the variable directly from the field, instead of doing a lookup.  This
+  // will work even if the variable has already been deleted.
+  var variable = field.getVariable();
 
   if (!variable) {
     throw Error('Tried to serialize a variable field with no variable.');
@@ -121,10 +122,10 @@ Blockly.Xml.fieldToDomVariable_ = function(field, workspace) {
  *     serialized.
  * @private
  */
-Blockly.Xml.fieldToDom_ = function(field, workspace) {
+Blockly.Xml.fieldToDom_ = function(field) {
   if (field.name && field.EDITABLE) {
     if (field instanceof Blockly.FieldVariable) {
-      return Blockly.Xml.fieldToDomVariable_(field, workspace);
+      return Blockly.Xml.fieldToDomVariable_(field);
     } else {
       var container = goog.dom.createDom('field', null, field.getValue());
       container.setAttribute('name', field.name);
@@ -143,10 +144,9 @@ Blockly.Xml.fieldToDom_ = function(field, workspace) {
  * @private
  */
 Blockly.Xml.allFieldsToDom_ = function(block, element) {
-  var workspace = block.workspace;
   for (var i = 0, input; input = block.inputList[i]; i++) {
     for (var j = 0, field; field = input.fieldRow[j]; j++) {
-      var fieldDom = Blockly.Xml.fieldToDom_(field, workspace);
+      var fieldDom = Blockly.Xml.fieldToDom_(field);
       if (fieldDom) {
         element.appendChild(fieldDom);
       }
@@ -728,8 +728,8 @@ Blockly.Xml.domToBlockHeadless_ = function(xmlBlock, workspace) {
                           'Shadow block not allowed non-shadow child.');
     }
     // Ensure this block doesn't have any variable inputs.
-    goog.asserts.assert(block.getVars().length == 0,
-        'Shadow blocks cannot have variable fields.');
+    goog.asserts.assert(block.getVarModels().length == 0,
+        'Shadow blocks cannot have variable references.');
     block.setShadow(true);
   }
   return block;

generators/dart.js

@@ -108,7 +108,7 @@ Blockly.Dart.init = function(workspace) {
   // Add user variables.
   var variables = workspace.getAllVariables();
   for (var i = 0; i < variables.length; i++) {
-    defvars[i] = Blockly.Dart.variableDB_.getName(variables[i].name,
+    defvars[i] = Blockly.Dart.variableDB_.getName(variables[i].getId(),
         Blockly.Variables.NAME_TYPE);
   }
 

generators/javascript.js

@@ -158,7 +158,7 @@ Blockly.JavaScript.init = function(workspace) {
   // Add user variables.
   var variables = workspace.getAllVariables();
   for (var i = 0; i < variables.length; i++) {
-    defvars[i] = Blockly.JavaScript.variableDB_.getName(variables[i].name,
+    defvars[i] = Blockly.JavaScript.variableDB_.getName(variables[i].getId(),
         Blockly.Variables.NAME_TYPE);
   }
 

generators/php.js

@@ -152,11 +152,9 @@ Blockly.PHP.init = function(workspace) {
   Blockly.PHP.variableDB_.setVariableMap(workspace.getVariableMap());
 
   var defvars = [];
-  var varName;
   var variables = workspace.getAllVariables();
   for (var i = 0, variable; variable = variables[i]; i++) {
-    varName = variable.name;
-    defvars[i] = Blockly.PHP.variableDB_.getName(varName,
+    defvars[i] = Blockly.PHP.variableDB_.getName(variable.getId(),
         Blockly.Variables.NAME_TYPE) + ';';
   }
 

generators/python.js

@@ -165,7 +165,7 @@ Blockly.Python.init = function(workspace) {
   var defvars = [];
   var variables = workspace.getAllVariables();
   for (var i = 0; i < variables.length; i++) {
-    defvars[i] = Blockly.Python.variableDB_.getName(variables[i].name,
+    defvars[i] = Blockly.Python.variableDB_.getName(variables[i].getId(),
         Blockly.Variables.NAME_TYPE) + ' = None';
   }