Add type and id info to xml by marisaleung · Pull Request #1112 · RaspberryPiFoundation/blockly

marisaleung · 2017-05-18T18:57:30Z

Add type and id info to generated xml.
Changed flyout's variable map to point to target workspace's variable map.
Wrote tests for FieldToDom() change in xml_tests.js

This change is

RoboErikG · 2017-05-18T19:01:27Z

core/xml.js

      container.setAttribute('name', field.name);
+      if (field instanceof Blockly.FieldVariable) {
+        var variable = block.workspace.getVariable(field.getValue());
+        variable = block.workspace.createVariable(field.getValue());


Why are you trying to create a variable right after getting it?

Meant to remove that. Fixing test and will push that change.

RoboErikG

Could you also add a description to the change since you're doing more than just adding type/id to xml in this PR?

RoboErikG · 2017-05-18T19:06:48Z

tests/jsunit/xml_test.js

+ * @param {!string} id The expected id of the variable.
+ * @param {!string} id The expected text of the variable.
+ */
+function xmlTest_checkFieldDomValues(fieldDom, name, type, id, text) {


checkVariable or checkFieldVariable -> checkVariableDomValues since this is specific to variable fields and can't be used for other fields.

That is a good point. Thanks!
Done.

RoboErikG · 2017-05-18T19:08:52Z

tests/jsunit/xml_test.js

+  xmlTest_setUpWithMockBlocks()
+  var block = new Blockly.Block(workspace, 'field_angle_test_block');
+  var resultFieldDom = Blockly.Xml.blockToDom(block).childNodes[0];
+  xmlTest_checkFieldDomValues(resultFieldDom, 'VAR', null, null, '90')


This feels kind of hacky. The id and type are specific to variables but you're passing in null because it's not a variable. You probably want a separate method for checking non-variable fields.

You are right, I updated it so it should be more clear.

RoboErikG · 2017-05-18T19:10:11Z

tests/jsunit/xml_test.js

+  var block = new Blockly.Block(workspace, 'field_variable_test_block');
+  block.inputList[0].fieldRow[0].setValue('name1');
+  var resultFieldDom = Blockly.Xml.blockToDom(block).childNodes[0];
+  xmlTest_checkFieldDomValues(resultFieldDom, 'VAR', 'type1', 'id1', 'name1')


Also add a test for the default case where type and id weren't specified (so type = '' and id is any string)

I think you mean when createVariable doesn't specify a type or id. If not, let me know.
Done.

marisaleung · 2017-05-18T21:07:45Z

I added a description and I rebased to get rid of the other commit so the description of this PR should be accurate now.

RoboErikG · 2017-05-18T21:10:10Z

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

rachel-fenichel · 2017-05-18T21:25:26Z

Reviewed 1 of 3 files at r1.
Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.

core/flyout.js, line 256 at r1 (raw file):

  this.workspace_.getGesture =
      this.targetWorkspace_.getGesture.bind(this.targetWorkspace_);
  // A flyout connected to a workspace doesn't have the updated variable map.

This is solving a different problem from the rest of this code, yes? If so, you can submit with this code and then make the fix I'm suggesting in a separate PR.

You're reaching into private variables twice in one line. I'd rather you call a function like
this.workspace_.useVariableMapFrom(this.targetWorkspace_), which internally calls getVariableMap on the other workspace.

tests/jsunit/xml_test.js, line 87 at r1 (raw file):

/**
 * Check the values of the field dom.

"Check the values of the variable field DOM."

tests/jsunit/xml_test.js, line 199 at r1 (raw file):

function test_blockToDom_fieldToDom_defaultCase() {
  // Expect type is '' and id is '1' since we don't specify type and id.

Move this to just above xmlTests_checkVariableDomValues

Comments from Reviewable

rachel-fenichel · 2017-05-18T21:25:34Z

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

marisaleung · 2017-05-19T22:47:11Z

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.

core/flyout.js, line 256 at r1 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

This is solving a different problem from the rest of this code, yes? If so, you can submit with this code and then make the fix I'm suggesting in a separate PR.

You're reaching into private variables twice in one line. I'd rather you call a function like
this.workspace_.useVariableMapFrom(this.targetWorkspace_), which internally calls getVariableMap on the other workspace.

Offline we decided to overwrite the function this.workspace_.getVariable() and this.workspace_.getVariableById() to refer to targetWorkspace_.variableMap_.

Done on a separate CL.

tests/jsunit/xml_test.js, line 87 at r1 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

"Check the values of the variable field DOM."

Done.

tests/jsunit/xml_test.js, line 199 at r1 (raw file):

Previously, rachel-fenichel (Rachel Fenichel) wrote…

Move this to just above xmlTests_checkVariableDomValues

Done.

Comments from Reviewable

Add xml tests for fieldToDom. Update workspace tests to pass with new changes.

marisaleung requested review from RoboErikG and rachel-fenichel May 18, 2017 18:57

RoboErikG reviewed May 18, 2017

View reviewed changes

marisaleung force-pushed the Add_type_and_id_info_to_xml branch 2 times, most recently from 7944e8c to c9fcfdf Compare May 18, 2017 21:06

Add type, id, and info to the generated xml.

1cd8e1f

Add xml tests for fieldToDom. Update workspace tests to pass with new changes.

marisaleung force-pushed the Add_type_and_id_info_to_xml branch from c9fcfdf to 1cd8e1f Compare May 19, 2017 22:48

marisaleung merged commit 90eeaae into RaspberryPiFoundation:develop May 19, 2017

marisaleung deleted the Add_type_and_id_info_to_xml branch May 23, 2017 20:39

Conversation

marisaleung commented May 18, 2017 • edited by RoboErikG Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RoboErikG 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!

marisaleung commented May 18, 2017

Uh oh!

RoboErikG commented May 18, 2017

Uh oh!

rachel-fenichel commented May 18, 2017

Uh oh!

rachel-fenichel commented May 18, 2017

Uh oh!

marisaleung commented May 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marisaleung commented May 18, 2017 •

edited by RoboErikG

Loading