diff --git a/src/engine/target.js b/src/engine/target.js index e524bff6..539cb21b 100644 --- a/src/engine/target.js +++ b/src/engine/target.js @@ -535,6 +535,7 @@ class Target extends EventEmitter { * then the local 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. */ + // TODO (#1360) This function is too long, add some helpers for the different chunks and cases... fixUpVariableReferences () { if (!this.runtime) return; // There's no runtime context to conflict with if (this.isStage) return; // Stage can't have variable conflicts with itself (and also can't be uploaded) @@ -562,6 +563,18 @@ class Target extends EventEmitter { } } const conflictIdsToReplace = Object.create(null); + const conflictNamesToReplace = Object.create(null); + + // Cache the list of all variable names by type so that we don't need to + // re-calculate this in every iteration of the following loop. + const varNamesByType = {}; + const allVarNames = type => { + const namesOfType = varNamesByType[type]; + if (namesOfType) return namesOfType; + varNamesByType[type] = this.runtime.getAllVarNamesOfType(type); + return varNamesByType[type]; + }; + for (const varId in allReferences) { // We don't care about which var ref we get, they should all have the same var info const varRef = allReferences[varId][0]; @@ -588,10 +601,25 @@ class Target extends EventEmitter { } } else { // We didn't find the referenced variable id anywhere, - // look it up by name and type to make sure there's no conflict - const existingVar = this.lookupVariableByNameAndType(varName, varType); - if (existingVar && !conflictIdsToReplace[varId]) { - conflictIdsToReplace[varId] = existingVar.id; + // Treat it as a reference to a global variable (from the original + // project this sprite was exported from). + // Check for whether a global variable of the same name and type exists, + // and if so, track it to merge with the existing global in a second pass of the blocks. + const existingVar = stage.lookupVariableByNameAndType(varName, varType); + if (existingVar) { + if (!conflictIdsToReplace[varId]) { + conflictIdsToReplace[varId] = existingVar.id; + } + } else { + // A global variable with the same name did not already exist, + // create a new one such that it does not conflict with any + // names of local variables of the same type. + const allNames = allVarNames(varType); + const freshName = StringUtil.unusedName(varName, allNames); + stage.createVariable(varId, freshName, varType); + if (!conflictNamesToReplace[varId]) { + conflictNamesToReplace[varId] = freshName; + } } } } @@ -603,7 +631,7 @@ class Target extends EventEmitter { const type = this.variables[varId].type; renameConflictingLocalVar(varId, name, type); } - // Finally, handle global var conflicts (e.g. a sprite is uploaded, and has + // Handle global var conflicts with existing global vars (e.g. a sprite is uploaded, and has // blocks referencing some variable that the sprite does not own, and this // variable conflicts with a global var) // In this case, we want to merge the new variable referenes with the @@ -613,6 +641,20 @@ class Target extends EventEmitter { const referencesToUpdate = allReferences[conflictId]; this.mergeVariables(conflictId, existingId, referencesToUpdate); } + + // Handle global var conflicts existing local vars (e.g a sprite is uploaded, + // and has blocks referencing some variable that the sprite does not own, and this + // variable conflcits with another sprite's local var). + // In this case, we want to go through the variable references and update + // the name of the variable in that reference. + for (const conflictId in conflictNamesToReplace) { + const newName = conflictNamesToReplace[conflictId]; + const referencesToUpdate = allReferences[conflictId]; + referencesToUpdate.map(ref => { + ref.referencingField.value = newName; + return ref; + }); + } } } diff --git a/test/unit/engine_target.js b/test/unit/engine_target.js index 098908e1..1e7d5a8b 100644 --- a/test/unit/engine_target.js +++ b/test/unit/engine_target.js @@ -387,6 +387,51 @@ test('fixUpVariableReferences fixes conflicting sprite local var without blocks t.end(); }); +test('fixUpVariableReferences fixes sprite global var conflicting with other sprite\'s local var', t => { + const runtime = new Runtime(); + + const stage = new Target(runtime); + stage.isStage = true; + + const target = new Target(runtime); + target.isStage = false; + + const existingTarget = new Target(runtime); + existingTarget.isStage = false; + + runtime.targets = [stage, target, existingTarget]; + + // Create a local variable on the pre-existing target + existingTarget.createVariable('pre-existing local var id', 'a mock variable', Variable.SCALAR_TYPE); + + target.blocks.createBlock(adapter(events.mockVariableBlock)[0]); + + t.equal(Object.keys(existingTarget.variables).length, 1); + const existingVariable = Object.values(existingTarget.variables)[0]; + t.equal(existingVariable.name, 'a mock variable'); + t.equal(Object.keys(target.variables).length, 0); + t.equal(Object.keys(stage.variables).length, 0); + t.type(target.blocks.getBlock('a block'), 'object'); + t.type(target.blocks.getBlock('a block').fields, 'object'); + t.type(target.blocks.getBlock('a block').fields.VARIABLE, 'object'); + t.equal(target.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id'); + + target.fixUpVariableReferences(); + + t.equal(Object.keys(existingTarget.variables).length, 1); + t.equal(existingVariable.name, 'a mock variable'); + t.equal(Object.keys(target.variables).length, 0); + t.equal(Object.keys(stage.variables).length, 1); + t.type(target.blocks.getBlock('a block'), 'object'); + t.type(target.blocks.getBlock('a block').fields, 'object'); + t.type(target.blocks.getBlock('a block').fields.VARIABLE, 'object'); + t.equal(target.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id'); + const newGlobal = stage.variables[Object.keys(stage.variables)[0]]; + t.equal(newGlobal.name, 'a mock variable2'); + + t.end(); +}); + test('fixUpVariableReferences does not change variable name if there is no variable conflict', t => { const runtime = new Runtime();