From 80b397f5320e5a5751ff605de2af2ba7f9e160c2 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Wed, 10 Jan 2018 17:27:43 -0800 Subject: [PATCH] Respond to more review comments --- core/field_variable.js | 7 ++++--- core/variables.js | 33 ++++++++++++++++++++------------- core/workspace.js | 4 ++-- core/xml.js | 10 ++++++++-- 4 files changed, 34 insertions(+), 20 deletions(-) diff --git a/core/field_variable.js b/core/field_variable.js index 4dec2f075..c0e3bb07a 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -130,8 +130,9 @@ Blockly.FieldVariable.prototype.getValue = function() { }; /** - * Get the text from this field. - * @return {string} Current text. + * Get the text from this field, which is the selected variable's name. + * @return {string} The selected variable's name, or the empty string if no + * variable is selected. */ Blockly.FieldVariable.prototype.getText = function() { return this.variable_ ? this.variable_.name : ''; @@ -171,7 +172,7 @@ Blockly.FieldVariable.prototype.setValue = function(id) { 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.sourceBlock_, 'field', this.name, oldValue, id)); } this.variable_ = variable; this.value_ = id; diff --git a/core/variables.js b/core/variables.js index 3eb3bcedf..2b591bf82 100644 --- a/core/variables.js +++ b/core/variables.js @@ -378,16 +378,20 @@ Blockly.Variables.generateVariableFieldXml_ = function(variableModel) { * @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. + * @param {string=} opt_name The string to use to look up or create the + * variable. + * @param {string=} opt_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); +Blockly.Variables.getOrCreateVariable = function(workspace, id, opt_name, + opt_type) { + var variable = Blockly.Variables.getVariable(workspace, id, opt_name, + opt_type); if (!variable) { - variable = Blockly.Variables.createVariable_(workspace, id, name, type); + variable = Blockly.Variables.createVariable_(workspace, id, opt_name, + opt_type); } return variable; }; @@ -405,7 +409,7 @@ Blockly.Variables.getOrCreateVariable = function(workspace, id, name, type) { * 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 + * @package */ Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) { var potentialVariableMap = workspace.getPotentialVariableMap(); @@ -422,6 +426,8 @@ Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) { if (!variable && potentialVariableMap) { variable = potentialVariableMap.getVariable(opt_name, opt_type); } + } else { + throw new Error('Tried to look up a variable by name without a type'); } return variable; }; @@ -431,25 +437,26 @@ Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) { * @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. + * @param {string=} opt_name The string to use to create the variable. + * @param {string=} opt_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) { +Blockly.Variables.createVariable_ = function(workspace, id, opt_name, + opt_type) { var potentialVariableMap = workspace.getPotentialVariableMap(); // Variables without names get uniquely named for this workspace. - if (!name) { + if (!opt_name) { var ws = workspace.isFlyout ? workspace.targetWorkspace : workspace; - name = Blockly.Variables.generateUniqueName(ws); + opt_name = Blockly.Variables.generateUniqueName(ws); } // Create a potential variable if in the flyout. if (potentialVariableMap) { - var variable = potentialVariableMap.createVariable(name, type, id); + var variable = potentialVariableMap.createVariable(opt_name, opt_type, id); } else { // In the main workspace, create a real variable. - var variable = workspace.createVariable(name, type, id); + var variable = workspace.createVariable(opt_name, opt_type, id); } return variable; }; diff --git a/core/workspace.js b/core/workspace.js index c72a30ad5..d4f9f27ae 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -86,7 +86,7 @@ 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 + * Blocks in the flyout can refer to variables that don't exist in the main * 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 @@ -227,7 +227,7 @@ Blockly.Workspace.prototype.createVariable = function(name, opt_type, opt_id) { }; /** - * Find all the uses of a named variable. + * Find all the uses of the given variable, which is identified by ID. * @param {string} id ID of the variable to find. * @return {!Array.} Array of block usages. */ diff --git a/core/xml.js b/core/xml.js index a3e42bc54..4d5a0a65e 100644 --- a/core/xml.js +++ b/core/xml.js @@ -88,7 +88,7 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) { /** * Encode a variable field as XML. - * @param {!Blockly.Field} field The field to encode. + * @param {!Blockly.FieldVariable} field The field to encode. * @return {?Element} XML element, or null if the field did not need to be * serialized. * @private @@ -96,12 +96,18 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) { Blockly.Xml.fieldToDomVariable_ = function(field) { var id = field.getValue(); // The field had not been initialized fully before being serialized. + // This can happen if a block is created directly through a call to + // workspace.newBlock instead of from XML. + // The new block will be serialized for the first time when firing a block + // creation event. if (id == null) { field.initModel(); id = field.getValue(); } // Get the variable directly from the field, instead of doing a lookup. This - // will work even if the variable has already been deleted. + // will work even if the variable has already been deleted. This can happen + // because the flyout defers deleting blocks until the next time the flyout is + // opened. var variable = field.getVariable(); if (!variable) {