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

feat: Insertion marker json deserialization 7316 #7364

Conversation

varshneydevansh
Copy link
Contributor

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Proposed Changes

Change the insertion marker createMarkerBlock code over to use JSON serialization and deserialization.

Behavior Before Change

In Blockly's XML serialization, child blocks are represented using nested XML elements. This means that if a block has child blocks connected to it, those child blocks will be represented as nested XML elements within the XML element representing the parent block. The structure of the XML serialization mirrors the structure of the blocks in the workspace.

Behavior After Change

The JSON serialization system in Blockly does something similar, but with a JSON structure instead of XML. The structure of the serialized data, whether XML or JSON, is designed to capture the hierarchical relationships between blocks and their connections.

Reason for Changes

Insertion markers duplicate the block being dragged, and then tell the duplicate to be an insertion marker.

Currently the duplication duplicates (ha) a lot of code from the serialization system. We think this is because in the past the XML system didn't have a good way to just serialize and deserialize a single block (without children). But the JSON system does have this!

Test Coverage

Documentation

Additional Information

@varshneydevansh varshneydevansh requested a review from a team as a code owner August 8, 2023 10:16
@varshneydevansh varshneydevansh changed the title Insertion marker json deserialization 7316 fix: Insertion marker json deserialization 7316 Aug 8, 2023
@github-actions github-actions bot added the PR: fix Fixes a bug label Aug 8, 2023
@varshneydevansh
Copy link
Contributor Author

The error - Did you forget to signal async completion?

is because am I missing fields with the modified serialization/de-serialization, or maybe I have to review the dependencies. Furthermore, I have not yet conducted any testing on this, as I am seeking confirmation whether these are the potential modifications that we intended.

Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

This looks great! Just one style change and then should be good to go =)

core/insertion_marker_manager.ts Outdated Show resolved Hide resolved
@varshneydevansh
Copy link
Contributor Author

This looks great! Just one style change and then should be good to go =)

I also wanted to know if we need to make any changes to the documents?

@varshneydevansh varshneydevansh changed the title fix: Insertion marker json deserialization 7316 feat: Insertion marker json deserialization 7316 Aug 9, 2023
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: fix Fixes a bug labels Aug 9, 2023
@BeksOmega BeksOmega merged commit 8421538 into google:develop Aug 9, 2023
8 checks passed
@BeksOmega
Copy link
Collaborator

This looks great! Just one style change and then should be good to go =)

I also wanted to know if we need to make any changes to the documents?

Nope I think this change should be good!

@varshneydevansh varshneydevansh deleted the insertion_marker_json_deserialization_7316 branch August 9, 2023 16:26
BeksOmega pushed a commit that referenced this pull request Aug 14, 2023
Don't add next block to the insertion marker when we do
an insertion marker json serialization.

Also, to keep consistent with the old behavior, we don't
need to add input blocks for the insertion marker.

And we don't need to do a full serialization here as it
will just become an insertion marker.

Resolves #7383

Address issues in PR #7364

Signed-off-by: Hollow Man <[email protected]>
ericblackmonGoogle pushed a commit that referenced this pull request Aug 14, 2023
Don't add next block to the insertion marker when we do
an insertion marker json serialization.

Also, to keep consistent with the old behavior, we don't
need to add input blocks for the insertion marker.

And we don't need to do a full serialization here as it
will just become an insertion marker.

Resolves #7383

Address issues in PR #7364

Signed-off-by: Hollow Man <[email protected]>
(cherry picked from commit 18ee0ec)
rachel-fenichel added a commit that referenced this pull request Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change insertion markers to use JSON deserialization when duplicating blocks
3 participants