Code cleanup, refactor, abstraction

This commit is contained in:
Karishma Chadha 2018-07-13 12:58:45 -04:00
parent 13df3e7bd6
commit 2b2fc2e561
4 changed files with 251 additions and 126 deletions

View file

@ -13,6 +13,7 @@ const Thread = require('./thread');
const log = require('../util/log');
const maybeFormatMessage = require('../util/maybe-format-message');
const StageLayering = require('./stage-layering');
const Variable = require('./variable');
// Virtual I/O devices.
const Clock = require('../io/clock');
@ -22,6 +23,9 @@ const Mouse = require('../io/mouse');
const MouseWheel = require('../io/mouseWheel');
const Video = require('../io/video');
const StringUtil = require('../util/string-util');
const uid = require('../util/uid');
const defaultBlockPackages = {
scratch3_control: require('../blocks/scratch3_control'),
scratch3_event: require('../blocks/scratch3_event'),
@ -1793,6 +1797,25 @@ class Runtime extends EventEmitter {
return varNames;
}
/**
* Create a new global variable avoiding conflicts with other variable names.
* @param {string} variableName The desired variable name for the new global variable.
* This can be turned into a fresh name as necessary.
* @param {string} optVarId An optional ID to use for the variable. A new one will be generated
* if a falsey value for this parameter is provided.
* @param {string} optVarType The type of the variable to create. Defaults to Variable.SCALAR_TYPE.
* @return {Variable} The new variable that was created.
*/
createNewGlobalVariable (variableName, optVarId, optVarType) {
const varType = (typeof optVarType === 'string') ? optVarType : Variable.SCALAR_TYPE;
const allVariableNames = this.getAllVarNamesOfType(varType);
const newName = StringUtil.unusedName(variableName, allVariableNames);
const variable = new Variable(optVarId || uid(), newName, varType);
const stage = this.getTargetForStage();
stage.variables[variable.id] = variable;
return variable;
}
/**
* Tell the runtime to request a redraw.
* Use after a clone/sprite has completed some visible operation on the stage.

View file

@ -80,29 +80,6 @@ class Target extends EventEmitter {
return this.id;
}
/**
* 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 unless the skipStage flag is true.
* For the stage, this is all stage variables.
* @param {string} type The variable type to search for; defaults to Variable.SCALAR_TYPE
* @param {?bool} skipStage Optional flag to skip the stage.
* @return {Array<string>} A list of variable names
*/
getAllVariableNamesInScopeByType (type, skipStage) {
if (typeof type !== 'string') type = Variable.SCALAR_TYPE;
skipStage = skipStage || false;
const targetVariables = Object.values(this.variables)
.filter(v => v.type === type)
.map(variable => variable.name);
if (skipStage || 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.
@ -336,6 +313,210 @@ class Target extends EventEmitter {
}
}
/**
* Post/edit sprite info.
* @param {object} data An object with sprite info data to set.
* @abstract
*/
postSpriteInfo () {}
/**
* Retrieve custom state associated with this target and the provided state ID.
* @param {string} stateId - specify which piece of state to retrieve.
* @returns {*} the associated state, if any was found.
*/
getCustomState (stateId) {
return this._customState[stateId];
}
/**
* Store custom state associated with this target and the provided state ID.
* @param {string} stateId - specify which piece of state to store on this target.
* @param {*} newValue - the state value to store.
*/
setCustomState (stateId, newValue) {
this._customState[stateId] = newValue;
}
/**
* Call to destroy a target.
* @abstract
*/
dispose () {
this._customState = {};
}
// Variable Conflict Resolution Helpers
/**
* 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 unless the skipStage flag is true.
* For the stage, this is all stage variables.
* @param {string} type The variable type to search for; defaults to Variable.SCALAR_TYPE
* @param {?bool} skipStage Optional flag to skip the stage.
* @return {Array<string>} A list of variable names
*/
getAllVariableNamesInScopeByType (type, skipStage) {
if (typeof type !== 'string') type = Variable.SCALAR_TYPE;
skipStage = skipStage || false;
const targetVariables = Object.values(this.variables)
.filter(v => v.type === type)
.map(variable => variable.name);
if (skipStage || this.isStage || !this.runtime) {
return targetVariables;
}
const stage = this.runtime.getTargetForStage();
const stageVariables = stage.getAllVariableNamesInScopeByType(type);
return targetVariables.concat(stageVariables);
}
/**
* Merge variable references with another variable.
* @param {string} idToBeMerged ID of the variable whose references need to be updated
* @param {string} idToMergeWith ID of the variable that the old references should be replaced with
* @param {?Array<Object>} optReferencesToUpdate Optional context of the change.
* Defaults to all the blocks in this target.
* @param {?string} optNewName New variable name to merge with. The old
* variable name in the references being updated should be replaced with this new name.
* If this parameter is not provided or is '', no name change occurs.
*/
mergeVariables (idToBeMerged, idToMergeWith, optReferencesToUpdate, optNewName) {
const referencesToChange = optReferencesToUpdate ||
// TODO should there be a separate helper function that traverses the blocks
// for all references for a given ID instead of doing the below..?
this.blocks.getAllVariableAndListReferences()[idToBeMerged];
referencesToChange.map(ref => {
ref.referencingField.id = idToMergeWith;
if (optNewName) {
ref.referencingField.value = optNewName;
}
return ref;
});
}
/**
* Share a local variable (and given references for that variable) to the stage.
* @param {string} varId The ID of the variable to share.
* @param {Array<object>} varRefs The list of variable references being shared,
* that reference the given variable ID. The names and IDs of these variable
* references will be updated to refer to the new (or pre-existing) global variable.
*/
shareLocalVariableToStage (varId, varRefs) {
if (!this.runtime) return;
const variable = this.variables[varId];
if (!variable) {
log.warn(`Cannot share a local variable to the stage if it's not local.`);
return;
}
const stage = this.runtime.getTargetForStage();
// If a local var is being shared with the stage,
// sharing will make the variable global, resulting in a conflict
// with the existing local variable. Preemptively Resolve this conflict
// by renaming the new global variable.
// First check if we've already done the local to global transition for this
// variable. If we have, merge it with the global variable we've already created.
const varIdForStage = `StageVarFromLocal_${varId}`;
let stageVar = stage.lookupVariableById(varIdForStage);
// If a global var doesn't already exist, create a new one with a fresh name.
// Use the ID we created above so that we can lookup this new variable in the
// future if we decide to share this same variable again.
if (!stageVar) {
const varName = variable.name;
const varType = variable.type;
const newStageName = `Stage: ${varName}`;
stageVar = this.runtime.createNewGlobalVariable(newStageName, varIdForStage, varType);
}
// Update all variable references to use the new name and ID
this.mergeVariables(varId, stageVar.id, varRefs, stageVar.name);
}
/**
* Share a local variable with a sprite, merging with one of the same name and
* type if it already exists on the sprite, or create a new one.
* @param {string} varId Id of the variable to share
* @param {Target} sprite The sprite to share the variable with
* @param {Array<object>} varRefs A list of all the variable references currently being shared.
*/
shareLocalVariableToSprite (varId, sprite, varRefs) {
if (!this.runtime) return;
if (this.isStage) return;
const variable = this.variables[varId];
if (!variable) {
log.warn(`Tried to call 'shareLocalVariableToSprite' with a non-local variable.`);
return;
}
const varName = variable.name;
const varType = variable.type;
// Check if the receiving sprite already has a variable of the same name and type
// and use the existing variable, otherwise create a new one.
const existingLocalVar = sprite.lookupVariableByNameAndType(varName, varType);
let newVarId;
if (existingLocalVar) {
newVarId = existingLocalVar.id;
} else {
const newVar = new Variable(null, varName, varType);
newVarId = newVar.id;
sprite.variables[newVarId] = newVar;
}
// Merge with the local variable on the new sprite.
this.mergeVariables(varId, newVarId, varRefs);
}
/**
* Given a list of variable referencing fields, shares those variables with
* the target with the provided id, resolving any variable conflicts that arise
* using the following rules:
*
* If this target is the stage, exit. There are no conflicts that arise
* from sharing variables from the stage to another sprite. The variables
* already exist globally, so no further action is needed.
*
* If a variable being referenced is a global variable, do nothing. The
* global variable already exists so no further action is needed.
*
* If a variable being referenced is local, and
* 1) The receiving target is a sprite:
* create a new local variable or merge with an existing local variable
* of the same name and type. Update all the referencing fields
* for the original variable to reference the new variable.
* 2) The receiving target is the stage:
* Create a new global variable with a fresh name and update all the referencing
* fields to reference the new variable.
*
* @param {Array<object>} blocks The blocks containing
* potential conflicting references to variables.
* @param {Target} receivingTarget The target receiving the variables
*/
resolveVariableSharingConflictsWithTarget (blocks, receivingTarget) {
if (this.isStage) return;
// Get all the variable references in the given list of blocks
const allVarListRefs = this.blocks.getAllVariableAndListReferences(blocks);
// For all the variables being referenced, check for which ones are local
// to this target, and resolve conflicts based on whether the receiving target
// is a sprite (with a conflicting local variable) or whether it is
// the stage (which cannot have local variables)
for (const varId in allVarListRefs) {
const currVar = this.variables[varId];
if (!currVar) continue; // The current variable is global, there shouldn't be any conflicts here, skip it.
// Get the list of references for the current variable id
const currVarListRefs = allVarListRefs[varId];
if (receivingTarget.isStage) {
this.shareLocalVariableToStage(varId, currVarListRefs);
} else {
this.shareLocalVariableToSprite(varId, receivingTarget, currVarListRefs);
}
}
}
/**
* Fixes up variable references in this target avoiding conflicts with
* pre-existing variables in the same scope.
@ -344,13 +525,14 @@ class Target extends EventEmitter {
* 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
* If this target has a block that references an existing global variable and that
* variable *does not* exist in this target (e.g. it was a global variable in the
* project the sprite was originally exported from), merge the variables. This entails
* fixing the variable references in this sprite to reference the id of the pre-existing global variable.
*
* If this 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.
* 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.
*/
fixUpVariableReferences () {
@ -405,6 +587,8 @@ 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;
@ -422,47 +606,15 @@ class Target extends EventEmitter {
// Finally, handle global var conflicts (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
// existing global variable
for (const conflictId in conflictIdsToReplace) {
const existingId = conflictIdsToReplace[conflictId];
allReferences[conflictId].map(varRef => {
varRef.referencingField.id = existingId;
return varRef;
});
const referencesToUpdate = allReferences[conflictId];
this.mergeVariables(conflictId, existingId, referencesToUpdate);
}
}
/**
* Post/edit sprite info.
* @param {object} data An object with sprite info data to set.
* @abstract
*/
postSpriteInfo () {}
/**
* Retrieve custom state associated with this target and the provided state ID.
* @param {string} stateId - specify which piece of state to retrieve.
* @returns {*} the associated state, if any was found.
*/
getCustomState (stateId) {
return this._customState[stateId];
}
/**
* Store custom state associated with this target and the provided state ID.
* @param {string} stateId - specify which piece of state to store on this target.
* @param {*} newValue - the state value to store.
*/
setCustomState (stateId, newValue) {
this._customState[stateId] = newValue;
}
/**
* Call to destroy a target.
* @abstract
*/
dispose () {
this._customState = {};
}
}
module.exports = Target;

View file

@ -998,68 +998,18 @@ class VirtualMachine extends EventEmitter {
* workspace of the given target.
* @param {!Array<object>} blocks Blocks to add.
* @param {!string} targetId Id of target to add blocks to.
* @param {?string} optFromTargetId Optional target id indicating that blocks are being
* shared from that target. This is needed for resolving any potential variable conflicts.
*/
shareBlocksToTarget (blocks, targetId) {
shareBlocksToTarget (blocks, targetId, optFromTargetId) {
const copiedBlocks = JSON.parse(JSON.stringify(blocks));
const currTarget = this.runtime.getEditingTarget();
const target = this.runtime.getTargetById(targetId);
// Resolve any potential local variable conflicts if the current sprite (sharing the love)
// is a non-stage target.
if (!currTarget.isStage) {
const allVarListRefs = currTarget.blocks.getAllVariableAndListReferences(
copiedBlocks);
for (const varId in allVarListRefs) {
const currVarListRefs = allVarListRefs[varId];
const currVar = currTarget.variables[varId];
if (!currVar) continue; // If the currVar is global, skip it
const varName = currVar.name;
const varType = currVar.type;
let newVarId = '';
let existingVar;
if (target.isStage) {
// If a local var is being shared with the stage,
// we'll prefix the ID of the current variable,
// and use a fresh, prefixed name for the new global variable.
// This way, multiple share the loves from the same target, referring to
// the same variable will map to the same new variable on the stage.
const varIdForStage = `StageVarFromLocal_${varId}`;
existingVar = target.lookupVariableById(varIdForStage);
if (!existingVar) {
let allVarNames = [];
for (const t of this.runtime.targets) {
// TODO allVarNames will probably contain duplicates, should we filter them out
// or create a set instead?
allVarNames = allVarNames.concat(t.getAllVariableNamesInScopeByType(varType));
}
const newStageVarName = StringUtil.unusedName(`Stage: ${varName}`, allVarNames);
existingVar = new Variable(varIdForStage, newStageVarName, varType);
target.variables[varIdForStage] = existingVar;
}
// Update all variable references to use the new name
currVarListRefs.map(ref => {
const field = ref.referencingField;
field.value = existingVar.name;
field.id = existingVar.id;
return ref;
});
} else {
existingVar = target.lookupVariableByNameAndType(
varName, varType);
if (existingVar) {
newVarId = existingVar.id;
} else {
const newVar = new Variable(null, varName, varType);
newVarId = newVar.id;
target.variables[newVarId] = newVar;
}
// Update all the variable references to use the new ID
currVarListRefs.map(ref => {
ref.referencingField.id = newVarId;
return ref;
});
}
}
if (optFromTargetId) {
// If the blocks are being shared from another target,
// resolve any possible variable conflicts that may arise.
const fromTarget = this.runtime.getTargetById(optFromTargetId);
fromTarget.resolveVariableSharingConflictsWithTarget(copiedBlocks, target);
}
for (let i = 0; i < copiedBlocks.length; i++) {

View file

@ -693,7 +693,7 @@ test('shareBlocksToTarget shares global variables without any name changes', t =
t.type(stage.blocks.getBlock('a block'), 'undefined');
// Share the block to the stage
vm.shareBlocksToTarget([target.blocks.getBlock('a block')], stage.id);
vm.shareBlocksToTarget([target.blocks.getBlock('a block')], stage.id, target.id);
// Verify that the block now exists on the target as well as the stage
t.type(target.blocks.getBlock('a block'), 'object');
@ -752,7 +752,7 @@ test('shareBlocksToTarget shares a local variable to the stage, creating a globa
t.type(stage.blocks.getBlock('a block'), 'undefined');
// Share the block to the stage
vm.shareBlocksToTarget([target.blocks.getBlock('a block')], stage.id);
vm.shareBlocksToTarget([target.blocks.getBlock('a block')], stage.id, target.id);
// Verify that the block still exists on the target and remains unchanged
t.type(target.blocks.getBlock('a block'), 'object');
@ -826,7 +826,7 @@ test('shareBlocksToTarget chooses a fresh name for a new global variable checkin
otherTarget.createVariable('a different var', 'Stage: a mock variable', Variable.SCALAR_TYPE);
// Share the block to the stage
vm.shareBlocksToTarget([target.blocks.getBlock('a block')], stage.id);
vm.shareBlocksToTarget([target.blocks.getBlock('a block')], stage.id, target.id);
// Verify that the block still exists on the target and remains unchanged
t.type(target.blocks.getBlock('a block'), 'object');