diff --git a/core/variables.js b/core/variables.js index e31dbbfe8..75209df92 100644 --- a/core/variables.js +++ b/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.} 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.} 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.} 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.'); }; /** diff --git a/core/xml.js b/core/xml.js index 0354841c4..e99a95a58 100644 --- a/core/xml.js +++ b/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)); diff --git a/generators/dart.js b/generators/dart.js index 0189d10da..4dfb9971f 100644 --- a/generators/dart.js +++ b/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)); diff --git a/generators/javascript.js b/generators/javascript.js index be62a4c19..d13ceb359 100644 --- a/generators/javascript.js +++ b/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)); diff --git a/generators/php.js b/generators/php.js index 1ba972bf4..cd53662dc 100644 --- a/generators/php.js +++ b/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) + ';'); diff --git a/generators/php/procedures.js b/generators/php/procedures.js index 879719810..c81eab344 100644 --- a/generators/php/procedures.js +++ b/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, diff --git a/generators/python.js b/generators/python.js index 259a3759b..347a41c2f 100644 --- a/generators/python.js +++ b/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'); diff --git a/generators/python/procedures.js b/generators/python/procedures.js index 7f4524ec5..520b3f589 100644 --- a/generators/python/procedures.js +++ b/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, diff --git a/tests/jsunit/index.html b/tests/jsunit/index.html index 500f2cb37..3987465a6 100644 --- a/tests/jsunit/index.html +++ b/tests/jsunit/index.html @@ -23,6 +23,7 @@ + diff --git a/tests/jsunit/test_utilities.js b/tests/jsunit/test_utilities.js index 39e63eeea..0c256e6f7 100644 --- a/tests/jsunit/test_utilities.js +++ b/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() { diff --git a/tests/jsunit/variables_test.js b/tests/jsunit/variables_test.js new file mode 100644 index 000000000..b6b305bdc --- /dev/null +++ b/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); + } +); diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index 1aaca1895..18cb30403 100644 --- a/tests/jsunit/workspace_test.js +++ b/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); diff --git a/tests/jsunit/workspace_undo_redo_test.js b/tests/jsunit/workspace_undo_redo_test.js index aa960d9f0..ce58c97d9 100644 --- a/tests/jsunit/workspace_undo_redo_test.js +++ b/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();