Bug in xml.js while deserialising variable fields

[jlaurens]

Lines 595-596 of xml.js read

            variable = workspace.createVariable(text, type,
              xmlChild.getAttribute(id));

but should read

            variable = workspace.createVariable(text, type,
              xmlChild.getAttribute('id'));

where id is not a variable name but an xml attribute key. Actually, the id variable contains the 'id' attribute of the parent node such that xmlChild.getAttribute(id) always return null.

[rachel-fenichel]

Correct! This is completely broken. Then again, so is everything else involving variable IDs this week. My current highest priority is to fix #1199, which will include a fix for this one.

If you're working with typed variables and getting wacky behaviour, please keep letting me know so I can test edge cases. The fixes will probably come all at once, when I land my fix for #1199.

[jlaurens]

I caught that one while pushing field serialization/deserialization out of xml.js.
Actually, all fields behave the same with respect to serialization, except FieldVariable.
But in my own app, I have custom fields that really do need more attributes while (de)serializing. I ended up with what follows (I could not manage to create a pull request...) which is rather close to what you just committed. In my own app, other custom fields have overriden serialization methods. HTH.

Blockly.Field.prototype.getSerializedXml_ = function() {
  var container = goog.dom.createDom('field', null, this.getValue());
  container.setAttribute('name', this.name);
  return container;
}

Blockly.FieldVariable.prototype.getSerializedXml = function() {
  var container = Blockly.FieldVariable.superClass_.getSerializedXml.call(this);
  var variable = this.sourceBlock_.workspace.getVariable(this.getValue());
  if (variable) {
    container.setAttribute('id', variable.getId());
    container.setAttribute('variableType', variable.type);
  }
  return container;
}

Blockly.Xml.blockToDom = function(block, opt_noId) {
  [...]
  for (var i = 0, input; input = block.inputList[i]; i++) {
    for (var j = 0, field; field = input.fieldRow[j]; j++) {
      if (field.name && field.EDITABLE) {
        element.appendChild(field.getSerializedXml());// <------------------ CHANGE HERE
      }
    }
  }
[...]
};

Blockly.Field.prototype.deserializeXml = function(xml) {
  this.setValue(xml.textContent);
}

Blockly.FieldVariable.prototype.deserializeXml = function(xml) {
  // TODO (marisaleung): When we change setValue and getValue to
  // interact with id's instead of names, update this so that we get
  // the variable based on id instead of textContent.
  var type = xml.getAttribute('variableType') || '';
  var workspace = this.sourceBlock_.workspace;
  var text = xml.textContent;
  var variable = workspace.getVariable(text);
  if (!variable) {
    variable = workspace.createVariable(text, type,
      xml.getAttribute('id'));
  }
  if (typeof(type) !== undefined && type !== null) {
    if (type !== variable.type) {
      throw Error('Serialized variable type with id \'' +
        variable.getId() + '\' had type ' + variable.type + ', and ' +
        'does not match variable field that references it: ' +
        Blockly.Xml.domToText(xml) + '.');
    }
  }
  this.setValue(text);
}

Blockly.Xml.domToBlockHeadless_ = function(xmlBlock, workspace) {
  [...]
      case 'field':
        var field = block.getField(name);
        if (!field) {
          console.warn('Ignoring non-existent field ' + name + ' in block ' +
                       prototypeName);
          break;
        }
        field.deserializeXml(xmlChild);//<----------------------CHANGE HERE
        break;
  [...]
};

[rachel-fenichel]

I think this is fixed. @jlaurens let me know if it's still an issue.

[AnmAtAnm]

Related: #1627