Make "switch costume" and "switch backdrop" blocks compatible with 2.0 #1517

joker314 · 2018-08-22T19:48:35Z

Resolves

Resolves #1490

Proposed Changes

switch backdrop to block is (EDIT: almost) compatible with 2.0 behaviour
switch costume to block is compatible with 2.0 behaviour
unit tests guard against future incompatibility
make "random backdrop" have lower priority than a costume with that name

Reason for Changes

Making sure that 2.0 projects still work in 3.0.

I placed the "random backdrop" feature as less important than a costume name. This means that if a 2.0 project already had a backdrop with the name "random backdrop", references to it by name will still hold. Was this a good idea?

This is different to how "previous backdrop" and "next backdrop" behave -- they can't be overridden (per 2.0); however this is the same behaviour as "previous costume" and "next costume", which can.

Test Coverage

2 automatic unit tests were added. They all pass after my changes. Expand below to see which failed before (more than was initially documented in the issue).

Assertions which failed prior to changes

Costumes

switch costume to NaN (e.g. 0/0) should flawlessly switch to the first costume.
switch costume to Infinity (e.g. 1/0) should flawlessly switch to the first costume.
switch costume to -Infinity (e.g. -1/0) should flawlessly switch to the first costume.
switch costume to previous backdrop
switch costume to next backdrop should do nothing
Strings should only be interpreted as indices if they contain at least one numeric digit.
switch costume to <1 = 0> should switch to the costume named "false" if it exists Switch costume/backdrop to boolean value switches to costume nr. 0 or 1 #1490

Backdrops

switch backdrop to [previous backdrop] decrements the backdrop even if there's a backdrop with that name
switch backdrop to [next backdrop] increments the backdrop even if there's a backdrop with that name
switch backdrop to NaN (e.g. 0/0) should flawlessly switch to the first backdrop.
switch backdrop to Infinity (e.g. 1/0) should flawlessly switch to the first backdrop.
switch backdrop to -Infinity (e.g. -1/0) should flawlessly switch to the first backdrop.

Manually tested with 239254767
Manually tested with 239255071
Unable to manually test with 86981455 as it wasn't a minimal example
Manually confirmed that switching costume by name still worked

joker314 · 2018-09-15T17:13:03Z

Regarding the unit tests added, would it be a good idea to split them into a separate file -- and, since the backdrop and costume tests are quite similar, use a loop/function for the common bits?

src/blocks/scratch3_looks.js

-                const numCostumes = target.getCostumes().length;
+            } else if (requestedCostume === 'previous costume') {
+                target.setCostume(target.currentCostume - 1);
+            } else if (!isNaN(requestedCostume) &&


src/blocks/scratch3_looks.js

+            } else if (requestedCostume === 'previous costume') {
+                target.setCostume(target.currentCostume - 1);
+            } else if (!isNaN(requestedCostume) &&
+                        (typeof requestedCostume !== 'string' || /\d/g.test(requestedCostume))) {


src/blocks/scratch3_looks.js

+        } else if (requestedBackdrop === 'next backdrop') {
+            stage.setCostume(stage.currentCostume + 1);
+        } else if (requestedBackdrop === 'previous backdrop') {
+            stage.setCostume(stage.currentCostume - 1);


thisandagain · 2018-10-09T17:44:18Z

Thanks a ton @joker314. In addition to the PR, I really appreciate the documentation and research in #1490. 😄

test/unit/blocks_looks.js

+    // Non-existant costumes do nothing
+    t.strictEqual(testCostume(['a', 'b', 'c', 'd'], 'e', 3), 3);
+
+    // Difference between string and numeric arguments


test/unit/blocks_looks.js

+     * @param {number} [currentCostume=1] The 1-indexed default costume for the sprite to start at.
+     * @return {number} The 1-indexed costume index on which the sprite lands.
+     */
+    const testCostume = (costumes, arg, currentCostume = 1) => {


kchadha

@joker314 Thanks for making this pull request and for detailing the Scratch 2.0 behavior in the comment on #1490. I apologize for the delay on this review.

I am requesting some minor changes/code cleanup, and would also like to request adding more comments in the code to illustrate what kinds of cases you're trying to address in which parts of the code.

Thanks for adding the thorough unit tests, they're awesome!

I added a comment about maybe making the switch backdrop behavior similar to switch costume in how it handles backdrops named 'previous backdrop' and 'next backdrop' but I think it's also okay to file that as a follow-up separate issue since what you have now is consistent with Scratch 2.0.

Move the helper functions `testCostume` and `testBackdrop` outside of their test definitions. In addition, define the latter in terms of the former.

The `switch costume` block accepts special values like "next costume" and "previous costume". If you create a costume with these names, these take priority over the special values. However, the `switch backdrop` block keeps these special values for values like "next backdrop", "previous backdrop", "random backdrop". It is impossible to navigate to such a backdrop by name via block. This commit also modifies tests to allow for this. BREAKING CHANGE: specially-named backdrops can now be navigated

joker314 · 2018-10-10T19:35:34Z

Thank you very much for the review @kchadha. I've addressed your thoughtful feedback, and have introduced the breaking change which makes backdrop special arguments be handled in a similar way to costumes.

This pull request is ready for re-review.

Here's a base for what one might want to include in the Compatibility wiki page, if it's useful:

Backdrops named 'previous backdrop' and 'next backdrop' can be switched to via blocks

In Scratch 2.0, switch backdrop to [next backdrop] would switch to the backdrop sequentially following the current backdrop, even if there was a backdrop literally named "next backdrop". The same behaviour is seen in "previous backdrop".

This differed to the behaviour of the switch costume block, which would not perform the block input's special function if there existed a costume with the corresponding name.

To make the blocks more intuitive, the switch backdrop block now uses the switch costume behaviour. This is also likely to be what a user would expect, since the workaround for incrementing a backdrop is trivial and stable, whereas the workaround for switching to a backdrop by name would require trying every costume inefficiently until it is found (unless the order in which the backdrops have been arranged is hard-coded)

We ensure that all code whose purpose may be confusing to grasp is commented; and we remove information that is no longer required.

kchadha

@joker314 Thank you for making the requested changes! This PR looks good to go.

joker314 added 2 commits August 22, 2018 20:38

Create unit tests for SWITCH COSTUME and SWITCH BACKDROP

c81f7ee

Make SWITCH COSTUME and SWITCH BACKDROP compatible with 2.0

3ef61e2

joker314 changed the title ~~Make settings costumes and backdrops through blocks compatible with 2.0~~ Make "switch costume" and "switch backdrop" blocks compatible with 2.0 Aug 22, 2018

thisandagain added the pr - needs review label Aug 23, 2018

thisandagain assigned kchadha Aug 23, 2018

thisandagain added this to the September 2018 milestone Aug 23, 2018

thisandagain requested a review from kchadha September 12, 2018 16:05

kchadha reviewed Oct 9, 2018

View reviewed changes

kchadha assigned thisandagain Oct 9, 2018

thisandagain modified the milestones: September 2018, October 2018 Oct 9, 2018

kchadha reviewed Oct 9, 2018

View reviewed changes

test/unit/blocks_looks.js Outdated

// Non-existant costumes do nothing

t.strictEqual(testCostume(['a', 'b', 'c', 'd'], 'e', 3), 3);

// Difference between string and numeric arguments

This comment was marked as abuse.

Sign in to view

kchadha reviewed Oct 9, 2018

View reviewed changes

test/unit/blocks_looks.js Show resolved Hide resolved

kchadha reviewed Oct 9, 2018

View reviewed changes

kchadha suggested changes Oct 9, 2018

View reviewed changes

kchadha added pr - changes requested and removed pr - needs review labels Oct 9, 2018

joker314 added 6 commits October 10, 2018 18:16

test: add more comments, add additional test

c90e033

refactor: use utility for whitespace-testing

df78a83

refactor: move helper functions for tests

f2a4609

Move the helper functions `testCostume` and `testBackdrop` outside of their test definitions. In addition, define the latter in terms of the former.

style(blocks_looks.js): add semicolon/space

1a7ce09

refactor: use better whitespace-test for backdrops too

0c18b49

docs: Provide more/clearer comments

9c128db

We ensure that all code whose purpose may be confusing to grasp is commented; and we remove information that is no longer required.

joker314 mentioned this pull request Oct 21, 2018

Should the library, or the compiler, absorb Scratch's unusual behaviours? leopard-js/leopard#9

Open

kchadha approved these changes Oct 29, 2018

View reviewed changes

kchadha added pr - good to merge and removed pr - changes requested labels Oct 29, 2018

kchadha merged commit be238d3 into scratchfoundation:develop Oct 29, 2018

joker314 deleted the costume-compatibility branch October 29, 2018 15:44

kchadha mentioned this pull request Feb 20, 2019

Dropped sprite names that are numbers do not work #1743

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make "switch costume" and "switch backdrop" blocks compatible with 2.0 #1517

Make "switch costume" and "switch backdrop" blocks compatible with 2.0 #1517

joker314 commented Aug 22, 2018 •

edited

Loading

joker314 commented Sep 15, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

thisandagain commented Oct 9, 2018

This comment was marked as abuse.

This comment was marked as abuse.

kchadha left a comment

joker314 commented Oct 10, 2018 •

edited

Loading

Backdrops named 'previous backdrop' and 'next backdrop' can be switched to via blocks

kchadha left a comment

Make "switch costume" and "switch backdrop" blocks compatible with 2.0 #1517

Make "switch costume" and "switch backdrop" blocks compatible with 2.0 #1517

Conversation

joker314 commented Aug 22, 2018 • edited Loading

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Costumes

Backdrops

joker314 commented Sep 15, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

thisandagain commented Oct 9, 2018

This comment was marked as abuse.

This comment was marked as abuse.

kchadha left a comment

Choose a reason for hiding this comment

joker314 commented Oct 10, 2018 • edited Loading

Backdrops named 'previous backdrop' and 'next backdrop' can be switched to via blocks

kchadha left a comment

Choose a reason for hiding this comment

joker314 commented Aug 22, 2018 •

edited

Loading

joker314 commented Oct 10, 2018 •

edited

Loading