Add variables to dropdown menu, and more goodies #3102

towerofnix · 2018-09-09T02:15:26Z

Resolves

Resolves scratchfoundation/scratch-vm#1425: "For this sprite only" (local) variables are now shown in the dropdown.

Resolves scratchfoundation/scratch-blocks#1418: This is unintentional (a side-effect of reusing the basic code for the object dropdown), but the property dropdown now accepts blocks.

Fixes backdrop options (backdrop name, backdrop #) from being shown in sprite dropdowns, and vice versa (x position, etc, in stage dropdown). I couldn't spot an issue for this one.

Depends on scratchfoundation/scratch-blocks#1712. Merge this one after that one!

Proposed Changes

This PR reworks how the PROPERTY dropdown of the "(property) of (object)" sensing reporter block works internally, allowing for the changes listed above.

Note that the property dropdown now accepts blocks. That's not intentional - it's just that I reused most of the code for the object dropdown. I'm not sure how to make it a "square" dropdown (i.e. one which does not accept blocks). In my opinion, letting it accept blocks is a good thing anyways, since it allows and encourages dynamically accessing variables (or even ordinary attributes, like "x position" and "direction"). The argument against it is that an unfamiliar user might try placing a block such as "volume" into that dropdown, and then be confused as to why it always returns 0. But I think that any such user would quickly figure out the error they made; I think the functional benefit greatly outweighs the minor potential difficulty.

That said, I'm okay with making the input not accept blocks, if it'd be preferred to merge this sooner and keep discussion about that topic in its particular issue.

There are some comments I have on specific parts of the code. I'll leave those in a review of the PR.

Reason for Changes

These are nearly all bug-fixes / enhancements which make the Scratch 3.0 editor match 2.0.

Test Coverage

I haven't added any new unit tests, but this has been tested manually:

The "of" block can be dragged out of the flyout with default or changed dropdowns without any errors.
With the stage selected, the dropdown includes (translated) options for properties relevant to the stage, and the project's global variables.
With a sprite selected, the dropdown includes (translated) options for properties relevant to sprites, and all variables which are local to that sprite, and no global variables.
With a block placed inside the object dropdown, the property dropdown includes generic properties applicable to sprites (x position, volume, etc), and no variables.
Variables are sorted the same way other variable menus are sorted.

towerofnix

Left miscellaneous comments (one bug which definitely needs work).

Just another thing to note - I haven't tested this, but I'm pretty sure that, if you rename a variable which is selected in the dropdown, it will not be updated in that dropdown. Of course, the user would expect it to be, since variable dropdowns do update. That'll take a little more work in this PR if wanted.

src/lib/blocks.js

@@ -110,6 +110,75 @@ export default function (vm) {
        return [[myself, '_myself_']].concat(spriteMenu());
    };

+    const variablePropertyMenu = function (thisValue) {


src/lib/blocks.js

+            [costumeName, 'costume name'],
+            [size, 'size'],
+            [volume, 'volume']
+        ];


src/lib/blocks.js

+        ];
+
+        const sourceBlock = thisValue.sourceBlock_; // This is the <shadow>.
+        const ofBlock = sourceBlock && sourceBlock.parentBlock_; // This is the "of" block.


src/lib/blocks.js

+                    const target = vm.runtime.getSpriteTargetByName(objectName);
+                    if (target) {
+                        // Pass true to skip the stage: we only want the sprite's own local variables.
+                        const variableNames = target.getAllVariableNamesInScopeByType('', true);


towerofnix · 2018-09-09T02:34:35Z

Travis is having a whole bunch of errors - they probably have to do with scratch-blocks not being updated by this PR. There aren't any errors when I do npm run test:unit on my computer.

src/lib/blocks.js

…ed by a block (This commit diff best viewed with -w / ?w=1.)

The getAllVariableNamesInScopeByType returns variable names in an arbitrary (unsorted) order. Use scratchBlocksUtils.compareStrings to sort the result using the same method as other places where variables are sorted (see scratchfoundation/scratch-blocks#1521).

towerofnix · 2018-09-15T19:46:13Z

Hmm... Tricky problem with this is that existing Scratch 3.0 projects are broken. 2.0 projects adjust properly, but 3.0 projects lose the data previously stored in the PROPERTY field (which is now an input). (Note the console message Ignoring non-existent field PROPERTY in block sensing_of from blockly.)

kchadha · 2018-10-29T14:38:54Z

@towerofnix, sorry for the long delay in reviewing this pull request.

I'm afraid since a side effect of this PR is that the property menu of the sensing_of block becomes droppable, we cannot accept this PR.

towerofnix · 2018-10-29T15:05:49Z

@kchadha No worries.

Any thoughts on the code which would be needed to implement this, then? The only other dropdown which is not-droppable and dynamically-filled is the variable/list/message dropdowns, but these are all implemented in ways which rely on accessing variables either specific the currently selected sprite or shared across all sprites. (The code for doing so is in scratch-blocks, which indeed does not have access to other sprites the same way the GUI and VM do.)

And I know the Scratch Team is not obligated to, but I request that you consider the paragraph I've quoted below. And, by necessity, I request that it be considered soon (i.e. not be backlogged): making the block droppable after the release of 3.0 will break many 3.0 projects, but making it before will break virtually none, due to so few existing.

Note that the property dropdown now accepts blocks. That's not intentional - it's just that I reused most of the code for the object dropdown. I'm not sure how to make it a "square" dropdown (i.e. one which does not accept blocks). In my opinion, letting it accept blocks is a good thing anyways, since it allows and encourages dynamically accessing variables (or even ordinary attributes, like "x position" and "direction"). The argument against it is that an unfamiliar user might try placing a block such as "volume" into that dropdown, and then be confused as to why it always returns 0. But I think that any such user would quickly figure out the error they made; I think the functional benefit greatly outweighs the minor potential difficulty.

towerofnix · 2018-10-29T15:17:07Z

PS, several existing projects would be broken by not making the input droppable. The fact is that unless scratchfoundation/scratch-vm#1030 - a rather colossal task nobody has gotten anywhere seriously far with since its creation in April - is resolved, any blocks which use hacked arguments will break. Several projects do place arguments inside the "of" block; while I don't have any statistic, it's one of the more common uses of hacked arguments that I've seen. Not making the input droppable means these projects will break.

kchadha · 2018-10-29T18:14:25Z

Any thoughts on the code which would be needed to implement this, then?

@towerofnix, that's a great question, as you pointed out, it's definitely not straightforward given the current examples we have of making block menus dynamic. @picklesrus and I looked into this a bit and the official blockly way to do it is to replace the "options" part of the JSON block definition for the property field with a function that populates the menu options. From just a preliminary look at what it would take to do this, there are definitely some tricky bits and also some architectural issues to figure out on our end, especially since the function will have to take in information about what is selected on the other menu in the block.

Looking at the original issue that this PR was trying to fix, I noticed that it does not have a help wanted label on it. I am inclined to close this and the related scratch-blocks PRs since the implementation for the issue fix will require some more thought.

towerofnix added 4 commits September 8, 2018 22:20

Make PROPERTY menu return local variables of selected sprite

ae647e5

Include typical sprite/stage properties in PROPERTY menu

278948a

Fix an issue where dragging out of the flyout wouldn't work sometimes

a2e3acc

Remove unwanted code

3f9d7e6

towerofnix mentioned this pull request Sep 9, 2018

Support for GUI overriding contents of "of" block property menu scratchfoundation/scratch-blocks#1712

Closed

Lint

ebb8f95

towerofnix commented Sep 9, 2018

View reviewed changes

joker314 reviewed Sep 9, 2018

View reviewed changes

src/lib/blocks.js Outdated Show resolved Hide resolved

towerofnix added 2 commits September 9, 2018 08:39

Include stage's 'local' variables in PROPERTY menu

f883344

Show list of generic sprite properties when OBJECT dropdown is occupi…

dda084c

…ed by a block (This commit diff best viewed with -w / ?w=1.)

thisandagain added the pr - needs review label Sep 11, 2018

thisandagain assigned kchadha Sep 11, 2018

thisandagain added this to the September 2018 milestone Sep 11, 2018

towerofnix mentioned this pull request Sep 19, 2018

Fields should not be saved separately from inputs scratchfoundation/scratch-parser#47

Open

kchadha assigned thisandagain Sep 27, 2018

kchadha added the needs-discussion label Sep 27, 2018

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

kchadha assigned towerofnix and unassigned thisandagain and kchadha Oct 29, 2018

kchadha closed this Oct 29, 2018

towerofnix mentioned this pull request Oct 30, 2018

Accept reporters in property field of sensing_of block scratchfoundation/scratch-blocks#1418

Open

benjiwheeler added needs discussion and removed needs-discussion labels May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add variables to dropdown menu, and more goodies #3102

Add variables to dropdown menu, and more goodies #3102

towerofnix commented Sep 9, 2018 •

edited

Loading

towerofnix left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

towerofnix commented Sep 9, 2018

towerofnix commented Sep 15, 2018

kchadha commented Oct 29, 2018

towerofnix commented Oct 29, 2018 •

edited

Loading

towerofnix commented Oct 29, 2018

kchadha commented Oct 29, 2018

Add variables to dropdown menu, and more goodies #3102

Add variables to dropdown menu, and more goodies #3102

Conversation

towerofnix commented Sep 9, 2018 • edited Loading

Resolves

Proposed Changes

Reason for Changes

Test Coverage

towerofnix left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

towerofnix commented Sep 9, 2018

towerofnix commented Sep 15, 2018

kchadha commented Oct 29, 2018

towerofnix commented Oct 29, 2018 • edited Loading

towerofnix commented Oct 29, 2018

kchadha commented Oct 29, 2018

towerofnix commented Sep 9, 2018 •

edited

Loading

towerofnix commented Oct 29, 2018 •

edited

Loading