From cc6eeb8c68a423651af21a482cd0e4771862c595 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 7 Dec 2017 10:43:12 -0800 Subject: [PATCH] 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); }