feat: procedure defs deserialize models (#6706)

* feat: add procedure defs deserializing models

* chore: rename model to model_

* chore: cleanup

* chore: fix tests failures

* fix: PR comments

blocks/procedures.js

@@ -92,6 +92,7 @@ const blocks = createBlockDefinitionsFromJsonArray([
       'procedure_def_validator_helper',
       'procedure_defnoreturn_get_caller_block_mixin',
       'procedure_defnoreturn_set_comment_helper',
+      'procedure_def_set_no_return_helper',
     ],
     'mutator': 'procedure_def_mutator',
   },
@@ -168,6 +169,7 @@ const blocks = createBlockDefinitionsFromJsonArray([
       'procedure_def_validator_helper',
       'procedure_defreturn_get_caller_block_mixin',
       'procedure_defreturn_set_comment_helper',
+      'procedure_def_set_return_helper',
     ],
     'mutator': 'procedure_def_mutator',
   },
@@ -279,10 +281,15 @@ exports.blocks = blocks;
 /** @this {Block} */
 const procedureDefGetDefMixin = function() {
   const mixin = {
-    model: null,
+    model_: null,
 
+    /**
+     * Returns the data model for this procedure block.
+     * @return {!IProcedureModel} The data model for this procedure
+     *     block.
+     */
     getProcedureModel() {
-      return this.model;
+      return this.model_;
     },
 
     /**
@@ -312,9 +319,9 @@ const procedureDefGetDefMixin = function() {
     },
   };
 
-  mixin.model =
+  mixin.model_ =
       new ObservableProcedureModel(this.workspace, this.getFieldValue('NAME'));
-  this.workspace.getProcedureMap().add(mixin.model);
+  this.workspace.getProcedureMap().add(mixin.getProcedureModel());
 
   this.mixin(mixin, true);
 };
@@ -395,8 +402,8 @@ const procedureDefUpdateShapeMixin = {
    * Updates the block to reflect the state of the procedure model.
    */
   doProcedureUpdate: function() {
-    this.setFieldValue(this.model.getName(), 'NAME');
-    this.setEnabled(this.model.getEnabled());
+    this.setFieldValue(this.getProcedureModel().getName(), 'NAME');
+    this.setEnabled(this.getProcedureModel().getEnabled());
     this.updateParameters_();
   },
 
@@ -405,7 +412,8 @@ const procedureDefUpdateShapeMixin = {
    * model.
    */
   updateParameters_: function() {
-    const params = this.model.getParameters().map((p) => p.getName());
+    const params =
+        this.getProcedureModel().getParameters().map((p) => p.getName());
     const paramString = params.length ?
         `${Msg['PROCEDURES_BEFORE_PARAMS']} ${params.join(', ')}` :
         '';
@@ -551,6 +559,17 @@ const procedureDefMutator = {
    * @this {Block}
    */
   domToMutation: function(xmlElement) {
+    for (let i = 0; i < xmlElement.childNodes.length; i++) {
+      const node = xmlElement.childNodes[i];
+      if (node.nodeName.toLowerCase() !== 'arg') continue;
+      this.getProcedureModel().insertParameter(
+          new ObservableParameterModel(
+              this.workspace, node.getAttribute('name'),
+              node.getAttribute('varid')),
+          i);
+    }
+
+    // TODO: Remove this data update code.
     this.arguments_ = [];
     this.argumentVarModels_ = [];
     for (let i = 0, childNode; (childNode = xmlElement.childNodes[i]); i++) {
@@ -610,6 +629,16 @@ const procedureDefMutator = {
    *     statements.
    */
   loadExtraState: function(state) {
+    if (state['params']) {
+      for (let i = 0; i < state['params'].length; i++) {
+        const param = state['params'][i];
+        this.getProcedureModel().insertParameter(
+            new ObservableParameterModel(this.workspace, param.name, param.id),
+            i);
+      }
+    }
+
+    // TODO: Remove this data update code.
     this.arguments_ = [];
     this.argumentVarModels_ = [];
     if (state['params']) {
@@ -694,14 +723,16 @@ const procedureDefMutator = {
     }
     this.updateParams_();
     Procedures.mutateCallers(this);
-    for (let i = this.model.getParameters().length; i >= 0; i--) {
-      this.model.deleteParameter(i);
+
+    const model = this.getProcedureModel();
+    for (let i = model.getParameters().length; i >= 0; i--) {
+      model.deleteParameter(i);
     }
 
     let i = 0;
     paramBlock = containerBlock.getInputTargetBlock('STACK');
     while (paramBlock && !paramBlock.isInsertionMarker()) {
-      this.model.insertParameter(
+      model.insertParameter(
           new ObservableParameterModel(
               this.workspace, paramBlock.getFieldValue('NAME'), paramBlock.id),
           i);
@@ -771,8 +802,8 @@ Extensions.registerMixin(
 const procedureDefOnChangeMixin = {
   onchange: function(e) {
     if (e.type === Events.BLOCK_CHANGE && e.blockId === this.id &&
-        e.element == 'disabled') {
-      this.model.setEnabled(!e.newValue);
+        e.element === 'disabled') {
+      this.getProcedureModel().setEnabled(!e.newValue);
     }
   },
 };
@@ -843,6 +874,20 @@ Extensions.registerMixin(
     'procedure_defreturn_get_caller_block_mixin',
     procedureDefReturnGetCallerBlockMixin);
 
+/** @this {Block} */
+const procedureDefSetNoReturnHelper = function() {
+  this.getProcedureModel().setReturnTypes(null);
+};
+Extensions.register(
+    'procedure_def_set_no_return_helper', procedureDefSetNoReturnHelper);
+
+/** @this {Block} */
+const procedureDefSetReturnHelper = function() {
+  this.getProcedureModel().setReturnTypes([]);
+};
+Extensions.register(
+    'procedure_def_set_return_helper', procedureDefSetReturnHelper);
+
 const validateProcedureParamMixin = {
   /**
    * Obtain a valid name for the procedure argument. Create a variable if

core/procedures/observable_parameter_model.ts

@@ -24,8 +24,8 @@ export class ObservableParameterModel implements IParameterModel {
   constructor(
       private readonly workspace: Workspace, name: string, id?: string) {
     this.id = id ?? genUid();
-    this.variable =
-        this.workspace.getVariable(name) ?? workspace.createVariable(name);
+    this.variable = this.workspace.getVariable(name) ??
+        workspace.createVariable(name, '', id);
   }
 
   /**

tests/mocha/blocks/procedures_test.js

@@ -452,6 +452,256 @@ suite('Procedures', function() {
     });
   });
 
+  suite('deserializing data models', function() {
+    suite('return types', function() {
+      test('procedure defs without returns have null return types', function() {
+        const json = {
+          'blocks': {
+            'languageVersion': 0,
+            'blocks': [
+              {
+                'type': 'procedures_defnoreturn',
+                'fields': {
+                  'NAME': 'test name',
+                },
+              },
+            ],
+          },
+        };
+        Blockly.serialization.workspaces.load(json, this.workspace);
+        const procedureModel =
+            this.workspace.getProcedureMap().getProcedures()[0];
+
+        chai.assert.isNull(
+            procedureModel.getReturnTypes(),
+            'Expected the return types to be null');
+      });
+
+      test('procedure defs with returns have array return types', function() {
+        const json = {
+          'blocks': {
+            'languageVersion': 0,
+            'blocks': [
+              {
+                'type': 'procedures_defreturn',
+                'fields': {
+                  'NAME': 'test name',
+                },
+              },
+            ],
+          },
+        };
+        Blockly.serialization.workspaces.load(json, this.workspace);
+        const procedureModel =
+            this.workspace.getProcedureMap().getProcedures()[0];
+
+        chai.assert.isArray(
+            procedureModel.getReturnTypes(),
+            'Expected the return types to be an array');
+      });
+    });
+
+    suite('json', function() {
+      test('procedure names get deserialized', function() {
+        const json = {
+          'blocks': {
+            'languageVersion': 0,
+            'blocks': [
+              {
+                'type': 'procedures_defnoreturn',
+                'fields': {
+                  'NAME': 'test name',
+                },
+              },
+            ],
+          },
+        };
+        Blockly.serialization.workspaces.load(json, this.workspace);
+        const procedureModel =
+            this.workspace.getProcedureMap().getProcedures()[0];
+
+        chai.assert.equal(
+            procedureModel.name,
+            'test name',
+            'Expected the name of the procedure model to equal the name ' +
+            'being deserialized.');
+      });
+
+      test('procedure parameter names get deserialized', function() {
+        const json = {
+          'blocks': {
+            'languageVersion': 0,
+            'blocks': [
+              {
+                'type': 'procedures_defnoreturn',
+                'fields': {
+                  'NAME': 'test name',
+                },
+                'extraState': {
+                  'params': [
+                    {
+                      'id': 'test id 1',
+                      'name': 'test name 1',
+                    },
+                    {
+                      'id': 'test id 2',
+                      'name': 'test name 2',
+                    },
+                  ],
+                },
+              },
+            ],
+          },
+        };
+        Blockly.serialization.workspaces.load(json, this.workspace);
+        const procedureModel =
+            this.workspace.getProcedureMap().getProcedures()[0];
+
+        chai.assert.equal(
+            procedureModel.getParameter(0).getName(),
+            'test name 1',
+            'Expected the name of the first parameter to equal the name ' +
+            'being deserialized.');
+        chai.assert.equal(
+            procedureModel.getParameter(1).getName(),
+            'test name 2',
+            'Expected the name of the second parameter to equal the name ' +
+            'being deserialized.');
+      });
+
+      test('procedure variables get matching IDs', function() {
+        const json = {
+          'blocks': {
+            'languageVersion': 0,
+            'blocks': [
+              {
+                'type': 'procedures_defnoreturn',
+                'extraState': {
+                  'params': [
+                    {
+                      'name': 'test param name',
+                      'id': 'test param id',
+                    },
+                  ],
+                },
+                'fields': {
+                  'NAME': 'test proc name',
+                },
+              },
+            ],
+          },
+          'variables': [
+            {
+              'name': 'test param name',
+              'id': 'test param id',
+            },
+          ],
+        };
+        Blockly.serialization.workspaces.load(json, this.workspace);
+        const procedureModel =
+            this.workspace.getProcedureMap().getProcedures()[0];
+
+        chai.assert.equal(
+            procedureModel.getParameter(0).getVariableModel().getId(),
+            'test param id',
+            'Expected the variable id to match the serialized param id');
+      });
+    });
+
+    suite('xml', function() {
+      test('procedure names get deserialized', function() {
+        const xml = Blockly.Xml.textToDom(
+            `<block type="procedures_defnoreturn">` +
+            `  <field name="NAME">test name</field>` +
+            `</block>`);
+        Blockly.Xml.domToBlock(xml, this.workspace);
+        const procedureModel =
+            this.workspace.getProcedureMap().getProcedures()[0];
+
+        chai.assert.equal(
+            procedureModel.name,
+            'test name',
+            'Expected the name of the procedure model to equal the name ' +
+            'being deserialized.');
+      });
+
+      test('procedure parameter names get deserialized', function() {
+        const xml = Blockly.Xml.textToDom(
+            `<block type="procedures_defnoreturn">` +
+            `  <mutation>` +
+            `    <arg name="test name 1" varid="test var id 1"/>` +
+            `    <arg name="test name 2" varid="test var id 2"/>` +
+            `  </mutation>` +
+            `  <field name="NAME">test name</field>` +
+            `</block>`);
+        Blockly.Xml.domToBlock(xml, this.workspace);
+        const procedureModel =
+            this.workspace.getProcedureMap().getProcedures()[0];
+
+        chai.assert.equal(
+            procedureModel.getParameter(0).getName(),
+            'test name 1',
+            'Expected the name of the first parameter to equal the name ' +
+            'being deserialized.');
+        chai.assert.equal(
+            procedureModel.getParameter(1).getName(),
+            'test name 2',
+            'Expected the name of the second parameter to equal the name ' +
+            'being deserialized.');
+      });
+
+      test('procedure variables get matching IDs', function() {
+        const json = {
+          'blocks': {
+            'languageVersion': 0,
+            'blocks': [
+              {
+                'type': 'procedures_defnoreturn',
+                'extraState': {
+                  'params': [
+                    {
+                      'name': 'test param name',
+                      'id': 'test param id',
+                    },
+                  ],
+                },
+                'fields': {
+                  'NAME': 'test proc name',
+                },
+              },
+            ],
+          },
+          'variables': [
+            {
+              'name': 'test param name',
+              'id': 'test param id',
+            },
+          ],
+        };
+        const xml = Blockly.Xml.textToDom(
+            `<xml>` +
+            `  <variables>` +
+            `    <variable id ="test param id">test param name</variable>` +
+            `  </variables>` +
+            `  <block type="procedures_defnoreturn">` +
+            `    <mutation>` +
+            `      <arg name="test param name" varid="test param id"/>` +
+            `    </mutation>` +
+            `    <field name="NAME">test name</field>` +
+            `  </block>` +
+            `</xml>`);
+        Blockly.Xml.domToWorkspace(xml, this.workspace);
+        const procedureModel =
+            this.workspace.getProcedureMap().getProcedures()[0];
+
+        chai.assert.equal(
+            procedureModel.getParameter(0).getVariableModel().getId(),
+            'test param id',
+            'Expected the variable id to match the serialized param id');
+      });
+    });
+  });
+
   suite('Renaming procedures', function() {
     test('callers are updated to have the new name', function() {
       const defBlock = createProcDefBlock(this.workspace);