diff --git a/src/engine/blocks.js b/src/engine/blocks.js index 13122c1f1..30f43a2af 100644 --- a/src/engine/blocks.js +++ b/src/engine/blocks.js @@ -6,6 +6,8 @@ const Clone = require('../util/clone'); const {Map} = require('immutable'); const BlocksExecuteCache = require('./blocks-execute-cache'); const log = require('../util/log'); +const Variable = require('./variable'); +const StringUtil = require('../util/string-util'); /** * @fileoverview @@ -670,6 +672,68 @@ class Blocks { this.resetCache(); } + /** + * Fixes up variable references in this blocks container avoiding conflicts with + * pre-existing variables in the same scope. + * This is used when uploading a new sprite (the given target) + * into an existing project, where the new sprite may contain references + * to variable names that already exist as global variables in the project + * (and thus are in scope for variable references in the given sprite). + * + * If the given target has a block that references an existing global variable and that + * variable *does not* exist in the target itself (e.g. it was a global variable in the + * project the sprite was originally exported from), fix the variable references in this sprite + * to reference the id of the pre-existing global variable. + * If the given target has a block that references an existing global variable and that + * variable does exist in the target itself (e.g. it's a local variable in the sprite being uploaded), + * then the variable is renamed to distinguish itself from the pre-existing variable. + * All blocks that reference the local variable will be updated to use the new name. + * @param {Target} target The new target being uploaded, with potential variable conflicts + * @param {Runtime} runtime The runtime context with any pre-existing variables + */ + fixUpVariableReferences (target, runtime) { + const blocks = this._blocks; + for (const blockId in blocks) { + let varOrListField = null; + let varType = null; + if (blocks[blockId].fields.VARIABLE) { + varOrListField = blocks[blockId].fields.VARIABLE; + varType = Variable.SCALAR_TYPE; + } else if (blocks[blockId].fields.LIST) { + varOrListField = blocks[blockId].fields.LIST; + varType = Variable.LIST_TYPE; + } + if (varOrListField) { + const currVarId = varOrListField.id; + const currVarName = varOrListField.value; + if (target.lookupVariableById(currVarId)) { + // Found a variable with the id in either the target or the stage, + // figure out which one. + if (target.variables.hasOwnProperty(currVarId)) { + // If the target has the variable, then check whether the stage + // has one with the same name and type. If it does, then rename + // this target specific variable so that there is a distinction. + const stage = runtime.getTargetForStage(); + if (stage && stage.variables && + stage.lookupVariableByNameAndType(currVarName, varType)) { + // TODO what should the new name be? + const newName = StringUtil.unusedName( + `${target.getName()}_${currVarName}`, + target.getAllVariableNamesInScopeByType(varType)); + target.renameVariable(currVarId, newName); + this.updateBlocksAfterVarRename(currVarId, newName); + } + } + } else { + const existingVar = target.lookupVariableByNameAndType(currVarName, varType); + if (existingVar) { + varOrListField.id = existingVar.id; + } + } + } + } + } + /** * Keep blocks up to date after a variable gets renamed. * @param {string} varId The id of the variable that was renamed diff --git a/src/engine/target.js b/src/engine/target.js index a8c6828ee..c62eb373b 100644 --- a/src/engine/target.js +++ b/src/engine/target.js @@ -80,14 +80,40 @@ class Target extends EventEmitter { } /** - * Look up a variable object, and create it if one doesn't exist. + * Get the names of all the variables of the given type that are in scope for this target. + * For targets that are not the stage, this includes any target-specific + * variables as well as any stage variables. + * For the stage, this is all stage variables. + * @param {string} type The variable type to search for; defaults to Variable.SCALAR_TYPE + * @return {Array} A list of variable names + */ + getAllVariableNamesInScopeByType (type) { + if (typeof type !== 'string') type = Variable.SCALAR_TYPE; + const targetVariables = Object.values(this.variables) + .filter(v => v.type === type) + .map(variable => variable.name); + if (this.isStage || !this.runtime) { + return targetVariables; + } + const stage = this.runtime.getTargetForStage(); + const stageVariables = stage.getAllVariableNamesInScopeByType(type); + return targetVariables.concat(stageVariables); + } + + /** + * Look up a variable object, first by id, and then by name if the id is not found. + * Create a new variable if both lookups fail. * @param {string} id Id of the variable. * @param {string} name Name of the variable. * @return {!Variable} Variable object. */ lookupOrCreateVariable (id, name) { - const variable = this.lookupVariableById(id); + let variable = this.lookupVariableById(id); if (variable) return variable; + + variable = this.lookupVariableByNameAndType(name, Variable.SCALAR_TYPE); + if (variable) return variable; + // No variable with this name exists - create it locally. const newVariable = new Variable(id, name, Variable.SCALAR_TYPE, false); this.variables[id] = newVariable; @@ -161,6 +187,40 @@ class Target extends EventEmitter { } } + /** + * Look up a variable object by its name and variable type. + * Search begins with local variables; then global variables if a local one + * was not found. + * @param {string} name Name of the variable. + * @param {string} type Type of the variable. Defaults to Variable.SCALAR_TYPE. + * @return {?Variable} Variable object if found, or null if not. + */ + lookupVariableByNameAndType (name, type) { + if (typeof name !== 'string') return; + if (typeof type !== 'string') type = Variable.SCALAR_TYPE; + + for (const varId in this.variables) { + const currVar = this.variables[varId]; + if (currVar.name === name && currVar.type === type) { + return currVar; + } + } + + if (this.runtime && !this.isStage) { + const stage = this.runtime.getTargetForStage(); + if (stage) { + for (const varId in stage.variables) { + const currVar = stage.variables[varId]; + if (currVar.name === name && currVar.type === type) { + return currVar; + } + } + } + } + + return null; + } + /** * Look up a list object for this target, and create it if one doesn't exist. * Search begins for local lists; then look for globals. @@ -169,8 +229,12 @@ class Target extends EventEmitter { * @return {!Varible} Variable object representing the found/created list. */ lookupOrCreateList (id, name) { - const list = this.lookupVariableById(id); + let list = this.lookupVariableById(id); if (list) return list; + + list = this.lookupVariableByNameAndType(name, Variable.LIST_TYPE); + if (list) return list; + // No variable with this name exists - create it locally. const newList = new Variable(id, name, Variable.LIST_TYPE, false); this.variables[id] = newList; @@ -240,10 +304,13 @@ class Target extends EventEmitter { name: 'VARIABLE', value: id }, this.runtime); - this.runtime.requestUpdateMonitor(Map({ - id: id, - params: blocks._getBlockParams(blocks.getBlock(variable.id)) - })); + const monitorBlock = blocks.getBlock(variable.id); + if (monitorBlock) { + this.runtime.requestUpdateMonitor(Map({ + id: id, + params: blocks._getBlockParams(monitorBlock) + })); + } } } diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index 768267682..afe615dfc 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -189,19 +189,14 @@ const parseScripts = function (scripts, blocks, addBroadcastMsg, getVariableId, * Create a callback for assigning fixed IDs to imported variables * Generator stores the global variable mapping in a closure * @param {!string} targetId the id of the target to scope the variable to - * @param {object=} initialVars Initial vars in scope if parsing a sprite * @return {string} variable ID */ const generateVariableIdGetter = (function () { let globalVariableNameMap = {}; const namer = (targetId, name) => `${targetId}-${name}`; - return function (targetId, topLevel, initialVars) { + return function (targetId, topLevel) { // Reset the global variable map if topLevel - if (topLevel) { - globalVariableNameMap = {}; - } else if (initialVars) { - globalVariableNameMap = initialVars; - } + if (topLevel) globalVariableNameMap = {}; return function (name) { if (topLevel) { // Store the name/id pair in the globalVariableNameMap globalVariableNameMap[name] = namer(targetId, name); @@ -339,10 +334,9 @@ const parseMonitorObject = (object, runtime, targets, extensions) => { * @param {ImportedExtensionsInfo} extensions - (in/out) parsed extension information will be stored here. * @param {boolean} topLevel - Whether this is the top-level object (stage). * @param {?object} zip - Optional zipped assets for local file import - * @param {object=} initialVarScope - Initial set of variables in scope if parsing a single sprite * @return {!Promise.>} Promise for the loaded targets when ready, or null for unsupported objects. */ -const parseScratchObject = function (object, runtime, extensions, topLevel, zip, initialVarScope) { +const parseScratchObject = function (object, runtime, extensions, topLevel, zip) { if (!object.hasOwnProperty('objName')) { if (object.hasOwnProperty('listName')) { // Shim these objects so they can be processed as monitors @@ -433,7 +427,7 @@ const parseScratchObject = function (object, runtime, extensions, topLevel, zip, // Create the first clone, and load its run-state from JSON. const target = sprite.createClone(topLevel ? StageLayering.BACKGROUND_LAYER : StageLayering.SPRITE_LAYER); - const getVariableId = generateVariableIdGetter(target.id, topLevel, initialVarScope); + const getVariableId = generateVariableIdGetter(target.id, topLevel); const globalBroadcastMsgObj = globalBroadcastMsgStateGenerator(topLevel); const addBroadcastMsg = globalBroadcastMsgObj.broadcastMsgMapUpdater; @@ -673,18 +667,14 @@ const sb2import = function (json, runtime, optForceSprite, zip) { extensionURLs: new Map() }; - const initialGlobalVars = {}; - if (optForceSprite && runtime.targets && runtime.getTargetForStage()) { - const stageVars = runtime.getTargetForStage().variables; - for (const varId in stageVars) { - const currVar = stageVars[varId]; - initialGlobalVars[currVar.name] = varId; - } - } - - // If parsing a sprite, use the current global vars as parent scope for the sprite - return parseScratchObject(json, runtime, extensions, !optForceSprite, zip, - (optForceSprite ? initialGlobalVars : null)) + return parseScratchObject(json, runtime, extensions, !optForceSprite, zip) + .then(targets => { + if (optForceSprite && targets.length === 1) { + const target = targets[0]; + target.blocks.fixUpVariableReferences(target, runtime); + } + return targets; + }) .then(targets => ({ targets, extensions diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js index 1c02c2aec..d63fa2049 100644 --- a/src/serialization/sb3.js +++ b/src/serialization/sb3.js @@ -936,10 +936,18 @@ const deserialize = function (json, runtime, zip, isSingleSprite) { return Promise.all( ((isSingleSprite ? [json] : json.targets) || []).map(target => parseScratchObject(target, runtime, extensions, zip)) - ).then(targets => ({ - targets, - extensions - })); + ) + .then(targets => { + if (isSingleSprite && targets.length === 1) { + const target = targets[0]; + target.blocks.fixUpVariableReferences(target, runtime); + } + return targets; + }) + .then(targets => ({ + targets, + extensions + })); }; module.exports = {