Refactor comments

src/engine/blocks.js

@@ -821,6 +821,14 @@ class Blocks {
             }
         }
 
+        // Delete comments attached to the block.
+        if (block.comment) {
+            const editingTarget = this.runtime.getEditingTarget();
+            if (editingTarget && editingTarget.comments.hasOwnProperty(block.comment)) {
+                delete editingTarget.comments[block.comment];
+            }
+        }
+
         // Delete any script starting with this block.
         this._deleteScript(blockId);
 

src/engine/target.js

@@ -20,9 +20,10 @@ class Target extends EventEmitter {
     /**
      * @param {Runtime} runtime Reference to the runtime.
      * @param {?Blocks} blocks Blocks instance for the blocks owned by this target.
+     * @param {?Object.<string, Comment>} comments Array of comments owned by this target.
      * @constructor
      */
-    constructor (runtime, blocks) {
+    constructor (runtime, blocks, comments) {
         super();
 
         if (!blocks) {
@@ -55,7 +56,7 @@ class Target extends EventEmitter {
          * Key is the comment id.
          * @type {Object.<string,*>}
          */
-        this.comments = {};
+        this.comments = comments || {};
         /**
          * Dictionary of custom state for this target.
          * This can be used to store target-specific custom state for blocks which need it.

src/sprites/rendered-target.js

@@ -15,7 +15,7 @@ class RenderedTarget extends Target {
      * @constructor
      */
     constructor (sprite, runtime) {
-        super(runtime, sprite.blocks);
+        super(runtime, sprite.blocks, sprite.comments);
 
         /**
          * Reference to the sprite that this is a render of.

src/sprites/sprite.js

@@ -1,10 +1,12 @@
 const RenderedTarget = require('./rendered-target');
 const Blocks = require('../engine/blocks');
+const Comment = require('../engine/comment');
 const {loadSoundFromAsset} = require('../import/load-sound');
 const {loadCostumeFromAsset} = require('../import/load-costume');
 const newBlockIds = require('../util/new-block-ids');
 const StringUtil = require('../util/string-util');
 const StageLayering = require('../engine/stage-layering');
+const log = require('../util/log');
 
 class Sprite {
     /**
@@ -54,6 +56,13 @@ class Sprite {
         if (this.runtime && this.runtime.audioEngine) {
             this.soundBank = this.runtime.audioEngine.createBank();
         }
+
+        /**
+         * Dictionary of comments for this target.
+         * Key is the comment id.
+         * @type {Object.<string, Comment>}
+         */
+        this.comments = {};
     }
 
     /**
@@ -140,11 +149,48 @@ class Sprite {
         const blocksContainer = this.blocks._blocks;
         const originalBlocks = Object.keys(blocksContainer).map(key => blocksContainer[key]);
         const copiedBlocks = JSON.parse(JSON.stringify(originalBlocks));
-        newBlockIds(copiedBlocks);
+        const oldToNew = newBlockIds(copiedBlocks);
         copiedBlocks.forEach(block => {
             newSprite.blocks.createBlock(block);
         });
 
+        Object.keys(this.comments).forEach(commentId => {
+            const comment = this.comments[commentId];
+            const newComment = new Comment(
+                null, // generate new comment id
+                comment.text,
+                comment.x,
+                comment.y,
+                comment.width,
+                comment.height,
+                comment.minimized
+            );
+            // If the comment is attached to a block, it has a blockId property for the block it's attached to.
+            // Because we generate new block IDs for all the duplicated blocks, change all the comments' attached-block
+            // IDs from the old block IDs to the new ones.
+            if (comment.blockId) {
+                const newBlockId = oldToNew[comment.blockId];
+                if (newBlockId) {
+                    newComment.blockId = newBlockId;
+                    const newBlock = newSprite.blocks.getBlock(newBlockId);
+                    if (newBlock) {
+                        newBlock.comment = newComment.id;
+                    } else {
+                        log.warn(`Could not find block with id ${newBlockId} associated with comment ${newComment.id}`);
+                    }
+                } else {
+                    // Comments did not get deleted when the block got deleted
+                    // Such comments have blockId, but because the block is deleted
+                    // oldToNew mapping does not include that blockId
+                    // Handle it as workspace comment
+                    // TODO do not load such comments when deserializing
+                    log.warn(`Could not find block with id ${comment.blockId
+                    } associated with comment ${comment.id} in the block ID mapping`);
+                }
+            }
+            newSprite.comments[newComment.id] = newComment;
+        });
+
 
         const allNames = this.runtime.targets.map(t => t.sprite.name);
         newSprite.name = StringUtil.unusedName(this.name, allNames);

src/util/new-block-ids.js

@@ -4,6 +4,7 @@ const uid = require('./uid');
  * Mutate the given blocks to have new IDs and update all internal ID references.
  * Does not return anything to make it clear that the blocks are updated in-place.
  * @param {array} blocks - blocks to be mutated.
+ * @returns {object.<string, string>} - mapping of old block ID to new block ID
  */
 module.exports = blocks => {
     const oldToNew = {};
@@ -30,4 +31,7 @@ module.exports = blocks => {
             blocks[i].next = oldToNew[blocks[i].next];
         }
     }
+
+    // There are other things that need this mapping e.g. comments
+    return oldToNew;
 };

test/unit/engine_blocks.js

@@ -600,6 +600,25 @@ test('delete inputs', t => {
     t.end();
 });
 
+test('delete block with comment', t => {
+    const b = new Blocks(new Runtime());
+    const fakeTarget = {
+        comments: {
+            bar: {
+                blockId: 'foo'
+            }
+        }
+    };
+    b.runtime.getEditingTarget = () => fakeTarget;
+    b.createBlock({
+        id: 'foo',
+        comment: 'bar'
+    });
+    b.deleteBlock('foo');
+    t.notOk(fakeTarget.comments.hasOwnProperty('bar'));
+    t.end();
+});
+
 test('updateAssetName function updates name in sound field', t => {
     const b = new Blocks(new Runtime());
     b.createBlock({

test/unit/sprites_rendered-target.js

@@ -45,6 +45,21 @@ test('blocks get new id on duplicate', t => {
     });
 });
 
+test('comments are duplicated when duplicating target', t => {
+    const r = new Runtime();
+    const s = new Sprite(null, r);
+    const rt = new RenderedTarget(s, r);
+    rt.createComment('commentid', null, 'testcomment', 0, 0, 100, 100, false);
+    t.ok(s.comments.hasOwnProperty('commentid'));
+    return rt.duplicate().then(duplicate => {
+        // Not ok because comment id should be re-generated
+        t.notOk(duplicate.comments.hasOwnProperty('commentid'));
+        t.ok(Object.keys(duplicate.comments).length === 1);
+        t.end();
+    });
+});
+
+
 test('direction', t => {
     const r = new Runtime();
     const s = new Sprite(null, r);