Add reference for runtime to blocks container
#1941
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…here where it was being referenced. Update calls to blocks constructor.
… blockContainer. It wasn't being used by the tests or the playground and it is not an issue with actual project serialization. Update test to stop passing in runtime to blocklyListen function.
…d RenderedTarget so blocks get created properly.
paulkaplan
approved these changes
Jan 30, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves
Update
Closes #1942.
Added runtime to calls to Blocks constructor in tests since it was necessary for the new tests in the upcoming erroneous project saving work.
Proposed Changes
Add a reference to the runtime in each block container instance.
Thank you to @chrisgarrity for pairing on this!
Reason for Changes
This is work towards fixing erroneous project saving. We want to eventually be able to track actual project changes introduced by changes to the blocks (e.g. creating a new block, changing a block field) vs. always emitting a project change whenever we receive a blockly event (https://github.com/LLK/scratch-vm/blob/develop/src/engine/blocks.js#L493). We will eventually remove that line and make that
emitProjectChangedcall within each of the___Blockfunctions (moveBlock, changeBlock, createBlock, deleteBlock) when a change actually takes place.Test Coverage
This is not a functionality change so we did not add any new tests.
We did add a warning for calling
changeBlockwithout the blocks container having a reference to the runtime. Previously this was an optional argument and none of the tests are testing any runtime needing functionality.We have filed a tech debt issue (#1942) to update all the tests to pass in a runtime when creating a new blocks container instance.We added a reference to theruntimeto the existing calls to the Blocks constructor in the tests.We also removed the optRuntime parameter that was being passed into
blocklyListenin one of the tests since it is no longer needed. That test already follows a code path (e.g. deserializing a project) that will construct the blocks containers in the right way.