[Google Blockly] keep variables in sync between workspaces

[mikeharv]

Recently, two related bug reports came in where Sprite Lab projects were behaving strangely due to variable blocks. My investigation led to the discovery of a few related bugs:

1. We were not using our custom flyout callback for the Variables category on the modal editor workspace - just on the main workspace.
2. When variables are created/renamed/deleted in one workspace, we did not mirror the events to the other workspaces. This could cause a student to create a score in on the main workspace, then not find it on the modal workspace. If they created it a second time there, it would conflict and throw an error.
3. If a toolbox block with a set id is dragged into a workspace, the new block has the same id, unless it's already used. However, Blockly only checks the target workspace. If a block was created on the modal workspace but the id was already in use on the hidden workspace, it would cause problems. If you opened the original function with this block, it was no longer there (because it had been moved to the new function).

Custom Variables Category

We customize the variable category to provide our own button/prompt for creating variables and to make sure that blocks from the level XML get merged with the auto-populated blocks driven by the variable map. When we added this to Google Blockly, we only added it to the main workspace, which resulted in this inconsistent experience:

Main Workspace:

Editor workspace:

Expected custom prompt:

Unexpected prompt:

Fixing this was just a matter of calling registerToolboxCategoryCallback on the editor workspace.

Duplicate block ids from toolbox blocks

If the toolbox XML for a block includes an ID, Blockly will try to give preserve it for the first block created on that workspace. Once the id is in use on that workspace, it will randomly generate new ones. We needed Blockly to preserve this functionality for us because we use these hard-coded block ids for UI tests that involve dragging blocks out of the toolbox. However, if this is done in the modal editor it has the potential to cause problems. Steps:

1. Open the modal editor.
2. Drag out a block with a hard-coded id.
3. Close the modal editor and re-open it with a different function
4. Drag out the same block

Because the block from step 2 is no longer on the editor workspace, Blockly doesn't detect the conflict so it allow the id to be used. The block create event is then mirrored to the hidden workspace, which fails because a block with that id already exists. The subsequent block event is also mirror to the hidden workspace, which "succeeds" by moving the first block into the new position intended for the second block. Now, if you re-open the first function, the expected block has disappeared.

I considered several alternatives here, but ultimately what worked was just to strip block ids out of the toolbox string before injecting the editor workspace. This should be fine because we don't have any UI tests for the modal editor that rely upon the static block id.

Mirroring variable events

Currently, on staging, we mirror events in the following ways:

• Procedure events are mirrored from the modal editor workspace to the main workspace
• All non-UI events are mirrored from the modal editor workspace to the hidden definition workspace

The first rule is needed so that we can create the right procedure call blocks and so that functions and behaviors are still defined on the main workspace. The second rule is needed so that our source of truth for definitions (ie. the hidden workspace) is always kept up to date.

What's missing from the system is proper mirroring of variable events. With Blockly, all variables are created within the global scope. We concatenate the generated code from the main and hidden workspace, so it shouldn't matter which workspace is active when a variable is created. If a variable is created (or renamed or deleted) from the main workspace, we should do the same thing on both the editor and hidden workspaces (and vice versa).

To solve this problem, we need mirror these events from the main workspace to both other workspaces. I did this via a new change listener. We also need to mirror from the modal editor workspace to both the main and hidden workspaces. For the first case, I just latched onto where we were already mirroring procedure events. For the second case, we were already handling this sufficiently because variable events are non-UI events.

The results of the changes here are variables can now be managed on either workspace and will stay in sync. If blocks are dragged out which reference a certain variable, it will either be found on that workspace's map (and all of them) or it will be created. And toolboxes will be stripped of block ids so that we don't continue having issues with duplicate block ids being used on the hidden workspace.

Full Bug Reports

The first was reported by @cnbrenci. From the ticket:

Probable Bug in Sprite Lab.
In project I create Variables “score”. After I create function “score+” and try to call variable “score” in function “score+”, variable is not there. Then I create variable with same name “score” and exit function. Use that function with variable included and save the project. Exit project and after I again enter that project my variable “score” used in function change name to “i”.
https://studio.code.org/projects/spritelab/6bBUKGra5IGR_JIpK8I-tr5Fvc-CUjGwoDkCzOzczMQ/edit

Cassi added on Slack:

Seems like there's also bug(s) around creating vars with the same name as ones in main scope too, I'm seeing uncaught errors in console about conflicting names that aren't showing any indication in UI, and sometimes reverts the var back and sometimes doesn't.
(errors like
Uncaught Error: Variable "mainScope" is already in use and its id is "9?(dNoz*5BTN%k%n)0K1" which conflicts with the passed in id, "(|5Et68[=ZwMGs$s`:4)".)

This screenshot shows a reproduction of the affected user's project, now working as expected:

The second was reported by @molly-moen. From the ticket:

In Sprite Lab, adding “location of” block to a function causes the “location of” block to not save when the function is closed. This happens in the second function that uses the “location of” block.
Repro steps:

• Create a new SpriteLab project
• Create a new function “do something” and edit it
• Add a “Make new sprite at” block to the function.
• Add a “location of” block to the “Make new sprite at” block
• Close the “do something” function
• Create another new function “do something2” and edit it
• Add a “Make new sprite at” block to the function
• Add a “location of” block to the “Make new sprite at” block
• Close the “do something2” function
• Edit the “do something2” function again
  Expected results:
  “do something2” should contain the “location of” block added in step 8. The code should look like this:

function do_something2() {
 makeNewSpriteAnon("purple bunny", locationOf({name: 'mySprite'}));
}

Actual results:
“do something2” has no “location of” block. The code looks like this:

function do_something2() {
 makeNewSpriteAnon("purple bunny", undefined);
}

In the dev console, I see this exception as soon as I add the “location of” block to the second function:

Uncaught Error: Variable "mySprite" is already in use and its id is "I59!Ry8~_7B$y(:#eF)j" which conflicts with the passed in id, "c6;ZQ]cqk;j|Vo]!pnI+".
  at t.createVariable (external var "$":1:18)
  at i.createVariable (external var "$":1:18)
  at T.run (external var "$":1:18)
  at external var "$":1:18
  at i.fireChangeListener (external var "$":1:18)
  at y (external var "$":1:18)
  at nrWrapper (edit:16:17823)

This screenshot demonstrates that the bug described above has been fixed:

Links

Jira: https://codedotorg.atlassian.net/browse/CT-573
Slack thread: https://codedotorg.slack.com/archives/C05DK21DAHK/p1715971343769239
Zendesk tickets:

1. https://codeorg.zendesk.com/agent/tickets/496608
2. https://codeorg.zendesk.com/agent/tickets/496843
   Broken user project: https://studio.code.org/projects/spritelab/6bBUKGra5IGR_JIpK8I-tr5Fvc-CUjGwoDkCzOzczMQ/view

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

[molly-moen]

One request for a comment, otherwise looks good!

[molly-moen]

Can you add a comment explaining why we need to do this?

[mikeharv]

Oh yes, good call! The reason is that the previous line wipes the map, so we rebuild it manually here. (This is also why we can’t just use the same map for all workspaces.)