Support LOOP and CONDITIONAL blocks in extensions

A LOOP block is like a conditional, but the LOOP block will be
re-evaluated after any child branch runs.

Also:
- Support using '---' as a block separator
- Refactor common code from `_registerExtensionPrimitives` and
  `_refreshExtensionPrimitives` into new `_fillExtensionCategory`
- Improve error reporting during block conversion

src/engine/runtime.js

@@ -8,6 +8,7 @@ const BlockType = require('../extension-support/block-type');
 const Sequencer = require('./sequencer');
 const Thread = require('./thread');
 const Profiler = require('./profiler');
+const log = require('../util/log');
 
 // Virtual I/O devices.
 const Clock = require('../io/clock');
@@ -55,6 +56,16 @@ const ArgumentTypeMap = (() => {
     return map;
 })();
 
+/**
+ * Predefined "Converted block info" for a separator between blocks in a block category
+ * @type {ConvertedBlockInfo}
+ */
+const ConvertedSeparator = {
+    info: {},
+    json: null,
+    xml: '<sep gap="36"/>'
+};
+
 /**
  * These constants are copied from scratch-blocks/core/constants.js
  * @TODO find a way to require() these... maybe make a scratch-blocks/dist/constants.js or something like that?
@@ -486,7 +497,7 @@ class Runtime extends EventEmitter {
 
     /**
      * Register the primitives provided by an extension.
-     * @param {ExtensionInfo} extensionInfo - information about the extension (id, blocks, etc.)
+     * @param {ExtensionMetadata} extensionInfo - information about the extension (id, blocks, etc.)
      * @private
      */
     _registerExtensionPrimitives (extensionInfo) {
@@ -504,30 +515,14 @@ class Runtime extends EventEmitter {
 
         this._blockInfo.push(categoryInfo);
 
-        for (const menuName in extensionInfo.menus) {
+        this._fillExtensionCategory(categoryInfo, extensionInfo);
-            if (extensionInfo.menus.hasOwnProperty(menuName)) {
-                const menuItems = extensionInfo.menus[menuName];
-                const convertedMenu = this._buildMenuForScratchBlocks(menuName, menuItems, categoryInfo);
-                categoryInfo.menus.push(convertedMenu);
-            }
-        }
-        for (const blockInfo of extensionInfo.blocks) {
-            const convertedBlock = this._convertForScratchBlocks(blockInfo, categoryInfo);
-            const opcode = convertedBlock.json.type;
-            categoryInfo.blocks.push(convertedBlock);
-            this._primitives[opcode] = convertedBlock.info.func;
-            if (blockInfo.blockType === BlockType.HAT) {
-                this._hats[opcode] = {edgeActivated: true}; /** @TODO let extension specify this */
-            }
-        }
 
         this.emit(Runtime.EXTENSION_ADDED, categoryInfo.blocks.concat(categoryInfo.menus));
     }
 
     /**
      * Reregister the primitives for an extension
-     * @param  {ExtensionInfo} extensionInfo - new info (results of running getInfo)
+     * @param  {ExtensionMetadata} extensionInfo - new info (results of running getInfo) for an extension
-     *                                         for an extension
      * @private
      */
     _refreshExtensionPrimitives (extensionInfo) {
@@ -536,22 +531,7 @@ class Runtime extends EventEmitter {
             if (extensionInfo.id === categoryInfo.id) {
                 categoryInfo.blocks = [];
                 categoryInfo.menus = [];
-                for (const menuName in extensionInfo.menus) {
+                this._fillExtensionCategory(categoryInfo, extensionInfo);
-                    if (extensionInfo.menus.hasOwnProperty(menuName)) {
-                        const menuItems = extensionInfo.menus[menuName];
-                        const convertedMenu = this._buildMenuForScratchBlocks(menuName, menuItems, categoryInfo);
-                        categoryInfo.menus.push(convertedMenu);
-                    }
-                }
-                for (const blockInfo of extensionInfo.blocks) {
-                    const convertedBlock = this._convertForScratchBlocks(blockInfo, categoryInfo);
-                    const opcode = convertedBlock.json.type;
-                    categoryInfo.blocks.push(convertedBlock);
-                    this._primitives[opcode] = convertedBlock.info.func;
-                    if (blockInfo.blockType === BlockType.HAT) {
-                        this._hats[opcode] = {edgeActivated: true}; /** @TODO let extension specify this */
-                    }
-                }
                 extensionBlocks = extensionBlocks.concat(categoryInfo.blocks, categoryInfo.menus);
             }
         }
@@ -559,6 +539,40 @@ class Runtime extends EventEmitter {
         this.emit(Runtime.BLOCKSINFO_UPDATE, extensionBlocks);
     }
 
+    /**
+     * Read extension information, convert menus and blocks, and store the results in the provided category object.
+     * @param {CategoryInfo} categoryInfo - the category to be filled
+     * @param {ExtensionMetadata} extensionInfo - the extension metadata to read
+     * @private
+     */
+    _fillExtensionCategory (categoryInfo, extensionInfo) {
+        for (const menuName in extensionInfo.menus) {
+            if (extensionInfo.menus.hasOwnProperty(menuName)) {
+                const menuItems = extensionInfo.menus[menuName];
+                const convertedMenu = this._buildMenuForScratchBlocks(menuName, menuItems, categoryInfo);
+                categoryInfo.menus.push(convertedMenu);
+            }
+        }
+        for (const blockInfo of extensionInfo.blocks) {
+            if (blockInfo === '---') {
+                categoryInfo.blocks.push(ConvertedSeparator);
+                continue;
+            }
+            try {
+                const convertedBlock = this._convertForScratchBlocks(blockInfo, categoryInfo);
+                const opcode = convertedBlock.json.type;
+                categoryInfo.blocks.push(convertedBlock);
+                this._primitives[opcode] = convertedBlock.info.func;
+                if (blockInfo.blockType === BlockType.HAT) {
+                    this._hats[opcode] = {edgeActivated: true};
+                    /** @TODO let extension specify this */
+                }
+            } catch (e) {
+                log.error('Error parsing block: ', {block: blockInfo, error: e});
+            }
+        }
+    }
+
     /**
      * Build the scratch-blocks JSON for a menu. Note that scratch-blocks treats menus as a special kind of block.
      * @param {string} menuName - the name of the menu
@@ -609,11 +623,12 @@ class Runtime extends EventEmitter {
      * Convert BlockInfo into scratch-blocks JSON & XML, and generate a proxy function.
      * @param {BlockInfo} blockInfo - the block to convert
      * @param {CategoryInfo} categoryInfo - the category for this block
-     * @returns {{info: BlockInfo, json: object, xml: string}} - the converted & original block information
+     * @returns {ConvertedBlockInfo} - the converted & original block information
      * @private
      */
     _convertForScratchBlocks (blockInfo, categoryInfo) {
         const extendedOpcode = `${categoryInfo.id}.${blockInfo.opcode}`;
+
         const blockJSON = {
             type: extendedOpcode,
             inputsInline: true,
@@ -621,19 +636,19 @@ class Runtime extends EventEmitter {
             colour: categoryInfo.color1,
             colourSecondary: categoryInfo.color2,
             colourTertiary: categoryInfo.color3,
-            args0: [],
             extensions: ['scratch_extension']
         };
-
+        const context = {
-        const inputList = [];
+            // TODO: store this somewhere so that we can map args appropriately after translation.
-
+            // This maps an arg name to its relative position in the original (usually English) block text.
-        // TODO: store this somewhere so that we can map args appropriately after translation.
+            // When displaying a block in another language we'll need to run a `replace` action similar to the one
-        // This maps an arg name to its relative position in the original (usually English) block text.
+            // below, but each `[ARG]` will need to be replaced with the number in this map.
-        // When displaying a block in another language we'll need to run a `replace` action similar to the one below,
+            argsMap: {},
-        // but each `[ARG]` will need to be replaced with the number in this map instead of `args0.length`.
+            blockJSON,
-        const argsMap = {};
+            categoryInfo,
-
+            blockInfo,
-        blockJSON.message0 = '';
+            inputList: []
+        };
 
         // If an icon for the extension exists, prepend it to each block, with a vertical separator.
         if (categoryInfo.blockIconURI) {
@@ -647,60 +662,12 @@ class Runtime extends EventEmitter {
             const separatorJSON = {
                 type: 'field_vertical_separator'
             };
-            blockJSON.args0.push(iconJSON);
+            blockJSON.args0 = [
-            blockJSON.args0.push(separatorJSON);
+                iconJSON,
+                separatorJSON
+            ];
         }
 
-        blockJSON.message0 += blockInfo.text.replace(/\[(.+?)]/g, (match, placeholder) => {
-            // Sanitize the placeholder to ensure valid XML
-            placeholder = placeholder.replace(/[<"&]/, '_');
-
-            const argJSON = {
-                type: 'input_value',
-                name: placeholder
-            };
-
-            const argInfo = blockInfo.arguments[placeholder] || {};
-            const argTypeInfo = ArgumentTypeMap[argInfo.type] || {};
-            const defaultValue = (typeof argInfo.defaultValue === 'undefined' ?
-                '' :
-                escapeHtml(argInfo.defaultValue.toString()));
-
-            if (argTypeInfo.check) {
-                argJSON.check = argTypeInfo.check;
-            }
-
-            const shadowType = (argInfo.menu ?
-                this._makeExtensionMenuId(argInfo.menu, categoryInfo.id) :
-                argTypeInfo.shadowType);
-            const fieldType = argInfo.menu || argTypeInfo.fieldType;
-
-            // <value> is the ScratchBlocks name for a block input.
-            inputList.push(`<value name="${placeholder}">`);
-
-            // The <shadow> is a placeholder for a reporter and is visible when there's no reporter in this input.
-            // Boolean inputs don't need to specify a shadow in the XML.
-            if (shadowType) {
-                inputList.push(`<shadow type="${shadowType}">`);
-
-                // <field> is a text field that the user can type into. Some shadows, like the color picker, don't allow
-                // text input and therefore don't need a field element.
-                if (fieldType) {
-                    inputList.push(`<field name="${fieldType}">${defaultValue}</field>`);
-                }
-
-                inputList.push('</shadow>');
-            }
-
-            inputList.push('</value>');
-
-            // scratch-blocks uses 1-based argument indexing
-            blockJSON.args0.push(argJSON);
-            const argNum = blockJSON.args0.length;
-            argsMap[placeholder] = argNum;
-            return `%${argNum}`;
-        });
-
         switch (blockInfo.blockType) {
         case BlockType.COMMAND:
             blockJSON.outputShape = ScratchBlocksConstants.OUTPUT_SHAPE_SQUARE;
@@ -722,33 +689,130 @@ class Runtime extends EventEmitter {
             blockJSON.nextStatement = null; // null = available connection; undefined = terminal
             break;
         case BlockType.CONDITIONAL:
-            // Statement inputs get names like 'SUBSTACK', 'SUBSTACK2', 'SUBSTACK3', ...
+        case BlockType.LOOP:
-            for (let branchNum = 1; branchNum <= blockInfo.branchCount; ++branchNum) {
+            blockInfo.branchCount = blockInfo.branchCount || 1;
-                blockJSON[`message${branchNum}`] = '%1';
-                blockJSON[`args${branchNum}`] = [{
-                    type: 'input_statement',
-                    name: `SUBSTACK${branchNum > 1 ? branchNum : ''}`
-                }];
-            }
             blockJSON.outputShape = ScratchBlocksConstants.OUTPUT_SHAPE_SQUARE;
             blockJSON.previousStatement = null; // null = available connection; undefined = hat
-            blockJSON.nextStatement = null; // null = available connection; undefined = terminal
+            if (!blockInfo.isTerminal) {
+                blockJSON.nextStatement = null; // null = available connection; undefined = terminal
+            }
             break;
         }
 
-        if (blockInfo.isTerminal) {
+        const blockText = Array.isArray(blockInfo.text) ? blockInfo.text : [blockInfo.text];
-            delete blockJSON.nextStatement;
+        let inTextNum = 0; // text for the next block "arm" is blockText[inTextNum]
+        let inBranchNum = 0; // how many branches have we placed into the JSON so far?
+        let outLineNum = 0; // used for scratch-blocks `message${outLineNum}` and `args${outLineNum}`
+        const convertPlaceholders = this._convertPlaceholders.bind(this, context);
+
+        // alternate between a block "arm" with text on it and an open slot for a substack
+        while (inTextNum < blockText.length || inBranchNum < blockInfo.branchCount) {
+            if (inTextNum < blockText.length) {
+                context.outLineNum = outLineNum;
+                const convertedText = blockText[inTextNum].replace(/\[(.+?)]/g, convertPlaceholders);
+                if (blockJSON[`message${outLineNum}`]) {
+                    blockJSON[`message${outLineNum}`] += convertedText;
+                } else {
+                    blockJSON[`message${outLineNum}`] = convertedText;
+                }
+                ++inTextNum;
+                ++outLineNum;
+            }
+            if (inBranchNum < blockInfo.branchCount) {
+                blockJSON[`message${outLineNum}`] = '%1';
+                blockJSON[`args${outLineNum}`] = [{
+                    type: 'input_statement',
+                    name: `SUBSTACK${inBranchNum > 0 ? inBranchNum + 1 : ''}`
+                }];
+                ++inBranchNum;
+                ++outLineNum;
+            }
         }
 
-        const blockXML = `<block type="${extendedOpcode}">${inputList.join('')}</block>`;
+        // Add icon to the bottom right of a loop block
+        if (blockInfo.blockType === BlockType.LOOP) {
+            blockJSON[`lastDummyAlign${outLineNum}`] = 'RIGHT';
+            blockJSON[`message${outLineNum}`] = '%1';
+            blockJSON[`args${outLineNum}`] = [{
+                type: 'field_image',
+                src: './static/blocks-media/repeat.svg', // TODO: use a constant or make this configurable?
+                width: 24,
+                height: 24,
+                alt: '*',
+                flip_rtl: true
+            }];
+            ++outLineNum;
+        }
+
+        const blockXML = `<block type="${extendedOpcode}">${context.inputList.join('')}</block>`;
 
         return {
-            info: blockInfo,
+            info: context.blockInfo,
-            json: blockJSON,
+            json: context.blockJSON,
             xml: blockXML
         };
     }
 
+    /**
+     * Helper for _convertForScratchBlocks which handles linearization of argument placeholders. Called as a callback
+     * from string#replace. In addition to the return value the JSON and XML items in the context will be filled.
+     * @param {object} context - information shared with _convertForScratchBlocks about the block, etc.
+     * @param {string} match - the overall string matched by the placeholder regex, including brackets: '[FOO]'.
+     * @param {string} placeholder - the name of the placeholder being matched: 'FOO'.
+     * @return {string} scratch-blocks placeholder for the argument: '%1'.
+     * @private
+     */
+    _convertPlaceholders (context, match, placeholder) {
+        // Sanitize the placeholder to ensure valid XML
+        placeholder = placeholder.replace(/[<"&]/, '_');
+
+        const argJSON = {
+            type: 'input_value',
+            name: placeholder
+        };
+
+        const argInfo = context.blockInfo.arguments[placeholder] || {};
+        const argTypeInfo = ArgumentTypeMap[argInfo.type] || {};
+        const defaultValue = (typeof argInfo.defaultValue === 'undefined' ?
+            '' :
+            escapeHtml(argInfo.defaultValue.toString()));
+
+        if (argTypeInfo.check) {
+            argJSON.check = argTypeInfo.check;
+        }
+
+        const shadowType = (argInfo.menu ?
+            this._makeExtensionMenuId(argInfo.menu, context.categoryInfo.id) :
+            argTypeInfo.shadowType);
+        const fieldType = argInfo.menu || argTypeInfo.fieldType;
+
+        // <value> is the ScratchBlocks name for a block input.
+        context.inputList.push(`<value name="${placeholder}">`);
+
+        // The <shadow> is a placeholder for a reporter and is visible when there's no reporter in this input.
+        // Boolean inputs don't need to specify a shadow in the XML.
+        if (shadowType) {
+            context.inputList.push(`<shadow type="${shadowType}">`);
+
+            // <field> is a text field that the user can type into. Some shadows, like the color picker, don't allow
+            // text input and therefore don't need a field element.
+            if (fieldType) {
+                context.inputList.push(`<field name="${fieldType}">${defaultValue}</field>`);
+            }
+
+            context.inputList.push('</shadow>');
+        }
+
+        context.inputList.push('</value>');
+
+        const argsName = `args${context.outLineNum}`;
+        const blockArgs = (context.blockJSON[argsName] = context.blockJSON[argsName] || []);
+        blockArgs.push(argJSON);
+        const argNum = blockArgs.length;
+        context.argsMap[placeholder] = argNum;
+        return `%${argNum}`;
+    }
+
     /**
      * @returns {string} scratch-blocks XML description for all dynamic blocks, wrapped in <category> elements.
      */

src/extension-support/block-type.js

@@ -15,6 +15,7 @@ const BlockType = {
 
     /**
      * Specialized command block which may or may not run a child branch
+     * The thread continues with the next block whether or not a child branch ran.
      */
     CONDITIONAL: 'conditional',
 
@@ -23,6 +24,12 @@ const BlockType = {
      */
     HAT: 'hat',
 
+    /**
+     * Specialized command block which may or may not run a child branch
+     * If a child branch runs, the thread evaluates the loop block again.
+     */
+    LOOP: 'loop',
+
     /**
      * General reporter with numeric or string value
      */

src/extension-support/extension-manager.js

@@ -35,13 +35,23 @@ const builtinExtensions = {
  * @property {Boolean|undefined} hideFromPalette - true if should not be appear in the palette. (default false)
  */
 
+/**
+ * @typedef {object} ConvertedBlockInfo - Raw extension block data paired with processed data ready for scratch-blocks
+ * @property {BlockInfo} info - the raw block info
+ * @property {object} json - the scratch-blocks JSON definition for this block
+ * @property {string} xml - the scratch-blocks XML definition for this block
+ */
+
 /**
  * @typedef {object} CategoryInfo - Information about a block category
  * @property {string} id - the unique ID of this category
+ * @property {string} name - the human-readable name of this category
+ * @property {string|undefined} blockIconURI - optional URI for the block icon image
  * @property {string} color1 - the primary color for this category, in '#rrggbb' format
  * @property {string} color2 - the secondary color for this category, in '#rrggbb' format
  * @property {string} color3 - the tertiary color for this category, in '#rrggbb' format
- * @property {Array.<BlockInfo>} block - the blocks in this category
+ * @property {Array.<ConvertedBlockInfo>} blocks - the blocks, separators, etc. in this category
+ * @property {Array.<object>} menus - the menus provided by this category
  */
 
 /**
@@ -205,7 +215,7 @@ class ExtensionManager {
     _registerExtensionInfo (serviceName, extensionInfo) {
         extensionInfo = this._prepareExtensionInfo(serviceName, extensionInfo);
         dispatch.call('runtime', '_registerExtensionPrimitives', extensionInfo).catch(e => {
-            log.error(`Failed to register primitives for extension on service ${serviceName}: ${JSON.stringify(e)}`);
+            log.error(`Failed to register primitives for extension on service ${serviceName}:`, e);
         });
     }
 
@@ -233,14 +243,23 @@ class ExtensionManager {
         extensionInfo.name = extensionInfo.name || extensionInfo.id;
         extensionInfo.blocks = extensionInfo.blocks || [];
         extensionInfo.targetTypes = extensionInfo.targetTypes || [];
-        extensionInfo.blocks = extensionInfo.blocks.reduce((result, blockInfo) => {
+        extensionInfo.blocks = extensionInfo.blocks.reduce((results, blockInfo) => {
             try {
-                result.push(this._prepareBlockInfo(serviceName, blockInfo));
+                let result;
+                switch (blockInfo) {
+                case '---': // separator
+                    result = '---';
+                    break;
+                default: // a BlockInfo object
+                    result = this._prepareBlockInfo(serviceName, blockInfo);
+                    break;
+                }
+                results.push(result);
             } catch (e) {
                 // TODO: more meaningful error reporting
                 log.error(`Error processing block: ${e.message}, Block:\n${JSON.stringify(blockInfo)}`);
             }
-            return result;
+            return results;
         }, []);
         extensionInfo.menus = extensionInfo.menus || [];
         extensionInfo.menus = this._prepareMenuInfo(serviceName, extensionInfo.menus);
@@ -309,24 +328,30 @@ class ExtensionManager {
             arguments: {}
         }, blockInfo);
         blockInfo.opcode = this._sanitizeID(blockInfo.opcode);
-        blockInfo.func = blockInfo.func ? this._sanitizeID(blockInfo.func) : blockInfo.opcode;
         blockInfo.text = blockInfo.text || blockInfo.opcode;
 
-        /**
+        if (blockInfo.blockType !== BlockType.EVENT) {
-         * This is only here because the VM performs poorly when blocks return promises.
+            blockInfo.func = blockInfo.func ? this._sanitizeID(blockInfo.func) : blockInfo.opcode;
-         * @TODO make it possible for the VM to resolve a promise and continue during the same frame.
+
-         */
+            /**
-        if (dispatch._isRemoteService(serviceName)) {
+             * This is only here because the VM performs poorly when blocks return promises.
-            blockInfo.func = dispatch.call.bind(dispatch, serviceName, blockInfo.func);
+             * @TODO make it possible for the VM to resolve a promise and continue during the same Scratch "tick"
-        } else {
+             */
-            const serviceObject = dispatch.services[serviceName];
+            if (dispatch._isRemoteService(serviceName)) {
-            const func = serviceObject[blockInfo.func];
+                blockInfo.func = dispatch.call.bind(dispatch, serviceName, blockInfo.func);
-            if (func) {
-                blockInfo.func = func.bind(serviceObject);
             } else {
-                throw new Error(`Could not find extension block function called ${blockInfo.func}`);
+                const serviceObject = dispatch.services[serviceName];
+                const func = serviceObject[blockInfo.func];
+                if (func) {
+                    blockInfo.func = func.bind(serviceObject);
+                } else if (blockInfo.blockType !== BlockType.EVENT) {
+                    throw new Error(`Could not find extension block function called ${blockInfo.func}`);
+                }
             }
+        } else if (blockInfo.func) {
+            log.warn(`Ignoring function "${blockInfo.func}" for event block ${blockInfo.opcode}`);
         }
+
         return blockInfo;
     }
 }

src/extension-support/extension-metadata.js

@@ -6,8 +6,8 @@
  * @property {string} blockIconURI - URI for an image to be placed on each block in this extension. Data URI ok.
  * @property {string} menuIconURI - URI for an image to be placed on this extension's category menu entry. Data URI ok.
  * @property {string} docsURI - link to documentation content for this extension.
- * @property {Array.<ExtensionBlockMetadata|string>} - the blocks provided by this extension, with optional separators.
+ * @property {Array.<ExtensionBlockMetadata|string>} blocks - the blocks provided by this extension, plus separators.
- * @property {Object.<ExtensionMenuMetadata>} - map of menu name to metadata about each of this extension's menus.
+ * @property {Object.<ExtensionMenuMetadata>} menus - map of menu name to metadata about each of this extension's menus.
  */
 
 /**