Revert "Remove all all instances calling setValue on variable fields with the name instead of the id" (#1296)

* Revert "Create WorkspaceViewport class (#1291)" This reverts commit 6c00d77c9e. * Revert "Remove all all instances calling setValue with name. (#1254)" This reverts commit 8e8b6b27af.
2026-01-10 02:17:09 +01:00 · 2017-08-28 16:55:44 -07:00
parent 6c00d77c9e
commit 5518873389
7 changed files with 79 additions and 119 deletions
--- a/core/block.js
+++ b/core/block.js
@@ -676,15 +676,7 @@ Blockly.Block.prototype.renameVar = function(oldName, newName) {
    for (var j = 0, field; field = input.fieldRow[j]; j++) {
      if (field instanceof Blockly.FieldVariable &&
          Blockly.Names.equals(oldName, field.getValue())) {
-        if (this.workspace) {
-          var variable = this.workspace.getVariable(newName);
-          if (variable && oldName.toLowerCase() !== newName.toLowerCase()) {
-            // If it is not just a case change, change the value.
-            field.setValue(variable.getId());
-            return;
-          }
-        }
-        field.setText(newName);
+        field.setValue(newName);
      }
    }
  }
--- a/core/field_variable.js
+++ b/core/field_variable.js
@@ -49,7 +49,7 @@ goog.require('goog.string');
 Blockly.FieldVariable = function(varname, opt_validator, opt_variableTypes) {
  Blockly.FieldVariable.superClass_.constructor.call(this,
      Blockly.FieldVariable.dropdownCreate, opt_validator);
-  this.setText(varname || '');
+  this.setValue(varname || '');
  this.variableTypes = opt_variableTypes;
 };
 goog.inherits(Blockly.FieldVariable, Blockly.FieldDropdown);
@@ -69,21 +69,19 @@ Blockly.FieldVariable.prototype.init = function() {
 };

 Blockly.FieldVariable.prototype.initModel = function() {
-  var workspace =
-    this.sourceBlock_.isInFlyout ?
-        this.sourceBlock_.workspace.targetWorkspace :
-        this.sourceBlock_.workspace;
  if (!this.getValue()) {
    // Variables without names get uniquely named for this workspace.
-    var variable = workspace.createVariable(
-      Blockly.Variables.generateUniqueName(workspace));
-    this.setValue(variable.getId());
+    var workspace =
+        this.sourceBlock_.isInFlyout ?
+            this.sourceBlock_.workspace.targetWorkspace :
+            this.sourceBlock_.workspace;
+    this.setValue(Blockly.Variables.generateUniqueName(workspace));
  }
  // If the selected variable doesn't exist yet, create it.
  // For instance, some blocks in the toolbox have variable dropdowns filled
  // in by default.
  if (!this.sourceBlock_.isInFlyout) {
-    this.sourceBlock_.workspace.createVariable(this.getText());
+    this.sourceBlock_.workspace.createVariable(this.getValue());
  }
 };

@@ -95,11 +93,6 @@ Blockly.FieldVariable.prototype.setSourceBlock = function(block) {
  goog.asserts.assert(!block.isShadow(),
      'Variable fields are not allowed to exist on shadow blocks.');
  Blockly.FieldVariable.superClass_.setSourceBlock.call(this, block);
-  // Set the value_ of this field since we now have access to a workspace.
-  var variable = block.workspace.getVariable(this.getText());
-  if (variable) {
-    this.setValue(variable.getId());
-  }
 };

 /**
@@ -112,25 +105,30 @@ Blockly.FieldVariable.prototype.getValue = function() {
 };

 /**
- * Set the field value and if the value is a valid variable, update the text.
+ * Set the variable name.
 * @param {string} value New text.
 */
 Blockly.FieldVariable.prototype.setValue = function(value) {
  var newValue = value;
  var newText = value;

-  if (this.hasSourceBlockWorkspace_()) {
+  if (this.sourceBlock_) {
    var variable = this.sourceBlock_.workspace.getVariableById(value);
    if (variable) {
      newText = variable.name;
    }
+    // TODO(marisaleung): Remove name lookup after converting all Field Variable
+    //     instances to use id instead of name.
+    else if (variable = this.sourceBlock_.workspace.getVariable(value)) {
+      newValue = variable.getId();
+    }
    if (Blockly.Events.isEnabled()) {
      Blockly.Events.fire(new Blockly.Events.BlockChange(
          this.sourceBlock_, 'field', this.name, this.value_, newValue));
    }
-    this.setText(newText);
  }
  this.value_ = newValue;
+  this.setText(newText);
 };

 /**
@@ -143,7 +141,7 @@ Blockly.FieldVariable.prototype.getVariableTypes_ = function() {
  var variableTypes = this.variableTypes;
  if (variableTypes === null) {
    // If variableTypes is null, return all variable types.
-    if (this.hasSourceBlockWorkspace_()) {
+    if (this.sourceBlock_) {
      var workspace = this.sourceBlock_.workspace;
      return workspace.getVariableTypes();
    }
@@ -219,31 +217,32 @@ Blockly.FieldVariable.dropdownCreate = function() {
 */
 Blockly.FieldVariable.prototype.onItemSelected = function(menu, menuItem) {
  var id = menuItem.getValue();
-  if (this.hasSourceBlockWorkspace_()) {
+  // TODO(marisaleung): change setValue() to take in an id as the parameter.
+  // Then remove itemText.
+  var itemText;
+  if (this.sourceBlock_ && this.sourceBlock_.workspace) {
    var workspace = this.sourceBlock_.workspace;
    var variable = workspace.getVariableById(id);
-    // If the item selected is a variable, set itemId to the variable id.
+    // If the item selected is a variable, set itemText to the variable name.
    if (variable) {
-      var itemId = variable.getId();
-      this.setValue(itemId);
-    } else if (id == Blockly.RENAME_VARIABLE_ID) {
+      itemText = variable.name;
+    }
+    else if (id == Blockly.RENAME_VARIABLE_ID) {
      // Rename variable.
      var currentName = this.getText();
      variable = workspace.getVariable(currentName);
      Blockly.Variables.renameVariable(workspace, variable);
+      return;
    } else if (id == Blockly.DELETE_VARIABLE_ID) {
      // Delete variable.
      workspace.deleteVariable(this.getText());
+      return;
    }
+
+    // Call any validation function, and allow it to override.
+    itemText = this.callValidator(itemText);
+  }
+  if (itemText !== null) {
+    this.setValue(itemText);
  }
 };
-
-/**
- * Returns whether this field variable has a source block with a workspace.
- * @return {boolean} True if it has a sourceblock with a workspace. False
- *     otherwise.
- * @private
- */
-Blockly.FieldVariable.prototype.hasSourceBlockWorkspace_ = function() {
-  return (this.sourceBlock_ && this.sourceBlock_.workspace);
-};
--- a/core/xml.js
+++ b/core/xml.js
@@ -586,15 +586,15 @@ Blockly.Xml.domToBlockHeadless_ = function(xmlBlock, workspace) {
        // Fall through.
      case 'field':
        var field = block.getField(name);
-        var value = xmlChild.textContent;
+        var text = xmlChild.textContent;
        if (field instanceof Blockly.FieldVariable) {
          // 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 = xmlChild.getAttribute('variableType') || '';
-          var variable = workspace.getVariable(value);
+          var variable = workspace.getVariable(text);
          if (!variable) {
-            variable = workspace.createVariable(value, type,
+            variable = workspace.createVariable(text, type,
              xmlChild.getAttribute(id));
          }
          if (typeof(type) !== undefined && type !== null) {
@@ -605,14 +605,13 @@ Blockly.Xml.domToBlockHeadless_ = function(xmlBlock, workspace) {
                Blockly.Xml.domToText(xmlChild) + '.');
            }
          }
-          value = variable.getId();
        }
        if (!field) {
          console.warn('Ignoring non-existent field ' + name + ' in block ' +
                       prototypeName);
          break;
        }
-        field.setValue(value);
+        field.setValue(text);
        break;
      case 'value':
      case 'statement':
--- a/tests/jsunit/field_variable_test.js
+++ b/tests/jsunit/field_variable_test.js
@@ -59,7 +59,7 @@ function test_fieldVariable_setValueMatchId() {
  var mockBlock = fieldVariable_mockBlock(workspace);
  fieldVariable.setSourceBlock(mockBlock);
  var event = new Blockly.Events.BlockChange(
-        mockBlock, 'field', undefined, 'RENAME_VARIABLE_ID', 'id2');
+        mockBlock, 'field', undefined, 'name1', 'id2');
  setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null);

  fieldVariable.setValue('id2');
@@ -76,10 +76,10 @@ function test_fieldVariable_setValueMatchName() {
  var mockBlock = fieldVariable_mockBlock(workspace);
  fieldVariable.setSourceBlock(mockBlock);
  var event = new Blockly.Events.BlockChange(
-        mockBlock, 'field', undefined, 'RENAME_VARIABLE_ID', 'id2');
+        mockBlock, 'field', undefined, 'name1', 'id2');
  setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null);

-  fieldVariable.setValue('id2');
+  fieldVariable.setValue('name2');
  assertEquals('name2', fieldVariable.getText());
  assertEquals('id2', fieldVariable.value_);
  fieldVariableTestWithMocks_tearDown();
@@ -94,7 +94,7 @@ function test_fieldVariable_setValueNoVariable() {
    'isShadow': function(){return false;}};
  fieldVariable.setSourceBlock(mockBlock);
  var event = new Blockly.Events.BlockChange(
-        mockBlock, 'field', undefined, 'RENAME_VARIABLE_ID', 'id1');
+        mockBlock, 'field', undefined, 'name1', 'id1');
  setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null);

  fieldVariable.setValue('id1');
@@ -241,31 +241,3 @@ function test_fieldVariable_getVariableTypes_emptyListVariableTypes() {
    workspace.dispose();
  }
 }
-
-function test_fieldVariable_setSourceBlock_ExistingVariable() {
- // Expect that the fieldVariable's value is set to 'id1'
-  fieldVariableTestWithMocks_setUp();
-  workspace.createVariable('name1', null, 'id1');
-  var fieldVariable = new Blockly.FieldVariable('name1');
-  var mockBlock = fieldVariable_mockBlock(workspace);
-
-  fieldVariable.setSourceBlock(mockBlock);
-
-  assertEquals('name1', fieldVariable.getText());
-  assertEquals('id1', fieldVariable.value_);
-  fieldVariableTestWithMocks_tearDown();
-}
-
-function test_fieldVariable_setSourceBlock_NotExistingVariable() {
- // Expect that the fieldVariable's value is set to the default
- // 'RENAME_VARIABLE_ID'
-  fieldVariableTestWithMocks_setUp();
-  var fieldVariable = new Blockly.FieldVariable('name1');
-  var mockBlock = fieldVariable_mockBlock(workspace);
-
-  fieldVariable.setSourceBlock(mockBlock);
-
-  assertEquals('name1', fieldVariable.getText());
-  assertEquals('RENAME_VARIABLE_ID', fieldVariable.value_);
-  fieldVariableTestWithMocks_tearDown();
-}
--- a/tests/jsunit/workspace_test.js
+++ b/tests/jsunit/workspace_test.js
@@ -47,13 +47,12 @@ function workspaceTest_tearDown() {

 /**
 * Create a test get_var_block.
- * @param {string} variableId The id of the variable to put into the variable
- *     field.
+ * @param {?string} variable_name The string to put into the variable field.
 * @return {!Blockly.Block} The created block.
 */
-function createMockBlock(variableId) {
+function createMockBlock(variable_name) {
  var block = new Blockly.Block(workspace, 'get_var_block');
-  block.inputList[0].fieldRow[0].setValue(variableId);
+  block.inputList[0].fieldRow[0].setValue(variable_name);
  return block;
 }

@@ -73,7 +72,7 @@ function test_emptyWorkspace() {
  }
 }

-function test__WorkspaceTest_flatWorkspace() {
+function test_flatWorkspace() {
  workspaceTest_setUp();
  try {
    var blockA = workspace.newBlock('');
@@ -162,9 +161,9 @@ function test_deleteVariable_InternalTrivial() {
  workspaceTest_setUp();
  var var_1 = workspace.createVariable('name1', 'type1', 'id1');
  workspace.createVariable('name2', 'type2', 'id2');
-  createMockBlock('id1');
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockBlock('name1');
+  createMockBlock('name1');
+  createMockBlock('name2');

  workspace.deleteVariableInternal_(var_1);
  var variable = workspace.getVariable('name1');
@@ -336,7 +335,7 @@ function test_renameVariable_OnlyOldNameBlockExists() {
  var oldName = 'name1';
  var newName = 'name2';
  workspace.createVariable(oldName, 'type1', 'id1');
-  createMockBlock('id1');
+  createMockBlock(oldName);

  workspace.renameVariable(oldName, newName);
  checkVariableValues(workspace, newName, 'type1', 'id1');
@@ -355,8 +354,8 @@ function test_renameVariable_TwoVariablesSameType() {
  var newName = 'name2';
  workspace.createVariable(oldName, 'type1', 'id1');
  workspace.createVariable(newName, 'type1', 'id2');
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockBlock(oldName);
+  createMockBlock(newName);

  workspace.renameVariable(oldName, newName);
  checkVariableValues(workspace, newName, 'type1', 'id2');
@@ -376,8 +375,8 @@ function test_renameVariable_TwoVariablesDifferentType() {
  var newName = 'name2';
  workspace.createVariable(oldName, 'type1', 'id1');
  workspace.createVariable(newName, 'type2', 'id2');
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockBlock(oldName);
+  createMockBlock(newName);

  try {
    workspace.renameVariable(oldName, newName);
@@ -400,7 +399,7 @@ function test_renameVariable_OldCase() {
  var oldCase = 'Name1';
  var newName = 'name1';
  workspace.createVariable(oldCase, 'type1', 'id1');
-  createMockBlock('id1');
+  createMockBlock(oldCase);

  workspace.renameVariable(oldCase, newName);
  checkVariableValues(workspace, newName, 'type1', 'id1');
@@ -417,8 +416,8 @@ function test_renameVariable_TwoVariablesAndOldCase() {
  var newName = 'name2';
  workspace.createVariable(oldName, 'type1', 'id1');
  workspace.createVariable(oldCase, 'type1', 'id2');
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockBlock(oldName);
+  createMockBlock(oldCase);

  workspace.renameVariable(oldName, newName);

@@ -444,8 +443,8 @@ function test_renameVariableById_TwoVariablesSameType() {
  var newName = 'name2';
  workspace.createVariable(oldName, 'type1', 'id1');
  workspace.createVariable(newName, 'type1', 'id2');
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockBlock(oldName);
+  createMockBlock(newName);

  workspace.renameVariableById('id1', newName);
  checkVariableValues(workspace, newName, 'type1', 'id2');
@@ -462,8 +461,8 @@ function test_deleteVariable_Trivial() {
  workspaceTest_setUp();
  workspace.createVariable('name1', 'type1', 'id1');
  workspace.createVariable('name2', 'type1', 'id2');
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockBlock('name1');
+  createMockBlock('name2');

  workspace.deleteVariable('name1');
  checkVariableValues(workspace, 'name2', 'type1', 'id2');
@@ -478,8 +477,8 @@ function test_deleteVariableById_Trivial() {
  workspaceTest_setUp();
  workspace.createVariable('name1', 'type1', 'id1');
  workspace.createVariable('name2', 'type1', 'id2');
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockBlock('name1');
+  createMockBlock('name2');

  workspace.deleteVariableById('id1');
  checkVariableValues(workspace, 'name2', 'type1', 'id2');
--- a/tests/jsunit/workspace_undo_redo_test.js
+++ b/tests/jsunit/workspace_undo_redo_test.js
@@ -66,13 +66,12 @@ function undoRedoTest_tearDown() {

 /**
 * Create a test get_var_block.
- * @param {string} variableId The id of the variable to put into the variable
- *     field.
+ * @param {string} variableName The string to put into the variable field.
 * @return {!Blockly.Block} The created block.
 */
-function createMockBlock(variableId) {
+function createMockBlock(variableName) {
  var block = new Blockly.Block(workspace, 'get_var_block');
-  block.inputList[0].fieldRow[0].setValue(variableId);
+  block.inputList[0].fieldRow[0].setValue(variableName);
  return block;
 }

@@ -149,8 +148,8 @@ function test_undoDeleteVariable_WithBlocks() {
  undoRedoTest_setUp();
  workspace.createVariable('name1', 'type1', 'id1');
  workspace.createVariable('name2', 'type2', 'id2');
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockBlock('name1');
+  createMockBlock('name2');
  workspace.deleteVariableById('id1');
  workspace.deleteVariableById('id2');

@@ -193,8 +192,8 @@ function test_redoAndUndoDeleteVariable_WithBlocks() {
  undoRedoTest_setUp();
  workspace.createVariable('name1', 'type1', 'id1');
  workspace.createVariable('name2', 'type2', 'id2');
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockBlock('name1');
+  createMockBlock('name2');
  workspace.deleteVariableById('id1');
  workspace.deleteVariableById('id2');

@@ -243,7 +242,7 @@ function test_redoAndUndoDeleteVariableTwice_NoBlocks() {
 function test_redoAndUndoDeleteVariableTwice_WithBlocks() {
  undoRedoTest_setUp();
  workspace.createVariable('name1', 'type1', 'id1');
-  createMockBlock('id1');
+  createMockBlock('name1');
  workspace.deleteVariableById('id1');
  workspace.deleteVariableById('id1');

@@ -304,7 +303,7 @@ function test_undoRedoRenameVariable_OneExists_NoBlocks() {
 function test_undoRedoRenameVariable_OneExists_WithBlocks() {
  undoRedoTest_setUp();
  workspace.createVariable('name1', '', 'id1');
-  createMockBlock('id1');
+  createMockBlock('name1');
  workspace.renameVariable('name1', 'name2');

  workspace.undo();
@@ -336,8 +335,8 @@ function test_undoRedoRenameVariable_BothExist_NoBlocks() {
 function test_undoRedoRenameVariable_BothExist_WithBlocks() {
  undoRedoTest_setUp();
  createTwoVarsEmptyType();
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockBlock('name1');
+  createMockBlock('name2');
  workspace.renameVariable('name1', 'name2');

  workspace.undo();
@@ -370,8 +369,8 @@ function test_undoRedoRenameVariable_BothExistCaseChange_NoBlocks() {
 function test_undoRedoRenameVariable_BothExistCaseChange_WithBlocks() {
  undoRedoTest_setUp();
  createTwoVarsEmptyType();
-  createMockBlock('id1');
-  createMockBlock('id2');
+  createMockBlock('name1');
+  createMockBlock('name2');
  workspace.renameVariable('name1', 'Name2');

  workspace.undo();
@@ -404,7 +403,7 @@ function test_undoRedoRenameVariable_OnlyCaseChange_NoBlocks() {
 function test_undoRedoRenameVariable_OnlyCaseChange_WithBlocks() {
  undoRedoTest_setUp();
  workspace.createVariable('name1', '', 'id1');
-  createMockBlock('id1');
+  createMockBlock('name1');
  workspace.renameVariable('name1', 'Name1');

  workspace.undo();
--- a/tests/jsunit/xml_test.js
+++ b/tests/jsunit/xml_test.js
@@ -287,7 +287,7 @@ function test_blockToDom_fieldToDom_trivial() {
  xmlTest_setUpWithMockBlocks();
  workspace.createVariable('name1', 'type1', 'id1');
  var block = new Blockly.Block(workspace, 'field_variable_test_block');
-  block.inputList[0].fieldRow[0].setValue('id1');
+  block.inputList[0].fieldRow[0].setValue('name1');
  var resultFieldDom = Blockly.Xml.blockToDom(block).childNodes[0];
  xmlTest_checkVariableFieldDomValues(resultFieldDom, 'VAR', 'type1', 'id1',
    'name1');
@@ -299,7 +299,7 @@ function test_blockToDom_fieldToDom_defaultCase() {
  setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1', '1']);
  workspace.createVariable('name1');
  var block = new Blockly.Block(workspace, 'field_variable_test_block');
-  block.inputList[0].fieldRow[0].setValue('1');
+  block.inputList[0].fieldRow[0].setValue('name1');
  var resultFieldDom = Blockly.Xml.blockToDom(block).childNodes[0];
  // Expect type is '' and id is '1' since we don't specify type and id.
  xmlTest_checkVariableFieldDomValues(resultFieldDom, 'VAR', '', '1', 'name1');
@@ -346,7 +346,7 @@ function test_variablesToDom_twoVariables_oneBlock() {
  workspace.createVariable('name1', 'type1', 'id1');
  workspace.createVariable('name2', 'type2', 'id2');
  var block = new Blockly.Block(workspace, 'field_variable_test_block');
-  block.inputList[0].fieldRow[0].setValue('id1');
+  block.inputList[0].fieldRow[0].setValue('name1');

  var resultDom = Blockly.Xml.variablesToDom(workspace.getAllVariables());
  assertEquals(2, resultDom.children.length);