Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Minecraft] dynamically add procedure call blocks to flyout #57609

Merged
merged 1 commit into from Mar 28, 2024

Conversation

mikeharv
Copy link
Contributor

@mikeharv mikeharv commented Mar 28, 2024

Nine Minecraft levels have a "top_level_procedure_autopopulate": "true" property which is handled in CDO Blockly by automatically adding call blocks for each definition block that has been loaded into the workspace. These levels are all clones of the "Free Play" level found at the end of the Hero progression:

image

This setting allows us to omit these call blocks from the toolbox XML and dynamically create them.

How it works in CDO Blockly

If inject is called with this option, it is added to the Blockly instance. If present, when blocks are loaded with domToBlockSpace, we trigger an update to the flyout:

https://github.com/code-dot-org/blockly/blob/f012d8262f21bae3e54fb11dd8bc29cf0d29f3cd/core/utils/xml.js#L350-L352

This in turn calls flyout_.show() using the current language tree (translated toolbox items):

https://github.com/code-dot-org/blockly/blob/f012d8262f21bae3e54fb11dd8bc29cf0d29f3cd/core/ui/block_space/block_space_editor.js#L668-L673

Google Blockly

The code linked above led me to investigate similar potential paths for Google Blockly. It is easy to add a topLevelProcedureAutopopulate to the Google Blockly wrapper, and the option was already being passed into the inject call by StudioApp.

If it's there, we need to update the flyout after we finish loading blocks to the workspace. CDO Blockly's udpateFlyout is custom, but we can still refresh the contents in Google Blockly by calling show() directly. The languageTree array still exists in Google Blockly. For type info and references, see below:

The languageTree is made up of toolbox items, typically of type BlockInfo: https://github.com/google/blockly/blob/d9ea9b7f44b021186654a153c14c80ce0057e048/core/utils/toolbox.ts#L18-L36

Proposal

We already have a well-established point in time right after blocks have been loaded. At this point, if the wrapper property is there, we can create new toolbox items based on the main workspace serialization source. Note that this setting is only meant to be used in levels without categories and without the modal function editor. For each toolbox item, I created a block element with a mutation that matches what would typically be found in the toolbox XML for a block like it.

This results in the blocks being created in the toolbox as expected:

image

Links

Minecraft bug tracking doc: https://docs.google.com/document/d/1EZ3LKUj091Yf2EXRIuU4Sw9oVzY8KcvU_sschVk8DA0/edit

Testing story

I tested that the call blocks work as expected and show up in the user's language. In an edge case scenario where a user has changed languages after saving progress, the toolbox will still match the names of the actual functions on the workspace.

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

Copy link
Contributor

@ebeastlake ebeastlake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good! Just trying to build a little understanding before ✅: Did you add this option to these levels specifically for this case, or was there already a level option being used like this before in CDO?

// options.languageTree is the translated toolbox info
if (workspace.getFlyout() && workspace.options?.languageTree) {
const callBlocks = [] as ToolboxItemInfo[];
const defBlocks = mainSource.blocks.blocks.filter(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, non-blocking: Prefer just to write out definition blocks here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. If there are no other changes requested, I may save this for an immediate follow-up so we don't have to rerun drone.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

});
if (callBlocks.length) {
// Add the new callblocks to the toolbox and refresh it.
workspace.options.languageTree.contents.push(...callBlocks);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the first place we're using language tree like this? It feels new to me, but I'm surprised to feel like I'm seeing it for the first time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See updated description and let me know if you have more questions. It's new to the main repo, but supported in both Blockly versions.

@mikeharv mikeharv marked this pull request as ready for review March 28, 2024 15:11
@mikeharv
Copy link
Contributor Author

Code looks good! Just trying to build a little understanding before ✅: Did you add this option to these levels specifically for this case, or was there already a level option being used like this before in CDO?

@ebeastlake This option was already present on these levels; it's just that our inject call wasn't doing anything with it. Description has been updated!

Copy link
Contributor

@ebeastlake ebeastlake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, Mike!

@mikeharv mikeharv merged commit 2431374 into staging Mar 28, 2024
2 checks passed
@mikeharv mikeharv deleted the mike/minecraft-call-blocks branch March 28, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants