[Google Blockly] Add shared functions and behaviors to start blocks by mikeharv · Pull Request #54820 · code-dot-org/code-dot-org

mikeharv · 2023-11-09T19:48:57Z

Context

If a Sprite Level is configured to include "shared functions and behaviors", we check for the presence of these specific behavior blocks and add them to the student's project if they are not found. Most commonly, a project will always be loaded into a level that includes shared behaviors, but we need to avoid duplicating them. However, a project could be started in a level without shared behaviors, but then loaded into a level that includes them. We also need to ensure that all appended procedures work as expected when the user is using a translation. While the list of shared behaviors does not change regularly, it's also possible for new ones to be added at any time.

There are two pieces of relevant block state at work:

behavior id: The behavior id typically matches the name of the behavior. For shared behaviors, the id is locked to the English name. For user-created behaviors, the id is dynamic and gets updated whenever the behavior is renamed. The logic for shared behavior ids is important to not break things when translated.
A "user-created" flag: This flag is added to new behavior definitions when created, signaling that they are not shared behaviors (or other important behaviors defined by the curriculum writer)

Changes to block state and serialization

In CDO Blockly, we create an id property right on the behavior block's "NAME" field. This is serialized in XML as an id attribute on the <title/> tag. (Title is equivalent to field in older versions of Blockly.) Mainline Blockly doesn't have the ability to deserialize random attributes like this; instead it expects us to use a mutator. When loading a project from XML/CDO Blockly, we will manually search for this field attribute and move it to the mutation element. The value will then be stored directly on the block (block.behaviorId) and serialized to JSON as part of extraState.
When a new behavior is created, CDO Blockly creates a block.userCreated property on the block. This gets serialized to a usercreated="true" attribute on the block element. (Note that this block property and XML attribute are missing on blocks not created by the user.) Again, we want to move this to a mutation. Now, Google Blockly should always create a block.userCreated property during deserialization and new block creation. This will be serialized to JSON as part of extraState.

Appending Shared Behaviors

Both versions of Blockly already support adding shared behavior blocks to a project, provided that the source is XML. However, because Google Blockly saves Sprite Lab projects using JSON, we need to be able to perform the same operation with either format. [Example 1: If a user's project contains no shared behaviors, but then the level is updated to include them, they should be added on next load. Example 2: If we eventually start saving level block configurations with JSON on levelbuilder, we will need to be able handle shared behaviors.
To accomplish this, we follow these general steps:

When preparing start blocks, check if the current start blocks are JSON and whether shared "functions" exist.
As shared functions are currently always in XML, convert them to JSON by loading them onto a headless workspace.
Compare both JSON serialization objects. If a matching behaviorId is found, skip adding that behavior.
- Note that while userCreated is not needed to determine a match directly, it is needed to ensure the ids are accurate.
If a behavior needs to be added, copy the block and its associated procedure (based on its procedureId property, provided by core Blockly)

Hiding the delete button

To match the logic of CDO Blockly, we want to hide the modal editor's delete button for shared behaviors. This can be accomplished simply now by checking the block.userCreated property if the definition block is a behavior.

Function	User Behavior	Shared Behavior

Links

https://codedotorg.atlassian.net/browse/CT-153

https://codedotorg.atlassian.net/browse/CT-143

Testing story

While the above summary captures the logic, a variety of scenarios are worth checking to be sure these changes work as expected. For each video capture below, a project that has been saved as JSON is reloaded with shared behaviors enabled. In some cases, this setting was off when creating the project and in other cases it was always on. Console logging is used to better illustrate what is happening, but console.log() commands were removed before committing.

Blank project with no shared behaviors | All shared behaviors added

The simplest case. If shared behaviors are not present, we expect all of them to be added.

no.shared.behaviors.mp4

In Spanish:

shared.behaviors.i18n.mp4

Normal project with shared behaviors | No behaviors added

If all shared behaviors are already present, we expect nothing to be added on next project load.

shared.behaviors.mp4

In Spanish:

shared.behaviors.i18n.mp4

Project with renamed shared behaviors | No behaviors added

If a user renames "fluttering" to "jumping" (or makes other code changes), the behaviorId is not changed. We don't add another "fluttering" behavior to replace the original.

renamed.shared.behavior.mp4

In Spanish, using a different renamed behavior:

renamed.shared.behaviors.i18n.mp4

No shared behaviors. User-created behavior named after a shared behavior, e.g. "wandering" | Most behaviors added, but no duplicates

If a user happens to create a behavior with a shared behavior's name (e.g. 'wandering'), its behaviorId will match and we will not add a second 'wandering' behavior.

2023-11-15.18-21-50.2023-11-16.12_15_11.mp4

In Spanish:

custom.wandering.i18n.mp4

Edge case: Project with shared behaviors. New shared behavior is created between loading. | Only new procedure added, no duplicates

Rather than adding a new behavior to my local environment, I simulated this by removing the 4 "looping" behaviors from the project serialization by manually editing the main.json file stored in S3.

removed.looping.behaviors.mp4

Deployment strategy

Follow-up work

Prevent users from clicking the "Delete" button unless a project is user-created.

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 · 2023-11-16T17:35:48Z

    startBlocks = config.level.lastAttempt || startBlocks;
  }

+  // Only used in Sprite Lab.


moved up and out of the if (isXml) condition

mikeharv · 2023-11-16T17:40:13Z

+    fieldDescription = block.getFieldValue('DESCRIPTION');
  }
-  return description;
+  return fieldDescription || block.description;


This change is unrelated to the PR but was committed. It ensures that description fields are properly populated for shared behaviors. These blocks have a description property but do not automatically have a value set in the new description field on the block.

molly-moen · 2023-11-16T19:42:32Z

For this case:

No shared behaviors. User-created behavior named after a shared behavior, e.g. "wandering" | Most behaviors added, but no duplicates

If a user is in a different language and they add a behavior named after a shared behavior in their language (ex. for Spanish they have a behavior named "nerviosisimo") will we add a duplicate behavior because the behavior id for that block will be "jittering"? Is that a problem?

mikeharv · 2023-11-16T20:35:12Z

For this case:

No shared behaviors. User-created behavior named after a shared behavior, e.g. "wandering" | Most behaviors added, but no duplicates

If a user is in a different language and they add a behavior named after a shared behavior in their language (ex. for Spanish they have a behavior named "nerviosisimo") will we add a duplicate behavior because the behavior id for that block will be "jittering"? Is that a problem?

Indeed. They'll end up with a "nerviosisimo2" behavior that acts like the shared behavior and they'll keep their original custom one too. This matches how things work on CDO Blockly, but it that doesn't mean it's the perfect logic. I do wonder how we feel about this as an edge case of edge cases or whether there's an obvious quick fix to prevent this difference.

(By edge case I mean that this would only impact projects that 1. were created in non-English, 2. were created without shared behaviors, 3. happen to have a custom behavior named exactly after the translation of one of the shared behaviors, 4. suddenly were able to reload this project in a level that includes shared behaviors, either due to a curriculum level getting updated -rare- or template-backed progression that switches this setting -also rare.)

molly-moen · 2023-11-16T20:41:03Z

@mikeharv that seems fine, it's a super niche edge case so as long as things don't break or they have 2 behaviors with conflicting names that seems reasonable to me.

molly-moen

Looks good, a couple comment suggestions and some clarifying questions for my understanding

molly-moen · 2023-11-16T20:48:49Z

  return block?.style?.colourPrimary;
 }
+
+export function appendSharedFunctions(startBlocksSource, functionsXml) {


mind adding a comment explaining what this function does?

molly-moen · 2023-11-16T20:51:01Z

  return combinedSerialization;
 }
+
+export function convertFunctionsXmlToJson(functionsXml) {


can you add comments on this function and appendProceduresToState?

molly-moen · 2023-11-16T20:52:07Z

+    blockElement.querySelector('mutation') ||
+    blockElement.ownerDocument.createElement('mutation');
+  // Place mutator before fields, values, and other nested blocks.
+  blockElement.insertBefore(mutationElement, blockElement.firstChild);


if the block element already has the mutationElement, will this add it again or do nothing?

I wondered the same thing! It turns out that this is safe because insertBefore will first remove the element from the element if needed, before re-inserting it.

https://developer.mozilla.org/en-US/docs/Web/API/Node/insertBefore

molly-moen · 2023-11-16T20:55:41Z

+  nameField.setValidator(function (newValue) {
+    // The default validator provided by mainline Blockly. Strips whitespace.
+    const rename = Blockly.Procedures.rename.bind(this);
+    const legalName = rename(newValue);


if it's not a legal name I assume the block won't actually be renamed, right?

So it seems like Procedures.rename basically ensures that we always end up with a legal name. It works by trimming leading/trailing white space, then ensuring it doesn't conflict with another procedure name, and finally adding a number on the end if does. If the new name is falsy (empty string) it gets replaced with a translation of the string "unnamed", provided that Blockly has one.

https://github.com/google/blockly/blob/925a7b9723ac46217c64a1842668fd0a66549449/core/procedures.ts#L193-L225

https://github.com/google/blockly/blob/925a7b9723ac46217c64a1842668fd0a66549449/core/procedures.ts#L113-L138

molly-moen · 2023-11-16T20:56:19Z

      }
    }
-    this.behaviorId = xmlElement.nextElementSibling.getAttribute('id');
+    this.behaviorId = xmlElement.getAttribute('behaviorId');


why don't we need to do anything with userCreated here?

userCreated just exists to tell Blockly that it should update the behaviorId when the behavior gets renamed.

Do we not need to preserve it when parsing xml?

Oh, you're right, I think we do. Apologies for not understanding your original question. This would be needed to get the property onto the block when loading XML. I did most of my testing with JSON sources, so I had a gap here. Will update!

molly-moen

Nice!

Mike Harvey added 6 commits November 9, 2023 14:48

set up cdoUtils

322f1a9

move todo

caaea2a

Merge branch 'staging' into mike/shared-behaviors

056adee

add behaviorId to mutator

787e438

add shared behavior blocks to json projects

309c41f

add unit tests

3632ba8

mikeharv marked this pull request as ready for review November 16, 2023 17:35

mikeharv requested a review from a team November 16, 2023 17:35

mikeharv commented Nov 16, 2023

View reviewed changes

hide delete button for shared behaviors

f84184b

molly-moen reviewed Nov 16, 2023

View reviewed changes

add comments

4672ce4

mikeharv requested a review from molly-moen November 17, 2023 13:13

molly-moen approved these changes Nov 17, 2023

View reviewed changes

mikeharv merged commit e30739b into staging Nov 17, 2023

mikeharv deleted the mike/shared-behaviors branch November 17, 2023 17:57

molly-moen mentioned this pull request Dec 4, 2023

[Blockly] Fix appendSharedFunctions bug #55296

Merged

8 tasks

Conversation

mikeharv commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes to block state and serialization

Appending Shared Behaviors

Hiding the delete button

Links

Testing story

Blank project with no shared behaviors | All shared behaviors added

Normal project with shared behaviors | No behaviors added

Project with renamed shared behaviors | No behaviors added

No shared behaviors. User-created behavior named after a shared behavior, e.g. "wandering" | Most behaviors added, but no duplicates

Edge case: Project with shared behaviors. New shared behavior is created between loading. | Only new procedure added, no duplicates

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

molly-moen commented Nov 16, 2023

Uh oh!

mikeharv commented Nov 16, 2023

Uh oh!

molly-moen commented Nov 16, 2023

Uh oh!

molly-moen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

molly-moen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mikeharv commented Nov 9, 2023 •

edited

Loading