From 4a8d97a4a8f5592965533dd686971120fd682841 Mon Sep 17 00:00:00 2001 From: apple502j <33279053+apple502j@users.noreply.github.com> Date: Tue, 30 Jun 2020 11:51:45 +0900 Subject: [PATCH 1/4] Move comments to Sprite This works similar to the blocks attribute. --- src/engine/target.js | 5 +++-- src/sprites/rendered-target.js | 2 +- src/sprites/sprite.js | 41 +++++++++++++++++++++++++++++++++- src/util/new-block-ids.js | 3 +++ 4 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/engine/target.js b/src/engine/target.js index 0f1895a3cf2..4e599b7edc3 100644 --- a/src/engine/target.js +++ b/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.} 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.} */ - 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. diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index b0c1ebfed94..54144c1d3a3 100644 --- a/src/sprites/rendered-target.js +++ b/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. diff --git a/src/sprites/sprite.js b/src/sprites/sprite.js index a38fa129c60..ab8e35365b2 100644 --- a/src/sprites/sprite.js +++ b/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.} + */ + this.comments = {}; } /** @@ -140,11 +149,41 @@ 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, // use new comment id + comment.text, + comment.x, + comment.y, + comment.width, + comment.height, + comment.minimized + ); + 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 { + 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); diff --git a/src/util/new-block-ids.js b/src/util/new-block-ids.js index 1fc5f696714..4c705e04bbd 100644 --- a/src/util/new-block-ids.js +++ b/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.} - mapping of old block ID to new block ID */ module.exports = blocks => { const oldToNew = {}; @@ -30,4 +31,6 @@ module.exports = blocks => { blocks[i].next = oldToNew[blocks[i].next]; } } + + return oldToNew; }; From 5d5196f80030ba17bcdd91f22025498af3afcdf4 Mon Sep 17 00:00:00 2001 From: apple502j <33279053+apple502j@users.noreply.github.com> Date: Tue, 30 Jun 2020 13:01:17 +0900 Subject: [PATCH 2/4] Delete comments attached to deleted blocks --- src/engine/blocks.js | 8 ++++++++ src/util/new-block-ids.js | 1 + 2 files changed, 9 insertions(+) diff --git a/src/engine/blocks.js b/src/engine/blocks.js index aca5da91b6e..f8d8da0c138 100644 --- a/src/engine/blocks.js +++ b/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.comments.hasOwnProperty(block.comment)) { + delete editingTarget.comments[block.comment]; + } + } + // Delete any script starting with this block. this._deleteScript(blockId); diff --git a/src/util/new-block-ids.js b/src/util/new-block-ids.js index 4c705e04bbd..a167e7f4884 100644 --- a/src/util/new-block-ids.js +++ b/src/util/new-block-ids.js @@ -32,5 +32,6 @@ module.exports = blocks => { } } + // There are other things that need this mapping e.g. comments return oldToNew; }; From e8e08214e328022473d6c13daa10dc6e7163c205 Mon Sep 17 00:00:00 2001 From: apple502j <33279053+apple502j@users.noreply.github.com> Date: Wed, 1 Jul 2020 16:01:30 +0900 Subject: [PATCH 3/4] Add tests --- src/engine/blocks.js | 2 +- test/unit/engine_blocks.js | 19 +++++++++++++++++++ test/unit/sprites_rendered-target.js | 15 +++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/engine/blocks.js b/src/engine/blocks.js index f8d8da0c138..8dd4df26333 100644 --- a/src/engine/blocks.js +++ b/src/engine/blocks.js @@ -824,7 +824,7 @@ class Blocks { // Delete comments attached to the block. if (block.comment) { const editingTarget = this.runtime.getEditingTarget(); - if (editingTarget.comments.hasOwnProperty(block.comment)) { + if (editingTarget && editingTarget.comments.hasOwnProperty(block.comment)) { delete editingTarget.comments[block.comment]; } } diff --git a/test/unit/engine_blocks.js b/test/unit/engine_blocks.js index f4451aa39fb..80b99b06dea 100644 --- a/test/unit/engine_blocks.js +++ b/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({ diff --git a/test/unit/sprites_rendered-target.js b/test/unit/sprites_rendered-target.js index 681a10d86db..41e74c158b0 100644 --- a/test/unit/sprites_rendered-target.js +++ b/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); From 0f211b76c2e5520c89cd42e9bfb4b171c1994901 Mon Sep 17 00:00:00 2001 From: apple502j <33279053+apple502j@users.noreply.github.com> Date: Wed, 1 Jul 2020 16:14:26 +0900 Subject: [PATCH 4/4] Clarify duplication behaviors --- src/engine/target.js | 2 +- src/sprites/sprite.js | 15 +++++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/engine/target.js b/src/engine/target.js index 4e599b7edc3..19aeba131ab 100644 --- a/src/engine/target.js +++ b/src/engine/target.js @@ -20,7 +20,7 @@ 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.} comments Array of comments owned by this target. + * @param {?Object.} comments Array of comments owned by this target. * @constructor */ constructor (runtime, blocks, comments) { diff --git a/src/sprites/sprite.js b/src/sprites/sprite.js index ab8e35365b2..bb6d920f38e 100644 --- a/src/sprites/sprite.js +++ b/src/sprites/sprite.js @@ -60,7 +60,7 @@ class Sprite { /** * Dictionary of comments for this target. * Key is the comment id. - * @type {Object.} + * @type {Object.} */ this.comments = {}; } @@ -157,7 +157,7 @@ class Sprite { Object.keys(this.comments).forEach(commentId => { const comment = this.comments[commentId]; const newComment = new Comment( - null, // use new comment id + null, // generate new comment id comment.text, comment.x, comment.y, @@ -165,6 +165,9 @@ class Sprite { 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) { @@ -173,10 +176,14 @@ class Sprite { if (newBlock) { newBlock.comment = newComment.id; } else { - log.warn(`Could not find block with id ${newBlockId - } associated with comment ${newComment.id}`); + 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`); }