diff --git a/core/field_variable.js b/core/field_variable.js index 9bff99214..0b9238f02 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -167,6 +167,8 @@ Blockly.FieldVariable.prototype.getText = function() { * variable. */ Blockly.FieldVariable.prototype.setValue = function(id) { + // TODO: Handle undo a change to go back to the default value. + // TODO: Handle null ID, which means "use default". var workspace = this.sourceBlock_.workspace; var variable = workspace.getVariableById(id); if (!variable) { @@ -189,6 +191,11 @@ Blockly.FieldVariable.prototype.setValue = function(id) { throw new Error('Variable type doesn\'t match this field! Type was ' + type); } + if (this.sourceBlock_ && Blockly.Events.isEnabled()) { + var oldValue = this.variable_ ? this.variable_.getId() : null; + Blockly.Events.fire(new Blockly.Events.BlockChange( + this.sourceBlock_, 'field', this.name, oldValue, variable.getId())); + } this.variable_ = variable; this.setText(variable.name); }; diff --git a/core/workspace.js b/core/workspace.js index 2c4205158..6fed60405 100644 --- a/core/workspace.js +++ b/core/workspace.js @@ -266,7 +266,6 @@ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { Blockly.Events.setGroup(true); var blocks = this.getAllBlocks(); - this.variableMap_.renameVariable(variable, newName, type); // Iterate through every block and update name. for (var i = 0; i < blocks.length; i++) { @@ -275,6 +274,8 @@ Blockly.Workspace.prototype.renameVariableById = function(id, newName) { blocks[i].renameVarById(newId, newId); } } + + this.variableMap_.renameVariable(variable, newName, type); Blockly.Events.setGroup(false); }; diff --git a/tests/jsunit/field_variable_test.js b/tests/jsunit/field_variable_test.js index 0cf1f11f8..6d3ef9d66 100644 --- a/tests/jsunit/field_variable_test.js +++ b/tests/jsunit/field_variable_test.js @@ -68,8 +68,9 @@ function test_fieldVariable_setValueMatchId() { var fieldVariable = fieldVariable_createAndInitField(workspace); + var oldId = fieldVariable.getValue(); var event = new Blockly.Events.BlockChange( - fieldVariable.sourceBlock_, 'field', undefined, 'name1', 'id2'); + fieldVariable.sourceBlock_, 'field', undefined, oldId, 'id2'); setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null); fieldVariable.setValue('id2'); @@ -87,10 +88,6 @@ function test_fieldVariable_setValueNoVariable() { return false; }; - var event = new Blockly.Events.BlockChange( - mockBlock, 'field', undefined, 'name1', 'id1'); - setUpMockMethod(mockControl_, Blockly.Events, 'fire', [event], null); - try { fieldVariable.setValue('id1'); // Calling setValue with a variable that doesn't exist throws an error. diff --git a/tests/jsunit/index.html b/tests/jsunit/index.html index 500f2cb37..96424df44 100644 --- a/tests/jsunit/index.html +++ b/tests/jsunit/index.html @@ -8,7 +8,7 @@ - + diff --git a/tests/jsunit/procedures_test.js b/tests/jsunit/procedures_test.js index 0284f3978..f993af892 100644 --- a/tests/jsunit/procedures_test.js +++ b/tests/jsunit/procedures_test.js @@ -37,14 +37,15 @@ function proceduresTest_setUpWithMockBlocks() { 'message0': '%1', 'args0': [ { + // TODO: Why is this a variable? It shouldn't need to be. 'type': 'field_variable', 'name': 'NAME', 'variable': 'item' } - ], + ] }]); Blockly.Blocks['procedure_mock_block'].getProcedureDef = function() { - return [this.getFieldValue('NAME'), [], false]; + return [this.getField('NAME').getText(), [], false]; }; } @@ -63,8 +64,9 @@ function test_isNameUsed_NoBlocks() { function test_isNameUsed_False() { proceduresTest_setUpWithMockBlocks(); + workspace.createVariable('name2', '', 'id2'); var block = new Blockly.Block(workspace, 'procedure_mock_block'); - block.setFieldValue('name2', 'NAME'); + block.setFieldValue('id2', 'NAME'); var result = Blockly.Procedures.isNameUsed('name1', workspace); assertFalse(result); @@ -73,8 +75,9 @@ function test_isNameUsed_False() { function test_isNameUsed_True() { proceduresTest_setUpWithMockBlocks(); + workspace.createVariable('name1', '', 'id1'); var block = new Blockly.Block(workspace, 'procedure_mock_block'); - block.setFieldValue('name1', 'NAME'); + block.setFieldValue('id1', 'NAME'); var result = Blockly.Procedures.isNameUsed('name1', workspace); assertTrue(result); diff --git a/tests/jsunit/test_utilities.js b/tests/jsunit/test_utilities.js index 6a80a3549..d7f6085bf 100644 --- a/tests/jsunit/test_utilities.js +++ b/tests/jsunit/test_utilities.js @@ -89,3 +89,22 @@ function checkVariableValues(container, name, type, id) { assertEquals(type, variable.type); assertEquals(id, variable.getId()); } + +/** + * Create a test get_var_block. + * Will fail if get_var_block isn't defined. + * TODO (fenichel): Rename to createMockVarBlock. + * @param {!string} variable_id The id of the variable to reference. + * @return {!Blockly.Block} The created block. + */ +function createMockBlock(variable_id) { + if (!Blockly.Blocks['get_var_block']) { + fail(); + } + // Turn off events to avoid testing XML at the same time. + Blockly.Events.disable(); + var block = new Blockly.Block(workspace, 'get_var_block'); + block.inputList[0].fieldRow[0].setValue(variable_id); + Blockly.Events.enable(); + return block; +} diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index d39eac1e4..6e4d960d4 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -24,42 +24,29 @@ goog.require('goog.testing.MockControl'); var workspace; var mockControl_; -Blockly.defineBlocksWithJsonArray([{ - "type": "get_var_block", - "message0": "%1", - "args0": [ - { - "type": "field_variable", - "name": "VAR", - "variableTypes": ["", "type1", "type2"] - } - ] -}]); function workspaceTest_setUp() { + Blockly.defineBlocksWithJsonArray([{ + "type": "get_var_block", + "message0": "%1", + "args0": [ + { + "type": "field_variable", + "name": "VAR", + "variableTypes": ["", "type1", "type2"] + } + ] + }]); workspace = new Blockly.Workspace(); mockControl_ = new goog.testing.MockControl(); } function workspaceTest_tearDown() { + delete Blockly.Blocks['get_var_block']; mockControl_.$tearDown(); workspace.dispose(); } -/** - * Create a test get_var_block. - * @param {?string} variable_name The string to put into the variable field. - * @return {!Blockly.Block} The created block. - */ -function createMockBlock(variable_id) { - // Turn off events to avoid testing XML at the same time. - Blockly.Events.disable(); - var block = new Blockly.Block(workspace, 'get_var_block'); - block.inputList[0].fieldRow[0].setValue(variable_id); - Blockly.Events.enable(); - return block; -} - function test_emptyWorkspace() { workspaceTest_setUp(); try { diff --git a/tests/jsunit/workspace_undo_redo_test.js b/tests/jsunit/workspace_undo_redo_test.js index a8befe33c..092dbb4a8 100644 --- a/tests/jsunit/workspace_undo_redo_test.js +++ b/tests/jsunit/workspace_undo_redo_test.js @@ -33,16 +33,6 @@ goog.require('goog.testing.MockControl'); var workspace; var mockControl_; var savedFireFunc = Blockly.Events.fire; -Blockly.defineBlocksWithJsonArray([{ - "type": "get_var_block", - "message0": "%1", - "args0": [ - { - "type": "field_variable", - "name": "VAR", - } - ] -}]); function temporary_fireEvent(event) { if (!Blockly.Events.isEnabled()) { @@ -53,28 +43,29 @@ function temporary_fireEvent(event) { } function undoRedoTest_setUp() { + Blockly.defineBlocksWithJsonArray([{ + "type": "get_var_block", + "message0": "%1", + "args0": [ + { + "type": "field_variable", + "name": "VAR", + "variableTypes": ["", "type1", "type2"] + } + ] + }]); workspace = new Blockly.Workspace(); mockControl_ = new goog.testing.MockControl(); Blockly.Events.fire = temporary_fireEvent; } function undoRedoTest_tearDown() { + delete Blockly.Blocks['get_var_block']; mockControl_.$tearDown(); workspace.dispose(); Blockly.Events.fire = savedFireFunc; } -/** - * Create a test get_var_block. - * @param {string} variableName The string to put into the variable field. - * @return {!Blockly.Block} The created block. - */ -function createMockBlock(variableName) { - var block = new Blockly.Block(workspace, 'get_var_block'); - block.inputList[0].fieldRow[0].setValue(variableName); - return block; -} - /** * Check that the top block with the given index contains a variable with * the given name. @@ -82,7 +73,7 @@ function createMockBlock(variableName) { * @param {string} name The expected name of the variable in the block. */ function undoRedoTest_checkBlockVariableName(blockIndex, name) { - var blockVarName = workspace.topBlocks_[blockIndex].getVars()[0]; + var blockVarName = workspace.topBlocks_[blockIndex].getVarModels()[0].name; assertEquals(name, blockVarName); } @@ -146,25 +137,23 @@ function test_undoDeleteVariable_NoBlocks() { function test_undoDeleteVariable_WithBlocks() { undoRedoTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name2'); + workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + createMockBlock('id1'); + createMockBlock('id2'); workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name2'); assertNull(workspace.getVariableById('id1')); - checkVariableValues(workspace, 'name2', '', 'id2'); + checkVariableValues(workspace, 'name2', 'type2', 'id2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name2'); undoRedoTest_checkBlockVariableName(1, 'name1'); - checkVariableValues(workspace, 'name1', '', 'id1'); - checkVariableValues(workspace, 'name2', '', 'id2'); + checkVariableValues(workspace, 'name1', 'type1', 'id1'); + checkVariableValues(workspace, 'name2', 'type2', 'id2'); undoRedoTest_tearDown(); } @@ -192,12 +181,10 @@ function test_redoAndUndoDeleteVariable_NoBlocks() { function test_redoAndUndoDeleteVariable_WithBlocks() { undoRedoTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable('name1', '', 'id1'); - workspace.createVariable('name2', '', 'id2'); - createMockBlock('name1'); - createMockBlock('name2'); + workspace.createVariable('name1', 'type1', 'id1'); + workspace.createVariable('name2', 'type2', 'id2'); + createMockBlock('id1'); + createMockBlock('id2'); workspace.deleteVariableById('id1'); workspace.deleteVariableById('id2'); @@ -214,7 +201,7 @@ function test_redoAndUndoDeleteVariable_WithBlocks() { // Expect that variable 'id2' is recreated undoRedoTest_checkBlockVariableName(0, 'name2'); assertNull(workspace.getVariableById('id1')); - checkVariableValues(workspace, 'name2', '', 'id2'); + checkVariableValues(workspace, 'name2', 'type2', 'id2'); undoRedoTest_tearDown(); } @@ -245,12 +232,11 @@ function test_redoAndUndoDeleteVariableTwice_NoBlocks() { function test_redoAndUndoDeleteVariableTwice_WithBlocks() { undoRedoTest_setUp(); - // TODO (#1199): make a similar test where the variable is given a non-empty - // type. - workspace.createVariable('name1', '', 'id1'); - createMockBlock('name1'); - workspace.deleteVariableById('id1'); - workspace.deleteVariableById('id1'); + var id = 'id1'; + workspace.createVariable('name1', 'type1', id); + createMockBlock(id); + workspace.deleteVariableById(id); + workspace.deleteVariableById(id); // Check the undoStack only recorded one delete event. var undoStack = workspace.undoStack_; @@ -261,45 +247,45 @@ function test_redoAndUndoDeleteVariableTwice_WithBlocks() { // undo delete workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); - checkVariableValues(workspace, 'name1', '', 'id1'); + checkVariableValues(workspace, 'name1', 'type1', id); // redo delete workspace.undo(true); assertEquals(0, workspace.topBlocks_.length); - assertNull(workspace.getVariableById('id1')); + assertNull(workspace.getVariableById(id)); // redo delete, nothing should happen workspace.undo(true); assertEquals(0, workspace.topBlocks_.length); - assertNull(workspace.getVariableById('id1')); + assertNull(workspace.getVariableById(id)); undoRedoTest_tearDown(); } -function test_undoRedoRenameVariable_NeitherVariableExists() { - // Expect that a variable with the name, 'name2', and the generated UUID, - // 'id2', to be created when rename is called. Undo removes this variable - // and redo recreates it. - undoRedoTest_setUp(); - setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, - ['rename_group', 'id2', 'delete_group']); - workspace.renameVariable('name1', 'name2'); +// TODO: Decide if this needs to be possible. +// function test_undoRedoRenameVariable_NeitherVariableExists() { +// // Expect that a variable with the name, 'name2', and the generated UUID, +// // 'id2', to be created when rename is called. Undo removes this variable +// // and redo recreates it. +// undoRedoTest_setUp(); +// setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, +// ['rename_group', 'id2', 'delete_group']); +// workspace.renameVariable('name1', 'name2'); - workspace.undo(); - assertNull(workspace.getVariableById('id2')); +// workspace.undo(); +// assertNull(workspace.getVariableById('id2')); - workspace.undo(true); - checkVariableValues(workspace, 'name2', '', 'id2'); - undoRedoTest_tearDown(); -} +// workspace.undo(true); +// checkVariableValues(workspace, 'name2', '', 'id2'); +// undoRedoTest_tearDown(); +// } function test_undoRedoRenameVariable_OneExists_NoBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); - workspace.renameVariable('name1', 'name2'); + workspace.renameVariableById('id1', 'name2'); workspace.undo(); checkVariableValues(workspace, 'name1', '', 'id1'); - assertNull(workspace.getVariable('name2')); workspace.undo(true); checkVariableValues(workspace, 'name2', '', 'id1'); @@ -309,13 +295,12 @@ function test_undoRedoRenameVariable_OneExists_NoBlocks() { function test_undoRedoRenameVariable_OneExists_WithBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); - createMockBlock('name1'); - workspace.renameVariable('name1', 'name2'); + createMockBlock('id1'); + workspace.renameVariableById('id1', 'name2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); checkVariableValues(workspace, 'name1', '', 'id1'); - assertNull(workspace.getVariable('name2')); workspace.undo(true); checkVariableValues(workspace, 'name2', '', 'id1'); @@ -326,7 +311,7 @@ function test_undoRedoRenameVariable_OneExists_WithBlocks() { function test_undoRedoRenameVariable_BothExist_NoBlocks() { undoRedoTest_setUp(); createTwoVarsEmptyType(); - workspace.renameVariable('name1', 'name2'); + workspace.renameVariableById('id1', 'name2'); workspace.undo(); checkVariableValues(workspace, 'name1', '', 'id1'); @@ -334,16 +319,16 @@ function test_undoRedoRenameVariable_BothExist_NoBlocks() { workspace.undo(true); checkVariableValues(workspace, 'name2', '', 'id2'); - assertNull(workspace.getVariable('name1')); + assertNull(workspace.getVariableById('id1')); undoRedoTest_tearDown(); } function test_undoRedoRenameVariable_BothExist_WithBlocks() { undoRedoTest_setUp(); createTwoVarsEmptyType(); - createMockBlock('name1'); - createMockBlock('name2'); - workspace.renameVariable('name1', 'name2'); + createMockBlock('id1'); + createMockBlock('id2'); + workspace.renameVariableById('id1', 'name2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); @@ -360,7 +345,7 @@ function test_undoRedoRenameVariable_BothExist_WithBlocks() { function test_undoRedoRenameVariable_BothExistCaseChange_NoBlocks() { undoRedoTest_setUp(); createTwoVarsEmptyType(); - workspace.renameVariable('name1', 'Name2'); + workspace.renameVariableById('id1', 'Name2'); workspace.undo(); checkVariableValues(workspace, 'name1', '', 'id1'); @@ -375,9 +360,9 @@ function test_undoRedoRenameVariable_BothExistCaseChange_NoBlocks() { function test_undoRedoRenameVariable_BothExistCaseChange_WithBlocks() { undoRedoTest_setUp(); createTwoVarsEmptyType(); - createMockBlock('name1'); - createMockBlock('name2'); - workspace.renameVariable('name1', 'Name2'); + createMockBlock('id1'); + createMockBlock('id2'); + workspace.renameVariableById('id1', 'Name2'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); @@ -387,7 +372,7 @@ function test_undoRedoRenameVariable_BothExistCaseChange_WithBlocks() { workspace.undo(true); checkVariableValues(workspace, 'Name2', '', 'id2'); - assertNull(workspace.getVariable('name1')); + assertNull(workspace.getVariableById('id1')); undoRedoTest_checkBlockVariableName(0, 'Name2'); undoRedoTest_checkBlockVariableName(1, 'Name2'); undoRedoTest_tearDown(); @@ -396,7 +381,7 @@ function test_undoRedoRenameVariable_BothExistCaseChange_WithBlocks() { function test_undoRedoRenameVariable_OnlyCaseChange_NoBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); - workspace.renameVariable('name1', 'Name1'); + workspace.renameVariableById('id1', 'Name1'); workspace.undo(); checkVariableValues(workspace, 'name1', '', 'id1'); @@ -409,8 +394,8 @@ function test_undoRedoRenameVariable_OnlyCaseChange_NoBlocks() { function test_undoRedoRenameVariable_OnlyCaseChange_WithBlocks() { undoRedoTest_setUp(); workspace.createVariable('name1', '', 'id1'); - createMockBlock('name1'); - workspace.renameVariable('name1', 'Name1'); + createMockBlock('id1'); + workspace.renameVariableById('id1', 'Name1'); workspace.undo(); undoRedoTest_checkBlockVariableName(0, 'name1'); diff --git a/tests/jsunit/xml_test.js b/tests/jsunit/xml_test.js index 0c2af7441..f8ebe19fb 100644 --- a/tests/jsunit/xml_test.js +++ b/tests/jsunit/xml_test.js @@ -286,10 +286,10 @@ function test_appendDomToWorkspace() { function test_blockToDom_fieldToDom_trivial() { xmlTest_setUpWithMockBlocks(); // TODO (#1199): make a similar test where the variable is given a non-empty - // type. + // type.f workspace.createVariable('name1', '', 'id1'); var block = new Blockly.Block(workspace, 'field_variable_test_block'); - block.inputList[0].fieldRow[0].setValue('name1'); + block.inputList[0].fieldRow[0].setValue('id1'); var resultFieldDom = Blockly.Xml.blockToDom(block).childNodes[0]; xmlTest_checkVariableFieldDomValues(resultFieldDom, 'VAR', '', 'id1', 'name1'); @@ -301,7 +301,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('name1'); + block.inputList[0].fieldRow[0].setValue('1'); 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');