Merge pull request #1229 from kchadha/imported-comment-positioning-fix

Update position for comments imported from 2.0 projects after auto-positioning.

src/engine/blocks.js

@@ -362,6 +362,18 @@ class Blocks {
                 const currTarget = optRuntime.getEditingTarget();
                 currTarget.createComment(e.commentId, e.blockId, e.text,
                     e.xy.x, e.xy.y, e.width, e.height, e.minimized);
+
+                if (currTarget.comments[e.commentId].x === null &&
+                    currTarget.comments[e.commentId].y === null) {
+                    // Block comments imported from 2.0 projects are imported with their
+                    // x and y coordinates set to null so that scratch-blocks can
+                    // auto-position them. If we are receiving a create event for these
+                    // comments, then the auto positioning should have taken place.
+                    // Update the x and y position of these comments to match the
+                    // one from the event.
+                    currTarget.comments[e.commentId].x = e.xy.x;
+                    currTarget.comments[e.commentId].y = e.xy.y;
+                }
             }
             break;
         case 'comment_change':

test/fixtures/events.json

@@ -71,5 +71,11 @@
         "xml": {
             "outerHTML": "<block type='operator_add' id='D;MqidqmaN}Dft)y#Bf`' x='80' y='98'><value name='NUM1'><shadow type='math_number' id='F[IFAdLbq8!q25+Nio@i'><field name='NUM'></field></shadow><block type='sensing_answer' id='D~ZQ|BYb1)xw4)8ziI%.'></block</value><value name='NUM2'><shadow type='math_number' id='|Sjv4!*X6;wj?QaCE{-9'><field name='NUM'></field></shadow></value></block>"
         }
+    },
+    "createcommentUpdatePosition": {
+        "name": "comment",
+        "type": "comment_create",
+        "commentId": "a comment",
+        "xy": {"x": 10, "y": 20}
     }
 }

test/unit/engine_target.js

@@ -189,3 +189,77 @@ test('lookupBroadcastMsg returns the var with given id if exists', t => {
 
     t.end();
 });
+
+test('createComment adds a comment to the target', t => {
+    const target = new Target();
+    const comments = target.comments;
+
+    t.equal(Object.keys(comments).length, 0);
+    target.createComment('a comment', null, 'some comment text',
+        10, 20, 200, 300, true);
+    t.equal(Object.keys(comments).length, 1);
+
+    const comment = comments['a comment'];
+    t.notEqual(comment, null);
+    t.equal(comment.blockId, null);
+    t.equal(comment.text, 'some comment text');
+    t.equal(comment.x, 10);
+    t.equal(comment.y, 20);
+    t.equal(comment.width, 200);
+    t.equal(comment.height, 300);
+    t.equal(comment.minimized, true);
+
+    t.end();
+});
+
+test('creating comment with id that already exists does not change existing comment', t => {
+    const target = new Target();
+    const comments = target.comments;
+
+    t.equal(Object.keys(comments).length, 0);
+    target.createComment('a comment', null, 'some comment text',
+        10, 20, 200, 300, true);
+    t.equal(Object.keys(comments).length, 1);
+
+    target.createComment('a comment', null,
+        'some new comment text', 40, 50, 300, 400, false);
+
+    const comment = comments['a comment'];
+    t.notEqual(comment, null);
+    // All of the comment properties should remain unchanged from the first
+    // time createComment was called
+    t.equal(comment.blockId, null);
+    t.equal(comment.text, 'some comment text');
+    t.equal(comment.x, 10);
+    t.equal(comment.y, 20);
+    t.equal(comment.width, 200);
+    t.equal(comment.height, 300);
+    t.equal(comment.minimized, true);
+
+    t.end();
+});
+
+test('creating a comment with a blockId also updates the comment property on the block', t => {
+    const target = new Target();
+    const comments = target.comments;
+    // Create a mock block on the target
+    target.blocks = {
+        'a mock block': {
+            id: 'a mock block'
+        }
+    };
+
+    // Mock the getBlock function that's used in commentCreate
+    target.blocks.getBlock = id => target.blocks[id];
+
+    t.equal(Object.keys(comments).length, 0);
+    target.createComment('a comment', 'a mock block', 'some comment text',
+        10, 20, 200, 300, true);
+    t.equal(Object.keys(comments).length, 1);
+
+    const comment = comments['a comment'];
+    t.equal(comment.blockId, 'a mock block');
+    t.equal(target.blocks.getBlock('a mock block').comment, 'a comment');
+
+    t.end();
+});

test/unit/virtual-machine.js

@@ -2,6 +2,7 @@ const test = require('tap').test;
 const VirtualMachine = require('../../src/virtual-machine.js');
 const Sprite = require('../../src/sprites/sprite.js');
 const Variable = require('../../src/engine/variable.js');
+const events = require('../fixtures/events.json');
 
 test('addSprite throws on invalid string', t => {
     const vm = new VirtualMachine();
@@ -558,3 +559,27 @@ test('getVariableValue', t => {
 
     t.end();
 });
+
+// Block Listener tests for comment
+test('comment_create event updates comment with null position', t => {
+    const vm = new VirtualMachine();
+    const spr = new Sprite(null, vm.runtime);
+    const target = spr.createClone();
+
+    target.createComment('a comment', null, 'some text',
+        null, null, 200, 300, false);
+    vm.runtime.targets = [target];
+    vm.editingTarget = target;
+    vm.runtime.setEditingTarget(target);
+
+    const comment = target.comments['a comment'];
+    t.equal(comment.x, null);
+    t.equal(comment.y, null);
+
+    vm.blockListener(events.createcommentUpdatePosition);
+
+    t.equal(comment.x, 10);
+    t.equal(comment.y, 20);
+
+    t.end();
+});