Addressing PR comments.

src/engine/comment.js

@@ -4,23 +4,26 @@
  */
 
 const uid = require('../util/uid');
-const cast = require('../util/cast');
+const Cast = require('../util/cast');
 
 class Comment {
     /**
-     * @param {string} id Id of the variable.
-     * @param {string} name Name of the variable.
-     * @param {string} type Type of the variable, one of '' or 'list'
-     * @param {boolean} isCloud Whether the variable is stored in the cloud.
+     * @param {string} id Id of the comment.
+     * @param {string} text Text content of the comment.
+     * @param {number} x X position of the comment on the workspace.
+     * @param {number} y Y position of the comment on the workspace.
+     * @param {number} width The width of the comment when it is full size.
+     * @param {number} height The height of the comment when it is full size.
+     * @param {boolean} minimized Whether the comment is minimized.
      * @constructor
-     */ /* TODO should the comment constructor take in an id? will we need this for sb3? */
+     */
     constructor (id, text, x, y, width, height, minimized) {
         this.id = id || uid();
         this.text = text;
         this.x = x;
         this.y = y;
-        this.width = Math.max(cast.toNumber(width), Comment.MIN_WIDTH);
-        this.height = Math.max(cast.toNumber(height), Comment.MIN_HEIGHT);
+        this.width = Math.max(Cast.toNumber(width), Comment.MIN_WIDTH);
+        this.height = Math.max(Cast.toNumber(height), Comment.MIN_HEIGHT);
         this.minimized = minimized || false;
         this.blockId = null;
     }

src/serialization/sb2.js

@@ -37,6 +37,12 @@ const CORE_EXTENSIONS = [
     'sound'
 ];
 
+// Adjust script coordinates to account for
+// larger block size in scratch-blocks.
+// @todo: Determine more precisely the right formulas here.
+const WORKSPACE_X_SCALE = 1.5;
+const WORKSPACE_Y_SCALE = 2.2;
+
 /**
  * Convert a Scratch 2.0 procedure string (e.g., "my_procedure %s %b %n")
  * into an argument map. This allows us to provide the expected inputs
@@ -166,11 +172,8 @@ const parseScripts = function (scripts, blocks, addBroadcastMsg, getVariableId,
             comments, scriptIndexForComment);
         scriptIndexForComment = newCommentIndex;
         if (parsedBlockList[0]) {
-            // Adjust script coordinates to account for
-            // larger block size in scratch-blocks.
-            // @todo: Determine more precisely the right formulas here.
-            parsedBlockList[0].x = scriptX * 1.5;
-            parsedBlockList[0].y = scriptY * 2.2;
+            parsedBlockList[0].x = scriptX * WORKSPACE_X_SCALE;
+            parsedBlockList[0].y = scriptY * WORKSPACE_Y_SCALE;
             parsedBlockList[0].topLevel = true;
             parsedBlockList[0].parent = null;
         }
@@ -449,17 +452,26 @@ const parseScratchObject = function (object, runtime, extensions, topLevel, zip)
     const blockComments = {};
     if (object.hasOwnProperty('scriptComments')) {
         const comments = object.scriptComments.map(commentDesc => {
+            const [
+                commentX,
+                commentY,
+                commentWidth,
+                commentHeight,
+                commentFullSize,
+                flattenedBlockIndex,
+                commentText
+            ] = commentDesc;
             const isBlockComment = commentDesc[5] >= 0;
             const newComment = new Comment(
                 null, // generate a new id for this comment
-                commentDesc[6], // text content of sb2 comment
+                commentText, // text content of sb2 comment
                 // Only serialize x & y position of comment if it's a workspace comment
                 // If it's a block comment, we'll let scratch-blocks handle positioning
-                isBlockComment ? null : commentDesc[0] * 1.5, // x position of comment
-                isBlockComment ? null : commentDesc[1] * 2.2, // y position of comment
-                commentDesc[2] * 1.5, // width of comment
-                commentDesc[3] * 2.2, // height of comment
-                !commentDesc[4] // commentDesc[4] -- false means minimized, true means full screen
+                isBlockComment ? null : commentX * WORKSPACE_X_SCALE,
+                isBlockComment ? null : commentY * WORKSPACE_Y_SCALE,
+                commentWidth * WORKSPACE_X_SCALE,
+                commentHeight * WORKSPACE_Y_SCALE,
+                !commentFullSize
             );
             if (isBlockComment) {
                 // commentDesc[5] refers to the index of the block that this
@@ -470,13 +482,13 @@ const parseScratchObject = function (object, runtime, extensions, topLevel, zip)
                 // index as the blockId property of the new comment. We will
                 // change this to refer to the actual block id of the corresponding
                 // block when that block gets created
-                newComment.blockId = commentDesc[5];
+                newComment.blockId = flattenedBlockIndex;
                 // Add this comment to the block comments object with its script index
                 // as the key
-                if (blockComments.hasOwnProperty(commentDesc[5])) {
-                    blockComments[commentDesc[5]].push(newComment);
+                if (blockComments.hasOwnProperty(flattenedBlockIndex)) {
+                    blockComments[flattenedBlockIndex].push(newComment);
                 } else {
-                    blockComments[commentDesc[5]] = [newComment];
+                    blockComments[flattenedBlockIndex] = [newComment];
                 }
             }
             return newComment;

src/virtual-machine.js

@@ -1005,7 +1005,7 @@ class VirtualMachine extends EventEmitter {
         );
 
         const variables = Object.keys(variableMap).map(k => variableMap[k]);
-        const wkspcComments = Object.keys(this.editingTarget.comments)
+        const workspaceComments = Object.keys(this.editingTarget.comments)
             .map(k => this.editingTarget.comments[k])
             .filter(c => c.blockId === null);
 
@@ -1013,7 +1013,7 @@ class VirtualMachine extends EventEmitter {
                             <variables>
                                 ${variables.map(v => v.toXML()).join()}
                             </variables>
-                            ${wkspcComments.map(c => c.toXML()).join()}
+                            ${workspaceComments.map(c => c.toXML()).join()}
                             ${this.editingTarget.blocks.toXML(this.editingTarget.comments)}
                         </xml>`;
 

test/integration/comments.js

@@ -0,0 +1,91 @@
+const path = require('path');
+const test = require('tap').test;
+const makeTestStorage = require('../fixtures/make-test-storage');
+const readFileToBuffer = require('../fixtures/readProjectFile').readFileToBuffer;
+const VirtualMachine = require('../../src/index');
+
+const projectUri = path.resolve(__dirname, '../fixtures/comments.sb2');
+const project = readFileToBuffer(projectUri);
+
+test('importing sb2 project with comments', t => {
+    const vm = new VirtualMachine();
+    vm.attachStorage(makeTestStorage());
+
+    // Evaluate playground data and exit
+    vm.on('playgroundData', e => {
+        const threads = JSON.parse(e.threads);
+        t.equal(threads.length, 0);
+
+        const stage = vm.runtime.targets[0];
+        const target = vm.runtime.targets[1];
+
+        const stageComments = Object.values(stage.comments);
+
+        // Stage has 1 comment, and it is minimized.
+        t.equal(stageComments.length, 1);
+        t.equal(stageComments[0].minimized, true);
+        t.equal(stageComments[0].text, 'A minimized stage comment.');
+        // The stage comment is a workspace comment
+        t.equal(stageComments[0].blockId, null);
+
+        // Sprite 1 has 6 Comments, 1 workspace comment, and 5 block comments
+        const targetComments = Object.values(target.comments);
+        t.equal(targetComments.length, 6);
+        const spriteWorkspaceComments = targetComments.filter(comment => comment.blockId === null);
+        t.equal(spriteWorkspaceComments.length, 1);
+        t.equal(spriteWorkspaceComments[0].minimized, false);
+        t.equal(spriteWorkspaceComments[0].text, 'This is a workspace comment.');
+
+        // Test the sprite block comments
+        const blockComments = targetComments.filter(comment => !!comment.blockId);
+        t.equal(blockComments.length, 5);
+
+        t.equal(blockComments[0].minimized, true);
+        t.equal(blockComments[0].text, '1. Green Flag Comment.');
+        const greenFlagBlock = target.blocks.getBlock(blockComments[0].blockId);
+        t.equal(greenFlagBlock.comment, blockComments[0].id);
+        t.equal(greenFlagBlock.opcode, 'event_whenflagclicked');
+
+        t.equal(blockComments[1].minimized, true);
+        t.equal(blockComments[1].text, '2. Turn 15 Degrees Comment.');
+        const turnRightBlock = target.blocks.getBlock(blockComments[1].blockId);
+        t.equal(turnRightBlock.comment, blockComments[1].id);
+        t.equal(turnRightBlock.opcode, 'motion_turnright');
+
+        t.equal(blockComments[2].minimized, false);
+        t.equal(blockComments[2].text, '3. Comment for a loop.');
+        const repeatBlock = target.blocks.getBlock(blockComments[2].blockId);
+        t.equal(repeatBlock.comment, blockComments[2].id);
+        t.equal(repeatBlock.opcode, 'control_repeat');
+
+        t.equal(blockComments[3].minimized, false);
+        t.equal(blockComments[3].text, '4. Comment for a block nested in a loop.');
+        const changeColorBlock = target.blocks.getBlock(blockComments[3].blockId);
+        t.equal(changeColorBlock.comment, blockComments[3].id);
+        t.equal(changeColorBlock.opcode, 'looks_changeeffectby');
+
+        t.equal(blockComments[4].minimized, false);
+        t.equal(blockComments[4].text, '5. Comment for a block outside of a loop.');
+        const stopAllBlock = target.blocks.getBlock(blockComments[4].blockId);
+        t.equal(stopAllBlock.comment, blockComments[4].id);
+        t.equal(stopAllBlock.opcode, 'control_stop');
+
+        t.end();
+        process.nextTick(process.exit);
+    });
+
+    // Start VM, load project, and run
+    t.doesNotThrow(() => {
+        vm.start();
+        vm.clear();
+        vm.setCompatibilityMode(false);
+        vm.setTurboMode(false);
+        vm.loadProject(project).then(() => {
+            vm.greenFlag();
+            setTimeout(() => {
+                vm.getPlaygroundData();
+                vm.stopAll();
+            }, 100);
+        });
+    });
+});

test/integration/import_sb2.js

@@ -30,6 +30,7 @@ test('default', t => {
         t.type(targets[0].id, 'string');
         t.type(targets[0].blocks, 'object');
         t.type(targets[0].variables, 'object');
+        t.type(targets[0].comments, 'object');
 
         t.equal(targets[0].isOriginal, true);
         t.equal(targets[0].currentCostume, 0);
@@ -40,6 +41,7 @@ test('default', t => {
         t.type(targets[1].id, 'string');
         t.type(targets[1].blocks, 'object');
         t.type(targets[1].variables, 'object');
+        t.type(targets[1].comments, 'object');
 
         t.equal(targets[1].isOriginal, true);
         t.equal(targets[1].currentCostume, 0);

test/unit/engine_target.js

@@ -12,6 +12,7 @@ test('spec', t => {
     t.type(target.id, 'string');
     t.type(target.blocks, 'object');
     t.type(target.variables, 'object');
+    t.type(target.comments, 'object');
     t.type(target._customState, 'object');
 
     t.type(target.createVariable, 'function');

test/unit/serialization_sb2.js

@@ -29,6 +29,7 @@ test('default', t => {
         t.type(targets[0].id, 'string');
         t.type(targets[0].blocks, 'object');
         t.type(targets[0].variables, 'object');
+        t.type(targets[0].comments, 'object');
 
         t.equal(targets[0].isOriginal, true);
         t.equal(targets[0].currentCostume, 0);
@@ -39,6 +40,7 @@ test('default', t => {
         t.type(targets[1].id, 'string');
         t.type(targets[1].blocks, 'object');
         t.type(targets[1].variables, 'object');
+        t.type(targets[1].comments, 'object');
 
         t.equal(targets[1].isOriginal, true);
         t.equal(targets[1].currentCostume, 0);