Refactor variable rename to handle UI actions more directly. (#726)

FieldDropdown now has onItemSelected(..) method to handle the menu item selection action. Variable renames and deletes are now handled here, instead of during validation.

Also fixes an issue deleting variables used by less than two blocks.

Fixes #723.

core/field_dropdown.js

@@ -105,16 +105,10 @@ Blockly.FieldDropdown.prototype.showEditor_ = function() {
   var thisField = this;
 
   function callback(e) {
+    var menu = this;
     var menuItem = e.target;
     if (menuItem) {
-      var value = menuItem.getValue();
-      if (thisField.sourceBlock_) {
-        // Call any validation function, and allow it to override.
-        value = thisField.callValidator(value);
-      }
-      if (value !== null) {
-        thisField.setValue(value);
-      }
+      thisField.onItemSelected(menu, menuItem);
     }
     Blockly.WidgetDiv.hideIfOwner(thisField);
   }
@@ -192,6 +186,22 @@ Blockly.FieldDropdown.prototype.showEditor_ = function() {
   menuDom.focus();
 };
 
+/**
+ * Handle the selection of an item in the dropdown menu.
+ * @param {goog.ui.Menu} menu The Menu component clicked.
+ * @param {goog.ui.MenuItem} menuItem The MenuItem selected within menu.
+ */
+Blockly.FieldDropdown.prototype.onItemSelected = function(menu, menuItem) {
+  var value = menuItem.getValue();
+  if (this.sourceBlock_) {
+    // Call any validation function, and allow it to override.
+    value = this.callValidator(value);
+  }
+  if (value !== null) {
+    this.setValue(value);
+  }
+}
+
 /**
  * Factor out common words in statically defined options.
  * Create prefix and/or suffix labels.

core/field_variable.js

@@ -48,6 +48,19 @@ Blockly.FieldVariable = function(varname, opt_validator) {
 };
 goog.inherits(Blockly.FieldVariable, Blockly.FieldDropdown);
 
+/**
+ * The menu item index for the rename variable option.
+ * @type {number}
+ */
+Blockly.FieldVariable.prototype.renameVarItemIndex_ = -1;
+
+/**
+ * The menu item index for the delete variable option.
+ * @type {number}
+ */
+Blockly.FieldVariable.prototype.deleteVarItemIndex_ = -1;
+
+
 /**
  * Install this dropdown on a block.
  */
@@ -115,7 +128,11 @@ Blockly.FieldVariable.dropdownCreate = function() {
     variableList.push(name);
   }
   variableList.sort(goog.string.caseInsensitiveCompare);
+
+  this.renameVarItemIndex_ = variableList.length;
   variableList.push(Blockly.Msg.RENAME_VARIABLE);
+
+  this.deleteVarItemIndex_ = variableList.length;
   variableList.push(Blockly.Msg.DELETE_VARIABLE.replace('%1', name));
   // Variables are not language-specific, use the name as both the user-facing
   // text and the internal representation.
@@ -127,29 +144,41 @@ Blockly.FieldVariable.dropdownCreate = function() {
 };
 
 /**
- * Event handler for a change in variable name.
+ * Handle the selection of an item in the variable dropdown menu.
  * Special case the 'Rename variable...' and 'Delete variable...' options.
  * In the rename case, prompt the user for a new name.
- * @param {string} text The selected dropdown menu option.
- * @return {null|undefined|string} An acceptable new variable name, or null if
- *     change is to be either aborted (cancel button) or has been already
- *     handled (rename), or undefined if an existing variable was chosen.
+ * @param {goog.ui.Menu} menu The Menu component clicked.
+ * @param {goog.ui.MenuItem} menuItem The MenuItem selected within menu.
  */
-Blockly.FieldVariable.prototype.classValidator = function(text) {
-  var workspace = this.sourceBlock_.workspace;
-  if (text == Blockly.Msg.RENAME_VARIABLE) {
-    var oldVar = this.getText();
-    Blockly.hideChaff();
-    text = Blockly.Variables.promptName(
-        Blockly.Msg.RENAME_VARIABLE_TITLE.replace('%1', oldVar), oldVar);
-    if (text) {
-      workspace.renameVariable(oldVar, text);
+Blockly.FieldVariable.prototype.onItemSelected = function(menu, menuItem) {
+  var menuLength = menu.getChildCount();
+  var itemText = menuItem.getValue();
+  if (this.sourceBlock_) {
+    var workspace = this.sourceBlock_.workspace;
+    if (this.renameVarItemIndex_ >= 0 &&
+        menu.getChildAt(this.renameVarItemIndex_) === menuItem) {
+      // Rename variable
+      var oldName = this.getText();
+      Blockly.hideChaff();
+      Blockly.Variables.promptName(
+          Blockly.Msg.RENAME_VARIABLE_TITLE.replace('%1', oldName), oldName,
+          function(newName) {
+            if (newName) {
+              workspace.renameVariable(oldName, newName);
+            }
+          });
+      return;
+    } else if (this.deleteVarItemIndex_ >= 0 &&
+        menu.getChildAt(this.deleteVarItemIndex_) === menuItem) {
+      // Delete variable
+      workspace.deleteVariable(this.getText());
+      return;
     }
-    return null;
-  } else if (text == Blockly.Msg.DELETE_VARIABLE.replace('%1',
-      this.getText())) {
-    workspace.deleteVariable(this.getText());
-    return null;
+
+    // Call any validation function, and allow it to override.
+    itemText = this.callValidator(itemText);
+  }
+  if (itemText !== null) {
+    this.setValue(itemText);
   }
-  return undefined;
 };

core/workspace.js

@@ -301,36 +301,44 @@ Blockly.Workspace.prototype.getVariableUses = function(name) {
  * @param {string} name Name of variable to delete.
  */
 Blockly.Workspace.prototype.deleteVariable = function(name) {
+  var workspace = this;
   var variableIndex = this.variableIndexOf(name);
   if (variableIndex != -1) {
+    // Check whether this variable is a function parameter before deleting.
     var uses = this.getVariableUses(name);
-    if (uses.length > 1) {
-      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;
-        }
+    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;
+    }
+    
+    function doDeletion() {
+      Blockly.Events.setGroup(true);
+      for (var i = 0; i < uses.length; i++) {
+        uses[i].dispose(true, false);
+      }
+      Blockly.Events.setGroup(false);
+      workspace.variableList.splice(variableIndex, 1);
+    }
+    if (uses.length > 1) {
+      // Confirm before deleting multiple blocks.
       Blockly.confirm(
           Blockly.Msg.DELETE_VARIABLE_CONFIRMATION.replace('%1', uses.length).
           replace('%2', name),
           function(ok) {
-            if (!ok) {
-              return;
+            if (ok) {
+              doDeletion();
             }
-
-            Blockly.Events.setGroup(true);
-            for (var i = 0; i < uses.length; i++) {
-              uses[i].dispose(true, false);
-            }
-            Blockly.Events.setGroup(false);
-            workspace.variableList.splice(variableIndex, 1);
           });
+    } else {
+      // No confirmation necessary for a single block.
+      doDeletion();
     }
   }
 };

demos/mirror/index.html

@@ -45,6 +45,9 @@
     <block type="math_arithmetic"></block>
     <block type="text"></block>
     <block type="text_print"></block>
+    <block type="variables_get"><field name="VAR">i</field></block>
+    <block type="variables_get"><field name="VAR">j</field></block>
+    <block type="variables_get"><field name="VAR">k</field></block>
   </xml>
 
   <script>