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

[Blockly] Add util for loading blocks #51941

Merged
merged 6 commits into from
May 17, 2023

Conversation

mikeharv
Copy link
Contributor

@mikeharv mikeharv commented May 17, 2023

This change is in preparation of the work to begin serializing a student's Blockly project with JSON. (See https://docs.google.com/document/d/1V367MAAYU0JXETVX6l48_6HW2h93WDzdljJc1ohLZPc/edit# for details.)

Currently, we rely on Blockly.Xml.domToBlockSpace throughout our repo to add blocks to a workspace as these blocks are always encoded in XML. This function is defined in both versions of Blockly so we can not just modify it within our repo. Instead, we can wrap it in a new helper utility that will give us more flexibility in how we load blocks.

This change simply replaces each instance of Blockly.Xml.domToBlockSpace with Blockly.cdoUtils.loadBlocksToWorkspace. This new function is defined in both the CDO Blockly wrapper and the Google Blockly wrapper (via cdoUtils.js). At this point we are simply telling the new function to call the old one, so there are no observable differences in functionality. Once merged, the Google Blockly version of loadBlocksToWorkspace will provide a starting place for beginning to support converting XML to JSON or loading JSON directly if it exists.

Links

Testing story

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

@mikeharv mikeharv changed the title Mike/add util for loading blocks [Blockly] Add util for loading blocks May 17, 2023
@@ -6,6 +6,10 @@ import DCDO from '@cdo/apps/dcdo';
import {APP_HEIGHT} from '@cdo/apps/p5lab/constants';
import {SOUND_PREFIX} from '@cdo/apps/assetManagement/assetPrefix';

export function loadBlocksToWorkspace(workspace, xml, stateToLoad) {
return Blockly.Xml.domToBlockSpace(workspace, xml);
Copy link
Contributor Author

@mikeharv mikeharv May 17, 2023

Choose a reason for hiding this comment

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

Today, we only ever reach this point with workspace and xml. As we add JSON sources to projects, we'd be able to ignore the xml and instead call or try Blockly.serialization.workspaces.load(stateToLoad, workspace).

We will always want to support cases where we have xml only. In that case, we can either call domToBlockSpace like we do above, or consider other paths like converting the xml to json and then using the same load method above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need 3 parameters here? Would there ever be a time when both xml and stateToLoad are both passed and used? Can we use 2 parameters like (workspace, state) and just check to see if state is json or xml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need 3 parameters because we're planning to save the json in parallel with the xml, at least for a time. This way, if we run into any issues loading stateToLoad we'll be able to fall back on the xml.

For context, see the table at this heading: https://docs.google.com/document/d/1V367MAAYU0JXETVX6l48_6HW2h93WDzdljJc1ohLZPc/edit#heading=h.33jxpkjaxa77

One thing that I like about this is that we'd be able to simply check that stateToLoad exists in order to be able to expect that it's JSON.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha - that makes sense. Thanks!

@mikeharv mikeharv marked this pull request as ready for review May 17, 2023 18:02
@mikeharv mikeharv requested review from fisher-alice and a team May 17, 2023 18:02
Copy link
Contributor

@fisher-alice fisher-alice left a comment

Choose a reason for hiding this comment

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

LGTM!

@mikeharv mikeharv merged commit b766323 into staging May 17, 2023
2 checks passed
@mikeharv mikeharv deleted the mike/add-util-for-loading-blocks branch May 17, 2023 20:42
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