From 9fce4d40d04af8d75541897b5f437a495688b6bf Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Tue, 5 Jun 2018 11:46:52 -0400 Subject: [PATCH 01/10] Fix bug with zero adding a costume/sound at index 0 --- src/sprites/rendered-target.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index b6df05e6c..a21bfa4ab 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -488,10 +488,10 @@ class RenderedTarget extends Target { * @param {?int} index Index at which to add costume */ addCostume (costumeObject, index) { - if (index) { - this.sprite.addCostumeAt(costumeObject, index); - } else { + if (typeof index === 'undefined') { this.sprite.addCostumeAt(costumeObject, this.sprite.costumes.length); + } else { + this.sprite.addCostumeAt(costumeObject, index); } } @@ -551,10 +551,10 @@ class RenderedTarget extends Target { addSound (soundObject, index) { const usedNames = this.sprite.sounds.map(sound => sound.name); soundObject.name = StringUtil.unusedName(soundObject.name, usedNames); - if (index) { - this.sprite.sounds.splice(index, 0, soundObject); - } else { + if (typeof index === 'undefined') { this.sprite.sounds.push(soundObject); + } else { + this.sprite.sounds.splice(index, 0, soundObject); } } From 5fd5b30ccfd1c03127e9b4a64ba0298edce993c7 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Tue, 5 Jun 2018 11:49:07 -0400 Subject: [PATCH 02/10] Add reorderCostume method and unit test to rendered target --- src/sprites/rendered-target.js | 22 +++++++++++++ test/unit/sprites_rendered-target.js | 49 ++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index a21bfa4ab..8010cec9d 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -640,6 +640,28 @@ class RenderedTarget extends Target { return this.sprite.costumes; } + /** + * Reorder costume list by moving costume at costumeIndex to newIndex. + * @param {!number} costumeIndex Index of the costume to move. + * @param {!number} newIndex New index for that costume. + */ + reorderCostume (costumeIndex, newIndex) { + const clampedNewIndex = + Math.max(0, Math.min(this.sprite.costumes.length - 1, newIndex)); + const clampedCostumeIndex = + Math.max(0, Math.min(this.sprite.costumes.length - 1, costumeIndex)); + + if (clampedNewIndex === clampedCostumeIndex) return; + const currentCostume = this.getCurrentCostume(); + const costume = this.sprite.costumes[clampedCostumeIndex]; + + // Use the sprite method for deleting costumes because setCostume is handled manually + this.sprite.deleteCostumeAt(clampedCostumeIndex); + + this.addCostume(costume, clampedNewIndex); + this.setCostume(this.getCostumeIndexByName(currentCostume.name)); + } + /** * Get full sound list * @return {object[]} list of sounds diff --git a/test/unit/sprites_rendered-target.js b/test/unit/sprites_rendered-target.js index 9400f4116..7f0ba3540 100644 --- a/test/unit/sprites_rendered-target.js +++ b/test/unit/sprites_rendered-target.js @@ -427,3 +427,52 @@ test('#renameCostume does not duplicate names', t => { t.equal(a.sprite.costumes[1].name, 'first2'); t.end(); }); + +test('#reorderCostume', t => { + const o1 = {id: 0}; + const o2 = {id: 1}; + const o3 = {id: 2}; + const o4 = {id: 3}; + const o5 = {id: 4}; + const s = new Sprite(); + const r = new Runtime(); + s.costumes = [o1, o2, o3, o4, o5]; + const a = new RenderedTarget(s, r); + const renderer = new FakeRenderer(); + a.renderer = renderer; + + const resetCostumes = () => { + a.setCostume(0); + s.costumes = [o1, o2, o3, o4, o5]; + }; + const costumeIds = () => a.sprite.costumes.map(c => c.id); + + resetCostumes(); + t.deepEquals(costumeIds(), [0, 1, 2, 3, 4]); + t.equals(a.currentCostume, 0); + + // Make sure reordering up and down works and current costume follows + resetCostumes(); + a.reorderCostume(0, 3); + t.deepEquals(costumeIds(), [1, 2, 3, 0, 4]); + t.equals(a.currentCostume, 3); // Index of id=0 + + resetCostumes(); + a.setCostume(1); + a.reorderCostume(3, 1); + t.deepEquals(costumeIds(), [0, 3, 1, 2, 4]); + t.equals(a.currentCostume, 2); // Index of id=1 + + // Out of bounds indices get clamped + resetCostumes(); + a.reorderCostume(10, 0); + t.deepEquals(costumeIds(), [4, 0, 1, 2, 3]); + t.equals(a.currentCostume, 1); // Index of id=0 + + resetCostumes(); + a.reorderCostume(2, -1000); + t.deepEquals(costumeIds(), [2, 0, 1, 3, 4]); + t.equals(a.currentCostume, 1); // Index of id=0 + + t.end(); +}); From d3fd3ff806db8d32ef1571dad37087234e89e761 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Tue, 5 Jun 2018 11:49:51 -0400 Subject: [PATCH 03/10] Add reorderSound and unit test to rendered-target --- src/sprites/rendered-target.js | 18 ++++++++++++ test/unit/sprites_rendered-target.js | 42 ++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index 8010cec9d..3e27f5fa5 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -662,6 +662,24 @@ class RenderedTarget extends Target { this.setCostume(this.getCostumeIndexByName(currentCostume.name)); } + /** + * Reorder sound list by moving sound at soundIndex to newIndex. + * @param {!number} soundIndex Index of the sound to move. + * @param {!number} newIndex New index for that sound. + */ + reorderSound (soundIndex, newIndex) { + const clampedNewIndex = + Math.max(0, Math.min(this.sprite.sounds.length - 1, newIndex)); + const clampedSoundIndex = + Math.max(0, Math.min(this.sprite.sounds.length - 1, soundIndex)); + + if (clampedNewIndex === clampedSoundIndex) return; + + const sound = this.sprite.sounds[clampedSoundIndex]; + this.deleteSound(clampedSoundIndex); + this.addSound(sound, clampedNewIndex); + } + /** * Get full sound list * @return {object[]} list of sounds diff --git a/test/unit/sprites_rendered-target.js b/test/unit/sprites_rendered-target.js index 7f0ba3540..ff1a24b4b 100644 --- a/test/unit/sprites_rendered-target.js +++ b/test/unit/sprites_rendered-target.js @@ -476,3 +476,45 @@ test('#reorderCostume', t => { t.end(); }); + +test('#reorderSound', t => { + const o1 = {id: 0, name: 'name0'}; + const o2 = {id: 1, name: 'name1'}; + const o3 = {id: 2, name: 'name2'}; + const o4 = {id: 3, name: 'name3'}; + const o5 = {id: 4, name: 'name4'}; + const s = new Sprite(); + const r = new Runtime(); + s.sounds = [o1, o2, o3, o4, o5]; + const a = new RenderedTarget(s, r); + const renderer = new FakeRenderer(); + a.renderer = renderer; + + const resetSounds = () => { + s.sounds = [o1, o2, o3, o4, o5]; + }; + const soundIds = () => a.sprite.sounds.map(c => c.id); + + resetSounds(); + t.deepEquals(soundIds(), [0, 1, 2, 3, 4]); + + // Make sure reordering up and down works and current sound follows + resetSounds(); + a.reorderSound(0, 3); + t.deepEquals(soundIds(), [1, 2, 3, 0, 4]); + + resetSounds(); + a.reorderSound(3, 1); + t.deepEquals(soundIds(), [0, 3, 1, 2, 4]); + + // Out of bounds indices get clamped + resetSounds(); + a.reorderSound(10, 0); + t.deepEquals(soundIds(), [4, 0, 1, 2, 3]); + + resetSounds(); + a.reorderSound(2, -1000); + t.deepEquals(soundIds(), [2, 0, 1, 3, 4]); + + t.end(); +}); From e6ebc33e9ee6dd1c196bfae2ef2f219d19b09226 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Tue, 5 Jun 2018 12:01:24 -0400 Subject: [PATCH 04/10] Add sound and costume reordering methods and tests to top-level API --- src/virtual-machine.js | 32 +++++++++++++++++++ test/unit/virtual-machine.js | 60 ++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+) diff --git a/src/virtual-machine.js b/src/virtual-machine.js index 1c9c2883d..ce7c76430 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -1033,6 +1033,38 @@ class VirtualMachine extends EventEmitter { return null; } + /** + * Reorder the costumes of a target if it exists. Return whether it succeeded. + * @param {!string} targetId ID of the target which owns the costumes. + * @param {!number} costumeIndex index of the costume to move. + * @param {!number} newIndex index that the costume should be moved to. + * @returns {boolean} whether the target was found for reordering. + */ + reorderCostume (targetId, costumeIndex, newIndex) { + const target = this.runtime.getTargetById(targetId); + if (target) { + target.reorderCostume(costumeIndex, newIndex); + return true; + } + return false; + } + + /** + * Reorder the sounds of a target if it exists. Return whether it succeeded. + * @param {!string} targetId ID of the target which owns the sounds. + * @param {!number} soundIndex index of the sound to move. + * @param {!number} newIndex index that the sound should be moved to. + * @returns {boolean} whether the target was found for reordering. + */ + reorderSound (targetId, soundIndex, newIndex) { + const target = this.runtime.getTargetById(targetId); + if (target) { + target.reorderSound(soundIndex, newIndex); + return true; + } + return false; + } + /** * Put a target into a "drag" state, during which its X/Y positions will be unaffected * by blocks. diff --git a/test/unit/virtual-machine.js b/test/unit/virtual-machine.js index f86267ae8..170b9893a 100644 --- a/test/unit/virtual-machine.js +++ b/test/unit/virtual-machine.js @@ -306,6 +306,66 @@ test('duplicateSprite assigns duplicated sprite a fresh name', t => { }); +test('reorderCostume', t => { + const vm = new VirtualMachine(); + vm.emitTargetsUpdate = () => {}; + + const spr = new Sprite(null, vm.runtime); + spr.name = 'foo'; + const target = spr.createClone(); + + // Stub out reorder on target, tested in rendered-target tests. + // Just want to know if it is called with the right params. + let costumeIndex = null; + let newIndex = null; + target.reorderCostume = (_costumeIndex, _newIndex) => { + costumeIndex = _costumeIndex; + newIndex = _newIndex; + }; + + vm.runtime.targets = [target]; + + t.equal(vm.reorderCostume('not-a-target', 0, 3), false); + t.equal(costumeIndex, null); + t.equal(newIndex, null); + + t.equal(vm.reorderCostume(target.id, 0, 3), true); + t.equal(costumeIndex, 0); + t.equal(newIndex, 3); + + t.end(); +}); + +test('reorderSound', t => { + const vm = new VirtualMachine(); + vm.emitTargetsUpdate = () => {}; + + const spr = new Sprite(null, vm.runtime); + spr.name = 'foo'; + const target = spr.createClone(); + + // Stub out reorder on target, tested in rendered-target tests. + // Just want to know if it is called with the right params. + let soundIndex = null; + let newIndex = null; + target.reorderSound = (_soundIndex, _newIndex) => { + soundIndex = _soundIndex; + newIndex = _newIndex; + }; + + vm.runtime.targets = [target]; + + t.equal(vm.reorderSound('not-a-target', 0, 3), false); + t.equal(soundIndex, null); // Make sure reorder function was not called somehow + t.equal(newIndex, null); + + t.equal(vm.reorderSound(target.id, 0, 3), true); + t.equal(soundIndex, 0); // Make sure reorder function was called correctly + t.equal(newIndex, 3); + + t.end(); +}); + test('emitWorkspaceUpdate', t => { const vm = new VirtualMachine(); const blocksToXML = comments => { From 837703d4ac3a4566dd132267c3b27ec25508f4c0 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Tue, 5 Jun 2018 12:12:43 -0400 Subject: [PATCH 05/10] Make reorder functions return if a change occurred. Gotta get that testing coverage percent up :) --- src/sprites/rendered-target.js | 9 +++++++-- test/unit/sprites_rendered-target.js | 22 ++++++++++++++-------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index 3e27f5fa5..739a813a4 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -644,6 +644,7 @@ class RenderedTarget extends Target { * Reorder costume list by moving costume at costumeIndex to newIndex. * @param {!number} costumeIndex Index of the costume to move. * @param {!number} newIndex New index for that costume. + * @returns {boolean} If a change occurred (i.e. if the indices do not match) */ reorderCostume (costumeIndex, newIndex) { const clampedNewIndex = @@ -651,7 +652,8 @@ class RenderedTarget extends Target { const clampedCostumeIndex = Math.max(0, Math.min(this.sprite.costumes.length - 1, costumeIndex)); - if (clampedNewIndex === clampedCostumeIndex) return; + if (clampedNewIndex === clampedCostumeIndex) return false; + const currentCostume = this.getCurrentCostume(); const costume = this.sprite.costumes[clampedCostumeIndex]; @@ -660,12 +662,14 @@ class RenderedTarget extends Target { this.addCostume(costume, clampedNewIndex); this.setCostume(this.getCostumeIndexByName(currentCostume.name)); + return true; } /** * Reorder sound list by moving sound at soundIndex to newIndex. * @param {!number} soundIndex Index of the sound to move. * @param {!number} newIndex New index for that sound. + * @returns {boolean} If a change occurred (i.e. if the indices do not match) */ reorderSound (soundIndex, newIndex) { const clampedNewIndex = @@ -673,11 +677,12 @@ class RenderedTarget extends Target { const clampedSoundIndex = Math.max(0, Math.min(this.sprite.sounds.length - 1, soundIndex)); - if (clampedNewIndex === clampedSoundIndex) return; + if (clampedNewIndex === clampedSoundIndex) return false; const sound = this.sprite.sounds[clampedSoundIndex]; this.deleteSound(clampedSoundIndex); this.addSound(sound, clampedNewIndex); + return true; } /** diff --git a/test/unit/sprites_rendered-target.js b/test/unit/sprites_rendered-target.js index ff1a24b4b..df1c8029c 100644 --- a/test/unit/sprites_rendered-target.js +++ b/test/unit/sprites_rendered-target.js @@ -451,26 +451,29 @@ test('#reorderCostume', t => { t.deepEquals(costumeIds(), [0, 1, 2, 3, 4]); t.equals(a.currentCostume, 0); + // Returns false if the costumes are the same an no change occurred + t.equal(a.reorderCostume(0, 0), false); + // Make sure reordering up and down works and current costume follows resetCostumes(); - a.reorderCostume(0, 3); + t.equal(a.reorderCostume(0, 3), true); t.deepEquals(costumeIds(), [1, 2, 3, 0, 4]); t.equals(a.currentCostume, 3); // Index of id=0 resetCostumes(); a.setCostume(1); - a.reorderCostume(3, 1); + t.equal(a.reorderCostume(3, 1), true); t.deepEquals(costumeIds(), [0, 3, 1, 2, 4]); t.equals(a.currentCostume, 2); // Index of id=1 // Out of bounds indices get clamped resetCostumes(); - a.reorderCostume(10, 0); + t.equal(a.reorderCostume(10, 0), true); t.deepEquals(costumeIds(), [4, 0, 1, 2, 3]); t.equals(a.currentCostume, 1); // Index of id=0 resetCostumes(); - a.reorderCostume(2, -1000); + t.equal(a.reorderCostume(2, -1000), true); t.deepEquals(costumeIds(), [2, 0, 1, 3, 4]); t.equals(a.currentCostume, 1); // Index of id=0 @@ -498,22 +501,25 @@ test('#reorderSound', t => { resetSounds(); t.deepEquals(soundIds(), [0, 1, 2, 3, 4]); + // Return false if indices are the same and no change occurred. + t.equal(a.reorderSound(100000, 99999), false); + // Make sure reordering up and down works and current sound follows resetSounds(); - a.reorderSound(0, 3); + t.equal(a.reorderSound(0, 3), true); t.deepEquals(soundIds(), [1, 2, 3, 0, 4]); resetSounds(); - a.reorderSound(3, 1); + t.equal(a.reorderSound(3, 1), true); t.deepEquals(soundIds(), [0, 3, 1, 2, 4]); // Out of bounds indices get clamped resetSounds(); - a.reorderSound(10, 0); + t.equal(a.reorderSound(10, 0), true); t.deepEquals(soundIds(), [4, 0, 1, 2, 3]); resetSounds(); - a.reorderSound(2, -1000); + t.equal(a.reorderSound(2, -1000), true); t.deepEquals(soundIds(), [2, 0, 1, 3, 4]); t.end(); From 1ebb736a2aa5c19196c93a6f5d947fea43c0ad29 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Wed, 6 Jun 2018 09:33:23 -0400 Subject: [PATCH 06/10] Check specifically for number in optional property --- src/sprites/rendered-target.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index 739a813a4..4d051114b 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -488,10 +488,10 @@ class RenderedTarget extends Target { * @param {?int} index Index at which to add costume */ addCostume (costumeObject, index) { - if (typeof index === 'undefined') { - this.sprite.addCostumeAt(costumeObject, this.sprite.costumes.length); - } else { + if (typeof index === 'number' && !isNaN(index)) { this.sprite.addCostumeAt(costumeObject, index); + } else { + this.sprite.addCostumeAt(costumeObject, this.sprite.costumes.length); } } @@ -551,10 +551,10 @@ class RenderedTarget extends Target { addSound (soundObject, index) { const usedNames = this.sprite.sounds.map(sound => sound.name); soundObject.name = StringUtil.unusedName(soundObject.name, usedNames); - if (typeof index === 'undefined') { - this.sprite.sounds.push(soundObject); - } else { + if (typeof index === 'number' && !isNaN(index)) { this.sprite.sounds.splice(index, 0, soundObject); + } else { + this.sprite.sounds.push(soundObject); } } From d6d72f6fa7f665c6c0fdca7535e1814c81b69fcf Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Wed, 6 Jun 2018 09:33:59 -0400 Subject: [PATCH 07/10] Use utility function for clamping --- src/sprites/rendered-target.js | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index 4d051114b..c104e2469 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -647,20 +647,18 @@ class RenderedTarget extends Target { * @returns {boolean} If a change occurred (i.e. if the indices do not match) */ reorderCostume (costumeIndex, newIndex) { - const clampedNewIndex = - Math.max(0, Math.min(this.sprite.costumes.length - 1, newIndex)); - const clampedCostumeIndex = - Math.max(0, Math.min(this.sprite.costumes.length - 1, costumeIndex)); + newIndex = MathUtil.clamp(newIndex, 0, this.sprite.costumes.length - 1); + costumeIndex = MathUtil.clamp(costumeIndex, this.sprite.costumes.length - 1, costumeIndex); - if (clampedNewIndex === clampedCostumeIndex) return false; + if (newIndex === costumeIndex) return false; const currentCostume = this.getCurrentCostume(); - const costume = this.sprite.costumes[clampedCostumeIndex]; + const costume = this.sprite.costumes[costumeIndex]; // Use the sprite method for deleting costumes because setCostume is handled manually - this.sprite.deleteCostumeAt(clampedCostumeIndex); + this.sprite.deleteCostumeAt(costumeIndex); - this.addCostume(costume, clampedNewIndex); + this.addCostume(costume, newIndex); this.setCostume(this.getCostumeIndexByName(currentCostume.name)); return true; } @@ -672,16 +670,14 @@ class RenderedTarget extends Target { * @returns {boolean} If a change occurred (i.e. if the indices do not match) */ reorderSound (soundIndex, newIndex) { - const clampedNewIndex = - Math.max(0, Math.min(this.sprite.sounds.length - 1, newIndex)); - const clampedSoundIndex = - Math.max(0, Math.min(this.sprite.sounds.length - 1, soundIndex)); + newIndex = MathUtil.clamp(newIndex, 0, this.sprite.costumes.length - 1); + soundIndex = MathUtil.clamp(soundIndex, this.sprite.sounds.length - 1, soundIndex); - if (clampedNewIndex === clampedSoundIndex) return false; + if (newIndex === soundIndex) return false; - const sound = this.sprite.sounds[clampedSoundIndex]; - this.deleteSound(clampedSoundIndex); - this.addSound(sound, clampedNewIndex); + const sound = this.sprite.sounds[soundIndex]; + this.deleteSound(soundIndex); + this.addSound(sound, newIndex); return true; } From 0cd50fbb2cc96a1385d53b085217e0d5c9bc63c7 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Wed, 6 Jun 2018 09:35:19 -0400 Subject: [PATCH 08/10] Propagate the return value of reorder functions, update comments --- src/virtual-machine.js | 12 +++++------- test/unit/sprites_rendered-target.js | 10 +++++++--- test/unit/virtual-machine.js | 6 ++++-- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/virtual-machine.js b/src/virtual-machine.js index ce7c76430..893f411fc 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -1038,29 +1038,27 @@ class VirtualMachine extends EventEmitter { * @param {!string} targetId ID of the target which owns the costumes. * @param {!number} costumeIndex index of the costume to move. * @param {!number} newIndex index that the costume should be moved to. - * @returns {boolean} whether the target was found for reordering. + * @returns {boolean} Whether a costume was reordered. */ reorderCostume (targetId, costumeIndex, newIndex) { const target = this.runtime.getTargetById(targetId); if (target) { - target.reorderCostume(costumeIndex, newIndex); - return true; + return target.reorderCostume(costumeIndex, newIndex); } return false; } /** - * Reorder the sounds of a target if it exists. Return whether it succeeded. + * Reorder the sounds of a target if it exists. Return whether it occured. * @param {!string} targetId ID of the target which owns the sounds. * @param {!number} soundIndex index of the sound to move. * @param {!number} newIndex index that the sound should be moved to. - * @returns {boolean} whether the target was found for reordering. + * @returns {boolean} Whether a sound was reordered. */ reorderSound (targetId, soundIndex, newIndex) { const target = this.runtime.getTargetById(targetId); if (target) { - target.reorderSound(soundIndex, newIndex); - return true; + return target.reorderSound(soundIndex, newIndex); } return false; } diff --git a/test/unit/sprites_rendered-target.js b/test/unit/sprites_rendered-target.js index df1c8029c..b9b3b9381 100644 --- a/test/unit/sprites_rendered-target.js +++ b/test/unit/sprites_rendered-target.js @@ -451,8 +451,10 @@ test('#reorderCostume', t => { t.deepEquals(costumeIds(), [0, 1, 2, 3, 4]); t.equals(a.currentCostume, 0); - // Returns false if the costumes are the same an no change occurred - t.equal(a.reorderCostume(0, 0), false); + // Returns false if the costumes are the same and no change occurred + t.equal(a.reorderCostume(3, 3), false); + t.equal(a.reorderCostume(999, 5000), false); // Clamped to the same values. + t.equal(a.reorderCostume(-999, -5000), false); // Make sure reordering up and down works and current costume follows resetCostumes(); @@ -502,7 +504,9 @@ test('#reorderSound', t => { t.deepEquals(soundIds(), [0, 1, 2, 3, 4]); // Return false if indices are the same and no change occurred. - t.equal(a.reorderSound(100000, 99999), false); + t.equal(a.reorderSound(3, 3), false); + t.equal(a.reorderSound(100000, 99999), false); // Clamped to the same values + t.equal(a.reorderSound(-100000, -99999), false); // Make sure reordering up and down works and current sound follows resetSounds(); diff --git a/test/unit/virtual-machine.js b/test/unit/virtual-machine.js index 170b9893a..22a69235f 100644 --- a/test/unit/virtual-machine.js +++ b/test/unit/virtual-machine.js @@ -321,6 +321,7 @@ test('reorderCostume', t => { target.reorderCostume = (_costumeIndex, _newIndex) => { costumeIndex = _costumeIndex; newIndex = _newIndex; + return true; // Do not need all the logic about if a reorder occurred. }; vm.runtime.targets = [target]; @@ -351,16 +352,17 @@ test('reorderSound', t => { target.reorderSound = (_soundIndex, _newIndex) => { soundIndex = _soundIndex; newIndex = _newIndex; + return true; // Do not need all the logic about if a reorder occurred. }; vm.runtime.targets = [target]; t.equal(vm.reorderSound('not-a-target', 0, 3), false); - t.equal(soundIndex, null); // Make sure reorder function was not called somehow + t.equal(soundIndex, null); // Make sure reorder function was not called somehow. t.equal(newIndex, null); t.equal(vm.reorderSound(target.id, 0, 3), true); - t.equal(soundIndex, 0); // Make sure reorder function was called correctly + t.equal(soundIndex, 0); // Make sure reorder function was called correctly. t.equal(newIndex, 3); t.end(); From bf2879ee0b8f527148b80848c49075c37732ce56 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Wed, 6 Jun 2018 15:01:02 -0400 Subject: [PATCH 09/10] Fix incorrect clamping --- src/sprites/rendered-target.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index c104e2469..f9607b8bc 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -648,7 +648,7 @@ class RenderedTarget extends Target { */ reorderCostume (costumeIndex, newIndex) { newIndex = MathUtil.clamp(newIndex, 0, this.sprite.costumes.length - 1); - costumeIndex = MathUtil.clamp(costumeIndex, this.sprite.costumes.length - 1, costumeIndex); + costumeIndex = MathUtil.clamp(costumeIndex, 0, this.sprite.costumes.length - 1); if (newIndex === costumeIndex) return false; @@ -670,8 +670,8 @@ class RenderedTarget extends Target { * @returns {boolean} If a change occurred (i.e. if the indices do not match) */ reorderSound (soundIndex, newIndex) { - newIndex = MathUtil.clamp(newIndex, 0, this.sprite.costumes.length - 1); - soundIndex = MathUtil.clamp(soundIndex, this.sprite.sounds.length - 1, soundIndex); + newIndex = MathUtil.clamp(newIndex, 0, this.sprite.sounds.length - 1); + soundIndex = MathUtil.clamp(soundIndex, 0, this.sprite.sounds.length - 1); if (newIndex === soundIndex) return false; From 28f425d53eb12f531101dfa50847c8d7f30985a4 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Wed, 6 Jun 2018 17:29:48 -0400 Subject: [PATCH 10/10] Set costume directly, no updates are needed --- src/sprites/rendered-target.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index f9607b8bc..d6fa70651 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -659,7 +659,7 @@ class RenderedTarget extends Target { this.sprite.deleteCostumeAt(costumeIndex); this.addCostume(costume, newIndex); - this.setCostume(this.getCostumeIndexByName(currentCostume.name)); + this.currentCostume = this.getCostumeIndexByName(currentCostume.name); return true; }