From 86e88aae92211d129ea3e0638cdf49143b5c24c2 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 30 Nov 2017 16:30:58 -0800 Subject: [PATCH 01/23] Can now create a variable with the button in the flyout; drag a block with a variable out of the flyout; handle default variable names; and import and export variables --- core/field_variable.js | 140 +++++++++++++++++++++++++++++++++++------ core/variables.js | 22 ++++--- core/workspace.js | 2 + core/xml.js | 85 ++++++++++++++++--------- 4 files changed, 192 insertions(+), 57 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index 3fa227c39..e00c6110c 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -47,13 +47,45 @@ goog.require('goog.string'); * @constructor */ Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes) { - Blockly.FieldVariable.superClass_.constructor.call(this, - Blockly.FieldVariable.dropdownCreate, opt_validator); - this.setValue(varname || ''); + // Don't call the FieldDropdown constructor. It'll try too hard. + this.menuGenerator_ = Blockly.FieldVariable.dropdownCreate; + this.size_ = new goog.math.Size(0, Blockly.BlockSvg.MIN_BLOCK_Y); + this.setValidator(opt_validator); + //this.setValue(varname || ''); + // TODO: Add opt_default_type to match default value. If not set, ''. + this.defaultVariableName = (varname || ''); + this.defaultType_ = ''; this.variableTypes = opt_variableTypes; }; goog.inherits(Blockly.FieldVariable, Blockly.FieldDropdown); +Blockly.FieldVariable.getOrCreateVariable = function(workspace, text, type, + id) { + var potentialVariableMap = workspace.isFlyout ? + workspace.targetWorkspace.potentialVariableMap_ : null; + if (id) { + var variable = workspace.getVariableById(id); + if (!variable && potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + } else { + var variable = workspace.getVariable(text, type); + if (!variable && potentialVariableMap) { + variable = potentialVariableMap.getVariable(text, type); + } + } + // Didn't find the variable. + if (!variable) { + if (potentialVariableMap) { + variable = potentialVariableMap.createVariable(text, type, id); + } else { + variable = workspace.createVariable(text, type, id); + } + } + + return variable; +}; + /** * Install this dropdown on a block. */ @@ -69,20 +101,39 @@ Blockly.FieldVariable.prototype.init = function() { }; Blockly.FieldVariable.prototype.initModel = function() { - if (!this.getValue()) { - // Variables without names get uniquely named for this workspace. - var workspace = - this.sourceBlock_.isInFlyout ? - this.sourceBlock_.workspace.targetWorkspace : - this.sourceBlock_.workspace; - this.setValue(Blockly.Variables.generateUniqueName(workspace)); - } - // If the selected variable doesn't exist yet, create it. - // For instance, some blocks in the toolbox have variable dropdowns filled - // in by default. - if (!this.sourceBlock_.isInFlyout) { - this.sourceBlock_.workspace.createVariable(this.getValue()); + // this.workspace_ = this.sourceBlock_.isInFlyout ? + // this.sourceBlock_.workspace.targetWorkspace : + // this.sourceBlock_.workspace; + // // TODO: Describe how the potential variable map is different from the variable + // // map; use getters. + // this.variableMap_ = this.sourceBlock_.isInFlyout ? + // this.workspace_.potentialVariableMap_ : this.workspace_.variableMap_; + // var name = this.defaultValue; + // if (!name) { + // // Variables without names get uniquely named for this workspace. + // name = Blockly.Variables.generateUniqueName(this.workspace_); + // } + // // If the selected variable doesn't exist yet, create it. + // // For instance, some blocks in the toolbox have variable dropdowns filled + // // in by default. + + // var variable = this.variableMap_.getVariable(name, this.defaultType_); + // if (!variable) { + // variable = this.variableMap_.createVariable(name, this.defaultType_); + // } + if (this.variable_) { + return; // Initialization already happened. } + this.workspace_ = this.sourceBlock_.workspace; + var variable = Blockly.FieldVariable.getOrCreateVariable( + this.workspace_, this.defaultVariableName, this.defaultType_, null); + this.setValue(variable.getId()); +}; + +Blockly.FieldVariable.dispose = function() { + Blockly.FieldVariable.superClass_.dispose.call(this); + this.workspace_ = null; + this.variableMap_ = null; }; /** @@ -101,14 +152,20 @@ Blockly.FieldVariable.prototype.setSourceBlock = function(block) { * @return {string} Current text. */ Blockly.FieldVariable.prototype.getValue = function() { - return this.getText(); + //return this.getText(); + return this.variable_ ? this.variable_.getId() : ''; +}; + +Blockly.FieldVariable.prototype.getText = function() { + //return this.getText(); + return this.variable_ ? this.variable_.name : ''; }; /** - * Set the variable name. + * Set the variable name. (DEPRECATED) * @param {string} value New text. */ -Blockly.FieldVariable.prototype.setValue = function(value) { +Blockly.FieldVariable.prototype.oldSetValue = function(value) { var newValue = value; var newText = value; @@ -131,6 +188,47 @@ Blockly.FieldVariable.prototype.setValue = function(value) { this.setText(newText); }; +/** + * Set the variable ID. + * @param {string} id New variable ID, which must reference an existing + * variable. + */ +Blockly.FieldVariable.prototype.setValue = function(id) { + var workspace = this.sourceBlock_.workspace; + //var variable = this.variableMap_.getVariableById(id); + var potentialVariableMap = workspace.isFlyout ? + workspace.targetWorkspace.potentialVariableMap_ : null; + var variable = workspace.getVariableById(id); + if (!variable && potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + if (!variable) { + throw new Error('Variable id doesn\'t point to a real variable! ID was ' + + id); + } + // Type checks! + var type = variable.type; + if (!this.typeIsAllowed_(type)) { + throw new Error('Variable type doesn\'t match this field! Type was ' + + type); + } + this.variable_ = variable; + this.setText(variable.name); +}; + +Blockly.FieldVariable.prototype.typeIsAllowed_ = function(type) { + var typeList = this.getVariableTypes_(); + if (!typeList) { + return true; // If it's null, all types are valid. + } + for (var i = 0; i < typeList.length; i++) { + if (type == typeList[i]) { + return true; + } + } + return false; +}; + /** * Return a list of variable types to include in the dropdown. * @return {!Array.} Array of variable types. @@ -138,6 +236,8 @@ Blockly.FieldVariable.prototype.setValue = function(value) { * @private */ Blockly.FieldVariable.prototype.getVariableTypes_ = function() { + // TODO: Why does this happen every time, instead of once when the workspace + // is set? Do we expect the variable types to change that much? var variableTypes = this.variableTypes; if (variableTypes === null) { // If variableTypes is null, return all variable types. @@ -234,7 +334,7 @@ Blockly.FieldVariable.prototype.onItemSelected = function(menu, menuItem) { return; } else if (id == Blockly.DELETE_VARIABLE_ID) { // Delete variable. - workspace.deleteVariable(this.getText()); + workspace.deleteVariableById(this.variable_.getId()); return; } diff --git a/core/variables.js b/core/variables.js index f2367fd13..4788595eb 100644 --- a/core/variables.js +++ b/core/variables.js @@ -330,12 +330,20 @@ Blockly.Variables.promptName = function(promptText, defaultText, callback) { Blockly.Variables.generateVariableFieldXml_ = function(variableModel) { // The variable name may be user input, so it may contain characters that need // to be escaped to create valid XML. - var element = goog.dom.createDom('field'); - element.setAttribute('name', 'VAR'); - element.setAttribute('variabletype', variableModel.type); - element.setAttribute('id', variableModel.getId()); - element.textContent = variableModel.name; + var typeString = variableModel.type; + if (typeString == '') { + typeString = '\'\''; + } + var text = '' + variableModel.name + ''; + return text; + // var element = goog.dom.createDom('field'); + // element.setAttribute('name', 'VAR'); + // element.setAttribute('variabletype', variableModel.type); + // element.setAttribute('id', variableModel.getId()); + // element.textContent = variableModel.name; - var xmlString = Blockly.Xml.domToText(element); - return xmlString; + // var xmlString = Blockly.Xml.domToText(element); + // return xmlString; }; diff --git a/core/workspace.js b/core/workspace.js index 9196333da..41ed1f915 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -84,6 +84,8 @@ Blockly.Workspace = function(opt_options) { * @private */ this.variableMap_ = new Blockly.VariableMap(this); + + this.potentialVariableMap_ = new Blockly.VariableMap(this); }; /** diff --git a/core/xml.js b/core/xml.js index 343cf8f04..a88de9a6e 100644 --- a/core/xml.js +++ b/core/xml.js @@ -86,6 +86,28 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) { return element; }; +Blockly.Xml.fieldToDomVariable_ = function(field, workspace) { + var potentialVariableMap = workspace.isFlyout ? + workspace.targetWorkspace.potentialVariableMap_ : null; + // Ugh that's not true at all. + var id = field.getValue(); + var variable = workspace.getVariableById(id); + if (!variable && potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + if (variable) { + var container = goog.dom.createDom('field', null, variable.name); + container.setAttribute('name', field.name); + container.setAttribute('id', variable.getId()); + container.setAttribute('variabletype', variable.type); + return container; + } else { + // something went wrong? + console.log('no variable in fieldtodom'); + return null; + } +}; + /** * Encode a field as XML. * @param {!Blockly.Field} field The field to encode. @@ -96,16 +118,13 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) { */ Blockly.Xml.fieldToDom_ = function(field, workspace) { if (field.name && field.EDITABLE) { - var container = goog.dom.createDom('field', null, field.getValue()); - container.setAttribute('name', field.name); if (field instanceof Blockly.FieldVariable) { - var variable = workspace.getVariable(field.getValue()); - if (variable) { - container.setAttribute('id', variable.getId()); - container.setAttribute('variabletype', variable.type); - } + return Blockly.Xml.fieldToDomVariable_(field, workspace); + } else { + var container = goog.dom.createDom('field', null, field.getValue()); + container.setAttribute('name', field.name); + return container; } - return container; } return null; }; @@ -698,6 +717,31 @@ Blockly.Xml.domToBlockHeadless_ = function(xmlBlock, workspace) { return block; }; +Blockly.Xml.domToFieldVariable_ = function(workspace, xml, text, field) { + // TODO (#1199): When we change setValue and getValue to + // interact with IDs instead of names, update this so that we get + // the variable based on ID instead of textContent. + var type = xml.getAttribute('variabletype') || ''; + if (type == '\'\'') { + type = ''; + } + // TODO: Consider using a different name (var_id?) because this is the + // node's ID. + var id = xml.id; + var variable = Blockly.FieldVariable.getOrCreateVariable(workspace, text, + type, id); + + // This should never happen :) + if (type != null && type !== variable.type) { + throw Error('Serialized variable type with id \'' + + variable.getId() + '\' had type ' + variable.type + ', and ' + + 'does not match variable field that references it: ' + + Blockly.Xml.domToText(xml) + '.'); + } + + field.setValue(variable.getId()); +}; + /** * Decode an XML field tag and set the value of that field on the given block. * @param {!Blockly.Block} block The block that is currently being deserialized. @@ -716,29 +760,10 @@ Blockly.Xml.domToField_ = function(block, fieldName, xml) { var workspace = block.workspace; var text = xml.textContent; if (field instanceof Blockly.FieldVariable) { - // TODO (#1199): When we change setValue and getValue to - // interact with IDs instead of names, update this so that we get - // the variable based on ID instead of textContent. - var type = xml.getAttribute('variabletype') || ''; - // TODO: Consider using a different name (varID?) because this is the - // node's ID. - var id = xml.id; - if (id) { - var variable = workspace.getVariableById(id); - } else { - var variable = workspace.getVariable(text, type); - } - if (!variable) { - variable = workspace.createVariable(text, type, id); - } - if (type != null && type !== variable.type) { - throw Error('Serialized variable type with id \'' + - variable.getId() + '\' had type ' + variable.type + ', and ' + - 'does not match variable field that references it: ' + - Blockly.Xml.domToText(xml) + '.'); - } + Blockly.Xml.domToFieldVariable_(workspace, xml, text, field); + } else { + field.setValue(text); } - field.setValue(text); }; /** From 112fcccb31fcf319c5536fa012ef80d87364ffa2 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 30 Nov 2017 17:12:39 -0800 Subject: [PATCH 02/23] Fix renaming --- core/block.js | 18 ++++++++++++++++ core/field_variable.js | 47 ++++++++---------------------------------- core/variables.js | 4 ++-- core/workspace.js | 16 +++++++++++--- 4 files changed, 42 insertions(+), 43 deletions(-) diff --git a/core/block.js b/core/block.js index 570670822..c16129a39 100644 --- a/core/block.js +++ b/core/block.js @@ -724,6 +724,24 @@ Blockly.Block.prototype.renameVar = function(oldName, newName) { } }; +/** + * Notification that a variable is renaming. + * If the name matches one of this block's variables, rename it. + * @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. + */ +Blockly.Block.prototype.renameVarById = function(oldId, newId) { + for (var i = 0, input; input = this.inputList[i]; i++) { + for (var j = 0, field; field = input.fieldRow[j]; j++) { + if (field instanceof Blockly.FieldVariable && + oldId == field.getValue()) { + field.setValue(newId); + } + } + } +}; + /** * Returns the language-neutral value from the field of a block. * @param {string} name The name of the field. diff --git a/core/field_variable.js b/core/field_variable.js index e00c6110c..2e2461dc4 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -161,33 +161,6 @@ Blockly.FieldVariable.prototype.getText = function() { return this.variable_ ? this.variable_.name : ''; }; -/** - * Set the variable name. (DEPRECATED) - * @param {string} value New text. - */ -Blockly.FieldVariable.prototype.oldSetValue = function(value) { - var newValue = value; - var newText = value; - - if (this.sourceBlock_) { - var variable = this.sourceBlock_.workspace.getVariableById(value); - if (variable) { - newText = variable.name; - } - // TODO(marisaleung): Remove name lookup after converting all Field Variable - // instances to use ID instead of name. - else if (variable = this.sourceBlock_.workspace.getVariable(value)) { - newValue = variable.getId(); - } - if (Blockly.Events.isEnabled()) { - Blockly.Events.fire(new Blockly.Events.BlockChange( - this.sourceBlock_, 'field', this.name, this.value_, newValue)); - } - } - this.value_ = newValue; - this.setText(newText); -}; - /** * Set the variable ID. * @param {string} id New variable ID, which must reference an existing @@ -319,17 +292,13 @@ Blockly.FieldVariable.prototype.onItemSelected = function(menu, menuItem) { var id = menuItem.getValue(); // TODO(marisaleung): change setValue() to take in an ID as the parameter. // Then remove itemText. - var itemText; if (this.sourceBlock_ && this.sourceBlock_.workspace) { var workspace = this.sourceBlock_.workspace; var variable = workspace.getVariableById(id); // If the item selected is a variable, set itemText to the variable name. - if (variable) { - itemText = variable.name; - } else if (id == Blockly.RENAME_VARIABLE_ID) { + if (id == Blockly.RENAME_VARIABLE_ID) { // Rename variable. - var currentName = this.getText(); - variable = workspace.getVariable(currentName); + variable = this.variable_; Blockly.Variables.renameVariable(workspace, variable); return; } else if (id == Blockly.DELETE_VARIABLE_ID) { @@ -338,10 +307,12 @@ Blockly.FieldVariable.prototype.onItemSelected = function(menu, menuItem) { return; } - // Call any validation function, and allow it to override. - itemText = this.callValidator(itemText); - } - if (itemText !== null) { - this.setValue(itemText); + // TODO: Call any validation function, and allow it to override. + // For now it's unclear whether the validator should act on the id. + //var validatedId = this.callValidator(variable.getId()); } + // if (variable.getId() !== null) { + // this.setValue(validatedId); + // } + this.setValue(id); }; diff --git a/core/variables.js b/core/variables.js index 4788595eb..26dc0b452 100644 --- a/core/variables.js +++ b/core/variables.js @@ -269,7 +269,7 @@ Blockly.Variables.createVariable = function(workspace, opt_callback, opt_type) { * Rename a variable with the given workspace, variableType, and oldName. * @param {!Blockly.Workspace} workspace The workspace on which to rename the * variable. - * @param {?Blockly.VariableModel} variable Variable to rename. + * @param {Blockly.VariableModel} variable Variable to rename. * @param {function(?string=)=} opt_callback A callback. It will * be passed an acceptable new variable name, or null if change is to be * aborted (cancel button), or undefined if an existing variable was chosen. @@ -282,7 +282,7 @@ Blockly.Variables.renameVariable = function(workspace, variable, Blockly.Msg.RENAME_VARIABLE_TITLE.replace('%1', variable.name), defaultName, function(newName) { if (newName) { - workspace.renameVariable(variable.name, newName); + workspace.renameVariableById(variable.getId(), newName); if (opt_callback) { opt_callback(newName); } diff --git a/core/workspace.js b/core/workspace.js index 41ed1f915..a8f7eccfa 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -248,6 +248,13 @@ Blockly.Workspace.prototype.renameVariableInternal_ = function( variable, newName, opt_type) { var type = variable ? variable.type : (opt_type || ''); var newVariable = this.getVariable(newName, type); + + // If a variable previously existed with the new name, we will coalesce the + // variables and use the ID of the existing variable with the new name. + // Otherwise, we change the current variable's name but not its ID. + var oldId = variable.getId(); + var newId = newVariable ? newVariable.getId() : oldId; + var oldCase; // Find if newVariable case is different. @@ -257,14 +264,17 @@ Blockly.Workspace.prototype.renameVariableInternal_ = function( Blockly.Events.setGroup(true); var blocks = this.getAllBlocks(); + this.variableMap_.renameVariable(variable, newName, type); + + // Iterate through every block and update name. for (var i = 0; i < blocks.length; i++) { - blocks[i].renameVar(variable.name, newName); + blocks[i].renameVarById(oldId, newId); if (oldCase) { - blocks[i].renameVar(oldCase, newName); + blocks[i].renameVarById(newId, newId); + //blocks[i].renameVar(oldCase, newName); } } - this.variableMap_.renameVariable(variable, newName, type); Blockly.Events.setGroup(false); }; From 25913884ab64e82794d442c1cf46b62d7dc517a5 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 30 Nov 2017 17:21:12 -0800 Subject: [PATCH 03/23] Delete by id --- core/block.js | 2 +- core/workspace.js | 25 ++++++++++++++++++++++++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/core/block.js b/core/block.js index c16129a39..58bc311a4 100644 --- a/core/block.js +++ b/core/block.js @@ -697,7 +697,7 @@ Blockly.Block.prototype.getVarModels = function() { if (field instanceof Blockly.FieldVariable) { // TODO (#1199): When we switch to tracking variables by ID, // update this. - var model = this.workspace.getVariable(field.getValue(), ''); + var model = this.workspace.getVariableById(field.getValue()); if (model) { vars.push(model); } diff --git a/core/workspace.js b/core/workspace.js index a8f7eccfa..6055dc4d0 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -355,6 +355,29 @@ Blockly.Workspace.prototype.getVariableUses = function(name, opt_type) { return uses; }; +/** + * Find all the uses of a named variable. + * TODO (#1199): Possibly delete this function. + * @param {string} id ID of the variable to find. + * @return {!Array.} Array of block usages. + */ +Blockly.Workspace.prototype.getVariableUsesById = function(id) { + var uses = []; + var blocks = this.getAllBlocks(); + // Iterate through every block and check the name. + for (var i = 0; i < blocks.length; i++) { + var blockVariables = blocks[i].getVarModels(); + if (blockVariables) { + for (var j = 0; j < blockVariables.length; j++) { + if (blockVariables[j].getId() == id) { + uses.push(blocks[i]); + } + } + } + } + return uses; +}; + /** * Delete a variable by the passed in name and all of its uses from this * workspace. May prompt the user for confirmation. @@ -419,7 +442,7 @@ Blockly.Workspace.prototype.deleteVariableById = function(id) { * @private */ Blockly.Workspace.prototype.deleteVariableInternal_ = function(variable) { - var uses = this.getVariableUses(variable.name); + var uses = this.getVariableUsesById(variable.getId()); Blockly.Events.setGroup(true); for (var i = 0; i < uses.length; i++) { uses[i].dispose(true, false); From fa02e56c17dd52455bce0031227658584c623e69 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Fri, 1 Dec 2017 14:04:10 -0800 Subject: [PATCH 04/23] Fix tests in field_variable_test.js --- core/field_variable.js | 20 +--- tests/jsunit/field_variable_test.js | 163 +++++++++++----------------- 2 files changed, 68 insertions(+), 115 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index 2e2461dc4..4d95a3f06 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -236,10 +236,12 @@ Blockly.FieldVariable.prototype.getVariableTypes_ = function() { * @this {Blockly.FieldVariable} */ Blockly.FieldVariable.dropdownCreate = function() { + if (!this.variable_) { + throw new Error('Tried to call dropdownCreate on a variable field with no' + + ' variable selected.'); + } var variableModelList = []; var name = this.getText(); - // Don't create a new variable if there is nothing selected. - var createSelectedVariable = name ? true : false; var workspace = null; if (this.sourceBlock_) { workspace = this.sourceBlock_.workspace; @@ -254,20 +256,9 @@ Blockly.FieldVariable.dropdownCreate = function() { var variables = workspace.getVariablesOfType(variableType); variableModelList = variableModelList.concat(variables); } - for (var i = 0; i < variableModelList.length; i++) { - if (createSelectedVariable && - goog.string.caseInsensitiveEquals(variableModelList[i].name, name)) { - createSelectedVariable = false; - break; - } - } - } - // Ensure that the currently selected variable is an option. - if (createSelectedVariable && workspace) { - var newVar = workspace.createVariable(name); - variableModelList.push(newVar); } variableModelList.sort(Blockly.VariableModel.compareByName); + var options = []; for (var i = 0; i < variableModelList.length; i++) { // Set the UUID as the internal representation of the variable. @@ -278,6 +269,7 @@ Blockly.FieldVariable.dropdownCreate = function() { options.push([Blockly.Msg.DELETE_VARIABLE.replace('%1', name), Blockly.DELETE_VARIABLE_ID]); } + return options; }; diff --git a/tests/jsunit/field_variable_test.js b/tests/jsunit/field_variable_test.js index 0ef296273..0cf1f11f8 100644 --- a/tests/jsunit/field_variable_test.js +++ b/tests/jsunit/field_variable_test.js @@ -44,10 +44,20 @@ function fieldVariable_mockBlock(workspace) { return {'workspace': workspace, 'isShadow': function(){return false;}}; } +function fieldVariable_createAndInitField(workspace) { + var fieldVariable = new Blockly.FieldVariable('name1'); + var mockBlock = fieldVariable_mockBlock(workspace); + fieldVariable.setSourceBlock(mockBlock); + // No view to initialize, but the model still needs work. + fieldVariable.initModel(); + return fieldVariable; +} + function test_fieldVariable_Constructor() { workspace = new Blockly.Workspace(); var fieldVariable = new Blockly.FieldVariable('name1'); - assertEquals('name1', fieldVariable.getText()); + // The field does not have a variable until after init() is called. + assertEquals('', fieldVariable.getText()); workspace.dispose(); } @@ -55,52 +65,41 @@ function test_fieldVariable_setValueMatchId() { // Expect the fieldVariable value to be set to variable name fieldVariableTestWithMocks_setUp(); workspace.createVariable('name2', null, 'id2'); - var fieldVariable = new Blockly.FieldVariable('name1'); - var mockBlock = fieldVariable_mockBlock(workspace); - fieldVariable.setSourceBlock(mockBlock); + + var fieldVariable = fieldVariable_createAndInitField(workspace); + var event = new Blockly.Events.BlockChange( - mockBlock, 'field', undefined, 'name1', 'id2'); + fieldVariable.sourceBlock_, 'field', undefined, 'name1', 'id2'); setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null); fieldVariable.setValue('id2'); assertEquals('name2', fieldVariable.getText()); - assertEquals('id2', fieldVariable.value_); - fieldVariableTestWithMocks_tearDown(); -} - -function test_fieldVariable_setValueMatchName() { - // Expect the fieldVariable value to be set to variable name - fieldVariableTestWithMocks_setUp(); - workspace.createVariable('name2', null, 'id2'); - var fieldVariable = new Blockly.FieldVariable('name1'); - var mockBlock = fieldVariable_mockBlock(workspace); - fieldVariable.setSourceBlock(mockBlock); - var event = new Blockly.Events.BlockChange( - mockBlock, 'field', undefined, 'name1', 'id2'); - setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null); - - fieldVariable.setValue('name2'); - assertEquals('name2', fieldVariable.getText()); - assertEquals('id2', fieldVariable.value_); + assertEquals('id2', fieldVariable.getValue()); fieldVariableTestWithMocks_tearDown(); } function test_fieldVariable_setValueNoVariable() { - // Expect the fieldVariable value to be set to the passed in string. No error - // should be thrown. fieldVariableTestWithMocks_setUp(); - var fieldVariable = new Blockly.FieldVariable('name1'); - var mockBlock = {'workspace': workspace, - 'isShadow': function(){return false;}}; - fieldVariable.setSourceBlock(mockBlock); + + var fieldVariable = fieldVariable_createAndInitField(workspace); + var mockBlock = fieldVariable.sourceBlock_; + mockBlock.isShadow = function() { + return false; + }; + var event = new Blockly.Events.BlockChange( mockBlock, 'field', undefined, 'name1', 'id1'); setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null); - fieldVariable.setValue('id1'); - assertEquals('id1', fieldVariable.getText()); - assertEquals('id1', fieldVariable.value_); - fieldVariableTestWithMocks_tearDown(); + try { + fieldVariable.setValue('id1'); + // Calling setValue with a variable that doesn't exist throws an error. + fail(); + } catch (e) { + // expected + } finally { + fieldVariableTestWithMocks_tearDown(); + } } function test_fieldVariable_dropdownCreateVariablesExist() { @@ -108,12 +107,12 @@ function test_fieldVariable_dropdownCreateVariablesExist() { workspace = new Blockly.Workspace(); workspace.createVariable('name1', '', 'id1'); workspace.createVariable('name2', '', 'id2'); + + var fieldVariable = fieldVariable_createAndInitField(workspace); + var result_options = Blockly.FieldVariable.dropdownCreate.call( - { - 'sourceBlock_': {'workspace': workspace}, - 'getText': function(){return 'name1';}, - 'getVariableTypes_': function(){return [''];} - }); + fieldVariable); + assertEquals(result_options.length, 3); isEqualArrays(result_options[0], ['name1', 'id1']); isEqualArrays(result_options[1], ['name2', 'id2']); @@ -121,77 +120,30 @@ function test_fieldVariable_dropdownCreateVariablesExist() { workspace.dispose(); } -function test_fieldVariable_dropdownCreateVariablesExist() { - // Expect that the dropdown options will contain the variables that exist. - workspace = new Blockly.Workspace(); - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - var result_options = Blockly.FieldVariable.dropdownCreate.call( - { - 'sourceBlock_': {'workspace': workspace}, - 'getText': function(){return 'name1';}, - 'getVariableTypes_': function(){return [''];} - }); - assertEquals(result_options.length, 3); - isEqualArrays(result_options[0], ['name1', 'id1']); - isEqualArrays(result_options[1], ['name2', 'id2']); - - workspace.dispose(); -} - -function test_fieldVariable_dropdownVariableAndTypeDoesNotExist() { - // Expect a variable will be created for the selected option. Expect the - // workspace variable map to contain the new variable once. +function test_fieldVariable_setValueNull() { + // This should no longer create a variable for the selected option. fieldVariableTestWithMocks_setUp(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['id1', null]); - var result_options = Blockly.FieldVariable.dropdownCreate.call( - { - 'sourceBlock_': {'workspace': workspace}, - 'getText': function(){return 'name1';}, - 'getVariableTypes_': function(){return [''];} - }); + var fieldVariable = fieldVariable_createAndInitField(workspace); + try { + fieldVariable.setValue(null); + fail(); + } catch (e) { + // expected + } finally { + fieldVariableTestWithMocks_tearDown(); + } - // Check the options. - assertEquals(2, result_options.length); - isEqualArrays(result_options[0], ['name1', 'id1']); - // Check the variable map. - assertEquals(1, workspace.getAllVariables().length); - checkVariableValues(workspace, 'name1', '', 'id1'); - - fieldVariableTestWithMocks_tearDown(); -} - -function test_fieldVariable_dropdownVariableDoesNotExistTypeDoes() { - // Expect a variable will be created for the selected option. Expect the - // workspace variable map to contain the new variable once. - fieldVariableTestWithMocks_setUp(); - workspace.createVariable('name1', '', 'id1'); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['id2', null]); - - var result_options = Blockly.FieldVariable.dropdownCreate.call( - { - 'sourceBlock_': {'workspace': workspace}, - 'getText': function(){return 'name2';}, - 'getVariableTypes_': function(){return [''];} - }); - - assertEquals(3, result_options.length); - isEqualArrays(result_options[0], ['name1', 'id1']); - isEqualArrays(result_options[1], ['name2', 'id2']); - assertEquals(2, workspace.variableMap_.getAllVariables().length); - checkVariableValues(workspace, 'name1', '', 'id1'); - checkVariableValues(workspace, 'name2', '', 'id2'); - - fieldVariableTestWithMocks_tearDown(); } function test_fieldVariable_getVariableTypes_undefinedVariableTypes() { // Expect that since variableTypes is undefined, only type empty string - // will be returned. + // will be returned (regardless of what types are available on the workspace). workspace = new Blockly.Workspace(); workspace.createVariable('name1', 'type1'); workspace.createVariable('name2', 'type2'); + var fieldVariable = new Blockly.FieldVariable('name1'); var resultTypes = fieldVariable.getVariableTypes_(); isEqualArrays(resultTypes, ['']); @@ -199,12 +151,14 @@ function test_fieldVariable_getVariableTypes_undefinedVariableTypes() { } function test_fieldVariable_getVariableTypes_givenVariableTypes() { - // Expect that since variableTypes is undefined, only type empty string - // will be returned. + // Expect that since variableTypes is defined, it will be the return value, + // regardless of what types are available on the workspace. workspace = new Blockly.Workspace(); workspace.createVariable('name1', 'type1'); workspace.createVariable('name2', 'type2'); - var fieldVariable = new Blockly.FieldVariable('name1', null, ['type1', 'type2']); + + var fieldVariable = new Blockly.FieldVariable( + 'name1', null, ['type1', 'type2']); var resultTypes = fieldVariable.getVariableTypes_(); isEqualArrays(resultTypes, ['type1', 'type2']); workspace.dispose(); @@ -212,13 +166,17 @@ function test_fieldVariable_getVariableTypes_givenVariableTypes() { function test_fieldVariable_getVariableTypes_nullVariableTypes() { // Expect all variable types to be returned. + // The variable does not need to be initialized to do this--it just needs a + // pointer to the workspace. workspace = new Blockly.Workspace(); workspace.createVariable('name1', 'type1'); workspace.createVariable('name2', 'type2'); + var fieldVariable = new Blockly.FieldVariable('name1'); var mockBlock = fieldVariable_mockBlock(workspace); fieldVariable.setSourceBlock(mockBlock); fieldVariable.variableTypes = null; + var resultTypes = fieldVariable.getVariableTypes_(); isEqualArrays(resultTypes, ['type1', 'type2']); workspace.dispose(); @@ -229,12 +187,15 @@ function test_fieldVariable_getVariableTypes_emptyListVariableTypes() { workspace = new Blockly.Workspace(); workspace.createVariable('name1', 'type1'); workspace.createVariable('name2', 'type2'); + var fieldVariable = new Blockly.FieldVariable('name1'); var mockBlock = fieldVariable_mockBlock(workspace); fieldVariable.setSourceBlock(mockBlock); fieldVariable.variableTypes = []; + try { fieldVariable.getVariableTypes_(); + fail(); } catch (e) { //expected } finally { From 494c4882be7c4cfa47f89cf4e968529e91dfc547 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Fri, 1 Dec 2017 14:05:37 -0800 Subject: [PATCH 05/23] Fix variable_map_test and variable_model_test --- core/xml.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/core/xml.js b/core/xml.js index a88de9a6e..f71c3b52a 100644 --- a/core/xml.js +++ b/core/xml.js @@ -87,13 +87,17 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) { }; Blockly.Xml.fieldToDomVariable_ = function(field, workspace) { - var potentialVariableMap = workspace.isFlyout ? - workspace.targetWorkspace.potentialVariableMap_ : null; // Ugh that's not true at all. var id = field.getValue(); var variable = workspace.getVariableById(id); - if (!variable && potentialVariableMap) { - variable = potentialVariableMap.getVariableById(id); + if (!variable) { + if (workspace.isFlyout && workspace.targetWorkspace) { + var potentialVariableMap = + workspace.targetWorkspace.potentialVariableMap_; + if (potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + } } if (variable) { var container = goog.dom.createDom('field', null, variable.name); From b78264a33e8bbde60862a1371e581037896f9941 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Fri, 1 Dec 2017 16:21:19 -0800 Subject: [PATCH 06/23] Update tests in workspace_test. Get rid of renameVariable --- core/field_variable.js | 14 +- core/workspace.js | 27 +--- tests/jsunit/workspace_test.js | 284 +++++++++++++++------------------ 3 files changed, 147 insertions(+), 178 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index 4d95a3f06..039147079 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -168,13 +168,17 @@ Blockly.FieldVariable.prototype.getText = function() { */ Blockly.FieldVariable.prototype.setValue = function(id) { var workspace = this.sourceBlock_.workspace; - //var variable = this.variableMap_.getVariableById(id); - var potentialVariableMap = workspace.isFlyout ? - workspace.targetWorkspace.potentialVariableMap_ : null; var variable = workspace.getVariableById(id); - if (!variable && potentialVariableMap) { - variable = potentialVariableMap.getVariableById(id); + if (!variable) { + if (workspace.isFlyout && workspace.targetWorkspace) { + var potentialVariableMap = + workspace.targetWorkspace.potentialVariableMap_; + if (potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + } } + if (!variable) { throw new Error('Variable id doesn\'t point to a real variable! ID was ' + id); diff --git a/core/workspace.js b/core/workspace.js index 6055dc4d0..3919190bd 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -241,12 +241,10 @@ Blockly.Workspace.prototype.updateVariableStore = function(clear) { * variable to rename with the given variable. * @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. */ Blockly.Workspace.prototype.renameVariableInternal_ = function( - variable, newName, opt_type) { - var type = variable ? variable.type : (opt_type || ''); + variable, newName) { + var type = variable ? variable.type : ''; var newVariable = this.getVariable(newName, type); // If a variable previously existed with the new name, we will coalesce the @@ -278,24 +276,6 @@ Blockly.Workspace.prototype.renameVariableInternal_ = function( Blockly.Events.setGroup(false); }; - -/** - * Rename a variable by updating its name in the variable map. Identify the - * variable to rename with the given name. - * TODO (#1199): Possibly delete this function. - * @param {string} oldName Variable to rename. - * @param {string} newName New variable name. - * @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.renameVariable = function(oldName, newName, - opt_type) { - var type = opt_type || ''; - // Warning: Prefer to use renameVariableById. - var variable = this.getVariable(oldName, type); - this.renameVariableInternal_(variable, newName, opt_type); -}; - /** * Rename a variable by updating its name in the variable map. Identify the * variable to rename with the given ID. @@ -304,6 +284,9 @@ Blockly.Workspace.prototype.renameVariable = function(oldName, newName, */ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { var variable = this.getVariableById(id); + if (!variable) { + throw new Error('Tried to rename a variable that didn\'t exist. ID: ' + id); + } this.renameVariableInternal_(variable, newName); }; diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index 59a7fa902..d39eac1e4 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -31,6 +31,7 @@ Blockly.defineBlocksWithJsonArray([{ { "type": "field_variable", "name": "VAR", + "variableTypes": ["", "type1", "type2"] } ] }]); @@ -50,9 +51,12 @@ function workspaceTest_tearDown() { * @param {?string} variable_name The string to put into the variable field. * @return {!Blockly.Block} The created block. */ -function createMockBlock(variable_name) { +function createMockBlock(variable_id) { + // Turn off events to avoid testing XML at the same time. + Blockly.Events.disable(); var block = new Blockly.Block(workspace, 'get_var_block'); - block.inputList[0].fieldRow[0].setValue(variable_name); + block.inputList[0].fieldRow[0].setValue(variable_id); + Blockly.Events.enable(); return block; } @@ -158,19 +162,17 @@ function test_getBlockById() { function test_deleteVariable_InternalTrivial() { workspaceTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - var var_1 = workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name1'); - createMockBlock('name2'); + var var_1 = workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + createMockBlock('id1'); + createMockBlock('id1'); + createMockBlock('id2'); workspace.deleteVariableInternal_(var_1); - var variable = workspace.getVariable('name1', ''); - var block_var_name = workspace.topBlocks_[0].getVars()[0]; + var variable = workspace.getVariableById('id1'); + var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; assertNull(variable); - checkVariableValues(workspace, 'name2', '', 'id2'); + checkVariableValues(workspace, 'name2', 'type2', 'id2'); assertEquals('name2', block_var_name); workspaceTest_tearDown(); } @@ -239,7 +241,7 @@ function test_updateVariableStore_ClearAndOneInUse() { try { workspace.updateVariableStore(true); checkVariableValues(workspace, 'name1', '', 'id1'); - var variabe = workspace.getVariable('name2', ''); + var variable = workspace.getVariable('name2', ''); assertNull(variable); } finally { workspaceTest_tearDown(); @@ -249,16 +251,21 @@ function test_updateVariableStore_ClearAndOneInUse() { function test_addTopBlock_TrivialFlyoutIsTrue() { workspaceTest_setUp(); workspace.isFlyout = true; - var block = createMockBlock(); - workspace.removeTopBlock(block); - setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', [block], - [['name1']]); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); + var targetWorkspace = new Blockly.Workspace(); + workspace.targetWorkspace = targetWorkspace; + targetWorkspace.createVariable('name1', '', '1'); + + // Flyout.init usually does this binding. + workspace.getVariableById = + targetWorkspace.getVariableById.bind(targetWorkspace); try { + var block = createMockBlock('1'); + workspace.removeTopBlock(block); workspace.addTopBlock(block); checkVariableValues(workspace, 'name1', '', '1'); } finally { + targetWorkspace.dispose(); workspaceTest_tearDown(); } } @@ -297,52 +304,41 @@ function test_clear_NoVariables() { } } -function test_renameVariable_NoBlocks() { - // Expect 'renameVariable' to create new variable with newName. +function test_renameVariable_NoReference() { + // Test renaming a variable in the simplest case: when no blocks refer to it. workspaceTest_setUp(); + var id = 'id1'; + var type = 'type1'; var oldName = 'name1'; var newName = 'name2'; - // Mocked setGroup to ensure only one call to the mocked genUid. - setUpMockMethod(mockControl_, Blockly.Events, 'setGroup', [true, false], - null); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); + workspace.createVariable(oldName, type, id); try { - workspace.renameVariable(oldName, newName); - checkVariableValues(workspace, 'name2', '', '1'); - var variable = workspace.getVariable(oldName, ''); - assertNull(variable); + workspace.renameVariableById(id, newName); + checkVariableValues(workspace, newName, type, id); + // Renaming should not have created a new variable. + assertEquals(1, workspace.getAllVariables().length); } finally { workspaceTest_tearDown(); } } -function test_renameVariable_SameNameNoBlocks() { - // Expect 'renameVariable' to create new variable with newName. - workspaceTest_setUp(); - var name = 'name1'; - workspace.createVariable(name, 'type1', 'id1'); - - workspace.renameVariable(name, name); - checkVariableValues(workspace, name, 'type1', 'id1'); - workspaceTest_tearDown(); -} - -function test_renameVariable_OnlyOldNameBlockExists() { +function test_renameVariable_ReferenceExists() { + // Test renaming a variable when a reference to it exists. // Expect 'renameVariable' to change oldName variable name to newName. workspaceTest_setUp(); + var id = 'id1'; + var type = 'type1'; var oldName = 'name1'; var newName = 'name2'; - workspace.createVariable(oldName, '', 'id1'); - createMockBlock(oldName); + workspace.createVariable(oldName, type, id); + createMockBlock(id); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.renameVariable(oldName, newName); - checkVariableValues(workspace, newName, '', 'id1'); - var variable = workspace.getVariable(oldName, ''); - var block_var_name = workspace.topBlocks_[0].getVars()[0]; - assertNull(variable); + workspace.renameVariableById(id, newName); + checkVariableValues(workspace, newName, type, id); + // Renaming should not have created a new variable. + assertEquals(1, workspace.getAllVariables().length); + var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; assertEquals(newName, block_var_name); workspaceTest_tearDown(); } @@ -351,151 +347,137 @@ function test_renameVariable_TwoVariablesSameType() { // Expect 'renameVariable' to change oldName variable name to newName. // Expect oldName block name to change to newName workspaceTest_setUp(); + var id1 = 'id1'; + var id2 = 'id2'; + var type = 'type1'; + var oldName = 'name1'; var newName = 'name2'; - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable(oldName, '', 'id1'); - workspace.createVariable(newName, '', 'id2'); - createMockBlock(oldName); - createMockBlock(newName); + // Create two variables of the same type. + workspace.createVariable(oldName, type, id1); + workspace.createVariable(newName, type, id2); + // Create blocks to refer to both of them. + createMockBlock(id1); + createMockBlock(id2); - workspace.renameVariable(oldName, newName); - checkVariableValues(workspace, newName, '', 'id2'); - var variable = workspace.getVariable(oldName); - var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; - var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; + workspace.renameVariableById(id1, newName); + checkVariableValues(workspace, newName, type, id2); + // The old variable should have been deleted. + var variable = workspace.getVariableById(id1); assertNull(variable); + + // There should only be one variable left. + assertEquals(1, workspace.getAllVariables().length); + + // References should have the correct names. + var block_var_name_1 = workspace.topBlocks_[0].getVarModels()[0].name; + var block_var_name_2 = workspace.topBlocks_[1].getVarModels()[0].name; assertEquals(newName, block_var_name_1); assertEquals(newName, block_var_name_2); + workspaceTest_tearDown(); } function test_renameVariable_TwoVariablesDifferentType() { - // Expect triggered error because of different types + // Expect the rename to succeed, because variables with different types are + // allowed to have the same name. workspaceTest_setUp(); + var id1 = 'id1'; + var id2 = 'id2'; + var type1 = 'type1'; + var type2 = 'type2'; + var oldName = 'name1'; var newName = 'name2'; - workspace.createVariable(oldName, 'type1', 'id1'); - workspace.createVariable(newName, 'type2', 'id2'); - createMockBlock(oldName); - createMockBlock(newName); + // Create two variables of different types. + workspace.createVariable(oldName, type1, id1); + workspace.createVariable(newName, type2, id2); + // Create blocks to refer to both of them. + createMockBlock(id1); + createMockBlock(id2); - try { - workspace.renameVariable(oldName, newName); - fail(); - } catch (e) { - // expected - } - checkVariableValues(workspace, oldName, 'type1', 'id1'); - checkVariableValues(workspace, newName, 'type2', 'id2'); - var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; - var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; - assertEquals(oldName, block_var_name_1); + workspace.renameVariableById(id1, newName); + + checkVariableValues(workspace, newName, type1, id1); + checkVariableValues(workspace, newName, type2, id2); + + // References shoul have the correct names. + var block_var_name_1 = workspace.topBlocks_[0].getVarModels()[0].name; + var block_var_name_2 = workspace.topBlocks_[1].getVarModels()[0].name; + assertEquals(newName, block_var_name_1); assertEquals(newName, block_var_name_2); + workspaceTest_tearDown(); } function test_renameVariable_OldCase() { - // Expect triggered error because of different types + // Rename a variable with a single reference. Update only the capitalization. workspaceTest_setUp(); var oldCase = 'Name1'; var newName = 'name1'; - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable(oldCase, '', 'id1'); - createMockBlock(oldCase); + var type = 'type1'; + var id = 'id1'; + workspace.createVariable(oldCase, type, id); + createMockBlock(id); - workspace.renameVariable(oldCase, newName); - checkVariableValues(workspace, newName, '', 'id1'); - var result_oldCase = workspace.getVariable(oldCase, '').name; - assertNotEquals(oldCase, result_oldCase); + workspace.renameVariableById(id, newName); + checkVariableValues(workspace, newName, type, id); + var variable = workspace.getVariableById(id); + assertNotEquals(oldCase, id.name); workspaceTest_tearDown(); } function test_renameVariable_TwoVariablesAndOldCase() { - // Expect triggered error because of different types + // Test renaming a variable to an in-use name, but with different + // capitalization. The new capitalization should apply everywhere. + + // TODO (fenichel): What about different capitalization but also different + // types? workspaceTest_setUp(); var oldName = 'name1'; var oldCase = 'Name2'; var newName = 'name2'; - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable(oldName, '', 'id1'); - workspace.createVariable(oldCase, '', 'id2'); - createMockBlock(oldName); - createMockBlock(oldCase); - workspace.renameVariable(oldName, newName); + var id1 = 'id1'; + var id2 = 'id2'; - checkVariableValues(workspace, newName, '', 'id2'); - var variable = workspace.getVariable(oldName); - var result_oldCase = workspace.getVariable(oldCase).name; - var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; - var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; + var type = 'type1'; + + workspace.createVariable(oldName, type, id1); + workspace.createVariable(oldCase, type, id2); + createMockBlock(id1); + createMockBlock(id2); + + workspace.renameVariableById(id1, newName); + + checkVariableValues(workspace, newName, type, id2); + + // The old variable should have been deleted. + var variable = workspace.getVariableById(id1); assertNull(variable); - assertNotEquals(oldCase, result_oldCase); + + // There should only be one variable left. + assertEquals(1, workspace.getAllVariables().length); + + // Blocks should now use the new capitalization. + var block_var_name_1 = workspace.topBlocks_[0].getVarModels()[0].name; + var block_var_name_2 = workspace.topBlocks_[1].getVarModels()[0].name; assertEquals(newName, block_var_name_1); assertEquals(newName, block_var_name_2); workspaceTest_tearDown(); } -// Extra testing not required for renameVariableById. It calls renameVariable -// and that has extensive testing. -function test_renameVariableById_TwoVariablesSameType() { - // Expect 'renameVariableById' to change oldName variable name to newName. - // Expect oldName block name to change to newName - workspaceTest_setUp(); - var oldName = 'name1'; - var newName = 'name2'; - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable(oldName, '', 'id1'); - workspace.createVariable(newName, '', 'id2'); - createMockBlock(oldName); - createMockBlock(newName); - - workspace.renameVariableById('id1', newName); - checkVariableValues(workspace, newName, '', 'id2'); - var variable = workspace.getVariable(oldName); - var block_var_name_1 = workspace.topBlocks_[0].getVars()[0]; - var block_var_name_2 = workspace.topBlocks_[1].getVars()[0]; - assertNull(variable); - assertEquals(newName, block_var_name_1); - assertEquals(newName, block_var_name_2); - workspaceTest_tearDown(); -} - -function test_deleteVariable_Trivial() { - workspaceTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name2'); - - workspace.deleteVariable('name1', ''); - checkVariableValues(workspace, 'name2', '', 'id2'); - var variable = workspace.getVariable('name1', ''); - var block_var_name = workspace.topBlocks_[0].getVars()[0]; - assertNull(variable); - assertEquals('name2', block_var_name); - workspaceTest_tearDown(); -} - function test_deleteVariableById_Trivial() { workspaceTest_setUp(); - // TODO (#1199): Make a version of this test that uses different types. - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name2'); + workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + createMockBlock('id1'); + createMockBlock('id2'); workspace.deleteVariableById('id1'); - checkVariableValues(workspace, 'name2', '', 'id2'); - var variable = workspace.getVariable('name1', ''); - var block_var_name = workspace.topBlocks_[0].getVars()[0]; + checkVariableValues(workspace, 'name2', 'type2', 'id2'); + var variable = workspace.getVariableById('id1'); + var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; assertNull(variable); assertEquals('name2', block_var_name); workspaceTest_tearDown(); From a8a83a77a7bb206a701c96361c5f5f32c810435a Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Fri, 1 Dec 2017 16:22:52 -0800 Subject: [PATCH 07/23] Move code from renameVariableInternal to renameVariableById --- core/workspace.js | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/core/workspace.js b/core/workspace.js index 3919190bd..2c4205158 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -238,13 +238,17 @@ Blockly.Workspace.prototype.updateVariableStore = function(clear) { /** * Rename a variable by updating its name in the variable map. Identify the - * variable to rename with the given variable. - * @param {?Blockly.VariableModel} variable Variable to rename. + * variable to rename with the given ID. + * @param {string} id ID of the variable to rename. * @param {string} newName New variable name. */ -Blockly.Workspace.prototype.renameVariableInternal_ = function( - variable, newName) { - var type = variable ? variable.type : ''; +Blockly.Workspace.prototype.renameVariableById = function(id, newName) { + var variable = this.getVariableById(id); + if (!variable) { + throw new Error('Tried to rename a variable that didn\'t exist. ID: ' + id); + } + + var type = variable.type; var newVariable = this.getVariable(newName, type); // If a variable previously existed with the new name, we will coalesce the @@ -264,32 +268,16 @@ Blockly.Workspace.prototype.renameVariableInternal_ = function( var blocks = this.getAllBlocks(); this.variableMap_.renameVariable(variable, newName, type); - // Iterate through every block and update name. for (var i = 0; i < blocks.length; i++) { blocks[i].renameVarById(oldId, newId); if (oldCase) { blocks[i].renameVarById(newId, newId); - //blocks[i].renameVar(oldCase, newName); } } Blockly.Events.setGroup(false); }; -/** - * Rename a variable by updating its name in the variable map. Identify the - * variable to rename with the given ID. - * @param {string} id ID of the variable to rename. - * @param {string} newName New variable name. - */ -Blockly.Workspace.prototype.renameVariableById = function(id, newName) { - var variable = this.getVariableById(id); - if (!variable) { - throw new Error('Tried to rename a variable that didn\'t exist. ID: ' + id); - } - this.renameVariableInternal_(variable, newName); -}; - /** * Create a variable with a given name, optional type, and optional ID. * @param {!string} name The name of the variable. This must be unique across From d1c2401fcf0793cb1e7cb416c88bfe398c3c62b4 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Mon, 4 Dec 2017 14:19:19 -0800 Subject: [PATCH 08/23] All but XML tests now pass --- core/field_variable.js | 7 ++ core/workspace.js | 3 +- tests/jsunit/field_variable_test.js | 7 +- tests/jsunit/index.html | 4 +- tests/jsunit/procedures_test.js | 11 +- tests/jsunit/test_utilities.js | 19 +++ tests/jsunit/workspace_test.js | 37 ++---- tests/jsunit/workspace_undo_redo_test.js | 143 ++++++++++------------- tests/jsunit/xml_test.js | 6 +- 9 files changed, 118 insertions(+), 119 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index 039147079..4dbe3e81a 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -167,6 +167,8 @@ Blockly.FieldVariable.prototype.getText = function() { * variable. */ Blockly.FieldVariable.prototype.setValue = function(id) { + // TODO: Handle undo a change to go back to the default value. + // TODO: Handle null ID, which means "use default". var workspace = this.sourceBlock_.workspace; var variable = workspace.getVariableById(id); if (!variable) { @@ -189,6 +191,11 @@ Blockly.FieldVariable.prototype.setValue = function(id) { throw new Error('Variable type doesn\'t match this field! Type was ' + type); } + if (this.sourceBlock_ && Blockly.Events.isEnabled()) { + var oldValue = this.variable_ ? this.variable_.getId() : null; + Blockly.Events.fire(new Blockly.Events.BlockChange( + this.sourceBlock_, 'field', this.name, oldValue, variable.getId())); + } this.variable_ = variable; this.setText(variable.name); }; diff --git a/core/workspace.js b/core/workspace.js index 2c4205158..6fed60405 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -266,7 +266,6 @@ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { Blockly.Events.setGroup(true); var blocks = this.getAllBlocks(); - this.variableMap_.renameVariable(variable, newName, type); // Iterate through every block and update name. for (var i = 0; i < blocks.length; i++) { @@ -275,6 +274,8 @@ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { blocks[i].renameVarById(newId, newId); } } + + this.variableMap_.renameVariable(variable, newName, type); Blockly.Events.setGroup(false); }; diff --git a/tests/jsunit/field_variable_test.js b/tests/jsunit/field_variable_test.js index 0cf1f11f8..6d3ef9d66 100644 --- a/tests/jsunit/field_variable_test.js +++ b/tests/jsunit/field_variable_test.js @@ -68,8 +68,9 @@ function test_fieldVariable_setValueMatchId() { var fieldVariable = fieldVariable_createAndInitField(workspace); + var oldId = fieldVariable.getValue(); var event = new Blockly.Events.BlockChange( - fieldVariable.sourceBlock_, 'field', undefined, 'name1', 'id2'); + fieldVariable.sourceBlock_, 'field', undefined, oldId, 'id2'); setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null); fieldVariable.setValue('id2'); @@ -87,10 +88,6 @@ function test_fieldVariable_setValueNoVariable() { return false; }; - var event = new Blockly.Events.BlockChange( - mockBlock, 'field', undefined, 'name1', 'id1'); - setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null); - try { fieldVariable.setValue('id1'); // Calling setValue with a variable that doesn't exist throws an error. diff --git a/tests/jsunit/index.html b/tests/jsunit/index.html index 500f2cb37..96424df44 100644 --- a/tests/jsunit/index.html +++ b/tests/jsunit/index.html @@ -8,7 +8,7 @@ - + diff --git a/tests/jsunit/procedures_test.js b/tests/jsunit/procedures_test.js index 0284f3978..f993af892 100644 --- a/tests/jsunit/procedures_test.js +++ b/tests/jsunit/procedures_test.js @@ -37,14 +37,15 @@ function proceduresTest_setUpWithMockBlocks() { 'message0': '%1', 'args0': [ { + // TODO: Why is this a variable? It shouldn't need to be. 'type': 'field_variable', 'name': 'NAME', 'variable': 'item' } - ], + ] }]); Blockly.Blocks['procedure_mock_block'].getProcedureDef = function() { - return [this.getFieldValue('NAME'), [], false]; + return [this.getField('NAME').getText(), [], false]; }; } @@ -63,8 +64,9 @@ function test_isNameUsed_NoBlocks() { function test_isNameUsed_False() { proceduresTest_setUpWithMockBlocks(); + workspace.createVariable('name2', '', 'id2'); var block = new Blockly.Block(workspace, 'procedure_mock_block'); - block.setFieldValue('name2', 'NAME'); + block.setFieldValue('id2', 'NAME'); var result = Blockly.Procedures.isNameUsed('name1', workspace); assertFalse(result); @@ -73,8 +75,9 @@ function test_isNameUsed_False() { function test_isNameUsed_True() { proceduresTest_setUpWithMockBlocks(); + workspace.createVariable('name1', '', 'id1'); var block = new Blockly.Block(workspace, 'procedure_mock_block'); - block.setFieldValue('name1', 'NAME'); + block.setFieldValue('id1', 'NAME'); var result = Blockly.Procedures.isNameUsed('name1', workspace); assertTrue(result); diff --git a/tests/jsunit/test_utilities.js b/tests/jsunit/test_utilities.js index 6a80a3549..d7f6085bf 100644 --- a/tests/jsunit/test_utilities.js +++ b/tests/jsunit/test_utilities.js @@ -89,3 +89,22 @@ function checkVariableValues(container, name, type, id) { assertEquals(type, variable.type); assertEquals(id, variable.getId()); } + +/** + * Create a test get_var_block. + * Will fail if get_var_block isn't defined. + * TODO (fenichel): Rename to createMockVarBlock. + * @param {!string} variable_id The id of the variable to reference. + * @return {!Blockly.Block} The created block. + */ +function createMockBlock(variable_id) { + if (!Blockly.Blocks['get_var_block']) { + fail(); + } + // Turn off events to avoid testing XML at the same time. + Blockly.Events.disable(); + var block = new Blockly.Block(workspace, 'get_var_block'); + block.inputList[0].fieldRow[0].setValue(variable_id); + Blockly.Events.enable(); + return block; +} diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index d39eac1e4..6e4d960d4 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -24,42 +24,29 @@ goog.require('goog.testing.MockControl'); var workspace; var mockControl_; -Blockly.defineBlocksWithJsonArray([{ - "type": "get_var_block", - "message0": "%1", - "args0": [ - { - "type": "field_variable", - "name": "VAR", - "variableTypes": ["", "type1", "type2"] - } - ] -}]); function workspaceTest_setUp() { + Blockly.defineBlocksWithJsonArray([{ + "type": "get_var_block", + "message0": "%1", + "args0": [ + { + "type": "field_variable", + "name": "VAR", + "variableTypes": ["", "type1", "type2"] + } + ] + }]); workspace = new Blockly.Workspace(); mockControl_ = new goog.testing.MockControl(); } function workspaceTest_tearDown() { + delete Blockly.Blocks['get_var_block']; mockControl_.$tearDown(); workspace.dispose(); } -/** - * Create a test get_var_block. - * @param {?string} variable_name The string to put into the variable field. - * @return {!Blockly.Block} The created block. - */ -function createMockBlock(variable_id) { - // Turn off events to avoid testing XML at the same time. - Blockly.Events.disable(); - var block = new Blockly.Block(workspace, 'get_var_block'); - block.inputList[0].fieldRow[0].setValue(variable_id); - Blockly.Events.enable(); - return block; -} - function test_emptyWorkspace() { workspaceTest_setUp(); try { diff --git a/tests/jsunit/workspace_undo_redo_test.js b/tests/jsunit/workspace_undo_redo_test.js index a8befe33c..092dbb4a8 100644 --- a/tests/jsunit/workspace_undo_redo_test.js +++ b/tests/jsunit/workspace_undo_redo_test.js @@ -33,16 +33,6 @@ goog.require('goog.testing.MockControl'); var workspace; var mockControl_; var savedFireFunc = Blockly.Events.fire; -Blockly.defineBlocksWithJsonArray([{ - "type": "get_var_block", - "message0": "%1", - "args0": [ - { - "type": "field_variable", - "name": "VAR", - } - ] -}]); function temporary_fireEvent(event) { if (!Blockly.Events.isEnabled()) { @@ -53,28 +43,29 @@ function temporary_fireEvent(event) { } function undoRedoTest_setUp() { + Blockly.defineBlocksWithJsonArray([{ + "type": "get_var_block", + "message0": "%1", + "args0": [ + { + "type": "field_variable", + "name": "VAR", + "variableTypes": ["", "type1", "type2"] + } + ] + }]); workspace = new Blockly.Workspace(); mockControl_ = new goog.testing.MockControl(); Blockly.Events.fire = temporary_fireEvent; } function undoRedoTest_tearDown() { + delete Blockly.Blocks['get_var_block']; mockControl_.$tearDown(); workspace.dispose(); Blockly.Events.fire = savedFireFunc; } -/** - * Create a test get_var_block. - * @param {string} variableName The string to put into the variable field. - * @return {!Blockly.Block} The created block. - */ -function createMockBlock(variableName) { - var block = new Blockly.Block(workspace, 'get_var_block'); - block.inputList[0].fieldRow[0].setValue(variableName); - return block; -} - /** * Check that the top block with the given index contains a variable with * the given name. @@ -82,7 +73,7 @@ function createMockBlock(variableName) { * @param {string} name The expected name of the variable in the block. */ function undoRedoTest_checkBlockVariableName(blockIndex, name) { - var blockVarName = workspace.topBlocks_[blockIndex].getVars()[0]; + var blockVarName = workspace.topBlocks_[blockIndex].getVarModels()[0].name; assertEquals(name, blockVarName); } @@ -146,25 +137,23 @@ function test_undoDeleteVariable_NoBlocks() { function test_undoDeleteVariable_WithBlocks() { undoRedoTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name2'); + workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + createMockBlock('id1'); + createMockBlock('id2'); workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name2'); assertNull(workspace.getVariableById('id1')); - checkVariableValues(workspace, 'name2', '', 'id2'); + checkVariableValues(workspace, 'name2', 'type2', 'id2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name2'); undoRedoTest_checkBlockVariableName(1, 'name1'); - checkVariableValues(workspace, 'name1', '', 'id1'); - checkVariableValues(workspace, 'name2', '', 'id2'); + checkVariableValues(workspace, 'name1', 'type1', 'id1'); + checkVariableValues(workspace, 'name2', 'type2', 'id2'); undoRedoTest_tearDown(); } @@ -192,12 +181,10 @@ function test_redoAndUndoDeleteVariable_NoBlocks() { function test_redoAndUndoDeleteVariable_WithBlocks() { undoRedoTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name2'); + workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + createMockBlock('id1'); + createMockBlock('id2'); workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); @@ -214,7 +201,7 @@ function test_redoAndUndoDeleteVariable_WithBlocks() { // Expect that variable 'id2' is recreated undoRedoTest_checkBlockVariableName(0, 'name2'); assertNull(workspace.getVariableById('id1')); - checkVariableValues(workspace, 'name2', '', 'id2'); + checkVariableValues(workspace, 'name2', 'type2', 'id2'); undoRedoTest_tearDown(); } @@ -245,12 +232,11 @@ function test_redoAndUndoDeleteVariableTwice_NoBlocks() { function test_redoAndUndoDeleteVariableTwice_WithBlocks() { undoRedoTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable('name1', '', 'id1'); - createMockBlock('name1'); - workspace.deleteVariableById('id1'); - workspace.deleteVariableById('id1'); + var id = 'id1'; + workspace.createVariable('name1', 'type1', id); + createMockBlock(id); + workspace.deleteVariableById(id); + workspace.deleteVariableById(id); // Check the undoStack only recorded one delete event. var undoStack = workspace.undoStack_; @@ -261,45 +247,45 @@ function test_redoAndUndoDeleteVariableTwice_WithBlocks() { // undo delete workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); - checkVariableValues(workspace, 'name1', '', 'id1'); + checkVariableValues(workspace, 'name1', 'type1', id); // redo delete workspace.undo(true); assertEquals(0, workspace.topBlocks_.length); - assertNull(workspace.getVariableById('id1')); + assertNull(workspace.getVariableById(id)); // redo delete, nothing should happen workspace.undo(true); assertEquals(0, workspace.topBlocks_.length); - assertNull(workspace.getVariableById('id1')); + assertNull(workspace.getVariableById(id)); undoRedoTest_tearDown(); } -function test_undoRedoRenameVariable_NeitherVariableExists() { - // Expect that a variable with the name, 'name2', and the generated UUID, - // 'id2', to be created when rename is called. Undo removes this variable - // and redo recreates it. - undoRedoTest_setUp(); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, - ['rename_group', 'id2', 'delete_group']); - workspace.renameVariable('name1', 'name2'); +// TODO: Decide if this needs to be possible. +// function test_undoRedoRenameVariable_NeitherVariableExists() { +// // Expect that a variable with the name, 'name2', and the generated UUID, +// // 'id2', to be created when rename is called. Undo removes this variable +// // and redo recreates it. +// undoRedoTest_setUp(); +// setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, +// ['rename_group', 'id2', 'delete_group']); +// workspace.renameVariable('name1', 'name2'); - workspace.undo(); - assertNull(workspace.getVariableById('id2')); +// workspace.undo(); +// assertNull(workspace.getVariableById('id2')); - workspace.undo(true); - checkVariableValues(workspace, 'name2', '', 'id2'); - undoRedoTest_tearDown(); -} +// workspace.undo(true); +// checkVariableValues(workspace, 'name2', '', 'id2'); +// undoRedoTest_tearDown(); +// } function test_undoRedoRenameVariable_OneExists_NoBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); - workspace.renameVariable('name1', 'name2'); + workspace.renameVariableById('id1', 'name2'); workspace.undo(); checkVariableValues(workspace, 'name1', '', 'id1'); - assertNull(workspace.getVariable('name2')); workspace.undo(true); checkVariableValues(workspace, 'name2', '', 'id1'); @@ -309,13 +295,12 @@ function test_undoRedoRenameVariable_OneExists_NoBlocks() { function test_undoRedoRenameVariable_OneExists_WithBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); - createMockBlock('name1'); - workspace.renameVariable('name1', 'name2'); + createMockBlock('id1'); + workspace.renameVariableById('id1', 'name2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); checkVariableValues(workspace, 'name1', '', 'id1'); - assertNull(workspace.getVariable('name2')); workspace.undo(true); checkVariableValues(workspace, 'name2', '', 'id1'); @@ -326,7 +311,7 @@ function test_undoRedoRenameVariable_OneExists_WithBlocks() { function test_undoRedoRenameVariable_BothExist_NoBlocks() { undoRedoTest_setUp(); createTwoVarsEmptyType(); - workspace.renameVariable('name1', 'name2'); + workspace.renameVariableById('id1', 'name2'); workspace.undo(); checkVariableValues(workspace, 'name1', '', 'id1'); @@ -334,16 +319,16 @@ function test_undoRedoRenameVariable_BothExist_NoBlocks() { workspace.undo(true); checkVariableValues(workspace, 'name2', '', 'id2'); - assertNull(workspace.getVariable('name1')); + assertNull(workspace.getVariableById('id1')); undoRedoTest_tearDown(); } function test_undoRedoRenameVariable_BothExist_WithBlocks() { undoRedoTest_setUp(); createTwoVarsEmptyType(); - createMockBlock('name1'); - createMockBlock('name2'); - workspace.renameVariable('name1', 'name2'); + createMockBlock('id1'); + createMockBlock('id2'); + workspace.renameVariableById('id1', 'name2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); @@ -360,7 +345,7 @@ function test_undoRedoRenameVariable_BothExist_WithBlocks() { function test_undoRedoRenameVariable_BothExistCaseChange_NoBlocks() { undoRedoTest_setUp(); createTwoVarsEmptyType(); - workspace.renameVariable('name1', 'Name2'); + workspace.renameVariableById('id1', 'Name2'); workspace.undo(); checkVariableValues(workspace, 'name1', '', 'id1'); @@ -375,9 +360,9 @@ function test_undoRedoRenameVariable_BothExistCaseChange_NoBlocks() { function test_undoRedoRenameVariable_BothExistCaseChange_WithBlocks() { undoRedoTest_setUp(); createTwoVarsEmptyType(); - createMockBlock('name1'); - createMockBlock('name2'); - workspace.renameVariable('name1', 'Name2'); + createMockBlock('id1'); + createMockBlock('id2'); + workspace.renameVariableById('id1', 'Name2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); @@ -387,7 +372,7 @@ function test_undoRedoRenameVariable_BothExistCaseChange_WithBlocks() { workspace.undo(true); checkVariableValues(workspace, 'Name2', '', 'id2'); - assertNull(workspace.getVariable('name1')); + assertNull(workspace.getVariableById('id1')); undoRedoTest_checkBlockVariableName(0, 'Name2'); undoRedoTest_checkBlockVariableName(1, 'Name2'); undoRedoTest_tearDown(); @@ -396,7 +381,7 @@ function test_undoRedoRenameVariable_BothExistCaseChange_WithBlocks() { function test_undoRedoRenameVariable_OnlyCaseChange_NoBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); - workspace.renameVariable('name1', 'Name1'); + workspace.renameVariableById('id1', 'Name1'); workspace.undo(); checkVariableValues(workspace, 'name1', '', 'id1'); @@ -409,8 +394,8 @@ function test_undoRedoRenameVariable_OnlyCaseChange_NoBlocks() { function test_undoRedoRenameVariable_OnlyCaseChange_WithBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); - createMockBlock('name1'); - workspace.renameVariable('name1', 'Name1'); + createMockBlock('id1'); + workspace.renameVariableById('id1', 'Name1'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); diff --git a/tests/jsunit/xml_test.js b/tests/jsunit/xml_test.js index 0c2af7441..f8ebe19fb 100644 --- a/tests/jsunit/xml_test.js +++ b/tests/jsunit/xml_test.js @@ -286,10 +286,10 @@ function test_appendDomToWorkspace() { function test_blockToDom_fieldToDom_trivial() { xmlTest_setUpWithMockBlocks(); // TODO (#1199): make a similar test where the variable is given a non-empty - // type. + // type.f workspace.createVariable('name1', '', 'id1'); var block = new Blockly.Block(workspace, 'field_variable_test_block'); - block.inputList[0].fieldRow[0].setValue('name1'); + block.inputList[0].fieldRow[0].setValue('id1'); var resultFieldDom = Blockly.Xml.blockToDom(block).childNodes[0]; xmlTest_checkVariableFieldDomValues(resultFieldDom, 'VAR', '', 'id1', 'name1'); @@ -301,7 +301,7 @@ function test_blockToDom_fieldToDom_defaultCase() { setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1', '1']); workspace.createVariable('name1'); var block = new Blockly.Block(workspace, 'field_variable_test_block'); - block.inputList[0].fieldRow[0].setValue('name1'); + block.inputList[0].fieldRow[0].setValue('1'); var resultFieldDom = Blockly.Xml.blockToDom(block).childNodes[0]; // Expect type is '' and id is '1' since we don't specify type and id. xmlTest_checkVariableFieldDomValues(resultFieldDom, 'VAR', '', '1', 'name1'); From 6bb048d1bbea10d1dcf245dc04718d84ef4e9426 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Mon, 4 Dec 2017 14:29:16 -0800 Subject: [PATCH 09/23] All tests pass --- core/variables.js | 8 -------- core/xml.js | 2 -- tests/jsunit/index.html | 4 ++-- tests/jsunit/xml_test.js | 18 ++++++++---------- 4 files changed, 10 insertions(+), 22 deletions(-) diff --git a/core/variables.js b/core/variables.js index 26dc0b452..d43ae7bb2 100644 --- a/core/variables.js +++ b/core/variables.js @@ -338,12 +338,4 @@ Blockly.Variables.generateVariableFieldXml_ = function(variableModel) { '" variabletype="' + typeString + '">' + variableModel.name + ''; return text; - // var element = goog.dom.createDom('field'); - // element.setAttribute('name', 'VAR'); - // element.setAttribute('variabletype', variableModel.type); - // element.setAttribute('id', variableModel.getId()); - // element.textContent = variableModel.name; - - // var xmlString = Blockly.Xml.domToText(element); - // return xmlString; }; diff --git a/core/xml.js b/core/xml.js index f71c3b52a..9946d3832 100644 --- a/core/xml.js +++ b/core/xml.js @@ -87,7 +87,6 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) { }; Blockly.Xml.fieldToDomVariable_ = function(field, workspace) { - // Ugh that's not true at all. var id = field.getValue(); var variable = workspace.getVariableById(id); if (!variable) { @@ -420,7 +419,6 @@ Blockly.Xml.domToWorkspace = function(xml, workspace) { } Blockly.Field.stopCache(); } - workspace.updateVariableStore(false); // Re-enable workspace resizing. if (workspace.setResizesEnabled) { workspace.setResizesEnabled(true); diff --git a/tests/jsunit/index.html b/tests/jsunit/index.html index 96424df44..619e4143b 100644 --- a/tests/jsunit/index.html +++ b/tests/jsunit/index.html @@ -8,7 +8,7 @@ - + diff --git a/tests/jsunit/xml_test.js b/tests/jsunit/xml_test.js index f8ebe19fb..a2f929e51 100644 --- a/tests/jsunit/xml_test.js +++ b/tests/jsunit/xml_test.js @@ -345,14 +345,14 @@ function test_variablesToDom_oneVariable() { function test_variablesToDom_twoVariables_oneBlock() { xmlTest_setUpWithMockBlocks(); - workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name1', '', 'id1'); workspace.createVariable('name2', 'type2', 'id2'); var block = new Blockly.Block(workspace, 'field_variable_test_block'); - block.inputList[0].fieldRow[0].setValue('name1'); + block.inputList[0].fieldRow[0].setValue('id1'); var resultDom = Blockly.Xml.variablesToDom(workspace.getAllVariables()); assertEquals(2, resultDom.children.length); - xmlTest_checkVariableDomValues(resultDom.children[0], 'type1', 'id1', + xmlTest_checkVariableDomValues(resultDom.children[0], '', 'id1', 'name1'); xmlTest_checkVariableDomValues(resultDom.children[1], 'type2', 'id2', 'name2'); @@ -380,14 +380,12 @@ function test_variableFieldXml_caseSensitive() { } }; - var generatedXml = Blockly.Variables.generateVariableFieldXml_(mockVariableModel); - // The field contains this XML tag as a result of how we're generating this - // XML. This is not desirable, but the goal of this test is to make sure - // we're preserving case-sensitivity. - var xmlns = 'xmlns="http://www.w3.org/1999/xhtml"'; + var generatedXml = + Blockly.Variables.generateVariableFieldXml_(mockVariableModel); var goldenXml = - '' + name + ''; + '>' + name + ''; assertEquals(goldenXml, generatedXml); } From c802125d374c3b21d17fcce91c90e7f924c05a8c Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Mon, 4 Dec 2017 15:46:44 -0800 Subject: [PATCH 10/23] Make unique variable names in the flyout --- core/field_variable.js | 9 ++++++++- core/flyout_base.js | 4 ++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/core/field_variable.js b/core/field_variable.js index 4dbe3e81a..b0cbf06a6 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -63,6 +63,7 @@ Blockly.FieldVariable.getOrCreateVariable = function(workspace, text, type, id) { var potentialVariableMap = workspace.isFlyout ? workspace.targetWorkspace.potentialVariableMap_ : null; + if (id) { var variable = workspace.getVariableById(id); if (!variable && potentialVariableMap) { @@ -76,6 +77,12 @@ Blockly.FieldVariable.getOrCreateVariable = function(workspace, text, type, } // Didn't find the variable. if (!variable) { + if (!text) { + var ws = workspace.isFlyout ? workspace.targetWorkspace : workspace; + // Variables without names get uniquely named for this workspace. + text = Blockly.Variables.generateUniqueName(ws); + } + if (potentialVariableMap) { variable = potentialVariableMap.createVariable(text, type, id); } else { @@ -126,7 +133,7 @@ Blockly.FieldVariable.prototype.initModel = function() { } this.workspace_ = this.sourceBlock_.workspace; var variable = Blockly.FieldVariable.getOrCreateVariable( - this.workspace_, this.defaultVariableName, this.defaultType_, null); + this.workspace_, name, this.defaultType_, null); this.setValue(variable.getId()); }; diff --git a/core/flyout_base.js b/core/flyout_base.js index c56ea6253..2e3a33b2c 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -415,6 +415,7 @@ Blockly.Flyout.prototype.hide = function() { this.workspace_.removeChangeListener(this.reflowWrapper_); this.reflowWrapper_ = null; } + // Do NOT delete the blocks here. Wait until Flyout.show. // https://neil.fraser.name/news/2014/08/09/ }; @@ -542,6 +543,9 @@ Blockly.Flyout.prototype.clearOldBlocks_ = function() { button.dispose(); } this.buttons_.length = 0; + + // Clear potential variables from the previous showing. + this.targetWorkspace_.potentialVariableMap_.clear(); }; /** From 39e689b458224a8a35fcf4e28911ece7feddf7b7 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Mon, 4 Dec 2017 15:52:53 -0800 Subject: [PATCH 11/23] Fix default variable name --- core/field_variable.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/field_variable.js b/core/field_variable.js index b0cbf06a6..f0918cbc6 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -133,7 +133,7 @@ Blockly.FieldVariable.prototype.initModel = function() { } this.workspace_ = this.sourceBlock_.workspace; var variable = Blockly.FieldVariable.getOrCreateVariable( - this.workspace_, name, this.defaultType_, null); + this.workspace_, this.defaultVariableName, this.defaultType_, null); this.setValue(variable.getId()); }; From e4844cd1209dec364d29d38ed19559f8c33cdba4 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Tue, 5 Dec 2017 13:08:01 -0800 Subject: [PATCH 12/23] Move getOrCreateVariable to variables.js --- blocks/procedures.js | 4 ++ core/field_variable.js | 109 +++++++++++---------------------------- core/flyout_base.js | 1 - core/variables.js | 113 +++++++++++++++++++++++++++++++++++++++-- core/workspace.js | 23 +++++++++ core/xml.js | 4 +- 6 files changed, 168 insertions(+), 86 deletions(-) diff --git a/blocks/procedures.js b/blocks/procedures.js index 0b5866188..5fc29d302 100644 --- a/blocks/procedures.js +++ b/blocks/procedures.js @@ -266,6 +266,10 @@ Blockly.Blocks['procedures_defnoreturn'] = { */ getVarModels: function() { var vars = []; + // Not fully initialized. + if (!this.arguments_) { + return vars; + } for (var i = 0, argName; argName = this.arguments_[i]; i++) { // TODO (#1199): When we switch to tracking variables by ID, // update this. diff --git a/core/field_variable.js b/core/field_variable.js index f0918cbc6..8829ddb17 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -47,11 +47,11 @@ goog.require('goog.string'); * @constructor */ Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes) { - // Don't call the FieldDropdown constructor. It'll try too hard. + // The FieldDropdown constructor would call setValue, which might create a + // spurious variable. Just do the relevant parts of the constructor. this.menuGenerator_ = Blockly.FieldVariable.dropdownCreate; this.size_ = new goog.math.Size(0, Blockly.BlockSvg.MIN_BLOCK_Y); this.setValidator(opt_validator); - //this.setValue(varname || ''); // TODO: Add opt_default_type to match default value. If not set, ''. this.defaultVariableName = (varname || ''); this.defaultType_ = ''; @@ -59,42 +59,11 @@ Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes) { }; goog.inherits(Blockly.FieldVariable, Blockly.FieldDropdown); -Blockly.FieldVariable.getOrCreateVariable = function(workspace, text, type, - id) { - var potentialVariableMap = workspace.isFlyout ? - workspace.targetWorkspace.potentialVariableMap_ : null; - - if (id) { - var variable = workspace.getVariableById(id); - if (!variable && potentialVariableMap) { - variable = potentialVariableMap.getVariableById(id); - } - } else { - var variable = workspace.getVariable(text, type); - if (!variable && potentialVariableMap) { - variable = potentialVariableMap.getVariable(text, type); - } - } - // Didn't find the variable. - if (!variable) { - if (!text) { - var ws = workspace.isFlyout ? workspace.targetWorkspace : workspace; - // Variables without names get uniquely named for this workspace. - text = Blockly.Variables.generateUniqueName(ws); - } - - if (potentialVariableMap) { - variable = potentialVariableMap.createVariable(text, type, id); - } else { - variable = workspace.createVariable(text, type, id); - } - } - - return variable; -}; /** - * Install this dropdown on a block. + * Initialize everything needed to render this field. This includes making sure + * that the field's value is valid. + * @public */ Blockly.FieldVariable.prototype.init = function() { if (this.fieldGroup_) { @@ -107,36 +76,26 @@ Blockly.FieldVariable.prototype.init = function() { this.initModel(); }; +/** + * Initialize the model for this field if it has not already been initialized. + * If the value has not been set to a variable by the first render, we make up a + * variable rather than let the value be invalid. + * @package + */ Blockly.FieldVariable.prototype.initModel = function() { - // this.workspace_ = this.sourceBlock_.isInFlyout ? - // this.sourceBlock_.workspace.targetWorkspace : - // this.sourceBlock_.workspace; - // // TODO: Describe how the potential variable map is different from the variable - // // map; use getters. - // this.variableMap_ = this.sourceBlock_.isInFlyout ? - // this.workspace_.potentialVariableMap_ : this.workspace_.variableMap_; - // var name = this.defaultValue; - // if (!name) { - // // Variables without names get uniquely named for this workspace. - // name = Blockly.Variables.generateUniqueName(this.workspace_); - // } - // // If the selected variable doesn't exist yet, create it. - // // For instance, some blocks in the toolbox have variable dropdowns filled - // // in by default. - - // var variable = this.variableMap_.getVariable(name, this.defaultType_); - // if (!variable) { - // variable = this.variableMap_.createVariable(name, this.defaultType_); - // } if (this.variable_) { return; // Initialization already happened. } this.workspace_ = this.sourceBlock_.workspace; - var variable = Blockly.FieldVariable.getOrCreateVariable( - this.workspace_, this.defaultVariableName, this.defaultType_, null); + var variable = Blockly.Variables.getOrCreateVariable( + this.workspace_, null, this.defaultVariableName, this.defaultType_); this.setValue(variable.getId()); }; +/** + * Dispose of this field. + * @public + */ Blockly.FieldVariable.dispose = function() { Blockly.FieldVariable.superClass_.dispose.call(this); this.workspace_ = null; @@ -154,15 +113,17 @@ Blockly.FieldVariable.prototype.setSourceBlock = function(block) { }; /** - * Get the variable's name (use a variableDB to convert into a real name). - * Unline a regular dropdown, variables are literal and have no neutral value. - * @return {string} Current text. + * Get the variable's ID. + * @return {string} Current variable's ID. */ Blockly.FieldVariable.prototype.getValue = function() { - //return this.getText(); return this.variable_ ? this.variable_.getId() : ''; }; +/** + * Get the text from this field. + * @return {string} Current text. + */ Blockly.FieldVariable.prototype.getText = function() { //return this.getText(); return this.variable_ ? this.variable_.name : ''; @@ -177,16 +138,7 @@ Blockly.FieldVariable.prototype.setValue = function(id) { // TODO: Handle undo a change to go back to the default value. // TODO: Handle null ID, which means "use default". var workspace = this.sourceBlock_.workspace; - var variable = workspace.getVariableById(id); - if (!variable) { - if (workspace.isFlyout && workspace.targetWorkspace) { - var potentialVariableMap = - workspace.targetWorkspace.potentialVariableMap_; - if (potentialVariableMap) { - variable = potentialVariableMap.getVariableById(id); - } - } - } + var variable = Blockly.Variables.getVariable(workspace, id); if (!variable) { throw new Error('Variable id doesn\'t point to a real variable! ID was ' + @@ -207,6 +159,12 @@ Blockly.FieldVariable.prototype.setValue = function(id) { this.setText(variable.name); }; +/** + * Check whether the given variable type is allowed on this field. + * @param {string} type The type to check. + * @return {boolean} True if the type is in the list of allowed types. + * @private + */ Blockly.FieldVariable.prototype.typeIsAllowed_ = function(type) { var typeList = this.getVariableTypes_(); if (!typeList) { @@ -300,16 +258,11 @@ Blockly.FieldVariable.dropdownCreate = function() { */ Blockly.FieldVariable.prototype.onItemSelected = function(menu, menuItem) { var id = menuItem.getValue(); - // TODO(marisaleung): change setValue() to take in an ID as the parameter. - // Then remove itemText. if (this.sourceBlock_ && this.sourceBlock_.workspace) { var workspace = this.sourceBlock_.workspace; - var variable = workspace.getVariableById(id); - // If the item selected is a variable, set itemText to the variable name. if (id == Blockly.RENAME_VARIABLE_ID) { // Rename variable. - variable = this.variable_; - Blockly.Variables.renameVariable(workspace, variable); + Blockly.Variables.renameVariable(workspace, this.variable_); return; } else if (id == Blockly.DELETE_VARIABLE_ID) { // Delete variable. diff --git a/core/flyout_base.js b/core/flyout_base.js index 2e3a33b2c..8b1421e39 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -415,7 +415,6 @@ Blockly.Flyout.prototype.hide = function() { this.workspace_.removeChangeListener(this.reflowWrapper_); this.reflowWrapper_ = null; } - // Do NOT delete the blocks here. Wait until Flyout.show. // https://neil.fraser.name/news/2014/08/09/ }; diff --git a/core/variables.js b/core/variables.js index d43ae7bb2..4d0167efa 100644 --- a/core/variables.js +++ b/core/variables.js @@ -65,13 +65,13 @@ Blockly.Variables.allUsedVariables = function(root) { // 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].getVars(); + var blockVariables = blocks[x].getVarModels(); if (blockVariables) { for (var y = 0; y < blockVariables.length; y++) { - var varName = blockVariables[y]; - // Variable name may be null if the block is only half-built. - if (varName) { - variableHash[varName.toLowerCase()] = varName; + 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; } } } @@ -339,3 +339,106 @@ Blockly.Variables.generateVariableFieldXml_ = function(variableModel) { '">' + variableModel.name + ''; return text; }; + +/** + * Helper function to look up or create a variable on the given workspace. + * If no variable exists, creates and returns it. + * @param {!Blockly.Workspace} workspace The workspace to search for the + * variable. It may be a flyout workspace or main workspace. + * @param {string} id The ID to use to look up or create the variable, or null. + * @param {string} name The string to use to look up or create the variable, + * @param {string} type The type to use to look up or create the variable. + * @return {!Blockly.VariableModel} The variable corresponding to the given ID + * or name + type combination. + * @package + */ +Blockly.Variables.getOrCreateVariable = function(workspace, id, name, type) { + var variable = Blockly.Variables.getVariable(workspace, id, name, type); + if (!variable) { + variable = Blockly.Variables.createVariable_(workspace, id, name, type); + } + return variable; +}; + +/** + * Look up a variable on the given workspace. + * Always looks in the main workspace before looking in the flyout workspace. + * Always prefers lookup by ID to lookup by name + type. + * @param {!Blockly.Workspace} workspace The workspace to search for the + * variable. It may be a flyout workspace or main workspace. + * @param {string} id The ID to use to look up the variable, or null. + * @param {string=} opt_name The string to use to look up the variable. Only + * used if lookup by ID fails. + * @param {string=} opt_type The type to use to look up the variable. Only used + * if lookup by ID fails. + * @return {?Blockly.VariableModel} The variable corresponding to the given ID + * or name + type combination, or null if not found. + * @private + */ +Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) { + var potentialVariableMap = + Blockly.Variables.getPotentialVariableMap_(workspace); + // Try to just get the variable, by ID if possible. + if (id) { + // Look in the real variable map before checking the potential variable map. + var variable = workspace.getVariableById(id); + if (!variable && potentialVariableMap) { + variable = potentialVariableMap.getVariableById(id); + } + } else if (opt_name && (opt_type != undefined)){ + // Otherwise look up by name and type. + var variable = workspace.getVariable(opt_name, opt_type); + if (!variable && potentialVariableMap) { + variable = potentialVariableMap.getVariable(opt_name, opt_type); + } + } + return variable; +}; + +/** + * Helper function to create a variable on the given workspace. + * @param {!Blockly.Workspace} workspace The workspace in which to create the + * variable. It may be a flyout workspace or main workspace. + * @param {string} id The ID to use to create the variable, or null. + * @param {string} name The string to use to create the variable. + * @param {string} type The type to use to create the variable. + * @return {!Blockly.VariableModel} The variable corresponding to the given ID + * or name + type combination. + * @private + */ +Blockly.Variables.createVariable_ = function(workspace, id, name, type) { + var potentialVariableMap = + Blockly.Variables.getPotentialVariableMap_(workspace); + // Variables without names get uniquely named for this workspace. + if (!name) { + var ws = workspace.isFlyout ? workspace.targetWorkspace : workspace; + name = Blockly.Variables.generateUniqueName(ws); + } + + // Create a potential variable if in the flyout. + if (potentialVariableMap) { + var variable = potentialVariableMap.createVariable(name, type, id); + } else { // In the main workspace, create a real variable. + var variable = workspace.createVariable(name, type, id); + } + return variable; +}; + +/** + * Blocks in the flyout can refer to variables that don't exist in the + * workspace. For instance, the "get item in list" block refers to an "item" + * variable regardless of whether the variable has been created yet. + * A FieldVariable must always refer to a Blockly.VariableModel. We reconcile + * these by tracking "potential" variables in the flyout. These variables + * become real when references to them are dragged into the main workspace. + * @param {!Blockly.Workspace} workspace The workspace to get the potential + * variable map from. + * @return {?Blockly.VariableMap} The potential variable map for the given + * workspace, or null if it was not a flyout workspace. + * @private + */ +Blockly.Variables.getPotentialVariableMap_ = function(workspace) { + return workspace.isFlyout && workspace.targetWorkspace ? + workspace.targetWorkspace.getPotentialVariableMap() : null; + +}; diff --git a/core/workspace.js b/core/workspace.js index 6fed60405..03477519f 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -85,6 +85,16 @@ Blockly.Workspace = function(opt_options) { */ this.variableMap_ = new Blockly.VariableMap(this); + /** + * Blocks in the flyout can refer to variables that don't exist in the + * workspace. For instance, the "get item in list" block refers to an "item" + * variable regardless of whether the variable has been created yet. + * A FieldVariable must always refer to a Blockly.VariableModel. We reconcile + * these by tracking "potential" variables in the flyout. These variables + * become real when references to them are dragged into the main workspace. + * @type {!Blockly.VariableMap} + * @private + */ this.potentialVariableMap_ = new Blockly.VariableMap(this); }; @@ -199,6 +209,7 @@ Blockly.Workspace.prototype.clear = function() { Blockly.Events.setGroup(false); } this.variableMap_.clear(); + this.potentialVariableMap_.clear(); }; /** @@ -628,6 +639,18 @@ Blockly.Workspace.prototype.getAllVariables = function() { return this.variableMap_.getAllVariables(); }; +/** + * Return the variable map that contains "potential" variables. These exist in + * the flyout but not in the workspace. + * TODO: Decide if this can be stored on the flyout workspace instead of the + * main workspace. + * @return {?Blockly.VariableMap} The potential variable map. + * @package + */ +Blockly.Workspace.prototype.getPotentialVariableMap = function() { + return this.potentialVariableMap_; +}; + /** * Database of all workspaces. * @private diff --git a/core/xml.js b/core/xml.js index 9946d3832..f9a0106c1 100644 --- a/core/xml.js +++ b/core/xml.js @@ -730,8 +730,8 @@ Blockly.Xml.domToFieldVariable_ = function(workspace, xml, text, field) { // TODO: Consider using a different name (var_id?) because this is the // node's ID. var id = xml.id; - var variable = Blockly.FieldVariable.getOrCreateVariable(workspace, text, - type, id); + var variable = + Blockly.Variables.getOrCreateVariable(workspace, id, text, type); // This should never happen :) if (type != null && type !== variable.type) { From 5169b2da18e4c34647f1abd0bfc63d5c70cc32f1 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Tue, 5 Dec 2017 13:44:08 -0800 Subject: [PATCH 13/23] 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 8829ddb17..df6e10e72 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); From 90e79c6ba9754e4869e8ec829f56fd5bf98a6bfa Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Tue, 5 Dec 2017 13:52:01 -0800 Subject: [PATCH 14/23] Remove ws.updateVariableStore and tests, and ws.getVariableUses --- core/workspace.js | 69 ---------------------------------- tests/jsunit/workspace_test.js | 69 ---------------------------------- 2 files changed, 138 deletions(-) diff --git a/core/workspace.js b/core/workspace.js index fda910f43..bcda8d927 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -212,41 +212,6 @@ Blockly.Workspace.prototype.clear = function() { this.potentialVariableMap_.clear(); }; -/** - * Walk the workspace and update the map of variables to only contain ones in - * use on the workspace. Use when loading new workspaces from disk. - * @param {boolean} clear True if the old variable map should be cleared. - */ -Blockly.Workspace.prototype.updateVariableStore = function(clear) { - // TODO: Sort - if (this.isFlyout) { - return; - } - var variableNames = Blockly.Variables.allUsedVariables(this); - var varList = []; - for (var i = 0, name; name = variableNames[i]; i++) { - // Get variable model with the used variable name. - var tempVar = this.getVariable(name); - if (tempVar) { - varList.push({'name': tempVar.name, 'type': tempVar.type, - 'id': tempVar.getId()}); - } else { - varList.push({'name': name, 'type': null, 'id': null}); - // TODO(marisaleung): Use variable.type and variable.getId() once variable - // instances are storing more than just name. - } - } - if (clear) { - this.variableMap_.clear(); - } - // Update the list in place so that the flyout's references stay correct. - for (var i = 0, varDict; varDict = varList[i]; i++) { - if (!this.getVariable(varDict.name)) { - this.createVariable(varDict.name, varDict.type, varDict.id); - } - } -}; - /** * Rename a variable by updating its name in the variable map. Identify the * variable to rename with the given ID. @@ -307,40 +272,6 @@ Blockly.Workspace.prototype.createVariable = function(name, opt_type, opt_id) { /** * Find all the uses of a named variable. - * TODO (#1199): Possibly delete this function. - * @param {string} name Name of variable. - * @param {string=} opt_type The type of the variable. If not provided it - * defaults to the empty string, which is a specific type. - * @return {!Array.} Array of block usages. - */ -Blockly.Workspace.prototype.getVariableUses = function(name, opt_type) { - var type = opt_type || ''; - var uses = []; - var blocks = this.getAllBlocks(); - // Iterate through every block and check the name. - for (var i = 0; i < blocks.length; i++) { - var blockVariables = blocks[i].getVarModels(); - if (blockVariables) { - for (var j = 0; j < blockVariables.length; j++) { - var varModel = blockVariables[j]; - var varName = varModel.name; - // Skip variables of the wrong type. - if (varModel.type != type) { - continue; - } - // Variable name may be null if the block is only half-built. - if (varName && name && Blockly.Names.equals(varName, name)) { - uses.push(blocks[i]); - } - } - } - } - return uses; -}; - -/** - * Find all the uses of a named variable. - * TODO (#1199): Possibly delete this function. * @param {string} id ID of the variable to find. * @return {!Array.} Array of block usages. */ diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index c97700f3e..999aa81f8 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -168,75 +168,6 @@ function test_deleteVariable_InternalTrivial() { // TODO(marisaleung): Test the alert for deleting a variable that is a procedure. -function test_updateVariableStore_TrivialNoClear() { - workspaceTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); - setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', - [workspace], [['name1', 'name2']]); - - try { - workspace.updateVariableStore(); - checkVariableValues(workspace, 'name1', 'type1', 'id1'); - checkVariableValues(workspace, 'name2', 'type2', 'id2'); - } finally { - workspaceTest_tearDown(); - } -} - -function test_updateVariableStore_NameNotInvariableMap_NoClear() { - workspaceTest_setUp(); - setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', - [workspace], [['name1']]); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); - - try { - workspace.updateVariableStore(); - checkVariableValues(workspace, 'name1', '', '1'); - } finally { - workspaceTest_tearDown(); - } -} - -function test_updateVariableStore_ClearAndAllInUse() { - workspaceTest_setUp(); - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - // TODO (#1199): get rid of updateVariableStore if possible. - setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', - [workspace], [['name1', 'name2']]); - - try { - workspace.updateVariableStore(true); - checkVariableValues(workspace, 'name1', '', 'id1'); - checkVariableValues(workspace, 'name2', '', 'id2'); - } finally { - workspaceTest_tearDown(); - } -} - -function test_updateVariableStore_ClearAndOneInUse() { - workspaceTest_setUp(); - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - // TODO (#1199): get rid of updateVariableStore if possible. - setUpMockMethod(mockControl_, Blockly.Variables, 'allUsedVariables', - [workspace], [['name1']]); - - try { - workspace.updateVariableStore(true); - checkVariableValues(workspace, 'name1', '', 'id1'); - var variable = workspace.getVariable('name2', ''); - assertNull(variable); - } finally { - workspaceTest_tearDown(); - } -} - function test_addTopBlock_TrivialFlyoutIsTrue() { workspaceTest_setUp(); workspace.isFlyout = true; From 1506a36b589e3e98cb775a0bb18a27bfa7f42e57 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 6 Dec 2017 14:41:56 -0800 Subject: [PATCH 15/23] Fix events for variable renaming --- core/block.js | 18 ++++++++ core/variable_map.js | 97 ++++++++++++++++++++++++++++++-------------- core/workspace.js | 37 ++++------------- core/xml.js | 23 +++++++---- 4 files changed, 108 insertions(+), 67 deletions(-) diff --git a/core/block.js b/core/block.js index 58bc311a4..d55b508c6 100644 --- a/core/block.js +++ b/core/block.js @@ -709,6 +709,7 @@ Blockly.Block.prototype.getVarModels = function() { /** * Notification that a variable is renaming. + * TODO (fenichel): consider deleting this. * If the name matches one of this block's variables, rename it. * @param {string} oldName Previous name of variable. * @param {string} newName Renamed variable. @@ -724,6 +725,23 @@ Blockly.Block.prototype.renameVar = function(oldName, newName) { } }; +/** + * Notification that a variable is renaming but keeping the same ID. If the + * variable is in use on this block, rerender to show the new name. + * @param {!Blockly.VariableModel} variable The variable being renamed. + * @public + */ +Blockly.Block.prototype.updateVarName = function(variable) { + for (var i = 0, input; input = this.inputList[i]; i++) { + for (var j = 0, field; field = input.fieldRow[j]; j++) { + if (field instanceof Blockly.FieldVariable && + variable.getId() == field.getValue()) { + field.setText(variable.name); + } + } + } +}; + /** * Notification that a variable is renaming. * If the name matches one of this block's variables, rename it. diff --git a/core/variable_map.js b/core/variable_map.js index 3a51fc450..c9a02aa43 100644 --- a/core/variable_map.js +++ b/core/variable_map.js @@ -60,45 +60,80 @@ 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 {!Blockly.VariableModel} variable Variable to rename. * @param {string} newName New variable name. * @package */ Blockly.VariableMap.prototype.renameVariable = function(variable, newName) { var type = variable.type; - var newVariable = this.getVariable(newName, type); - var variableIndex = -1; - var newVariableIndex = -1; + var conflictVar = this.getVariable(newName, type); + // The IDs may match if the rename is a simple case change (name1 -> Name1). + if (!conflictVar || conflictVar.getId() == variable.getId()) { + this.renameVariableNoConflict_(variable, newName); + } else { + this.renameVariableWithConflict_(variable, newName, conflictVar); + } +}; +/** + * Update the name of the given variable and refresh all references to it. + * The new name must not conflict with any existing variable names. + * @param {!Blockly.VariableModel} variable Variable to rename. + * @param {string} newName New variable name. + * @private + */ +Blockly.VariableMap.prototype.renameVariableNoConflict_ = function(variable, newName) { + Blockly.Events.fire(new Blockly.Events.VarRename(variable, newName)); + variable.name = newName; + var blocks = this.workspace.getAllBlocks(); + for (var i = 0; i < blocks.length; i++) { + blocks[i].updateVarName(variable); + } +}; + +/** + * Update the name of the given variable to the same name as an existing + * variable. The two variables are coalesced into a single variable with the ID + * of the existing variable that was already using newName. + * Refresh all references to the variable. + * @param {!Blockly.VariableModel} variable Variable to rename. + * @param {string} newName New variable name. + * @param {!Blockly.VariableModel} conflictVar The variable that was already + * using newName. + * @private + */ +Blockly.VariableMap.prototype.renameVariableWithConflict_ = function(variable, + newName, conflictVar) { + var type = variable.type; + + Blockly.Events.setGroup(true); + Blockly.Events.fire(new Blockly.Events.VarRename(conflictVar, newName)); + var oldCase = conflictVar.name; + conflictVar.name = newName; + + // These blocks refer to the same variable but the case may have changed. + // No change events should be fired here. + var blocks = this.workspace.getAllBlocks(); + if (newName != oldCase) { + for (var i = 0; i < blocks.length; i++) { + blocks[i].updateVarName(conflictVar); + } + } + + // These blocks now refer to a different variable. + // These will fire change events. + for (var i = 0; i < blocks.length; i++) { + blocks[i].renameVarById(variable.getId(), conflictVar.getId()); + } + + // Finally delete the original variable, which is now unreferenced. + Blockly.Events.fire(new Blockly.Events.VarDelete(variable)); + // And remove it from the list. var variableList = this.getVariablesOfType(type); - if (variable) { - variableIndex = variableList.indexOf(variable); - } - if (newVariable) { // see if I can get rid of newVariable dependency - newVariableIndex = variableList.indexOf(newVariable); - } + var variableIndex = variableList.indexOf(variable); + this.variableMap_[type].splice(variableIndex, 1); - if (variableIndex == -1 && newVariableIndex == -1) { - this.createVariable(newName, ''); - console.log('Tried to rename an non-existent variable.'); - } else if (variableIndex == newVariableIndex || - variableIndex != -1 && newVariableIndex == -1) { - // Only changing case, or renaming to a completely novel name. - var variableToRename = this.variableMap_[type][variableIndex]; - Blockly.Events.fire(new Blockly.Events.VarRename(variableToRename, - newName)); - variableToRename.name = newName; - } else if (variableIndex != -1 && newVariableIndex != -1) { - // Renaming one existing variable to another existing variable. - // The case might have changed, so we update the destination ID. - var variableToRename = this.variableMap_[type][newVariableIndex]; - Blockly.Events.fire(new Blockly.Events.VarRename(variableToRename, - newName)); - var variableToDelete = this.variableMap_[type][variableIndex]; - Blockly.Events.fire(new Blockly.Events.VarDelete(variableToDelete)); - variableToRename.name = newName; - this.variableMap_[type].splice(variableIndex, 1); - } + Blockly.Events.setGroup(false); }; /** diff --git a/core/workspace.js b/core/workspace.js index bcda8d927..698f523eb 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -224,35 +224,7 @@ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { throw new Error('Tried to rename a variable that didn\'t exist. ID: ' + id); } - var type = variable.type; - var newVariable = this.getVariable(newName, type); - - // If a variable previously existed with the new name, we will coalesce the - // variables and use the ID of the existing variable with the new name. - // Otherwise, we change the current variable's name but not its ID. - var oldId = variable.getId(); - var newId = newVariable ? newVariable.getId() : oldId; - - var oldCase; - - // Find if newVariable case is different. - if (newVariable && newVariable.name != newName) { - oldCase = newVariable.name; - } - - Blockly.Events.setGroup(true); - var blocks = this.getAllBlocks(); - - // Iterate through every block and update name. - for (var i = 0; i < blocks.length; i++) { - blocks[i].renameVarById(oldId, newId); - if (oldCase) { - blocks[i].renameVarById(newId, newId); - } - } - this.variableMap_.renameVariable(variable, newName); - Blockly.Events.setGroup(false); }; /** @@ -569,6 +541,15 @@ Blockly.Workspace.prototype.getPotentialVariableMap = function() { return this.potentialVariableMap_; }; +/** + * Return the map of all variables on the workspace. + * @return {?Blockly.VariableMap} The variable map. + * @package + */ +Blockly.Workspace.prototype.getVariableMap = function() { + return this.variableMap_; +}; + /** * Database of all workspaces. * @private diff --git a/core/xml.js b/core/xml.js index f9a0106c1..e1eaaa3a3 100644 --- a/core/xml.js +++ b/core/xml.js @@ -106,7 +106,7 @@ Blockly.Xml.fieldToDomVariable_ = function(field, workspace) { return container; } else { // something went wrong? - console.log('no variable in fieldtodom'); + console.warn('no variable in fieldtodom'); return null; } }; @@ -500,6 +500,7 @@ Blockly.Xml.domToBlock = function(xmlBlock, workspace) { try { var topBlock = Blockly.Xml.domToBlockHeadless_(xmlBlock, workspace); if (workspace.rendered) { + // TODO (fenichel): Otherwise call initModel? // Hide connections to speed up assembly. topBlock.setConnectionsHidden(true); // Generate list of all blocks. @@ -719,19 +720,25 @@ Blockly.Xml.domToBlockHeadless_ = function(xmlBlock, workspace) { return block; }; +/** + * Decode an XML variable field tag and set the value of that field. + * @param {!Blockly.Workspace} workspace The workspace that is currently being + * deserialized. + * @param {!Element} xml The field tag to decode. + * @param {string} text The text content of the XML tag. + * @param {!Blockly.FieldVariable} field The field on which the value will be + * set. + * @private + */ Blockly.Xml.domToFieldVariable_ = function(workspace, xml, text, field) { - // TODO (#1199): When we change setValue and getValue to - // interact with IDs instead of names, update this so that we get - // the variable based on ID instead of textContent. var type = xml.getAttribute('variabletype') || ''; + // TODO (fenichel): Does this need to be explicit or not? if (type == '\'\'') { type = ''; } - // TODO: Consider using a different name (var_id?) because this is the - // node's ID. - var id = xml.id; + var variable = - Blockly.Variables.getOrCreateVariable(workspace, id, text, type); + Blockly.Variables.getOrCreateVariable(workspace, xml.id, text, type); // This should never happen :) if (type != null && type !== variable.type) { From 38d134aa4fe4b38e933cef77df9b1b8bac18db3f Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 6 Dec 2017 15:49:20 -0800 Subject: [PATCH 16/23] Tests --- blocks/procedures.js | 2 +- core/block.js | 7 +--- tests/jsunit/test_utilities.js | 14 +++++++ tests/jsunit/workspace_test.js | 52 ++++++++---------------- tests/jsunit/workspace_undo_redo_test.js | 51 ++++++++--------------- 5 files changed, 49 insertions(+), 77 deletions(-) diff --git a/blocks/procedures.js b/blocks/procedures.js index d8fc6f30a..a6800f934 100644 --- a/blocks/procedures.js +++ b/blocks/procedures.js @@ -271,7 +271,7 @@ Blockly.Blocks['procedures_defnoreturn'] = { return vars; } for (var i = 0, argName; argName = this.arguments_[i]; i++) { - // TODO (#1199): When we switch to tracking variables by ID, + // TODO (#1494): When we switch to tracking procedure arguments by ID, // update this. var model = this.workspace.getVariable(argName, ''); if (model) { diff --git a/core/block.js b/core/block.js index d55b508c6..5080c3b37 100644 --- a/core/block.js +++ b/core/block.js @@ -695,12 +695,7 @@ Blockly.Block.prototype.getVarModels = function() { for (var i = 0, input; input = this.inputList[i]; i++) { for (var j = 0, field; field = input.fieldRow[j]; j++) { if (field instanceof Blockly.FieldVariable) { - // TODO (#1199): When we switch to tracking variables by ID, - // update this. - var model = this.workspace.getVariableById(field.getValue()); - if (model) { - vars.push(model); - } + vars.push(this.workspace.getVariableById(field.getValue())); } } } diff --git a/tests/jsunit/test_utilities.js b/tests/jsunit/test_utilities.js index d7f6085bf..aa5840d9e 100644 --- a/tests/jsunit/test_utilities.js +++ b/tests/jsunit/test_utilities.js @@ -108,3 +108,17 @@ function createMockBlock(variable_id) { Blockly.Events.enable(); return block; } + +function createTwoVariablesAndBlocks(workspace) { + // Create two variables of different types. + workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + // Create blocks to refer to both of them. + createMockBlock('id1'); + createMockBlock('id2'); +} + +function createVariableAndBlock(workspace) { + workspace.createVariable('name1', 'type1', 'id1'); + createMockBlock('id1'); +} diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index 999aa81f8..e991106a3 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -247,15 +247,12 @@ function test_renameVariable_ReferenceExists() { // Test renaming a variable when a reference to it exists. // Expect 'renameVariable' to change oldName variable name to newName. workspaceTest_setUp(); - var id = 'id1'; - var type = 'type1'; - var oldName = 'name1'; var newName = 'name2'; - workspace.createVariable(oldName, type, id); - createMockBlock(id); - workspace.renameVariableById(id, newName); - checkVariableValues(workspace, newName, type, id); + createVariableAndBlock(workspace); + + workspace.renameVariableById('id1', newName); + checkVariableValues(workspace, newName, 'type1', 'id1'); // Renaming should not have created a new variable. assertEquals(1, workspace.getAllVariables().length); var block_var_name = workspace.topBlocks_[0].getVarModels()[0].name; @@ -302,24 +299,13 @@ function test_renameVariable_TwoVariablesDifferentType() { // Expect the rename to succeed, because variables with different types are // allowed to have the same name. workspaceTest_setUp(); - var id1 = 'id1'; - var id2 = 'id2'; - var type1 = 'type1'; - var type2 = 'type2'; + createTwoVariablesAndBlocks(workspace); - var oldName = 'name1'; var newName = 'name2'; - // Create two variables of different types. - workspace.createVariable(oldName, type1, id1); - workspace.createVariable(newName, type2, id2); - // Create blocks to refer to both of them. - createMockBlock(id1); - createMockBlock(id2); + workspace.renameVariableById('id1', newName); - workspace.renameVariableById(id1, newName); - - checkVariableValues(workspace, newName, type1, id1); - checkVariableValues(workspace, newName, type2, id2); + checkVariableValues(workspace, newName, 'type1', 'id1'); + checkVariableValues(workspace, newName, 'type2', 'id2'); // References shoul have the correct names. var block_var_name_1 = workspace.topBlocks_[0].getVarModels()[0].name; @@ -333,17 +319,14 @@ function test_renameVariable_TwoVariablesDifferentType() { function test_renameVariable_OldCase() { // Rename a variable with a single reference. Update only the capitalization. workspaceTest_setUp(); - var oldCase = 'Name1'; - var newName = 'name1'; - var type = 'type1'; - var id = 'id1'; - workspace.createVariable(oldCase, type, id); - createMockBlock(id); + var newName = 'Name1'; - workspace.renameVariableById(id, newName); - checkVariableValues(workspace, newName, type, id); - var variable = workspace.getVariableById(id); - assertNotEquals(oldCase, id.name); + createVariableAndBlock(workspace); + + workspace.renameVariableById('id1', newName); + checkVariableValues(workspace, newName, 'type1', 'id1'); + var variable = workspace.getVariableById('id1'); + assertNotEquals('name1', variable.name); workspaceTest_tearDown(); } @@ -389,10 +372,7 @@ function test_renameVariable_TwoVariablesAndOldCase() { function test_deleteVariableById_Trivial() { workspaceTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); - createMockBlock('id1'); - createMockBlock('id2'); + createTwoVariablesAndBlocks(workspace); workspace.deleteVariableById('id1'); checkVariableValues(workspace, 'name2', 'type2', 'id2'); diff --git a/tests/jsunit/workspace_undo_redo_test.js b/tests/jsunit/workspace_undo_redo_test.js index 092dbb4a8..f014c8ba2 100644 --- a/tests/jsunit/workspace_undo_redo_test.js +++ b/tests/jsunit/workspace_undo_redo_test.js @@ -82,10 +82,14 @@ function createTwoVarsEmptyType() { workspace.createVariable('name2', '', 'id2'); } -function test_undoCreateVariable_Trivial() { - undoRedoTest_setUp(); +function createTwoVarsDifferentTypes() { workspace.createVariable('name1', 'type1', 'id1'); workspace.createVariable('name2', 'type2', 'id2'); +} + +function test_undoCreateVariable_Trivial() { + undoRedoTest_setUp(); + createTwoVarsDifferentTypes(); workspace.undo(); checkVariableValues(workspace, 'name1', 'type1', 'id1'); @@ -98,8 +102,7 @@ function test_undoCreateVariable_Trivial() { function test_redoAndUndoCreateVariable_Trivial() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); + createTwoVarsDifferentTypes(); workspace.undo(); workspace.undo(true); @@ -120,8 +123,7 @@ function test_redoAndUndoCreateVariable_Trivial() { function test_undoDeleteVariable_NoBlocks() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); + createTwoVarsDifferentTypes(); workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); @@ -137,10 +139,9 @@ function test_undoDeleteVariable_NoBlocks() { function test_undoDeleteVariable_WithBlocks() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); - createMockBlock('id1'); - createMockBlock('id2'); + + createTwoVariablesAndBlocks(workspace); + workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); @@ -159,8 +160,9 @@ function test_undoDeleteVariable_WithBlocks() { function test_redoAndUndoDeleteVariable_NoBlocks() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); + + createTwoVarsDifferentTypes(); + workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); @@ -181,10 +183,9 @@ function test_redoAndUndoDeleteVariable_NoBlocks() { function test_redoAndUndoDeleteVariable_WithBlocks() { undoRedoTest_setUp(); - workspace.createVariable('name1', 'type1', 'id1'); - workspace.createVariable('name2', 'type2', 'id2'); - createMockBlock('id1'); - createMockBlock('id2'); + + createTwoVariablesAndBlocks(workspace); + workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); @@ -261,24 +262,6 @@ function test_redoAndUndoDeleteVariableTwice_WithBlocks() { undoRedoTest_tearDown(); } -// TODO: Decide if this needs to be possible. -// function test_undoRedoRenameVariable_NeitherVariableExists() { -// // Expect that a variable with the name, 'name2', and the generated UUID, -// // 'id2', to be created when rename is called. Undo removes this variable -// // and redo recreates it. -// undoRedoTest_setUp(); -// setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, -// ['rename_group', 'id2', 'delete_group']); -// workspace.renameVariable('name1', 'name2'); - -// workspace.undo(); -// assertNull(workspace.getVariableById('id2')); - -// workspace.undo(true); -// checkVariableValues(workspace, 'name2', '', 'id2'); -// undoRedoTest_tearDown(); -// } - function test_undoRedoRenameVariable_OneExists_NoBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); From 637a909de738c51ac2205311a40703535569eb6d Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 6 Dec 2017 15:56:18 -0800 Subject: [PATCH 17/23] Tests --- tests/jsunit/index.html | 2 +- tests/jsunit/procedures_test.js | 1 - tests/jsunit/test_utilities.js | 18 ++++++++++++++++++ tests/jsunit/workspace_test.js | 14 ++------------ tests/jsunit/workspace_undo_redo_test.js | 14 ++------------ 5 files changed, 23 insertions(+), 26 deletions(-) diff --git a/tests/jsunit/index.html b/tests/jsunit/index.html index 619e4143b..500f2cb37 100644 --- a/tests/jsunit/index.html +++ b/tests/jsunit/index.html @@ -25,9 +25,9 @@ + - diff --git a/tests/jsunit/procedures_test.js b/tests/jsunit/procedures_test.js index f993af892..9d1468af2 100644 --- a/tests/jsunit/procedures_test.js +++ b/tests/jsunit/procedures_test.js @@ -37,7 +37,6 @@ function proceduresTest_setUpWithMockBlocks() { 'message0': '%1', 'args0': [ { - // TODO: Why is this a variable? It shouldn't need to be. 'type': 'field_variable', 'name': 'NAME', 'variable': 'item' diff --git a/tests/jsunit/test_utilities.js b/tests/jsunit/test_utilities.js index aa5840d9e..39e63eeea 100644 --- a/tests/jsunit/test_utilities.js +++ b/tests/jsunit/test_utilities.js @@ -122,3 +122,21 @@ function createVariableAndBlock(workspace) { workspace.createVariable('name1', 'type1', 'id1'); createMockBlock('id1'); } + +function defineGetVarBlock() { + Blockly.defineBlocksWithJsonArray([{ + "type": "get_var_block", + "message0": "%1", + "args0": [ + { + "type": "field_variable", + "name": "VAR", + "variableTypes": ["", "type1", "type2"] + } + ] + }]); +} + +function undefineGetVarBlock() { + delete Blockly.Blocks['get_var_block']; +} diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index e991106a3..20421d711 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -26,23 +26,13 @@ var workspace; var mockControl_; function workspaceTest_setUp() { - Blockly.defineBlocksWithJsonArray([{ - "type": "get_var_block", - "message0": "%1", - "args0": [ - { - "type": "field_variable", - "name": "VAR", - "variableTypes": ["", "type1", "type2"] - } - ] - }]); + defineGetVarBlock(); workspace = new Blockly.Workspace(); mockControl_ = new goog.testing.MockControl(); } function workspaceTest_tearDown() { - delete Blockly.Blocks['get_var_block']; + undefineGetVarBlock(); mockControl_.$tearDown(); workspace.dispose(); } diff --git a/tests/jsunit/workspace_undo_redo_test.js b/tests/jsunit/workspace_undo_redo_test.js index f014c8ba2..aa960d9f0 100644 --- a/tests/jsunit/workspace_undo_redo_test.js +++ b/tests/jsunit/workspace_undo_redo_test.js @@ -43,24 +43,14 @@ function temporary_fireEvent(event) { } function undoRedoTest_setUp() { - Blockly.defineBlocksWithJsonArray([{ - "type": "get_var_block", - "message0": "%1", - "args0": [ - { - "type": "field_variable", - "name": "VAR", - "variableTypes": ["", "type1", "type2"] - } - ] - }]); + defineGetVarBlock(); workspace = new Blockly.Workspace(); mockControl_ = new goog.testing.MockControl(); Blockly.Events.fire = temporary_fireEvent; } function undoRedoTest_tearDown() { - delete Blockly.Blocks['get_var_block']; + undefineGetVarBlock(); mockControl_.$tearDown(); workspace.dispose(); Blockly.Events.fire = savedFireFunc; From abc83b12d7cc3abd728e20cb3bfe2ff8f8b07abf Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 6 Dec 2017 16:07:21 -0800 Subject: [PATCH 18/23] Fix checkmarks when the variable dropdown is open. --- core/field_variable.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/field_variable.js b/core/field_variable.js index df6e10e72..fe7647fe9 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -56,6 +56,7 @@ Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes) { this.defaultVariableName = (varname || ''); this.defaultType_ = ''; this.variableTypes = opt_variableTypes; + this.value_ = null; }; goog.inherits(Blockly.FieldVariable, Blockly.FieldDropdown); @@ -155,6 +156,7 @@ Blockly.FieldVariable.prototype.setValue = function(id) { this.sourceBlock_, 'field', this.name, oldValue, variable.getId())); } this.variable_ = variable; + this.value_ = id; this.setText(variable.name); }; From 0909af76e5dec2a2704697c8b8c208beb99d7f5b Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 6 Dec 2017 16:41:17 -0800 Subject: [PATCH 19/23] Fix missing events for variable creation --- core/flyout_base.js | 30 ++---------------------------- core/variables.js | 29 ++++++++++++++++++++++++++++- core/xml.js | 8 ++++++++ 3 files changed, 38 insertions(+), 29 deletions(-) diff --git a/core/flyout_base.js b/core/flyout_base.js index 8b1421e39..dacab6169 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -599,33 +599,6 @@ Blockly.Flyout.prototype.onMouseDown_ = function(e) { } }; -/** - * Helper function to get the list of variables that have been added to the - * workspace after adding a new block, using the given list of variables that - * were in the workspace before the new block was added. - * @param {!Array.} originalVariables The array of - * variables that existed in the workspace before adding the new block. - * @return {!Array.} The new array of variables that were - * freshly added to the workspace after creating the new block, or [] if no - * new variables were added to the workspace. - * @private - */ -Blockly.Flyout.prototype.getAddedVariables_ = function(originalVariables) { - var allCurrentVariables = this.targetWorkspace_.getAllVariables(); - var addedVariables = []; - if (originalVariables.length != allCurrentVariables.length) { - for (var i = 0; i < allCurrentVariables.length; i++) { - var variable = allCurrentVariables[i]; - // For any variable that is present in allCurrentVariables but not - // present in originalVariables, add the variable to addedVariables. - if (!originalVariables.includes(variable)) { - addedVariables.push(variable); - } - } - } - return addedVariables; -}; - /** * Create a copy of this block on the workspace. * @param {!Blockly.BlockSvg} originalBlock The block to copy from the flyout. @@ -646,7 +619,8 @@ Blockly.Flyout.prototype.createBlock = function(originalBlock) { Blockly.Events.enable(); } - var newVariables = this.getAddedVariables_(variablesBeforeCreation); + var newVariables = Blockly.Variables.getAddedVariables(this.targetWorkspace_, + variablesBeforeCreation); if (Blockly.Events.isEnabled()) { Blockly.Events.setGroup(true); diff --git a/core/variables.js b/core/variables.js index c6e3462da..562068660 100644 --- a/core/variables.js +++ b/core/variables.js @@ -440,5 +440,32 @@ Blockly.Variables.createVariable_ = function(workspace, id, name, type) { Blockly.Variables.getPotentialVariableMap_ = function(workspace) { return workspace.isFlyout && workspace.targetWorkspace ? workspace.targetWorkspace.getPotentialVariableMap() : null; - +}; + +/** + * Helper function to get the list of variables that have been added to the + * workspace after adding a new block, using the given list of variables that + * were in the workspace before the new block was added. + * @param {!Blockly.Workspace} workspace The workspace to inspect. + * @param {!Array.} originalVariables The array of + * variables that existed in the workspace before adding the new block. + * @return {!Array.} The new array of variables that were + * freshly added to the workspace after creating the new block, or [] if no + * new variables were added to the workspace. + * @package + */ +Blockly.Variables.getAddedVariables = function(workspace, originalVariables) { + var allCurrentVariables = workspace.getAllVariables(); + var addedVariables = []; + if (originalVariables.length != allCurrentVariables.length) { + for (var i = 0; i < allCurrentVariables.length; i++) { + var variable = allCurrentVariables[i]; + // For any variable that is present in allCurrentVariables but not + // present in originalVariables, add the variable to addedVariables. + if (!originalVariables.includes(variable)) { + addedVariables.push(variable); + } + } + } + return addedVariables; }; diff --git a/core/xml.js b/core/xml.js index e1eaaa3a3..db8121131 100644 --- a/core/xml.js +++ b/core/xml.js @@ -497,6 +497,7 @@ Blockly.Xml.domToBlock = function(xmlBlock, workspace) { } // Create top-level block. Blockly.Events.disable(); + var variablesBeforeCreation = workspace.getAllVariables(); try { var topBlock = Blockly.Xml.domToBlockHeadless_(xmlBlock, workspace); if (workspace.rendered) { @@ -529,6 +530,13 @@ Blockly.Xml.domToBlock = function(xmlBlock, workspace) { } if (Blockly.Events.isEnabled()) { Blockly.Events.fire(new Blockly.Events.BlockCreate(topBlock)); + var newVariables = Blockly.Variables.getAddedVariables(workspace, + variablesBeforeCreation); + // Fire a VarCreate event for each (if any) new variable created. + for(var i = 0; i < newVariables.length; i++) { + var thisVariable = newVariables[i]; + Blockly.Events.fire(new Blockly.Events.VarCreate(thisVariable)); + } } return topBlock; }; From 3cecc0f604fab6661aab5bab41b8f12a3aa1d9a5 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 6 Dec 2017 16:57:46 -0800 Subject: [PATCH 20/23] Avoid spurious rename event --- core/variable_map.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/variable_map.js b/core/variable_map.js index c9a02aa43..2dc70f8ff 100644 --- a/core/variable_map.js +++ b/core/variable_map.js @@ -107,14 +107,16 @@ Blockly.VariableMap.prototype.renameVariableWithConflict_ = function(variable, var type = variable.type; Blockly.Events.setGroup(true); - Blockly.Events.fire(new Blockly.Events.VarRename(conflictVar, newName)); var oldCase = conflictVar.name; - conflictVar.name = newName; - // These blocks refer to the same variable but the case may have changed. - // No change events should be fired here. var blocks = this.workspace.getAllBlocks(); if (newName != oldCase) { + // Only fire a rename event if the case changed. + Blockly.Events.fire(new Blockly.Events.VarRename(conflictVar, newName)); + conflictVar.name = newName; + + // These blocks refer to the same variable but the case changed. + // No change events should be fired here. for (var i = 0; i < blocks.length; i++) { blocks[i].updateVarName(conflictVar); } From cc6eeb8c68a423651af21a482cd0e4771862c595 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 7 Dec 2017 10:43:12 -0800 Subject: [PATCH 21/23] Clean up TODOs and move potential variable map to the flyout workspace --- core/block.js | 2 +- core/field_variable.js | 4 +--- core/flyout_base.js | 2 +- core/variables.js | 24 ++---------------------- core/workspace.js | 4 +--- core/xml.js | 3 +-- 6 files changed, 7 insertions(+), 32 deletions(-) diff --git a/core/block.js b/core/block.js index 5080c3b37..1c20108b7 100644 --- a/core/block.js +++ b/core/block.js @@ -704,7 +704,7 @@ Blockly.Block.prototype.getVarModels = function() { /** * Notification that a variable is renaming. - * TODO (fenichel): consider deleting this. + * TODO (#1498): consider deleting this. * If the name matches one of this block's variables, rename it. * @param {string} oldName Previous name of variable. * @param {string} newName Renamed variable. diff --git a/core/field_variable.js b/core/field_variable.js index fe7647fe9..79a42d094 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -52,7 +52,7 @@ Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes) { this.menuGenerator_ = Blockly.FieldVariable.dropdownCreate; this.size_ = new goog.math.Size(0, Blockly.BlockSvg.MIN_BLOCK_Y); this.setValidator(opt_validator); - // TODO: Add opt_default_type to match default value. If not set, ''. + // TODO (#1499): Add opt_default_type to match default value. If not set, ''. this.defaultVariableName = (varname || ''); this.defaultType_ = ''; this.variableTypes = opt_variableTypes; @@ -135,8 +135,6 @@ Blockly.FieldVariable.prototype.getText = function() { * variable. */ Blockly.FieldVariable.prototype.setValue = function(id) { - // TODO: Handle undo a change to go back to the default value. - // TODO: Handle null ID, which means "use default". var workspace = this.sourceBlock_.workspace; var variable = Blockly.Variables.getVariable(workspace, id); diff --git a/core/flyout_base.js b/core/flyout_base.js index dacab6169..29ca24d18 100644 --- a/core/flyout_base.js +++ b/core/flyout_base.js @@ -544,7 +544,7 @@ Blockly.Flyout.prototype.clearOldBlocks_ = function() { this.buttons_.length = 0; // Clear potential variables from the previous showing. - this.targetWorkspace_.potentialVariableMap_.clear(); + this.workspace_.getPotentialVariableMap().clear(); }; /** diff --git a/core/variables.js b/core/variables.js index 562068660..676223c24 100644 --- a/core/variables.js +++ b/core/variables.js @@ -376,8 +376,7 @@ Blockly.Variables.getOrCreateVariable = function(workspace, id, name, type) { * @private */ Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) { - var potentialVariableMap = - Blockly.Variables.getPotentialVariableMap_(workspace); + var potentialVariableMap = workspace.getPotentialVariableMap(); // Try to just get the variable, by ID if possible. if (id) { // Look in the real variable map before checking the potential variable map. @@ -407,8 +406,7 @@ Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) { * @private */ Blockly.Variables.createVariable_ = function(workspace, id, name, type) { - var potentialVariableMap = - Blockly.Variables.getPotentialVariableMap_(workspace); + var potentialVariableMap = workspace.getPotentialVariableMap(); // Variables without names get uniquely named for this workspace. if (!name) { var ws = workspace.isFlyout ? workspace.targetWorkspace : workspace; @@ -424,24 +422,6 @@ Blockly.Variables.createVariable_ = function(workspace, id, name, type) { return variable; }; -/** - * Blocks in the flyout can refer to variables that don't exist in the - * workspace. For instance, the "get item in list" block refers to an "item" - * variable regardless of whether the variable has been created yet. - * A FieldVariable must always refer to a Blockly.VariableModel. We reconcile - * these by tracking "potential" variables in the flyout. These variables - * become real when references to them are dragged into the main workspace. - * @param {!Blockly.Workspace} workspace The workspace to get the potential - * variable map from. - * @return {?Blockly.VariableMap} The potential variable map for the given - * workspace, or null if it was not a flyout workspace. - * @private - */ -Blockly.Variables.getPotentialVariableMap_ = function(workspace) { - return workspace.isFlyout && workspace.targetWorkspace ? - workspace.targetWorkspace.getPotentialVariableMap() : null; -}; - /** * Helper function to get the list of variables that have been added to the * workspace after adding a new block, using the given list of variables that diff --git a/core/workspace.js b/core/workspace.js index 698f523eb..6505560ce 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -532,13 +532,11 @@ Blockly.Workspace.prototype.getAllVariables = function() { /** * Return the variable map that contains "potential" variables. These exist in * the flyout but not in the workspace. - * TODO: Decide if this can be stored on the flyout workspace instead of the - * main workspace. * @return {?Blockly.VariableMap} The potential variable map. * @package */ Blockly.Workspace.prototype.getPotentialVariableMap = function() { - return this.potentialVariableMap_; + return this.isFlyout ? this.potentialVariableMap_ : null; }; /** diff --git a/core/xml.js b/core/xml.js index db8121131..ec1da0e18 100644 --- a/core/xml.js +++ b/core/xml.js @@ -91,8 +91,7 @@ Blockly.Xml.fieldToDomVariable_ = function(field, workspace) { var variable = workspace.getVariableById(id); if (!variable) { if (workspace.isFlyout && workspace.targetWorkspace) { - var potentialVariableMap = - workspace.targetWorkspace.potentialVariableMap_; + var potentialVariableMap = workspace.getPotentialVariableMap(); if (potentialVariableMap) { variable = potentialVariableMap.getVariableById(id); } From 73253ea64f13282d97e2ce8865c26503c47a3a72 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 7 Dec 2017 13:12:20 -0800 Subject: [PATCH 22/23] Respond to review comments in variable_map --- core/field_variable.js | 2 ++ core/variable_map.js | 32 ++++++++++++++------------------ 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index 79a42d094..18e2d9cc2 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -135,6 +135,8 @@ Blockly.FieldVariable.prototype.getText = function() { * variable. */ Blockly.FieldVariable.prototype.setValue = function(id) { + // What do I do when id is null? That happens when undoing a change event + // for the first time the value was set. var workspace = this.sourceBlock_.workspace; var variable = Blockly.Variables.getVariable(workspace, id); diff --git a/core/variable_map.js b/core/variable_map.js index 2dc70f8ff..5081db09a 100644 --- a/core/variable_map.js +++ b/core/variable_map.js @@ -67,12 +67,15 @@ Blockly.VariableMap.prototype.clear = function() { Blockly.VariableMap.prototype.renameVariable = function(variable, newName) { var type = variable.type; var conflictVar = this.getVariable(newName, type); + var blocks = this.workspace.getAllBlocks(); + Blockly.Events.setGroup(true); // The IDs may match if the rename is a simple case change (name1 -> Name1). if (!conflictVar || conflictVar.getId() == variable.getId()) { - this.renameVariableNoConflict_(variable, newName); + this.renameVariableAndUses_(variable, newName, blocks); } else { - this.renameVariableWithConflict_(variable, newName, conflictVar); + this.renameVariableWithConflict_(variable, newName, conflictVar, blocks); } + Blockly.Events.setGroup(false); }; /** @@ -80,12 +83,14 @@ Blockly.VariableMap.prototype.renameVariable = function(variable, newName) { * The new name must not conflict with any existing variable names. * @param {!Blockly.VariableModel} variable Variable to rename. * @param {string} newName New variable name. + * @param {!Array.} blocks The list of all blocks in the + * workspace. * @private */ -Blockly.VariableMap.prototype.renameVariableNoConflict_ = function(variable, newName) { +Blockly.VariableMap.prototype.renameVariableAndUses_ = function(variable, + newName, blocks) { Blockly.Events.fire(new Blockly.Events.VarRename(variable, newName)); variable.name = newName; - var blocks = this.workspace.getAllBlocks(); for (var i = 0; i < blocks.length; i++) { blocks[i].updateVarName(variable); } @@ -100,26 +105,18 @@ Blockly.VariableMap.prototype.renameVariableNoConflict_ = function(variable, new * @param {string} newName New variable name. * @param {!Blockly.VariableModel} conflictVar The variable that was already * using newName. + * @param {!Array.} blocks The list of all blocks in the + * workspace. * @private */ Blockly.VariableMap.prototype.renameVariableWithConflict_ = function(variable, - newName, conflictVar) { + newName, conflictVar, blocks) { var type = variable.type; - - Blockly.Events.setGroup(true); var oldCase = conflictVar.name; - var blocks = this.workspace.getAllBlocks(); if (newName != oldCase) { - // Only fire a rename event if the case changed. - Blockly.Events.fire(new Blockly.Events.VarRename(conflictVar, newName)); - conflictVar.name = newName; - - // These blocks refer to the same variable but the case changed. - // No change events should be fired here. - for (var i = 0; i < blocks.length; i++) { - blocks[i].updateVarName(conflictVar); - } + // Simple rename to change the case and update references. + this.renameVariableAndUses_(conflictVar, newName, blocks); } // These blocks now refer to a different variable. @@ -135,7 +132,6 @@ Blockly.VariableMap.prototype.renameVariableWithConflict_ = function(variable, var variableIndex = variableList.indexOf(variable); this.variableMap_[type].splice(variableIndex, 1); - Blockly.Events.setGroup(false); }; /** From 77a47aee94d9dd9255bc778c6204e2889196ad26 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 7 Dec 2017 16:08:00 -0800 Subject: [PATCH 23/23] Fix html escaping and flyouts opening --- core/block.js | 7 ++++++- core/variables.js | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/core/block.js b/core/block.js index 1c20108b7..c6f400619 100644 --- a/core/block.js +++ b/core/block.js @@ -695,7 +695,12 @@ Blockly.Block.prototype.getVarModels = function() { for (var i = 0, input; input = this.inputList[i]; i++) { for (var j = 0, field; field = input.fieldRow[j]; j++) { if (field instanceof Blockly.FieldVariable) { - vars.push(this.workspace.getVariableById(field.getValue())); + var model = this.workspace.getVariableById(field.getValue()); + // Check if the variable actually exists (and isn't just a potential + // variable). + if (model) { + vars.push(model); + } } } } diff --git a/core/variables.js b/core/variables.js index 676223c24..b8d40028b 100644 --- a/core/variables.js +++ b/core/variables.js @@ -336,7 +336,7 @@ Blockly.Variables.generateVariableFieldXml_ = function(variableModel) { } var text = '' + variableModel.name + ''; + '">' + goog.string.htmlEscape(variableModel.name) + ''; return text; };