From bc1da9fa449610824201802984be6e9de4e32c5b Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Mon, 5 Nov 2018 12:57:09 -0500 Subject: [PATCH 1/4] Move extension ID parsing into a helper and add test --- src/serialization/sb3.js | 30 +++++++++++++++++++++--------- test/unit/serialization_sb3.js | 11 +++++++++++ 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js index e3433258b..852c696dc 100644 --- a/src/serialization/sb3.js +++ b/src/serialization/sb3.js @@ -271,6 +271,19 @@ const compressInputTree = function (block, blocks) { return block; }; +/** + * Get non-core extension ID for a given sb3 opcode. + * @param {!string} opcode The opcode to examine for extension. + * @return {?string} The extension ID, if it exists/is not a core extension. + */ +const getExtensionIdForOpcode = function (opcode) { + const index = opcode.indexOf('_'); + const prefix = opcode.substring(0, index); + if (CORE_EXTENSIONS.indexOf(prefix) === -1) { + if (prefix !== '') return prefix; + } +}; + /** * Serialize the given blocks object (representing all the blocks for the target * currently being serialized.) @@ -285,10 +298,9 @@ const serializeBlocks = function (blocks) { for (const blockID in blocks) { if (!blocks.hasOwnProperty(blockID)) continue; obj[blockID] = serializeBlock(blocks[blockID], blocks); - const index = blocks[blockID].opcode.indexOf('_'); - const prefix = blocks[blockID].opcode.substring(0, index); - if (CORE_EXTENSIONS.indexOf(prefix) === -1) { - if (prefix !== '') extensionIDs.add(prefix); + const extensionID = getExtensionIdForOpcode(blocks[blockID].opcode); + if (extensionID) { + extensionIDs.add(extensionID); } } // once we have completed a first pass, do a second pass on block inputs @@ -812,10 +824,9 @@ const parseScratchObject = function (object, runtime, extensions, zip) { blocks.createBlock(blockJSON); // If the block is from an extension, record it. - const index = blockJSON.opcode.indexOf('_'); - const prefix = blockJSON.opcode.substring(0, index); - if (CORE_EXTENSIONS.indexOf(prefix) === -1) { - if (prefix !== '') extensions.extensionIDs.add(prefix); + const extensionID = getExtensionIdForOpcode(blockJSON.opcode); + if (extensionID) { + extensions.extensionIDs.add(extensionID); } } } @@ -1049,5 +1060,6 @@ module.exports = { serialize: serialize, deserialize: deserialize, deserializeBlocks: deserializeBlocks, - serializeBlocks: serializeBlocks + serializeBlocks: serializeBlocks, + getExtensionIdForOpcode: getExtensionIdForOpcode }; diff --git a/test/unit/serialization_sb3.js b/test/unit/serialization_sb3.js index 185cb87aa..4e85cb587 100644 --- a/test/unit/serialization_sb3.js +++ b/test/unit/serialization_sb3.js @@ -221,3 +221,14 @@ test('deserializeBlocks on already deserialized input', t => { t.end(); }); }); + +test('getExtensionIdForOpcode', t => { + t.equal(sb3.getExtensionIdForOpcode('wedo_loopy'), 'wedo'); + + // does not consider CORE to be extensions + t.false(sb3.getExtensionIdForOpcode('control_loopy')); + + // only considers things before the first underscore + t.equal(sb3.getExtensionIdForOpcode('hello_there_loopy'), 'hello'); + t.end(); +}); From c8ad1955d4fd3d0c896695f35a4c142158973ce9 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Mon, 5 Nov 2018 13:00:35 -0500 Subject: [PATCH 2/4] Load extensions before sharing blocks. This commit fixes the unit tests so the assertions are made after the promise resolves --- src/virtual-machine.js | 23 +++++- test/unit/virtual-machine.js | 141 ++++++++++++++++++----------------- 2 files changed, 90 insertions(+), 74 deletions(-) diff --git a/src/virtual-machine.js b/src/virtual-machine.js index 1aaae9578..764242d31 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -1107,6 +1107,7 @@ class VirtualMachine extends EventEmitter { * @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. + * @return {!Promise} Promise that resolves when the extensions and blocks have been added. */ shareBlocksToTarget (blocks, targetId, optFromTargetId) { const copiedBlocks = JSON.parse(JSON.stringify(blocks)); @@ -1119,10 +1120,24 @@ class VirtualMachine extends EventEmitter { fromTarget.resolveVariableSharingConflictsWithTarget(copiedBlocks, target); } - for (let i = 0; i < copiedBlocks.length; i++) { - target.blocks.createBlock(copiedBlocks[i]); - } - target.blocks.updateTargetSpecificBlocks(target.isStage); + // Create a unique set of extensionIds that are not yet loaded + const extensionIDs = new Set(copiedBlocks + .map(b => sb3.getExtensionIdForOpcode(b.opcode)) + .filter(id => !!id) // Remove ids that do not exist + .filter(id => !this.extensionManager.isExtensionLoaded(id)) // and remove loaded extensions + ); + + // Create an array promises for extensions to load + const extensionPromises = Array.from(extensionIDs, + id => this.extensionManager.loadExtensionURL(id) + ); + + return Promise.all(extensionPromises).then(() => { + copiedBlocks.forEach(block => { + target.blocks.createBlock(block); + }); + target.blocks.updateTargetSpecificBlocks(target.isStage); + }); } /** diff --git a/test/unit/virtual-machine.js b/test/unit/virtual-machine.js index 05926f521..b93ccc351 100644 --- a/test/unit/virtual-machine.js +++ b/test/unit/virtual-machine.js @@ -744,28 +744,29 @@ 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, target.id); + vm.shareBlocksToTarget([target.blocks.getBlock('a block')], stage.id, target.id).then(() => { - // Verify that the block now exists on the target as well as the stage - 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'); + // Verify that the block now exists on the target as well as the stage + 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'); - t.type(stage.blocks.getBlock('a block'), 'object'); - t.type(stage.blocks.getBlock('a block').fields, 'object'); - t.type(stage.blocks.getBlock('a block').fields.VARIABLE, 'object'); - t.equal(stage.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id'); + t.type(stage.blocks.getBlock('a block'), 'object'); + t.type(stage.blocks.getBlock('a block').fields, 'object'); + t.type(stage.blocks.getBlock('a block').fields.VARIABLE, 'object'); + t.equal(stage.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id'); - // Verify that the variables haven't changed, the variable still exists on the - // stage, it should still have the same name and value, and there should be - // no variables on the target. - t.equal(Object.keys(target.variables).length, 0); - t.equal(Object.keys(stage.variables).length, 1); - t.equal(stage.variables['mock var id'].name, 'a mock variable'); - t.equal(vm.getVariableValue(stage.id, 'mock var id'), 10); + // Verify that the variables haven't changed, the variable still exists on the + // stage, it should still have the same name and value, and there should be + // no variables on the target. + t.equal(Object.keys(target.variables).length, 0); + t.equal(Object.keys(stage.variables).length, 1); + t.equal(stage.variables['mock var id'].name, 'a mock variable'); + t.equal(vm.getVariableValue(stage.id, 'mock var id'), 10); - t.end(); + t.end(); + }); }); test('shareBlocksToTarget shares a local variable to the stage, creating a global variable with a new name', t => { @@ -803,36 +804,36 @@ 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, target.id); + vm.shareBlocksToTarget([target.blocks.getBlock('a block')], stage.id, target.id).then(() => { + // Verify that the block still exists on the target and remains unchanged + 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'); - // Verify that the block still exists on the target and remains unchanged - 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'); + t.type(stage.blocks.getBlock('a block'), 'object'); + t.type(stage.blocks.getBlock('a block').fields, 'object'); + t.type(stage.blocks.getBlock('a block').fields.VARIABLE, 'object'); + t.equal(stage.blocks.getBlock('a block').fields.VARIABLE.id, 'StageVarFromLocal_mock var id'); - t.type(stage.blocks.getBlock('a block'), 'object'); - t.type(stage.blocks.getBlock('a block').fields, 'object'); - t.type(stage.blocks.getBlock('a block').fields.VARIABLE, 'object'); - t.equal(stage.blocks.getBlock('a block').fields.VARIABLE.id, 'StageVarFromLocal_mock var id'); + // Verify that a new global variable was created, the old one still exists on + // the target and still has the same name and value, and the new one has + // a new name and value 0. + t.equal(Object.keys(target.variables).length, 1); + t.equal(target.variables['mock var id'].name, 'a mock variable'); + t.equal(vm.getVariableValue(target.id, 'mock var id'), 10); - // Verify that a new global variable was created, the old one still exists on - // the target and still has the same name and value, and the new one has - // a new name and value 0. - t.equal(Object.keys(target.variables).length, 1); - t.equal(target.variables['mock var id'].name, 'a mock variable'); - t.equal(vm.getVariableValue(target.id, 'mock var id'), 10); + // Verify that a new variable was created on the stage, with a new name and new id + t.equal(Object.keys(stage.variables).length, 1); + t.type(stage.variables['mock var id'], 'undefined'); + const newGlobalVar = Object.values(stage.variables)[0]; + t.equal(newGlobalVar.name, 'Stage: a mock variable'); + const newId = newGlobalVar.id; + t.notEqual(newId, 'mock var id'); + t.equals(vm.getVariableValue(stage.id, newId), 0); - // Verify that a new variable was created on the stage, with a new name and new id - t.equal(Object.keys(stage.variables).length, 1); - t.type(stage.variables['mock var id'], 'undefined'); - const newGlobalVar = Object.values(stage.variables)[0]; - t.equal(newGlobalVar.name, 'Stage: a mock variable'); - const newId = newGlobalVar.id; - t.notEqual(newId, 'mock var id'); - t.equals(vm.getVariableValue(stage.id, newId), 0); - - t.end(); + t.end(); + }); }); test('shareBlocksToTarget chooses a fresh name for a new global variable checking for conflicts on all sprites', t => { @@ -877,36 +878,36 @@ 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, target.id); + vm.shareBlocksToTarget([target.blocks.getBlock('a block')], stage.id, target.id).then(() => { + // Verify that the block still exists on the target and remains unchanged + 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'); - // Verify that the block still exists on the target and remains unchanged - 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'); + t.type(stage.blocks.getBlock('a block'), 'object'); + t.type(stage.blocks.getBlock('a block').fields, 'object'); + t.type(stage.blocks.getBlock('a block').fields.VARIABLE, 'object'); + t.equal(stage.blocks.getBlock('a block').fields.VARIABLE.id, 'StageVarFromLocal_mock var id'); - t.type(stage.blocks.getBlock('a block'), 'object'); - t.type(stage.blocks.getBlock('a block').fields, 'object'); - t.type(stage.blocks.getBlock('a block').fields.VARIABLE, 'object'); - t.equal(stage.blocks.getBlock('a block').fields.VARIABLE.id, 'StageVarFromLocal_mock var id'); + // Verify that a new global variable was created, the old one still exists on + // the target and still has the same name and value, and the new one has + // a new name and value 0. + t.equal(Object.keys(target.variables).length, 1); + t.equal(target.variables['mock var id'].name, 'a mock variable'); + t.equal(vm.getVariableValue(target.id, 'mock var id'), 10); - // Verify that a new global variable was created, the old one still exists on - // the target and still has the same name and value, and the new one has - // a new name and value 0. - t.equal(Object.keys(target.variables).length, 1); - t.equal(target.variables['mock var id'].name, 'a mock variable'); - t.equal(vm.getVariableValue(target.id, 'mock var id'), 10); + // Verify that a new variable was created on the stage, with a new name and new id + t.equal(Object.keys(stage.variables).length, 1); + t.type(stage.variables['mock var id'], 'undefined'); + const newGlobalVar = Object.values(stage.variables)[0]; + t.equal(newGlobalVar.name, 'Stage: a mock variable2'); + const newId = newGlobalVar.id; + t.notEqual(newId, 'mock var id'); + t.equals(vm.getVariableValue(stage.id, newId), 0); - // Verify that a new variable was created on the stage, with a new name and new id - t.equal(Object.keys(stage.variables).length, 1); - t.type(stage.variables['mock var id'], 'undefined'); - const newGlobalVar = Object.values(stage.variables)[0]; - t.equal(newGlobalVar.name, 'Stage: a mock variable2'); - const newId = newGlobalVar.id; - t.notEqual(newId, 'mock var id'); - t.equals(vm.getVariableValue(stage.id, newId), 0); - - t.end(); + t.end(); + }); }); test('Setting turbo mode emits events', t => { From 979ed66594ed52f8c2656a6748d2d6037f8f98d9 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Mon, 5 Nov 2018 13:01:00 -0500 Subject: [PATCH 3/4] Add a unit test to ensure extensions are loaded on shareBlocksToTarget --- test/unit/virtual-machine.js | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/unit/virtual-machine.js b/test/unit/virtual-machine.js index b93ccc351..371cd96cf 100644 --- a/test/unit/virtual-machine.js +++ b/test/unit/virtual-machine.js @@ -910,6 +910,35 @@ test('shareBlocksToTarget chooses a fresh name for a new global variable checkin }); }); +test('shareBlocksToTarget loads extensions that have not yet been loaded', t => { + const vm = new VirtualMachine(); + const runtime = vm.runtime; + const spr1 = new Sprite(null, runtime); + const stage = spr1.createClone(); + runtime.targets = [stage]; + + const fakeBlocks = [ + {opcode: 'loaded_fakeblock'}, + {opcode: 'notloaded_fakeblock'} + ]; + + // Stub the extension manager + const loadedIds = []; + vm.extensionManager = { + isExtensionLoaded: id => id === 'loaded', + loadExtensionURL: id => new Promise(resolve => { + loadedIds.push(id); + resolve(); + }) + }; + + vm.shareBlocksToTarget(fakeBlocks, stage.id).then(() => { + // Verify that only the not-loaded extension gets loaded + t.deepEqual(loadedIds, ['notloaded']); + t.end(); + }); +}); + test('Setting turbo mode emits events', t => { let turboMode = null; From ce9414405198938fe1ee9198d382bcaaa46771d9 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Wed, 7 Nov 2018 10:11:16 -0500 Subject: [PATCH 4/4] Fixup from comments --- src/serialization/sb3.js | 2 +- test/unit/serialization_sb3.js | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js index 852c696dc..668305bcf 100644 --- a/src/serialization/sb3.js +++ b/src/serialization/sb3.js @@ -274,7 +274,7 @@ const compressInputTree = function (block, blocks) { /** * Get non-core extension ID for a given sb3 opcode. * @param {!string} opcode The opcode to examine for extension. - * @return {?string} The extension ID, if it exists/is not a core extension. + * @return {?string} The extension ID, if it exists and is not a core extension. */ const getExtensionIdForOpcode = function (opcode) { const index = opcode.indexOf('_'); diff --git a/test/unit/serialization_sb3.js b/test/unit/serialization_sb3.js index 4e85cb587..caf31e2a8 100644 --- a/test/unit/serialization_sb3.js +++ b/test/unit/serialization_sb3.js @@ -230,5 +230,9 @@ test('getExtensionIdForOpcode', t => { // only considers things before the first underscore t.equal(sb3.getExtensionIdForOpcode('hello_there_loopy'), 'hello'); + + // does not return anything for opcodes with no extension + t.false(sb3.getExtensionIdForOpcode('hello')); + t.end(); });