Handle comments attached to variable blocks

scratchfoundation · Jun 30, 2020 · 255d105 · 255d105
1 parent 5d5196f
commit 255d105
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 7 deletions.
diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js
@@ -801,6 +801,8 @@ const deserializeFields = function (fields) {
  * @return {object} input is modified and returned
  */
 const deserializeBlocks = function (blocks) {
+    // oldToNew is used to fix comments attached to variable blocks: see #1893
+    const oldToNew = {};
     for (const blockId in blocks) {
         if (!Object.prototype.hasOwnProperty.call(blocks, blockId)) {
             continue;
@@ -811,14 +813,14 @@ const deserializeBlocks = function (blocks) {
             // delete the old entry in object.blocks and replace it w/the
             // deserialized object
             delete blocks[blockId];
-            deserializeInputDesc(block, null, false, blocks);
+            oldToNew[blockId] = deserializeInputDesc(block, null, false, blocks);
             continue;
         }
         block.id = blockId; // add id back to block since it wasn't serialized
         block.inputs = deserializeInputs(block.inputs, blockId, blocks);
         block.fields = deserializeFields(block.fields);
     }
-    return blocks;
+    return ({blocks, oldToNew});
 };
 
 
@@ -930,12 +932,15 @@ const parseScratchObject = function (object, runtime, extensions, zip, assets) {
     // @todo: For now, load all Scratch objects (stage/sprites) as a Sprite.
     const sprite = new Sprite(blocks, runtime);
 
+    // Used later when parsing comments
+    let oldToNew = {};
+
     // Sprite/stage name from JSON.
     if (object.hasOwnProperty('name')) {
         sprite.name = object.name;
     }
     if (object.hasOwnProperty('blocks')) {
-        deserializeBlocks(object.blocks);
+        oldToNew = deserializeBlocks(object.blocks).oldToNew;
         // Take a second pass to create objects and add extensions
         for (const blockId in object.blocks) {
             if (!object.blocks.hasOwnProperty(blockId)) continue;
@@ -1031,7 +1036,22 @@ const parseScratchObject = function (object, runtime, extensions, zip, assets) {
                 comment.minimized
             );
             if (comment.blockId) {
-                newComment.blockId = comment.blockId;
+                if (oldToNew.hasOwnProperty(comment.blockId)) {
+                    // ID for the block is changed during deserialization
+                    newComment.blockId = oldToNew[comment.blockId];
+                } else {
+                    newComment.blockId = comment.blockId;
+                }
+                // does the block really exist? if so, add reference to the comment
+                const block = target.blocks.getBlock(newComment.blockId);
+                if (block) {
+                    block.comment = commentId;
+                } else {
+                    // LLK/scratch-vm#2501
+                    log.warn(`${commentId} is attached to a block with ID ${newComment.blockId
+                    }, but the block does not exist.`);
+                    continue;
+                }
             }
             target.comments[newComment.id] = newComment;
         }

diff --git a/test/unit/serialization_sb3.js b/test/unit/serialization_sb3.js
@@ -259,7 +259,7 @@ test('deserializeBlocks', t => {
         .then(() => {
             const blocks = vm.runtime.targets[1].blocks._blocks;
             const serialized = sb3.serializeBlocks(blocks)[0];
-            const deserialized = sb3.deserializeBlocks(serialized);
+            const deserialized = sb3.deserializeBlocks(serialized).blocks;
             t.equal(Object.keys(deserialized).length, Object.keys(blocks).length, 'same number of blocks');
             t.end();
         });
@@ -271,8 +271,8 @@ test('deserializeBlocks on already deserialized input', t => {
         .then(() => {
             const blocks = vm.runtime.targets[1].blocks._blocks;
             const serialized = sb3.serializeBlocks(blocks)[0];
-            const deserialized = sb3.deserializeBlocks(serialized);
-            const deserializedAgain = sb3.deserializeBlocks(deserialized);
+            const deserialized = sb3.deserializeBlocks(serialized).blocks;
+            const deserializedAgain = sb3.deserializeBlocks(deserialized).blocks;
             t.deepEqual(deserialized, deserializedAgain, 'no change from second pass of deserialize');
             t.end();
         });