diff --git a/core/block.js b/core/block.js index d22449de6..ead47347d 100644 --- 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); } } } diff --git a/core/field_variable.js b/core/field_variable.js index 3fc169b26..3e06c96ff 100644 --- 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); -}; diff --git a/core/xml.js b/core/xml.js index 0029967c2..64f001560 100644 --- 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': diff --git a/tests/jsunit/field_variable_test.js b/tests/jsunit/field_variable_test.js index e1bd64197..0ef296273 100644 --- 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(); -} diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index 96696a961..17755f158 100644 --- 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'); diff --git a/tests/jsunit/workspace_undo_redo_test.js b/tests/jsunit/workspace_undo_redo_test.js index 5a4140d30..33dc855d7 100644 --- 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(); diff --git a/tests/jsunit/xml_test.js b/tests/jsunit/xml_test.js index 5999779b7..ea682dfb7 100644 --- 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);