From 34b0aff6370bf54bbefa24e25745a633796bb0ba Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Mon, 4 Dec 2017 18:01:29 -0500 Subject: [PATCH] Bugfix for scratch-gui issue #994, where executing a broadcast block from the flyout was creating a conflicting variable, causing a fatal error. --- src/blocks/scratch3_event.js | 50 +++++++++++++++++++----------------- src/engine/target.js | 23 +++++++++++------ test/unit/blocks_event.js | 6 +++-- test/unit/engine_target.js | 17 ++---------- 4 files changed, 48 insertions(+), 48 deletions(-) diff --git a/src/blocks/scratch3_event.js b/src/blocks/scratch3_event.js index 5fbaa5b7d..5ee2fab1d 100644 --- a/src/blocks/scratch3_event.js +++ b/src/blocks/scratch3_event.js @@ -56,36 +56,40 @@ class Scratch3EventBlocks { } broadcast (args, util) { - const broadcastVar = util.runtime.getTargetForStage().lookupOrCreateBroadcastMsg( + const broadcastVar = util.runtime.getTargetForStage().lookupBroadcastMsg( args.BROADCAST_OPTION.id, args.BROADCAST_OPTION.name); - const broadcastOption = broadcastVar.name; - util.startHats('event_whenbroadcastreceived', { - BROADCAST_OPTION: broadcastOption - }); + if (broadcastVar) { + const broadcastOption = broadcastVar.name; + util.startHats('event_whenbroadcastreceived', { + BROADCAST_OPTION: broadcastOption + }); + } } broadcastAndWait (args, util) { - const broadcastVar = util.runtime.getTargetForStage().lookupOrCreateBroadcastMsg( + const broadcastVar = util.runtime.getTargetForStage().lookupBroadcastMsg( args.BROADCAST_OPTION.id, args.BROADCAST_OPTION.name); - const broadcastOption = broadcastVar.name; - // Have we run before, starting threads? - if (!util.stackFrame.startedThreads) { - // No - start hats for this broadcast. - util.stackFrame.startedThreads = util.startHats( - 'event_whenbroadcastreceived', { - BROADCAST_OPTION: broadcastOption + if (broadcastVar) { + const broadcastOption = broadcastVar.name; + // Have we run before, starting threads? + if (!util.stackFrame.startedThreads) { + // No - start hats for this broadcast. + util.stackFrame.startedThreads = util.startHats( + 'event_whenbroadcastreceived', { + BROADCAST_OPTION: broadcastOption + } + ); + if (util.stackFrame.startedThreads.length === 0) { + // Nothing was started. + return; } - ); - if (util.stackFrame.startedThreads.length === 0) { - // Nothing was started. - return; } - } - // We've run before; check if the wait is still going on. - const instance = this; - const waiting = util.stackFrame.startedThreads.some(thread => instance.runtime.isActiveThread(thread)); - if (waiting) { - util.yield(); + // We've run before; check if the wait is still going on. + const instance = this; + const waiting = util.stackFrame.startedThreads.some(thread => instance.runtime.isActiveThread(thread)); + if (waiting) { + util.yield(); + } } } } diff --git a/src/engine/target.js b/src/engine/target.js index 39273fcc2..4e574177e 100644 --- a/src/engine/target.js +++ b/src/engine/target.js @@ -4,6 +4,7 @@ const Blocks = require('./blocks'); const Variable = require('../engine/variable'); const uid = require('../util/uid'); const {Map} = require('immutable'); +const log = require('../util/log'); /** * @fileoverview @@ -93,19 +94,25 @@ class Target extends EventEmitter { } /** - * Look up a broadcast message object, and create it if one doesn't exist. + * Look up a broadcast message object with the given id and return it + * if it exists. * @param {string} id Id of the variable. * @param {string} name Name of the variable. * @return {!Variable} Variable object. */ - lookupOrCreateBroadcastMsg (id, name) { + lookupBroadcastMsg (id, name) { const broadcastMsg = this.lookupVariableById(id); - if (broadcastMsg) return broadcastMsg; - // No variable with this name exists - create it locally. - const newBroadcastMsg = new Variable(id, name, - Variable.BROADCAST_MESSAGE_TYPE, false); - this.variables[id] = newBroadcastMsg; - return newBroadcastMsg; + if (broadcastMsg) { + if (broadcastMsg.name !== name) { + log.error(`Found broadcast message with id: ${id}, but` + + `its name, ${broadcastMsg.name} did not match expected name ${name}.`); + } + if (broadcastMsg.type !== Variable.BROADCAST_MESSAGE_TYPE) { + log.error(`Found variable with id: ${id}, but its type ${broadcastMsg.type}` + + `did not match expected type ${Variable.BROADCAST_MESSAGE_TYPE}`); + } + return broadcastMsg; + } } /** diff --git a/test/unit/blocks_event.js b/test/unit/blocks_event.js index 35a1f0b4f..672ea332c 100644 --- a/test/unit/blocks_event.js +++ b/test/unit/blocks_event.js @@ -6,6 +6,7 @@ const Event = require('../../src/blocks/scratch3_event'); const Runtime = require('../../src/engine/runtime'); const Target = require('../../src/engine/target'); const Thread = require('../../src/engine/thread'); +const Variable = require('../../src/engine/variable'); test('#760 - broadcastAndWait', t => { const broadcastAndWaitBlock = { @@ -52,6 +53,7 @@ test('#760 - broadcastAndWait', t => { b.createBlock(receiveMessageBlock); const tgt = new Target(rt, b); tgt.isStage = true; + tgt.createVariable('testBroadcastID', 'message', Variable.BROADCAST_MESSAGE_TYPE); rt.targets.push(tgt); let th = rt._pushThread('broadcastAndWaitBlock', t); @@ -67,12 +69,12 @@ test('#760 - broadcastAndWait', t => { // yields when some thread is active t.strictEqual(th.status, Thread.STATUS_YIELD); th.status = Thread.STATUS_RUNNING; - e.broadcastAndWait({BROADCAST_OPTION: 'message'}, util); + e.broadcastAndWait({BROADCAST_OPTION: {id: 'testBroadcastID', name: 'message'}}, util); t.strictEqual(th.status, Thread.STATUS_YIELD); // does not yield once all threads are done th.status = Thread.STATUS_RUNNING; rt.threads[1].status = Thread.STATUS_DONE; - e.broadcastAndWait({BROADCAST_OPTION: 'message'}, util); + e.broadcastAndWait({BROADCAST_OPTION: {id: 'testBroadcastID', name: 'message'}}, util); t.strictEqual(th.status, Thread.STATUS_RUNNING); // restarts done threads that are in runtime threads diff --git a/test/unit/engine_target.js b/test/unit/engine_target.js index 4f663906d..30ff5eb82 100644 --- a/test/unit/engine_target.js +++ b/test/unit/engine_target.js @@ -174,20 +174,7 @@ test('lookupOrCreateList returns list if one with given id exists', t => { t.end(); }); -test('lookupOrCreateBroadcastMsg creates a var if one does not exist', t => { - const target = new Target(); - const variables = target.variables; - - t.equal(Object.keys(variables).length, 0); - const broadcastVar = target.lookupOrCreateBroadcastMsg('foo', 'bar'); - t.equal(Object.keys(variables).length, 1); - t.equal(broadcastVar.id, 'foo'); - t.equal(broadcastVar.name, 'bar'); - - t.end(); -}); - -test('lookupOrCreateBroadcastMsg returns the var with given id if exists', t => { +test('lookupBroadcastMsg returns the var with given id if exists', t => { const target = new Target(); const variables = target.variables; @@ -195,7 +182,7 @@ test('lookupOrCreateBroadcastMsg returns the var with given id if exists', t => target.createVariable('foo', 'bar', Variable.BROADCAST_MESSAGE_TYPE); t.equal(Object.keys(variables).length, 1); - const broadcastMsg = target.lookupOrCreateBroadcastMsg('foo', 'bar'); + const broadcastMsg = target.lookupBroadcastMsg('foo', 'bar'); t.equal(Object.keys(variables).length, 1); t.equal(broadcastMsg.id, 'foo'); t.equal(broadcastMsg.name, 'bar');