Add initialize_global and global_declaration_entry blocks

[TannerGabriel]

As described in #35 it would be nice to allow previousStatement and nextStatement to make multiple declarations look neater. To avoid confusion like #22 I would also only allow declaring global variables at the top level.

[mark-friedman]

If I were to do something like this, @TannerGabriel, I think I would create another block, similar to the "initialize local" block that the "global initialize" blocks that you are proposing would have to go into. You could use the Blockly type checking mechanisms to ensure that only those "global initialize" blocks could go in there. That would fit a bit better with the general idea in the plugin (which comes from App Inventor) that anything with next/prev connectors does not generate code when placed at the top level of the workspace. I.e., that the need to be within some kind of container block.

A potential advantage of this approach is that you could, optionally, limit it so that there is at most only one of these "initialize globals" blocks on a workspace, which would show all the global variables for a program. So your globals wouldn't be scattered all around the place.

Note that you are free to use the fields and API in this plugin to create any blocks you want in your own app that uses the plugin. In this PR I'm only commenting on blocks that the plugin itself provides.

[TannerGabriel]

Hey @mark-friedman, I'm coming back to this after some time. I’ve warmed up to your idea while using the plugin, but I want to clarify a few points since this change has a bigger impact than I first expected:

• If I understand correctly, the global_declaration block should then only be valid inside of the new initialize_global block, so we would change the current _checkPlacement to validate this
• Since the global_declaration would not be top-level anymore, we would need to change the getGlobalNames implementation inside of FieldLexicalVariable. This would also break the compatibility with saved programs of older versions (breaking change).

Let me know if I’m missing anything. I don’t think a small feature like this justifies a breaking change, so if my understanding is correct, I’d either look for a different approach or hold off on implementing it for now.

[mark-friedman]

Hey @mark-friedman, I'm coming back to this after some time. I’ve warmed up to your idea while using the plugin, but I want to clarify a few points since this change has a bigger impact than I first expected:

• If I understand correctly, the global_declaration block should then only be valid inside of the new initialize_global block, so we would change the current _checkPlacement to validate this

I am suggesting creating a new block similar to the global_declaration, but with some new type. Let's call it global_declaration2 It would have previous and next connections .

I was suggesting using the Blockly type system to enforce that the global_declarations block must be placed in the new initialize_global block and that only global_declaration2 blocks could be placed in the new initialize_global block. There are some examples documented here that do something similar, though not exactly same kind of thing.

• Since the global_declaration would not be top-level anymore, we would need to change the getGlobalNames implementation inside of FieldLexicalVariable. This would also break the compatibility with saved programs of older versions (breaking change).

I would still maintain the global_declaration block without previous and next connections for developers who want to use the current way of declaring global variables. The new global_declaration2 block would be for developers who want the kind of "grouped" global declarations that you want. We might want some checking to make sure that developers don't mix and match. getGlobalNames would, presumably, have to be changed to check for the new initialize_global block as well as it's existing mechanism.

Let me know if I’m missing anything. I don’t think a small feature like this justifies a breaking change, so if my understanding is correct, I’d either look for a different approach or hold off on implementing it for now.

[TannerGabriel]

Yeah, that makes a lot more sense. I went ahead and implemented it this way, keeping the changes to getGlobalNames as minimal as possible. Let me know if you have any suggestions or change requests. If not, we can add the new blocks to the documentation.

[mark-friedman]

@TannerGabriel Did you see my earlier suggestion about using the Blockly type system to help enforce that a global_declarations2 block must be placed in the new initialize_global block and also that only global_declaration2 blocks could be placed in the new initialize_global block.

Also note that the general approach in the plugin to invalid block placement is to set a warning and not to disable invalid blocks. This, of course, is related to your PR #71.

I was suggesting using the Blockly type system to enforce that the global_declarations block must be placed in the new initialize_global block and that only global_declaration2 blocks could be placed in the new initialize_global block. There are some examples documented here that do something similar, though not exactly same kind of thing.

[TannerGabriel]

Yes, I saw your earlier suggestions. As I understand it, the Blockly type system only defines which blocks can connect, but it doesn’t stop blocks from being placed at the top level. Connection checks also pass if either block has a null type (unless the strict connection plugin is enabled), which means blocks like simple_local_declaration_statement can still connect to global_declaration2 even after restricting them. My current implementation follows the “Restricting Greater Context” section of the connection checking docs. Let me know if I’ve overlooked anything.

[TannerGabriel]

Regarding block disabling, I’ve already implemented the optional disable setting in #71 and will add it here once #71 is merged.

[mark-friedman]

As I understand it, the Blockly type system only defines which blocks can connect, but it doesn’t stop blocks from being placed at the top level. Connection checks also pass if either block has a null type (unless the strict connection plugin is enabled) ...

Good point! I forgot about the null exception and adding the strict connection plugin would probably have undesired consequences.

[mark-friedman]

From my testing, it looks like there is some inconsistancies in what happens when other blocks are placed within the new initialize_global block. Here's an example:

Note that in some cases there is a warning but not in others. I believe that we always want the warning.

[TannerGabriel]

Thanks for the review. The issue occurs with the lexical_variable_set, lexical_variable_get, and call_noreturn blocks because the warningHandler was overriding existing warnings. More generally, the implementation replaced the current warning text whenever a new one was added and sets all warnings to null if the warningHandler succeeds. To support multiple warnings from different components, I introduced the opt_id parameter to all setWarningText calls.

I also fixed another bug related to block disabling when dragging multiple blocks or reloading the page.

[mark-friedman]

Style changes requested.

[mark-friedman]

LGTM

[mark-friedman]

Do you want to document the new blocks in the README file in this PR or do it in another PR?

[TannerGabriel]

I'm fine with both, whatever you prefer. Should I also only disable the blocks if disableInvalidBlocks is set now that #71 is merged?

[mark-friedman]

I'm fine with both, whatever you prefer.

If you don't mind, then we should document the new blocks, and the warning not to mix and match the two types of global declarations, in this PR.

Should I also only disable the blocks if disableInvalidBlocks is set now that #71 is merged?

That makes sense to me.

[mark-friedman]

LGTM