Merge pull request #943 from fsih/privateSprite

Make sprite.costumes private

src/blocks/scratch3_looks.js

@@ -332,7 +332,7 @@ class Scratch3LooksBlocks {
         }
         if (target === this.runtime.getTargetForStage()) {
             // Target is the stage - start hats.
-            const newName = target.sprite.costumes[target.currentCostume].name;
+            const newName = target.getCostumes()[target.currentCostume].name;
             return this.runtime.startHats('event_whenbackdropswitchesto', {
                 BACKDROP: newName
             });
@@ -442,7 +442,7 @@ class Scratch3LooksBlocks {
             return stage.currentCostume + 1;
         }
         // Else return name
-        return stage.sprite.costumes[stage.currentCostume].name;
+        return stage.getCostumes()[stage.currentCostume].name;
     }
 
     getCostumeNumberName (args, util) {
@@ -450,7 +450,7 @@ class Scratch3LooksBlocks {
             return util.target.currentCostume + 1;
         }
         // Else return name
-        return util.target.sprite.costumes[util.target.currentCostume].name;
+        return util.target.getCostumes()[util.target.currentCostume].name;
     }
 }
 

src/blocks/scratch3_sensing.js

@@ -237,7 +237,7 @@ class Scratch3SensingBlocks {
 
             case 'backdrop #': return attrTarget.currentCostume + 1;
             case 'backdrop name':
-                return attrTarget.sprite.costumes[attrTarget.currentCostume].name;
+                return attrTarget.getCostumes()[attrTarget.currentCostume].name;
             case 'volume': return; // @todo: Keep this in mind for sound blocks!
             }
         } else {
@@ -247,7 +247,7 @@ class Scratch3SensingBlocks {
             case 'direction': return attrTarget.direction;
             case 'costume #': return attrTarget.currentCostume + 1;
             case 'costume name':
-                return attrTarget.sprite.costumes[attrTarget.currentCostume].name;
+                return attrTarget.getCostumes()[attrTarget.currentCostume].name;
             case 'size': return attrTarget.size;
             case 'volume': return; // @todo: above, keep in mind for sound blocks..
             }

src/sprites/rendered-target.js

@@ -381,7 +381,7 @@ class RenderedTarget extends Target {
             index, 0, this.sprite.costumes.length - 1
         );
         if (this.renderer) {
-            const costume = this.sprite.costumes[this.currentCostume];
+            const costume = this.getCostumes()[this.currentCostume];
             const drawableProperties = {
                 skinId: costume.skinId,
                 costumeResolution: costume.bitmapResolution
@@ -410,12 +410,10 @@ class RenderedTarget extends Target {
      * @param {?int} index Index at which to add costume
      */
     addCostume (costumeObject, index) {
-        const usedNames = this.sprite.costumes.map(costume => costume.name);
-        costumeObject.name = StringUtil.unusedName(costumeObject.name, usedNames);
         if (index) {
-            this.sprite.costumes.splice(index, 0, costumeObject);
+            this.sprite.addCostumeAt(costumeObject, index);
         } else {
-            this.sprite.costumes.push(costumeObject);
+            this.sprite.addCostumeAt(costumeObject, this.sprite.costumes.length);
         }
     }
 
@@ -428,9 +426,9 @@ class RenderedTarget extends Target {
         const usedNames = this.sprite.costumes
             .filter((costume, index) => costumeIndex !== index)
             .map(costume => costume.name);
-        const oldName = this.sprite.costumes[costumeIndex].name;
+        const oldName = this.getCostumes()[costumeIndex].name;
         const newUnusedName = StringUtil.unusedName(newName, usedNames);
-        this.sprite.costumes[costumeIndex].name = newUnusedName;
+        this.getCostumes()[costumeIndex].name = newUnusedName;
 
         if (this.isStage) {
             // Since this is a backdrop, go through all targets and
@@ -454,9 +452,7 @@ class RenderedTarget extends Target {
         const originalCostumeCount = this.sprite.costumes.length;
         if (originalCostumeCount === 1) return;
 
-        this.sprite.costumes = this.sprite.costumes
+        this.sprite.deleteCostumeAt(index);
-            .slice(0, index)
-            .concat(this.sprite.costumes.slice(index + 1));
 
         if (index === this.currentCostume && index === originalCostumeCount - 1) {
             this.setCostume(index - 1);
@@ -542,7 +538,7 @@ class RenderedTarget extends Target {
      */
     getCostumeIndexByName (costumeName) {
         for (let i = 0; i < this.sprite.costumes.length; i++) {
-            if (this.sprite.costumes[i].name === costumeName) {
+            if (this.getCostumes()[i].name === costumeName) {
                 return i;
             }
         }
@@ -554,7 +550,7 @@ class RenderedTarget extends Target {
      * @return {object} current costume
      */
     getCurrentCostume () {
-        return this.sprite.costumes[this.currentCostume];
+        return this.getCostumes()[this.currentCostume];
     }
 
     /**
@@ -580,7 +576,7 @@ class RenderedTarget extends Target {
     updateAllDrawableProperties () {
         if (this.renderer) {
             const renderedDirectionScale = this._getRenderedDirectionAndScale();
-            const costume = this.sprite.costumes[this.currentCostume];
+            const costume = this.getCostumes()[this.currentCostume];
             const bitmapResolution = costume.bitmapResolution || 1;
             const props = {
                 position: [this.x, this.y],

src/sprites/sprite.js

@@ -36,7 +36,7 @@ class Sprite {
          * }
          * @type {Array.<!Object>}
          */
-        this.costumes = [];
+        this.costumes_ = [];
         /**
          * List of sounds for this sprite.
         */
@@ -48,6 +48,51 @@ class Sprite {
         this.clones = [];
     }
     
+    /**
+     * Add an array of costumes, taking care to avoid duplicate names.
+     * @param {!Array<object>} costumes Array of objects representing costumes.
+     */
+    set costumes (costumes) {
+        this.costumes_ = [];
+        for (const costume of costumes) {
+            this.addCostumeAt(costume, this.costumes_.length);
+        }
+    }
+
+    /**
+     * Get full costume list
+     * @return {object[]} list of costumes. Note that mutating the returned list will not
+     *     mutate the list on the sprite. The sprite list should be mutated by calling
+     *     addCostumeAt, deleteCostumeAt, or setting costumes.
+     */
+    get costumes () {
+        return this.costumes_.slice(0);
+    }
+
+    /**
+     * Add a costume at the given index, taking care to avoid duplicate names.
+     * @param {!object} costumeObject Object representing the costume.
+     * @param {!int} index Index at which to add costume
+     */
+    addCostumeAt (costumeObject, index) {
+        if (!costumeObject.name) {
+            costumeObject.name = '';
+        }
+        const usedNames = this.costumes_.map(costume => costume.name);
+        costumeObject.name = StringUtil.unusedName(costumeObject.name, usedNames);
+        this.costumes_.splice(index, 0, costumeObject);
+    }
+
+    /**
+     * Delete a costume by index.
+     * @param {number} index Costume index to be deleted
+     */
+    deleteCostumeAt (index) {
+        this.costumes_ = this.costumes_
+            .slice(0, index)
+            .concat(this.costumes_.slice(index + 1));
+    }
+
     /**
      * Create a clone of this sprite.
      * @returns {!RenderedTarget} Newly created clone.
@@ -89,7 +134,7 @@ class Sprite {
 
         const assetPromises = [];
 
-        newSprite.costumes = this.costumes.map(costume => {
+        newSprite.costumes = this.costumes_.map(costume => {
             const newCostume = Object.assign({}, costume);
             const costumeAsset = this.runtime.storage.get(costume.assetId);
             assetPromises.push(loadCostumeFromAsset(newCostume, costumeAsset, this.runtime));

src/virtual-machine.js

@@ -465,7 +465,7 @@ class VirtualMachine extends EventEmitter {
      * @return {string} the costume's SVG string, or null if it's not an SVG costume.
      */
     getCostumeSvg (costumeIndex) {
-        const id = this.editingTarget.sprite.costumes[costumeIndex].assetId;
+        const id = this.editingTarget.getCostumes()[costumeIndex].assetId;
         if (id && this.runtime && this.runtime.storage &&
                 this.runtime.storage.get(id).dataFormat === 'svg') {
             return this.runtime.storage.get(id).decodeText();
@@ -481,7 +481,7 @@ class VirtualMachine extends EventEmitter {
      * @param {number} rotationCenterY y of point about which the costume rotates, relative to its upper left corner
      */
     updateSvg (costumeIndex, svg, rotationCenterX, rotationCenterY) {
-        const costume = this.editingTarget.sprite.costumes[costumeIndex];
+        const costume = this.editingTarget.getCostumes()[costumeIndex];
         if (costume && this.runtime && this.runtime.renderer) {
             costume.rotationCenterX = rotationCenterX;
             costume.rotationCenterY = rotationCenterY;
@@ -510,7 +510,7 @@ class VirtualMachine extends EventEmitter {
         return loadCostume(md5ext, backdropObject, this.runtime).then(() => {
             const stage = this.runtime.getTargetForStage();
             stage.addCostume(backdropObject);
-            stage.setCostume(stage.sprite.costumes.length - 1);
+            stage.setCostume(stage.getCostumes().length - 1);
         });
     }
 

test/unit/blocks_looks.js

@@ -4,6 +4,9 @@ const Runtime = require('../../src/engine/runtime');
 const util = {
     target: {
         currentCostume: 0, // Internally, current costume is 0 indexed
+        getCostumes: function () {
+            return this.sprite.costumes;
+        },
         sprite: {
             costumes: [
                 {name: 'first name'},

test/unit/sprites_rendered-target.js

@@ -117,7 +117,9 @@ test('deleteCostume', t => {
     //    Costume 3
     a.setCostume(0);
     a.deleteCostume(0);
-    t.deepEqual(a.sprite.costumes, [o2, o3]);
+    t.equals(a.sprite.costumes.length, 2);
+    t.equals(a.sprite.costumes[0].id, 2);
+    t.equals(a.sprite.costumes[1].id, 3);
     t.equals(a.currentCostume, 0);
 
     //    Costume 1          Costume 1
@@ -126,7 +128,9 @@ test('deleteCostume', t => {
     a.sprite.costumes = [o1, o2, o3];
     a.setCostume(1);
     a.deleteCostume(1);
-    t.deepEqual(a.sprite.costumes, [o1, o3]);
+    t.equals(a.sprite.costumes.length, 2);
+    t.equals(a.sprite.costumes[0].id, 1);
+    t.equals(a.sprite.costumes[1].id, 3);
     t.equals(a.currentCostume, 1);
 
     //    Costume 1          Costume 1
@@ -135,14 +139,17 @@ test('deleteCostume', t => {
     a.sprite.costumes = [o1, o2, o3];
     a.setCostume(2);
     a.deleteCostume(2);
-    t.deepEqual(a.sprite.costumes, [o1, o2]);
+    t.equals(a.sprite.costumes.length, 2);
+    t.equals(a.sprite.costumes[0].id, 1);
+    t.equals(a.sprite.costumes[1].id, 2);
     t.equals(a.currentCostume, 1);
 
     // Refuses to delete only costume
     a.sprite.costumes = [o1];
     a.setCostume(0);
     a.deleteCostume(0);
-    t.deepEqual(a.sprite.costumes, [o1]);
+    t.equals(a.sprite.costumes.length, 1);
+    t.equals(a.sprite.costumes[0].id, 1);
     t.equals(a.currentCostume, 0);
 
     //   Costume 1          Costume 1
@@ -153,7 +160,11 @@ test('deleteCostume', t => {
     a.sprite.costumes = [o1, o2, o3, o4, o5];
     a.setCostume(3);
     a.deleteCostume(1);
-    t.deepEqual(a.sprite.costumes, [o1, o3, o4, o5]);
+    t.equals(a.sprite.costumes.length, 4);
+    t.equals(a.sprite.costumes[0].id, 1);
+    t.equals(a.sprite.costumes[1].id, 3);
+    t.equals(a.sprite.costumes[2].id, 4);
+    t.equals(a.sprite.costumes[3].id, 5);
     t.equals(a.currentCostume, 2);
 
     //   Costume 1          Costume 1
@@ -164,7 +175,11 @@ test('deleteCostume', t => {
     a.sprite.costumes = [o1, o2, o3, o4, o5];
     a.setCostume(1);
     a.deleteCostume(3);
-    t.deepEqual(a.sprite.costumes, [o1, o2, o3, o5]);
+    t.equals(a.sprite.costumes.length, 4);
+    t.equals(a.sprite.costumes[0].id, 1);
+    t.equals(a.sprite.costumes[1].id, 2);
+    t.equals(a.sprite.costumes[2].id, 3);
+    t.equals(a.sprite.costumes[3].id, 5);
     t.equals(a.currentCostume, 1);
 
     //   Costume 1          Costume 1
@@ -175,7 +190,11 @@ test('deleteCostume', t => {
     a.sprite.costumes = [o1, o2, o3, o4, o5];
     a.setCostume(1);
     a.deleteCostume(4);
-    t.deepEqual(a.sprite.costumes, [o1, o2, o3, o4]);
+    t.equals(a.sprite.costumes.length, 4);
+    t.equals(a.sprite.costumes[0].id, 1);
+    t.equals(a.sprite.costumes[1].id, 2);
+    t.equals(a.sprite.costumes[2].id, 3);
+    t.equals(a.sprite.costumes[3].id, 4);
     t.equals(a.currentCostume, 1);
     t.end();
 });
@@ -333,9 +352,11 @@ test('#stopAll clears graphics effects', t => {
 test('#getCostumes returns the costumes', t => {
     const spr = new Sprite();
     const a = new RenderedTarget(spr, null);
-    const costumes = [1, 2, 3];
+    a.sprite.costumes = [{id: 1}, {id: 2}, {id: 3}];
-    a.sprite.costumes = costumes;
+    t.equals(a.getCostumes().length, 3);
-    t.equals(a.getCostumes(), costumes);
+    t.equals(a.getCostumes()[0].id, 1);
+    t.equals(a.getCostumes()[1].id, 2);
+    t.equals(a.getCostumes()[2].id, 3);
     t.end();
 });
 
@@ -352,11 +373,10 @@ test('#toJSON returns the sounds and costumes', t => {
     const spr = new Sprite();
     const a = new RenderedTarget(spr, null);
     const sounds = [1, 2, 3];
-    const costumes = ['a', 'b', 'c'];
     a.sprite.sounds = sounds;
-    a.sprite.costumes = costumes;
+    a.sprite.costumes = [{id: 1}, {id: 2}, {id: 3}];
     t.same(a.toJSON().sounds, sounds);
-    t.same(a.toJSON().costumes, costumes);
+    t.same(a.toJSON().costumes, a.sprite.costumes);
     t.end();
 });
 
@@ -372,9 +392,11 @@ test('#addSound does not duplicate names', t => {
 test('#addCostume does not duplicate names', t => {
     const spr = new Sprite();
     const a = new RenderedTarget(spr, null);
-    a.sprite.costumes = [{name: 'first'}];
     a.addCostume({name: 'first'});
-    t.deepEqual(a.sprite.costumes, [{name: 'first'}, {name: 'first2'}]);
+    a.addCostume({name: 'first'});
+    t.equal(a.sprite.costumes.length, 2);
+    t.equal(a.sprite.costumes[0].name, 'first');
+    t.equal(a.sprite.costumes[1].name, 'first2');
     t.end();
 });
 
@@ -394,8 +416,12 @@ test('#renameCostume does not duplicate names', t => {
     const a = new RenderedTarget(spr, null);
     a.sprite.costumes = [{name: 'first'}, {name: 'second'}];
     a.renameCostume(0, 'first'); // Shouldn't increment the name, noop
-    t.deepEqual(a.sprite.costumes, [{name: 'first'}, {name: 'second'}]);
+    t.equal(a.sprite.costumes.length, 2);
+    t.equal(a.sprite.costumes[0].name, 'first');
+    t.equal(a.sprite.costumes[1].name, 'second');
     a.renameCostume(1, 'first');
-    t.deepEqual(a.sprite.costumes, [{name: 'first'}, {name: 'first2'}]);
+    t.equal(a.sprite.costumes.length, 2);
+    t.equal(a.sprite.costumes[0].name, 'first');
+    t.equal(a.sprite.costumes[1].name, 'first2');
     t.end();
 });