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

XML/JSON change for disabled blocks #8095

Closed
1 task done
NeilFraser opened this issue May 13, 2024 · 6 comments · Fixed by #8104
Closed
1 task done

XML/JSON change for disabled blocks #8095

NeilFraser opened this issue May 13, 2024 · 6 comments · Fixed by #8104
Assignees
Labels
issue: bug Describes why the code or behaviour is wrong

Comments

@NeilFraser
Copy link
Contributor

Check for duplicates

  • I have searched for similar issues before opening a new one.

Description

The XML save format is changing from

disabled="true"

to

disabled-reasons="MANUALLY_DISABLED"

Loading the former in v11 results in this console warning:

disabled was deprecated in v11 and will be deleted in v12.
Use disabled-reasons with the value "MANUALLY_DISABLED" instead.

It is utterly unprecedented to deprecate a core part of the XML parser. XML lives forever in external databases. This implies that v12 will no longer be able to load existing user code!

The offending code is here:
https://github.com/google/blockly/blob/rc/v11.0.0/core/xml.ts#L1024

Reproduction steps

  1. Open the 'Logic' category in the playground. Since the 'null' block is disabled, a warning is printed.

Stack trace

No response

Screenshots

No response

Browsers

No response

@NeilFraser NeilFraser added issue: bug Describes why the code or behaviour is wrong issue: triage Issues awaiting triage by a Blockly team member labels May 13, 2024
@NeilFraser NeilFraser changed the title XML change for disabled XML/JSON change for disabled blocks May 13, 2024
@NeilFraser
Copy link
Contributor Author

The same issue is true for JSON serialization.

Furthermore, the new XML/JSON serialization is not backwards compatible. In practice this is a lesser problem, but none-the-less, there's no reason to needlessly break V10 versions of Blockly getting V11 serialization.

@NeilFraser
Copy link
Contributor Author

This was introduced by #7958. @johnnesky

@BeksOmega
Copy link
Collaborator

Good point, we shouldn't delete support for the old format, but we should migrate to the new one, because it fixes several bugs.

So the TODO here is to remove the deprecation warnings.

@NeilFraser
Copy link
Contributor Author

Ok, I'll take it.

@NeilFraser NeilFraser self-assigned this May 13, 2024
@NeilFraser NeilFraser removed the issue: triage Issues awaiting triage by a Blockly team member label May 13, 2024
@johnnesky
Copy link
Member

Yeah we can remove the deprecation warnings.

If you don't mind redundant data, we could also continue to serialize the old properties (when the disabled reason MANUALLY_DISABLED is present) for backwards compatibility.

@BeksOmega
Copy link
Collaborator

I think it's fine as long as we deserialize the old attributes. Serializing them is less important, and there are partners that really care about keeping save file sizes small since they're financially constrained (I.E. App Inventor will be grumpy with me).

NeilFraser added a commit that referenced this issue May 14, 2024
We must continue to support existing XML and JSON serialization formats since they are in external databases.

The decision was taken not to make the save formats compatible with older versions of Blockly.  We could not think of a convincing use-case for a newer Blockly's output to be read by an older Blockly.  If such a case appears, then our decision should change.

Resolves #8095.
NeilFraser added a commit that referenced this issue May 14, 2024
We must continue to support existing XML and JSON serialization formats since they are in external databases.

The decision was taken not to make the save formats compatible with older versions of Blockly.  We could not think of a convincing use-case for a newer Blockly's output to be read by an older Blockly.  If such a case appears, then our decision should change.

Resolves #8095.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug Describes why the code or behaviour is wrong
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants