Merge pull request scratchfoundation#1301 from kchadha/variable-scope

Variable scope

Author: kchadha

src/engine/blocks.js

@@ -263,6 +263,7 @@ class Blocks {
             return;
         }
         const stage = optRuntime.getTargetForStage();
+        const editingTarget = optRuntime.getEditingTarget();
 
         // UI event: clicked scripts toggle in the runtime.
         if (e.element === 'stackclick') {
@@ -330,35 +331,52 @@ class Blocks {
             this.deleteBlock(e.blockId);
             break;
         case 'var_create':
-            // New variables being created by the user are all global.
-            // Check if this variable exists on the current target or stage.
-            // If not, create it on the stage.
-            // TODO create global and local variables when UI provides a way.
-            if (optRuntime.getEditingTarget()) {
-                if (!optRuntime.getEditingTarget().lookupVariableById(e.varId)) {
-                    stage.createVariable(e.varId, e.varName, e.varType);
+            // Check if the variable being created is global or local
+            // If local, create a local var on the current editing target, as long
+            // as there are no conflicts, and the current target is actually a sprite
+            // If global or if the editing target is not present or we somehow got
+            // into a state where a local var was requested for the stage,
+            // create a stage (global) var after checking for name conflicts
+            // on all the sprites.
+            if (e.isLocal && editingTarget && !editingTarget.isStage) {
+                if (!editingTarget.lookupVariableById(e.varId)) {
+                    editingTarget.createVariable(e.varId, e.varName, e.varType);
+                }
+            } else {
+                // Check for name conflicts in all of the targets
+                const allTargets = optRuntime.targets.filter(t => t.isOriginal);
+                for (const target of allTargets) {
+                    if (target.lookupVariableByNameAndType(e.varName, e.varType, true)) {
+                        return;
+                    }
                 }
-            } else if (!stage.lookupVariableById(e.varId)) {
-                // Since getEditingTarget returned null, we now need to
-                // explicitly check if the stage has the variable, and
-                // create one if not.
                 stage.createVariable(e.varId, e.varName, e.varType);
             }
             break;
         case 'var_rename':
-            stage.renameVariable(e.varId, e.newName);
-            // Update all the blocks that use the renamed variable.
-            if (optRuntime) {
+            if (editingTarget && editingTarget.hasOwnProperty(e.varId)) {
+                // This is a local variable, rename on the current target
+                editingTarget.renameVariable(e.varId, e.newName);
+                // Update all the blocks on the current target that use
+                // this variable
+                editingTarget.blocks.updateBlocksAfterVarRename(e.varId, e.newName);
+            } else {
+                // This is a global variable
+                stage.renameVariable(e.varId, e.newName);
+                // Update all blocks on all targets that use the renamed variable
                 const targets = optRuntime.targets;
                 for (let i = 0; i < targets.length; i++) {
                     const currTarget = targets[i];
                     currTarget.blocks.updateBlocksAfterVarRename(e.varId, e.newName);
                 }
             }
             break;
-        case 'var_delete':
-            stage.deleteVariable(e.varId);
+        case 'var_delete': {
+            const target = (editingTarget && editingTarget.hasOwnProperty(e.varId)) ?
+                editingTarget : stage;
+            target.deleteVariable(e.varId);
             break;
+        }
         case 'comment_create':
             if (optRuntime && optRuntime.getEditingTarget()) {
                 const currTarget = optRuntime.getEditingTarget();

src/engine/runtime.js

@@ -1715,6 +1715,15 @@ class Runtime extends EventEmitter {
         return this._editingTarget;
     }
 
+    getAllVarNamesOfType (varType) {
+        let varNames = [];
+        for (const target of this.targets) {
+            const targetVarNames = target.getAllVariableNamesInScopeByType(varType, true);
+            varNames = varNames.concat(targetVarNames);
+        }
+        return varNames;
+    }
+
     /**
      * Tell the runtime to request a redraw.
      * Use after a clone/sprite has completed some visible operation on the stage.

src/engine/target.js

@@ -83,17 +83,19 @@ class Target extends EventEmitter {
     /**
      * 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.
+     * 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) {
+    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 (this.isStage || !this.runtime) {
+        if (skipStage || this.isStage || !this.runtime) {
             return targetVariables;
         }
         const stage = this.runtime.getTargetForStage();
@@ -194,11 +196,13 @@ class Target extends EventEmitter {
      * was not found.
      * @param {string} name Name of the variable.
      * @param {string} type Type of the variable. Defaults to Variable.SCALAR_TYPE.
+     * @param {?bool} skipStage Optional flag to skip checking the stage
      * @return {?Variable} Variable object if found, or null if not.
      */
-    lookupVariableByNameAndType (name, type) {
+    lookupVariableByNameAndType (name, type, skipStage) {
         if (typeof name !== 'string') return;
         if (typeof type !== 'string') type = Variable.SCALAR_TYPE;
+        skipStage = skipStage || false;
 
         for (const varId in this.variables) {
             const currVar = this.variables[varId];
@@ -207,7 +211,7 @@ class Target extends EventEmitter {
             }
         }
 
-        if (this.runtime && !this.isStage) {
+        if (!skipStage && this.runtime && !this.isStage) {
             const stage = this.runtime.getTargetForStage();
             if (stage) {
                 for (const varId in stage.variables) {

src/engine/variable.js

@@ -33,8 +33,9 @@ class Variable {
         }
     }
 
-    toXML () {
-        return `<variable type="${this.type}" id="${this.id}">${this.name}</variable>`;
+    toXML (isLocal) {
+        isLocal = (isLocal === true);
+        return `<variable type="${this.type}" id="${this.id}" islocal="${isLocal}">${this.name}</variable>`;
     }
 
     /**

src/virtual-machine.js

@@ -1074,19 +1074,21 @@ class VirtualMachine extends EventEmitter {
             const id = messageIds[i];
             delete this.runtime.getTargetForStage().variables[id];
         }
-        const variableMap = Object.assign({},
-            this.runtime.getTargetForStage().variables,
-            this.editingTarget.variables
-        );
+        const globalVarMap = Object.assign({}, this.runtime.getTargetForStage().variables);
+        const localVarMap = this.editingTarget.isStage ?
+            Object.create(null) :
+            Object.assign({}, this.editingTarget.variables);
 
-        const variables = Object.keys(variableMap).map(k => variableMap[k]);
+        const globalVariables = Object.keys(globalVarMap).map(k => globalVarMap[k]);
+        const localVariables = Object.keys(localVarMap).map(k => localVarMap[k]);
         const workspaceComments = Object.keys(this.editingTarget.comments)
             .map(k => this.editingTarget.comments[k])
             .filter(c => c.blockId === null);
 
         const xmlString = `<xml xmlns="http://www.w3.org/1999/xhtml">
                             <variables>
-                                ${variables.map(v => v.toXML()).join()}
+                                ${globalVariables.map(v => v.toXML()).join()}
+                                ${localVariables.map(v => v.toXML(true)).join()}
                             </variables>
                             ${workspaceComments.map(c => c.toXML()).join()}
                             ${this.editingTarget.blocks.toXML(this.editingTarget.comments)}