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

Added fromXml and toXml to Fields #2407

Merged
merged 1 commit into from
Apr 25, 2019

Conversation

BeksOmega
Copy link
Collaborator

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

#1627

Proposed Changes

Adds fromXml and toXml methods to fields that Blockly.Xml.domToField_ and fieldToDom_ can call.

Also changes the playground over to the new value-attribute rather than textContent based xml because... I thought it would be nice I guess ¯\_(ツ)_/¯

Reason for Changes

Some fields (e.g. variable fields) need more information to be serialized to xml than just the field name and value. This adds functions that custom fields can override so people can impliment more advanced fields without having to change the core.

Test Coverage

Added backwards compatibility mocha tests:
FromXMLToXML_Mocha

Also here's a gif of some things serializing and deserializing:
Serialization
That generated this XML:

<xml xmlns="http://www.w3.org/1999/xhtml">
  <variables>
    <variable type="" id="4=m(NP7VfKepgVGbq99y">am i item?</variable>
  </variables>
  <block type="test_fields_angle" id="pcasU-Ep,|#V,:HX]wN~" x="13" y="12">
    <field name="FIELDNAME" value="88"></field>
  </block>
  <block type="test_fields_date" id="wIadJFYd:h^^;oE1=XK}" x="13" y="62">
    <field name="FIELDNAME" value="2019-04-24"></field>
  </block>
  <block type="test_fields_checkbox" id="]b#S7?J!hd9]dLeUm_70" x="13" y="113">
    <field name="CHECKBOX" value="FALSE"></field>
  </block>
  <block type="test_fields_colour" id="~?Dg`]Hj=9Y_qjg?88*Z" x="13" y="163">
    <field name="COLOUR" value="#ffff00"></field>
  </block>
  <block type="test_fields_text_input" id="~(^KZ}SBbC?+Z0-0a/K6" x="13" y="213">
    <field name="TEXT_INPUT" value="am i default?"></field>
  </block>
  <block type="test_fields_variable" id="QEy@*.M-w4:^{(Y),k$%" x="12" y="262">
    <field name="VARIABLE" id="4=m(NP7VfKepgVGbq99y" variablename="am i item?" variabletype=""></field>
  </block>
  <block type="test_fields_label_serializable" id="A5)khRy04vFL~CPD*A`T" x="13" y="313">
    <field name="LABEL" value="BAAA"></field>
  </block>
</xml>

Switching the playground over also allowed me to test that the deserialization was working.

Tested on:

  • Desktop Chrome

Additional Information

It may be better to call the 'variablename' attribute 'value' (even though it's more confusing) because I think having them be different is more prone to errors (i.e. when I was switching the playground over I messed it up a bunch).

Also I'm cool with removing the value attribute stuff all together, that was just in the original solution proposal so I went with it.

@rachel-fenichel rachel-fenichel self-requested a review April 25, 2019 21:52
@rachel-fenichel
Copy link
Collaborator

Also I'm cool with removing the value attribute stuff all together, that was just in the original solution proposal so I went with it.

I'd like to undo this part.

I see that it's backwards compatible for loading, but changing the save format is still problematic for anyone doing things like diffing XML to see if two workspaces are the same.

@BeksOmega
Copy link
Collaborator Author

@rachel-fenichel Do you want to revert the information stored in the variablename attribute to being stored in the textContent as well?

@rachel-fenichel
Copy link
Collaborator

Yes--I want to keep the XML format entirely the same across this change, and only move the logic for parsing it.

@BeksOmega
Copy link
Collaborator Author

All XML format changes have been reverted.

@rachel-fenichel rachel-fenichel merged commit de6bf6e into google:develop Apr 25, 2019
@BeksOmega BeksOmega deleted the feature/FieldsXML branch April 26, 2019 02:15
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