feat: Add stack option to procedures_defreturn block

[TannerGabriel]

Closes #26

[mark-friedman]

Thanks for this! Can you verity (and maybe even add a unit test) that a workspace saved in an app which used an older version of this plugin will work as expected if loaded into an app using this newer version? Basically I just want to make sure that we don't mess up developers who upgrade their plugin.

[TannerGabriel]

Thank you for the feedback. I added a unit test that uses a block definition from the current main branch, and I also manually tested it against the current main. If you’d like me to add additional tests for specific older versions, let me know.

Also, if you know a better way to manipulate mutations in unit tests than domToMutation, let me know.

[mark-friedman]

Thank you for the feedback. I added a unit test that uses a block definition from the current main branch, and I also manually tested it against the current main. If you’d like me to add additional tests for specific older versions, let me know.

Also, if you know a better way to manipulate mutations in unit tests than domToMutation, let me know.
Thanks for the tests!

Given that the current suggested way to implement mutations is via JSON serialization using saveExtraState and loadExtraState, I would suggest using those in your block definition and unit tests.

That said, users of previous versions of the plugin might have used XML serialization or JSON serialization for saving and loading workspaces. So it might be worthwhile to have unit tests for both of those scenarios.

[TannerGabriel]

This is closely related to the question posed in #68 about adding saveExtraState and loadExtraState to all blocks to fully support newer Blockly versions. I can add the functions for the procedures here and rewrite the tests, but if we truly want to update it in the whole repository, it might be preferable to make a separate issue and PR.

[mark-friedman]

I wrote a comment in #68 about the general issue you brought up. Regardless of the resolution of that issue, I think it makes sense to use the JSON-based mutation functions in this PR. Am I missing something?

[TannerGabriel]

Not really. I was only making sure since it would be the first JSON functions introduced in the application and the other unit tests also use XML instead.

The only thing this implies is that I need to implement the saveExtraState and loadExtraState of the procedures_defnoreturn block (similar to #68) since the procedure_defreturn depends on that implementation.

I will make the adjustments and notify you once the PR is ready for re-review.

[mark-friedman]

I believe that it is fine to use the XML-based mutation serialization with some blocks and JSON-based mutation serialization for other blocks. I also believe, though a little less strongly, that you can use both types of mutation serialization with either XML or JSON workspace serialization. Maybe you can test that.

[mark-friedman]

Note that if you have saveExtraState then I'm pretty sure that mutationToDom won't get called. So you should have your new saveExtraState also save the state that mutationToDom is saving. Then you can simplify things by getting rid of mutationToDom for that block.
You can do a similar thing for loadExtraState, but in that case you can't get rid of domToMutation because you'll need that to deserialize old saved programs.
You can take a look at this guide to upgrading from Blockly's XML serialization to JSON.

[TannerGabriel]

Thanks for the guide. The saveExtraState and loadExtraState methods are already functionally equivalent to mutationToDom and domToMutation. I can remove mutationToDom in this PR, but then we should probably also switch the toolbox to JSON and update the lexical variables file the same way. If you’d rather, we can defer that to a separate PR and keep the XML path intact for now.

[mark-friedman]

... we should probably also switch the toolbox to JSON

Sure, though the toolbox isn't really a part of the plugin, per se; it's really here just for testing.

... and update the lexical variables file the same way. If you’d rather, we can defer that to a separate PR and keep the XML path intact for now.

Yeah, let's do that in a separate PR.

[TannerGabriel]

Thanks! Do you see anything else that should be updated or adjusted?

[mark-friedman]

I just thought of a couple of related points with respect to serialization. One is that unfortunately App Inventor has it's own set of save/load methods and they currently require that block's use domToMutation and mutationToDom for serializing their extra state. I'm not quite sure if that affects this PR, but keep it in mind.

Another, more general point, is that some apps which use this plugin, like App Inventor, are very sensitive to the size of their save files, due to cloud costs of storage and to a smaller degree of cloud network costs. So, when adding new extra state try to minimize the resulting serialization by having reasonable defaults (e.g. false or 0) so that you can in many cases, especially for preexisting projects, avoid any extra saved state. I think that this does apply to this PR.

It's possible that @mjgallag might want to consider adding support for blocks' loadExtraState and saveExtraState in App Inventor's Blockly.Xml.blockToDom and Blockly.Xml.domToBlock functions, which would allow us to migrate to just using loadExtraState and saveExtraState. We'd need to be a little careful, though, because we might still want the save file to contain XML rather than JSON. I think that there is a belief that that provides smaller file sizes.

[TannerGabriel]

Thanks for the information. I’ll take another look at the size of the saved data, but this PR should only introduce a single boolean for the stack option. The other variables should keep the same default values as in XML. For existing projects (defined in XML), the definitions are converted to JSON (deserialized with domToMutation and serialized with saveExtraState) when the program is loaded. I’m not sure how AppInventor stores data in the cloud, but if they replace the XML definition with the JSON one, the size difference should be minimal.

I don’t have strict requirements here and am open to suggestions. I personally need the JSON definitions, but I can also extend the classes outside the plugin if that’s preferable. It may be worth waiting until we’ve looked at this issue
in the Blockly V12 update to see whether JSON definitions are actually required to resolve it.