From 1ce46ab88b67317b5e25766170d4b888d0ecf405 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Tue, 5 Dec 2017 13:08:01 -0800 Subject: [PATCH] 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 d6d011dbc..14e2b1fbf 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) {