From 61db23c78a57ddde20aae279345cf5901e458fa8 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Tue, 5 Dec 2017 13:44:08 -0800 Subject: [PATCH] Get rid of workspace.deleteVariable --- blocks/procedures.js | 5 ++- core/field_variable.js | 1 - core/variable_map.js | 8 ++-- core/variables.js | 32 +++++++------- core/workspace.js | 81 ++++++++++++++-------------------- tests/jsunit/workspace_test.js | 4 +- 6 files changed, 59 insertions(+), 72 deletions(-) diff --git a/blocks/procedures.js b/blocks/procedures.js index 5fc29d302..d8fc6f30a 100644 --- a/blocks/procedures.js +++ b/blocks/procedures.js @@ -467,12 +467,13 @@ Blockly.Blocks['procedures_mutatorarg'] = { if (source && source.workspace && source.workspace.options && source.workspace.options.parentWorkspace) { var workspace = source.workspace.options.parentWorkspace; - var variable = workspace.getVariable(newText); + var variableType = ''; + var variable = workspace.getVariable(newText, variableType); // If there is a case change, rename the variable. if (variable && variable.name !== newText) { workspace.renameVariableById(variable.getId(), newText); } else { - workspace.createVariable(newText); + workspace.createVariable(newText, variableType); } } } diff --git a/core/field_variable.js b/core/field_variable.js index 14e2b1fbf..5276d1d25 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -125,7 +125,6 @@ Blockly.FieldVariable.prototype.getValue = function() { * @return {string} Current text. */ Blockly.FieldVariable.prototype.getText = function() { - //return this.getText(); return this.variable_ ? this.variable_.name : ''; }; diff --git a/core/variable_map.js b/core/variable_map.js index 6af990a97..3a51fc450 100644 --- a/core/variable_map.js +++ b/core/variable_map.js @@ -62,12 +62,10 @@ 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 {string} newName New variable name. - * @param {string=} opt_type The type of the variable to create if variable was - * null. + * @package */ -Blockly.VariableMap.prototype.renameVariable = function(variable, newName, - opt_type) { - var type = variable ? variable.type : (opt_type || ''); +Blockly.VariableMap.prototype.renameVariable = function(variable, newName) { + var type = variable.type; var newVariable = this.getVariable(newName, type); var variableIndex = -1; var newVariableIndex = -1; diff --git a/core/variables.js b/core/variables.js index 4d0167efa..c6e3462da 100644 --- a/core/variables.js +++ b/core/variables.js @@ -64,7 +64,6 @@ Blockly.Variables.allUsedVariables = function(root) { var variableHash = Object.create(null); // Iterate through every block and add each variable to the hash. for (var x = 0; x < blocks.length; x++) { - // TODO (#1199) Switch to IDs. var blockVariables = blocks[x].getVarModels(); if (blockVariables) { for (var y = 0; y < blockVariables.length; y++) { @@ -275,24 +274,25 @@ Blockly.Variables.createVariable = function(workspace, opt_callback, opt_type) { * aborted (cancel button), or undefined if an existing variable was chosen. */ Blockly.Variables.renameVariable = function(workspace, variable, - opt_callback) { + opt_callback) { // This function needs to be named so it can be called recursively. var promptAndCheckWithAlert = function(defaultName) { - Blockly.Variables.promptName( - Blockly.Msg.RENAME_VARIABLE_TITLE.replace('%1', variable.name), defaultName, - function(newName) { - if (newName) { - workspace.renameVariableById(variable.getId(), newName); - if (opt_callback) { - opt_callback(newName); + var promptText = + Blockly.Msg.RENAME_VARIABLE_TITLE.replace('%1', variable.name); + Blockly.Variables.promptName(promptText, defaultName, + function(newName) { + if (newName) { + workspace.renameVariableById(variable.getId(), newName); + if (opt_callback) { + opt_callback(newName); + } + } else { + // User canceled prompt without a value. + if (opt_callback) { + opt_callback(null); + } } - } else { - // User canceled prompt without a value. - if (opt_callback) { - opt_callback(null); - } - } - }); + }); }; promptAndCheckWithAlert(''); }; diff --git a/core/workspace.js b/core/workspace.js index 03477519f..fda910f43 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -286,7 +286,7 @@ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { } } - this.variableMap_.renameVariable(variable, newName, type); + this.variableMap_.renameVariable(variable, newName); Blockly.Events.setGroup(false); }; @@ -361,49 +361,6 @@ Blockly.Workspace.prototype.getVariableUsesById = function(id) { return uses; }; -/** - * Delete a variable by the passed in name and all of its uses from this - * workspace. May prompt the user for confirmation. - * TODO (#1199): Possibly delete this function. - * @param {string} name Name of variable to delete. - * @param {string=} opt_type The type of the variable. If not provided it - * defaults to the empty string, which is a specific type. - */ -Blockly.Workspace.prototype.deleteVariable = function(name, opt_type) { - var type = opt_type || ''; - // Check whether this variable is a function parameter before deleting. - var uses = this.getVariableUses(name, type); - 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; - var variable = workspace.getVariable(name, type); - if (uses.length > 1) { - // Confirm before deleting multiple blocks. - Blockly.confirm( - Blockly.Msg.DELETE_VARIABLE_CONFIRMATION.replace('%1', - String(uses.length)). - replace('%2', name), - function(ok) { - if (ok) { - workspace.deleteVariableInternal_(variable); - } - }); - } else { - // No confirmation necessary for a single block. - this.deleteVariableInternal_(variable); - } -}; - /** * Delete a variables by the passed in ID and all of its uses from this * workspace. May prompt the user for confirmation. @@ -412,7 +369,37 @@ Blockly.Workspace.prototype.deleteVariable = function(name, opt_type) { Blockly.Workspace.prototype.deleteVariableById = function(id) { var variable = this.getVariableById(id); if (variable) { - this.deleteVariableInternal_(variable); + // Check whether this variable is a function parameter before deleting. + var variableName = variable.name; + var uses = this.getVariableUsesById(id); + for (var i = 0, block; block = uses[i]; i++) { + if (block.type == 'procedures_defnoreturn' || + block.type == 'procedures_defreturn') { + var procedureName = block.getFieldValue('NAME'); + var deleteText = Blockly.Msg.CANNOT_DELETE_VARIABLE_PROCEDURE. + replace('%1', variableName). + replace('%2', procedureName); + Blockly.alert(deleteText); + return; + } + } + + var workspace = this; + if (uses.length > 1) { + // Confirm before deleting multiple blocks. + var confirmText = Blockly.Msg.DELETE_VARIABLE_CONFIRMATION. + replace('%1', String(uses.length)). + replace('%2', variableName); + Blockly.confirm(confirmText, + function(ok) { + if (ok) { + workspace.deleteVariableInternal_(variable, uses); + } + }); + } else { + // No confirmation necessary for a single block. + this.deleteVariableInternal_(variable, uses); + } } else { console.warn("Can't delete non-existent variable: " + id); } @@ -422,10 +409,10 @@ Blockly.Workspace.prototype.deleteVariableById = function(id) { * Deletes a variable and all of its uses from this workspace without asking the * user for confirmation. * @param {!Blockly.VariableModel} variable Variable to delete. + * @param {!Array.} uses An array of uses of the variable. * @private */ -Blockly.Workspace.prototype.deleteVariableInternal_ = function(variable) { - var uses = this.getVariableUsesById(variable.getId()); +Blockly.Workspace.prototype.deleteVariableInternal_ = function(variable, uses) { Blockly.Events.setGroup(true); for (var i = 0; i < uses.length; i++) { uses[i].dispose(true, false); diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index 6e4d960d4..c97700f3e 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -155,7 +155,9 @@ function test_deleteVariable_InternalTrivial() { createMockBlock('id1'); createMockBlock('id2'); - workspace.deleteVariableInternal_(var_1); + var uses = workspace.getVariableUsesById(var_1.getId()); + workspace.deleteVariableInternal_(var_1, uses); + var variable = workspace.getVariableById('id1'); var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; assertNull(variable);