Merge pull request #504 from paulkaplan/validate-sprite-names

Don't rename sprites to empty string names

src/util/string-util.js

@@ -0,0 +1,17 @@
+var StringUtil = function () {};
+
+StringUtil.withoutTrailingDigits = function (s) {
+    var i = s.length - 1;
+    while ((i >= 0) && ('0123456789'.indexOf(s.charAt(i)) > -1)) i--;
+    return s.slice(0, i + 1);
+};
+
+StringUtil.unusedName = function (name, existingNames) {
+    if (existingNames.indexOf(name) < 0) return name;
+    name = StringUtil.withoutTrailingDigits(name);
+    var i = 2;
+    while (existingNames.indexOf(name + i) >= 0) i++;
+    return name + i;
+};
+
+module.exports = StringUtil;

src/virtual-machine.js

@@ -3,6 +3,9 @@ var util = require('util');
 
 var Runtime = require('./engine/runtime');
 var sb2import = require('./import/sb2import');
+var StringUtil = require('./util/string-util');
+
+var RESERVED_NAMES = ['_mouse_', '_stage_', '_edge_', '_myself_', '_random_'];
 
 /**
  * Handles connections between blocks, stage, and extensions.
@@ -204,7 +207,15 @@ VirtualMachine.prototype.renameSprite = function (targetId, newName) {
         if (!sprite) {
             throw new Error('No sprite associated with this target.');
         }
-        sprite.name = newName;
+        if (newName && RESERVED_NAMES.indexOf(newName) === -1) {
+            var names = this.runtime.targets.filter(function (runtimeTarget) {
+                return runtimeTarget.isSprite();
+            }).map(function (runtimeTarget) {
+                return runtimeTarget.sprite.name;
+            });
+
+            sprite.name = StringUtil.unusedName(newName, names);
+        }
         this.emitTargetsUpdate();
     } else {
         throw new Error('No target with the provided id.');

test/integration/complex.js

@@ -26,7 +26,7 @@ test('complex', function (t) {
         var targets = data.targetList;
         for (var i in targets) {
             if (targets[i].isStage === true) continue;
-            if (targets[i].name === 'test') continue;
+            if (targets[i].name.match(/test/)) continue;
 
             vm.setEditingTarget(targets[i].id);
             vm.renameSprite(targets[i].id, 'test');

test/unit/util_string.js

@@ -0,0 +1,57 @@
+var test = require('tap').test;
+var StringUtil = require('../../src/util/string-util');
+
+test('withoutTrailingDigits', function (t) {
+    t.strictEqual(StringUtil.withoutTrailingDigits('boeing747'), 'boeing');
+    t.strictEqual(StringUtil.withoutTrailingDigits('boeing747 '), 'boeing747 ');
+    t.strictEqual(StringUtil.withoutTrailingDigits('boeing𝟨'), 'boeing𝟨');
+    t.strictEqual(StringUtil.withoutTrailingDigits('boeing 747'), 'boeing ');
+    t.strictEqual(StringUtil.withoutTrailingDigits('747'), '');
+    t.end();
+});
+
+test('unusedName', function (t) {
+    t.strictEqual(
+        StringUtil.unusedName(
+            'name',
+            ['not the same name']
+        ),
+        'name'
+    );
+    t.strictEqual(
+        StringUtil.unusedName(
+            'name',
+            ['name']
+        ),
+        'name2'
+    );
+    t.strictEqual(
+        StringUtil.unusedName(
+            'name',
+            ['name30']
+        ),
+        'name'
+    );
+    t.strictEqual(
+        StringUtil.unusedName(
+            'name',
+            ['name', 'name2']
+        ),
+        'name3'
+    );
+    t.strictEqual(
+        StringUtil.unusedName(
+            'name',
+            ['name', 'name3']
+        ),
+        'name2'
+    );
+    t.strictEqual(
+        StringUtil.unusedName(
+            'boeing747',
+            ['boeing747']
+        ),
+        'boeing2' // Yup, this matches scratch-flash...
+    );
+    t.end();
+});

test/unit/virtual-machine.js

@@ -0,0 +1,96 @@
+var test = require('tap').test;
+var VirtualMachine = require('../../src/virtual-machine.js');
+
+test('renameSprite throws when there is no sprite with that id', function (t) {
+    var vm = new VirtualMachine();
+    vm.runtime.getTargetById = () => null;
+    t.throws(
+        (() => vm.renameSprite('id', 'name')),
+        new Error('No target with the provided id.')
+    );
+    t.end();
+});
+
+test('renameSprite throws when used on a non-sprite target', function (t) {
+    var vm = new VirtualMachine();
+    var fakeTarget = {
+        isSprite: () => false
+    };
+    vm.runtime.getTargetById = () => (fakeTarget);
+    t.throws(
+        (() => vm.renameSprite('id', 'name')),
+        new Error('Cannot rename non-sprite targets.')
+    );
+    t.end();
+});
+
+test('renameSprite throws when there is no sprite for given target', function (t) {
+    var vm = new VirtualMachine();
+    var fakeTarget = {
+        sprite: null,
+        isSprite: () => true
+    };
+    vm.runtime.getTargetById = () => (fakeTarget);
+    t.throws(
+        (() => vm.renameSprite('id', 'name')),
+        new Error('No sprite associated with this target.')
+    );
+    t.end();
+});
+
+test('renameSprite sets the sprite name', function (t) {
+    var vm = new VirtualMachine();
+    var fakeTarget = {
+        sprite: {name: 'original'},
+        isSprite: () => true
+    };
+    vm.runtime.getTargetById = () => (fakeTarget);
+    vm.renameSprite('id', 'not-original');
+    t.equal(fakeTarget.sprite.name, 'not-original');
+    t.end();
+});
+
+test('renameSprite does not set sprite names to an empty string', function (t) {
+    var vm = new VirtualMachine();
+    var fakeTarget = {
+        sprite: {name: 'original'},
+        isSprite: () => true
+    };
+    vm.runtime.getTargetById = () => (fakeTarget);
+    vm.renameSprite('id', '');
+    t.equal(fakeTarget.sprite.name, 'original');
+    t.end();
+});
+
+test('renameSprite does not set sprite names to reserved names', function (t) {
+    var vm = new VirtualMachine();
+    var fakeTarget = {
+        sprite: {name: 'original'},
+        isSprite: () => true
+    };
+    vm.runtime.getTargetById = () => (fakeTarget);
+    vm.renameSprite('id', '_mouse_');
+    t.equal(fakeTarget.sprite.name, 'original');
+    t.end();
+});
+
+test('renameSprite increments from existing sprite names', function (t) {
+    var vm = new VirtualMachine();
+    vm.emitTargetsUpdate = () => {};
+    vm.runtime.targets = [{
+        id: 'id1',
+        isSprite: () => true,
+        sprite: {
+            name: 'this name'
+        }
+    }, {
+        id: 'id2',
+        isSprite: () => true,
+        sprite: {
+            name: 'that name'
+        }
+    }];
+    vm.renameSprite('id1', 'that name');
+    t.equal(vm.runtime.targets[0].sprite.name, 'that name2');
+    t.end();
+});