Merge pull request #1575 from rachel-fenichel/bugfix/1538

Fix #1538

core/variables.js

@@ -47,20 +47,13 @@ Blockly.Variables.NAME_TYPE = Blockly.VARIABLE_CATEGORY_NAME;
 /**
  * Find all user-created variables that are in use in the workspace.
  * For use by generators.
- * @param {!Blockly.Block|!Blockly.Workspace} root Root block or workspace.
- * @return {!Array.<string>} Array of variable names.
+ * To get a list of all variables on a workspace, including unused variables,
+ * call Workspace.getAllVariables.
+ * @param {!Blockly.Workspace} ws The workspace to search for variables.
+ * @return {!Array.<!Blockly.VariableModel>} Array of variable models.
  */
-Blockly.Variables.allUsedVariables = function(root) {
-  var blocks;
-  if (root instanceof Blockly.Block) {
-    // Root is Block.
-    blocks = root.getDescendants();
-  } else if (root.getAllBlocks) {
-    // Root is Workspace.
-    blocks = root.getAllBlocks();
-  } else {
-    throw 'Not Block or Workspace: ' + root;
-  }
+Blockly.Variables.allUsedVarModels = function(ws) {
+  var blocks = ws.getAllBlocks();
   var variableHash = Object.create(null);
   // Iterate through every block and add each variable to the hash.
   for (var x = 0; x < blocks.length; x++) {
@@ -68,36 +61,32 @@ Blockly.Variables.allUsedVariables = function(root) {
     if (blockVariables) {
       for (var y = 0; y < blockVariables.length; y++) {
         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;
+          variableHash[variable.getId()] = variable;
         }
       }
     }
   }
   // Flatten the hash into a list.
   var variableList = [];
-  for (var name in variableHash) {
-    variableList.push(variableHash[name]);
+  for (var id in variableHash) {
+    variableList.push(variableHash[id]);
   }
   return variableList;
 };
 
 /**
- * Find all variables that the user has created through the workspace or
- * toolbox.  For use by generators.
- * @param {!Blockly.Workspace} root The workspace to inspect.
- * @return {!Array.<Blockly.VariableModel>} Array of variable models.
+ * Find all user-created variables that are in use in the workspace and return
+ * only their names.
+ * For use by generators.
+ * To get a list of all variables on a workspace, including unused variables,
+ * call Workspace.getAllVariables.
+ * @deprecated January 2018
  */
-Blockly.Variables.allVariables = function(root) {
-  if (root instanceof Blockly.Block) {
-    // Root is Block.
-    console.warn('Deprecated call to Blockly.Variables.allVariables ' +
-                 'with a block instead of a workspace.  You may want ' +
-                 'Blockly.Variables.allUsedVariables');
-    return {};
-  }
-  return root.getAllVariables();
+Blockly.Variables.allUsedVariables = function() {
+  console.warn('Deprecated call to Blockly.Variables.allUsedVariables. ' +
+      'Use Blockly.Variables.allUsedVarModels instead.\nIf this is a major ' +
+      'issue please file a bug on GitHub.');
 };
 
 /**

core/xml.js

@@ -42,7 +42,8 @@ goog.require('goog.dom');
  */
 Blockly.Xml.workspaceToDom = function(workspace, opt_noId) {
   var xml = goog.dom.createDom('xml');
-  xml.appendChild(Blockly.Xml.variablesToDom(workspace.getAllVariables()));
+  xml.appendChild(Blockly.Xml.variablesToDom(
+      Blockly.Variables.allUsedVarModels(workspace)));
   var blocks = workspace.getTopBlocks(true);
   for (var i = 0, block; block = blocks[i]; i++) {
     xml.appendChild(Blockly.Xml.blockToDomWithXY(block, opt_noId));

generators/dart.js

@@ -112,8 +112,8 @@ Blockly.Dart.init = function(workspace) {
         Blockly.Names.DEVELOPER_VARIABLE_TYPE));
   }
 
-  // Add user variables.
-  var variables = workspace.getAllVariables();
+  // Add user variables, but only ones that are being used.
+  var variables = Blockly.Variables.allUsedVarModels(workspace);
   for (var i = 0; i < variables.length; i++) {
     defvars.push(Blockly.Dart.variableDB_.getName(variables[i].getId(),
         Blockly.Variables.NAME_TYPE));

generators/javascript.js

@@ -165,8 +165,8 @@ Blockly.JavaScript.init = function(workspace) {
         Blockly.Names.DEVELOPER_VARIABLE_TYPE));
   }
 
-  // Add user variables.
-  var variables = workspace.getAllVariables();
+  // Add user variables, but only ones that are being used.
+  var variables = Blockly.Variables.allUsedVarModels(workspace);
   for (var i = 0; i < variables.length; i++) {
     defvars.push(Blockly.JavaScript.variableDB_.getName(variables[i].getId(),
         Blockly.Variables.NAME_TYPE));

generators/php.js

@@ -159,8 +159,8 @@ Blockly.PHP.init = function(workspace) {
         Blockly.Names.DEVELOPER_VARIABLE_TYPE) + ';');
   }
 
-  // Add user-created variables.
-  var variables = workspace.getAllVariables();
+  // Add user variables, but only ones that are being used.
+  var variables = Blockly.Variables.allUsedVarModels(workspace);
   for (var i = 0, variable; variable = variables[i]; i++) {
     defvars.push(Blockly.PHP.variableDB_.getName(variable.getId(),
         Blockly.Variables.NAME_TYPE) + ';');

generators/php/procedures.js

@@ -35,7 +35,7 @@ Blockly.PHP['procedures_defreturn'] = function(block) {
   var globals = [];
   var varName;
   var workspace = block.workspace;
-  var variables = workspace.getAllVariables() || [];
+  var variables = Blockly.Variables.allUsedVarModels(workspace) || [];
   for (var i = 0, variable; variable = variables[i]; i++) {
     varName = variable.name;
     if (block.arguments_.indexOf(varName) == -1) {
@@ -49,7 +49,8 @@ Blockly.PHP['procedures_defreturn'] = function(block) {
     globals.push(Blockly.PHP.variableDB_.getName(devVarList[i],
         Blockly.Names.DEVELOPER_VARIABLE_TYPE));
   }
-  globals = globals.length ? Blockly.PHP.INDENT + 'global ' + globals.join(', ') + ';\n' : '';
+  globals = globals.length ?
+      Blockly.PHP.INDENT + 'global ' + globals.join(', ') + ';\n' : '';
 
   var funcName = Blockly.PHP.variableDB_.getName(
       block.getFieldValue('NAME'), Blockly.Procedures.NAME_TYPE);
@@ -57,8 +58,8 @@ Blockly.PHP['procedures_defreturn'] = function(block) {
   if (Blockly.PHP.STATEMENT_PREFIX) {
     var id = block.id.replace(/\$/g, '$$$$');  // Issue 251.
     branch = Blockly.PHP.prefixLines(
-        Blockly.PHP.STATEMENT_PREFIX.replace(/%1/g,
-        '\'' + id + '\''), Blockly.PHP.INDENT) + branch;
+        Blockly.PHP.STATEMENT_PREFIX.replace(
+            /%1/g, '\'' + id + '\''), Blockly.PHP.INDENT) + branch;
   }
   if (Blockly.PHP.INFINITE_LOOP_TRAP) {
     branch = Blockly.PHP.INFINITE_LOOP_TRAP.replace(/%1/g,

generators/python.js

@@ -170,8 +170,8 @@ Blockly.Python.init = function(workspace) {
         Blockly.Names.DEVELOPER_VARIABLE_TYPE) + ' = None');
   }
 
-  // Add user-created variables.
-  var variables = workspace.getAllVariables();
+  // Add user variables, but only ones that are being used.
+  var variables = Blockly.Variables.allUsedVarModels(workspace);
   for (var i = 0; i < variables.length; i++) {
     defvars.push(Blockly.Python.variableDB_.getName(variables[i].getId(),
         Blockly.Variables.NAME_TYPE) + ' = None');

generators/python/procedures.js

@@ -36,7 +36,7 @@ Blockly.Python['procedures_defreturn'] = function(block) {
   var globals = [];
   var varName;
   var workspace = block.workspace;
-  var variables = workspace.getAllVariables() || [];
+  var variables = Blockly.Variables.allUsedVarModels(workspace) || [];
   for (var i = 0, variable; variable = variables[i]; i++) {
     varName = variable.name;
     if (block.arguments_.indexOf(varName) == -1) {
@@ -51,15 +51,16 @@ Blockly.Python['procedures_defreturn'] = function(block) {
         Blockly.Names.DEVELOPER_VARIABLE_TYPE));
   }
 
-  globals = globals.length ? Blockly.Python.INDENT + 'global ' + globals.join(', ') + '\n' : '';
-  var funcName = Blockly.Python.variableDB_.getName(block.getFieldValue('NAME'),
-      Blockly.Procedures.NAME_TYPE);
+  globals = globals.length ?
+      Blockly.Python.INDENT + 'global ' + globals.join(', ') + '\n' : '';
+  var funcName = Blockly.Python.variableDB_.getName(
+      block.getFieldValue('NAME'), Blockly.Procedures.NAME_TYPE);
   var branch = Blockly.Python.statementToCode(block, 'STACK');
   if (Blockly.Python.STATEMENT_PREFIX) {
     var id = block.id.replace(/\$/g, '$$$$');  // Issue 251.
     branch = Blockly.Python.prefixLines(
-        Blockly.Python.STATEMENT_PREFIX.replace(/%1/g,
-        '\'' + id + '\''), Blockly.Python.INDENT) + branch;
+        Blockly.Python.STATEMENT_PREFIX.replace(
+            /%1/g, '\'' + id + '\''), Blockly.Python.INDENT) + branch;
   }
   if (Blockly.Python.INFINITE_LOOP_TRAP) {
     branch = Blockly.Python.INFINITE_LOOP_TRAP.replace(/%1/g,

tests/jsunit/index.html

@@ -23,6 +23,7 @@
     <script src="names_test.js"></script>
     <script src="procedures_test.js"></script>
     <script src="utils_test.js"></script>
+    <script src="variables_test.js"></script>
     <script src="variable_map_test.js"></script>
     <script src="variable_model_test.js"></script>
     <script src="widget_div_test.js"></script>

tests/jsunit/test_utilities.js

@@ -93,11 +93,12 @@ function checkVariableValues(container, name, type, id) {
 /**
  * Create a test get_var_block.
  * Will fail if get_var_block isn't defined.
- * TODO (fenichel): Rename to createMockVarBlock.
+ * @param {!Blockly.Workspace} workspace The workspace on which to create the
+ *     block.
  * @param {!string} variable_id The id of the variable to reference.
  * @return {!Blockly.Block} The created block.
  */
-function createMockBlock(variable_id) {
+function createMockVarBlock(workspace, variable_id) {
   if (!Blockly.Blocks['get_var_block']) {
     fail();
   }
@@ -114,13 +115,13 @@ function createTwoVariablesAndBlocks(workspace) {
   workspace.createVariable('name1', 'type1', 'id1');
   workspace.createVariable('name2', 'type2', 'id2');
   // Create blocks to refer to both of them.
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockVarBlock(workspace, 'id1');
+  createMockVarBlock(workspace, 'id2');
 }
 
 function createVariableAndBlock(workspace) {
   workspace.createVariable('name1', 'type1', 'id1');
-  createMockBlock('id1');
+  createMockVarBlock(workspace, 'id1');
 }
 
 function defineGetVarBlock() {

tests/jsunit/variables_test.js

@@ -0,0 +1,99 @@
+/**
+ * @license
+ * Visual Blocks Editor
+ *
+ * Copyright 2018 Google Inc.
+ * https://developers.google.com/blockly/
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * @fileoverview Tests for variable utility functions in Blockly
+ * @author fenichel@google.com (Rachel Fenichel)
+ */
+'use strict';
+
+goog.require('goog.testing');
+
+function variablesTest_setUp() {
+  defineGetVarBlock();
+  var workspace = new Blockly.Workspace();
+  workspace.createVariable('foo', 'type1', '1');
+  workspace.createVariable('bar', 'type1', '2');
+  workspace.createVariable('baz', 'type1', '3');
+  return workspace;
+}
+
+function variablesTest_tearDown(workspace) {
+  undefineGetVarBlock();
+  workspace.dispose();
+}
+
+function buildVariablesTest(testImpl) {
+  return function() {
+    var workspace = variablesTest_setUp();
+    try {
+      testImpl(workspace);
+    } finally {
+      variablesTest_tearDown(workspace);
+    }
+  };
+}
+
+var test_allUsedVarModels = buildVariablesTest(
+    function(workspace) {
+      createMockVarBlock(workspace, '1');
+      createMockVarBlock(workspace, '2');
+      createMockVarBlock(workspace, '3');
+
+      var result = Blockly.Variables.allUsedVarModels(workspace);
+      assertEquals('Expected three variables in the list of used variables',
+          3, result.length);
+    }
+);
+
+var test_allUsedVarModels_someUnused = buildVariablesTest(
+    function(workspace) {
+      createMockVarBlock(workspace, '2');
+
+      var result = Blockly.Variables.allUsedVarModels(workspace);
+      assertEquals('Expected one variable in the list of used variables',
+          1, result.length);
+      assertEquals('Expected variable with ID 2 in the list of used variables',
+          '2', result[0].getId());
+    }
+);
+
+var test_allUsedVarModels_varUsedTwice = buildVariablesTest(
+    function(workspace) {
+      createMockVarBlock(workspace, '2');
+      createMockVarBlock(workspace, '2');
+
+      var result = Blockly.Variables.allUsedVarModels(workspace);
+      // Using the same variable multiple times should not change the number of
+      // elements in the list.
+      assertEquals('Expected one variable in the list of used variables',
+          1, result.length);
+      assertEquals('Expected variable with ID 2 in the list of used variables',
+          '2', result[0].getId());
+    }
+);
+
+var test_allUsedVarModels_allUnused = buildVariablesTest(
+    function(workspace) {
+      var result = Blockly.Variables.allUsedVarModels(workspace);
+      assertEquals('Expected no variables in the list of used variables',
+          0, result.length);
+    }
+);

tests/jsunit/workspace_test.js

@@ -141,9 +141,9 @@ function test_deleteVariable_InternalTrivial() {
   workspaceTest_setUp();
   var var_1 = workspace.createVariable('name1', 'type1', 'id1');
   workspace.createVariable('name2', 'type2', 'id2');
-  createMockBlock('id1');
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockVarBlock(workspace, 'id1');
+  createMockVarBlock(workspace, 'id1');
+  createMockVarBlock(workspace, 'id2');
 
   var uses = workspace.getVariableUsesById(var_1.getId());
   workspace.deleteVariableInternal_(var_1, uses);
@@ -169,7 +169,7 @@ function test_addTopBlock_TrivialFlyoutIsTrue() {
   workspace.variableMap_ = targetWorkspace.getVariableMap();
 
   try {
-    var block = createMockBlock('1');
+    var block = createMockVarBlock(workspace, '1');
     workspace.removeTopBlock(block);
     workspace.addTopBlock(block);
     checkVariableValues(workspace, 'name1', '', '1');
@@ -266,8 +266,8 @@ function test_renameVariable_TwoVariablesSameType() {
   workspace.createVariable(oldName, type, id1);
   workspace.createVariable(newName, type, id2);
   // Create blocks to refer to both of them.
-  createMockBlock(id1);
-  createMockBlock(id2);
+  createMockVarBlock(workspace, id1);
+  createMockVarBlock(workspace, id2);
 
   workspace.renameVariableById(id1, newName);
   checkVariableValues(workspace, newName, type, id2);
@@ -340,8 +340,8 @@ function test_renameVariable_TwoVariablesAndOldCase() {
 
   workspace.createVariable(oldName, type, id1);
   workspace.createVariable(oldCase, type, id2);
-  createMockBlock(id1);
-  createMockBlock(id2);
+  createMockVarBlock(workspace, id1);
+  createMockVarBlock(workspace, id2);
 
   workspace.renameVariableById(id1, newName);
 

tests/jsunit/workspace_undo_redo_test.js

@@ -225,7 +225,7 @@ function test_redoAndUndoDeleteVariableTwice_WithBlocks() {
   undoRedoTest_setUp();
   var id = 'id1';
   workspace.createVariable('name1', 'type1', id);
-  createMockBlock(id);
+  createMockVarBlock(workspace, id);
   workspace.deleteVariableById(id);
   workspace.deleteVariableById(id);
 
@@ -268,7 +268,7 @@ function test_undoRedoRenameVariable_OneExists_NoBlocks() {
 function test_undoRedoRenameVariable_OneExists_WithBlocks() {
   undoRedoTest_setUp();
   workspace.createVariable('name1', '', 'id1');
-  createMockBlock('id1');
+  createMockVarBlock(workspace, 'id1');
   workspace.renameVariableById('id1', 'name2');
 
   workspace.undo();
@@ -299,8 +299,8 @@ function test_undoRedoRenameVariable_BothExist_NoBlocks() {
 function test_undoRedoRenameVariable_BothExist_WithBlocks() {
   undoRedoTest_setUp();
   createTwoVarsEmptyType();
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockVarBlock(workspace, 'id1');
+  createMockVarBlock(workspace, 'id2');
   workspace.renameVariableById('id1', 'name2');
 
   workspace.undo();
@@ -333,8 +333,8 @@ function test_undoRedoRenameVariable_BothExistCaseChange_NoBlocks() {
 function test_undoRedoRenameVariable_BothExistCaseChange_WithBlocks() {
   undoRedoTest_setUp();
   createTwoVarsEmptyType();
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockVarBlock(workspace, 'id1');
+  createMockVarBlock(workspace, 'id2');
   workspace.renameVariableById('id1', 'Name2');
 
   workspace.undo();
@@ -367,7 +367,7 @@ function test_undoRedoRenameVariable_OnlyCaseChange_NoBlocks() {
 function test_undoRedoRenameVariable_OnlyCaseChange_WithBlocks() {
   undoRedoTest_setUp();
   workspace.createVariable('name1', '', 'id1');
-  createMockBlock('id1');
+  createMockVarBlock(workspace, 'id1');
   workspace.renameVariableById('id1', 'Name1');
 
   workspace.undo();