From 6218750207cabf82a264e64877306ff8d8248214 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Tue, 19 Dec 2017 11:28:23 -0800 Subject: [PATCH 1/2] Fix events for variable fields on new blocks; fix tests --- core/block.js | 15 +- core/field_variable.js | 12 +- core/xml.js | 39 ++-- tests/jsunit/event_test.js | 367 ++++++++++++++++++++------------- tests/jsunit/workspace_test.js | 12 +- tests/jsunit/xml_test.js | 25 ++- 6 files changed, 299 insertions(+), 171 deletions(-) diff --git a/core/block.js b/core/block.js index c6f400619..9da8878d2 100644 --- a/core/block.js +++ b/core/block.js @@ -155,8 +155,21 @@ Blockly.Block = function(workspace, prototypeName, opt_id) { // Record initial inline state. /** @type {boolean|undefined} */ this.inputsInlineDefault = this.inputsInline; + + // Fire a create event. if (Blockly.Events.isEnabled()) { - Blockly.Events.fire(new Blockly.Events.BlockCreate(this)); + var existingGroup = Blockly.Events.getGroup(); + if (!existingGroup) { + Blockly.Events.setGroup(true); + } + try { + Blockly.Events.fire(new Blockly.Events.BlockCreate(this)); + } finally { + if (!existingGroup) { + Blockly.Events.setGroup(false); + } + } + } // Bind an onchange function, if it exists. if (goog.isFunction(this.onchange)) { diff --git a/core/field_variable.js b/core/field_variable.js index bc9132412..5a5ab5def 100644 --- a/core/field_variable.js +++ b/core/field_variable.js @@ -90,7 +90,15 @@ Blockly.FieldVariable.prototype.initModel = function() { this.workspace_ = this.sourceBlock_.workspace; var variable = Blockly.Variables.getOrCreateVariable( this.workspace_, null, this.defaultVariableName, this.defaultType_); - this.setValue(variable.getId()); + + // Don't fire a change event for this setValue. It would have null as the + // old value, which is not valid. + Blockly.Events.disable(); + try { + this.setValue(variable.getId()); + } finally { + Blockly.Events.enable(); + } }; /** @@ -118,7 +126,7 @@ Blockly.FieldVariable.prototype.setSourceBlock = function(block) { * @return {string} Current variable's ID. */ Blockly.FieldVariable.prototype.getValue = function() { - return this.variable_ ? this.variable_.getId() : ''; + return this.variable_ ? this.variable_.getId() : null; }; /** diff --git a/core/xml.js b/core/xml.js index ec1da0e18..248e8c1c6 100644 --- a/core/xml.js +++ b/core/xml.js @@ -86,28 +86,31 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) { return element; }; +/** + * Encode a variable field as XML. + * @param {!Blockly.Field} field The field to encode. + * @param {!Blockly.Workspace} workspace The workspace that the field is in. + * @return {?Element} XML element, or null if the field did not need to be + * serialized. + * @private + */ Blockly.Xml.fieldToDomVariable_ = function(field, workspace) { var id = field.getValue(); - var variable = workspace.getVariableById(id); + // The field had not been initialized fully before being serialized. + if (id == null) { + field.initModel(); + id = field.getValue(); + } + var variable = Blockly.Variables.getVariable(workspace, id); + if (!variable) { - if (workspace.isFlyout && workspace.targetWorkspace) { - var potentialVariableMap = workspace.getPotentialVariableMap(); - if (potentialVariableMap) { - variable = potentialVariableMap.getVariableById(id); - } - } - } - if (variable) { - var container = goog.dom.createDom('field', null, variable.name); - container.setAttribute('name', field.name); - container.setAttribute('id', variable.getId()); - container.setAttribute('variabletype', variable.type); - return container; - } else { - // something went wrong? - console.warn('no variable in fieldtodom'); - return null; + throw Error('Tried to serialize a variable field with no variable.'); } + var container = goog.dom.createDom('field', null, variable.name); + container.setAttribute('name', field.name); + container.setAttribute('id', variable.getId()); + container.setAttribute('variabletype', variable.type); + return container; }; /** diff --git a/tests/jsunit/event_test.js b/tests/jsunit/event_test.js index 3221ed927..8bb485712 100644 --- a/tests/jsunit/event_test.js +++ b/tests/jsunit/event_test.js @@ -37,6 +37,7 @@ function eventTest_setUp() { function eventTest_setUpWithMockBlocks() { eventTest_setUp(); + // TODO: Replace with defineGetVarBlock(); Blockly.defineBlocksWithJsonArray([{ 'type': 'field_variable_test_block', 'message0': '%1', @@ -47,10 +48,16 @@ function eventTest_setUpWithMockBlocks() { 'variable': 'item' } ], + }, + { + 'type': 'simple_test_block', + 'message0': 'simple test block' }]); } function eventTest_tearDown() { + delete Blockly.Blocks['field_variable_test_block']; + delete Blockly.Blocks['simple_test_block']; mockControl_.$tearDown(); workspace.dispose(); } @@ -63,34 +70,47 @@ function eventTest_tearDownWithMockBlocks() { function test_abstract_constructor_block() { eventTest_setUpWithMockBlocks(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, '1'); - var block = new Blockly.Block(workspace, 'field_variable_test_block'); - var event = new Blockly.Events.Abstract(block); - assertUndefined(event.varId); - checkExactEventValues(event, {'blockId': '1', 'workspaceId': workspace.id, - 'group': '', 'recordUndo': true}); - eventTest_tearDownWithMockBlocks(); + try { + var block = createSimpleTestBlock(workspace); + + // Here's the event we care about. + var event = new Blockly.Events.Abstract(block); + assertUndefined(event.varId); + checkExactEventValues(event, {'blockId': '1', 'workspaceId': workspace.id, + 'group': '', 'recordUndo': true}); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_abstract_constructor_variable() { eventTest_setUpWithMockBlocks(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, '1'); - var variable = workspace.createVariable('name1', 'type1', 'id1'); - var event = new Blockly.Events.Abstract(variable); - assertUndefined(event.blockId); - checkExactEventValues(event, {'varId': 'id1', - 'workspaceId': workspace.id, 'group': '', 'recordUndo': true}); - eventTest_tearDownWithMockBlocks(); + try { + var variable = workspace.createVariable('name1', 'type1', 'id1'); + + var event = new Blockly.Events.Abstract(variable); + assertUndefined(event.blockId); + checkExactEventValues(event, {'varId': 'id1', + 'workspaceId': workspace.id, 'group': '', 'recordUndo': true}); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_abstract_constructor_null() { eventTest_setUpWithMockBlocks(); - var event = new Blockly.Events.Abstract(null); - assertUndefined(event.blockId); - assertUndefined(event.workspaceId); - checkExactEventValues(event, {'group': '', 'recordUndo': true}); - eventTest_tearDownWithMockBlocks(); + try { + var event = new Blockly.Events.Abstract(null); + assertUndefined(event.blockId); + assertUndefined(event.workspaceId); + checkExactEventValues(event, {'group': '', 'recordUndo': true}); + } finally { + eventTest_tearDownWithMockBlocks(); + } } +// Test util function checkCreateEventValues(event, block, ids, type) { var expected_xml = Blockly.Xml.domToText(Blockly.Xml.blockToDom(block)); var result_xml = Blockly.Xml.domToText(event.xml); @@ -99,6 +119,7 @@ function checkCreateEventValues(event, block, ids, type) { assertEquals(type, event.type); } +// Test util function checkDeleteEventValues(event, block, ids, type) { var expected_xml = Blockly.Xml.domToText(Blockly.Xml.blockToDom(block)); var result_xml = Blockly.Xml.domToText(event.oldXml); @@ -107,6 +128,7 @@ function checkDeleteEventValues(event, block, ids, type) { assertEquals(type, event.type); } +// Test util function checkExactEventValues(event, values) { var keys = Object.keys(values); for (var i = 0, field; field = keys[i]; i++) { @@ -114,155 +136,212 @@ function checkExactEventValues(event, values) { } } +// Test util +function createSimpleTestBlock(workspace) { + // Disable events while constructing the block: this is a test of the + // Blockly.Event constructors, not the block constructor. + Blockly.Events.disable(); + var block = new Blockly.Block(workspace, 'simple_test_block'); + Blockly.Events.enable(); + return block; +} + function test_create_constructor() { eventTest_setUpWithMockBlocks(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); - var block = new Blockly.Block(workspace, 'field_variable_test_block'); - var event = new Blockly.Events.Create(block); - checkCreateEventValues(event, block, ['1'], 'create'); - eventTest_tearDownWithMockBlocks(); + try { + var block = createSimpleTestBlock(workspace); + + var event = new Blockly.Events.Create(block); + checkCreateEventValues(event, block, ['1'], 'create'); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_blockCreate_constructor() { // expect that blockCreate behaves the same as create. eventTest_setUpWithMockBlocks(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); - var block = new Blockly.Block(workspace, 'field_variable_test_block'); - var event = new Blockly.Events.BlockCreate(block); - checkCreateEventValues(event, block, ['1'], 'create'); - eventTest_tearDownWithMockBlocks(); + try { + var block = createSimpleTestBlock(workspace); + + var event = new Blockly.Events.BlockCreate(block); + checkCreateEventValues(event, block, ['1'], 'create'); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_delete_constructor() { eventTest_setUpWithMockBlocks(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); - var block = new Blockly.Block(workspace, 'field_variable_test_block'); - var event = new Blockly.Events.Delete(block); - checkDeleteEventValues(event, block, ['1'], 'delete'); - eventTest_tearDownWithMockBlocks(); + try { + var block = createSimpleTestBlock(workspace); + var event = new Blockly.Events.Delete(block); + checkDeleteEventValues(event, block, ['1'], 'delete'); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_blockDelete_constructor() { eventTest_setUpWithMockBlocks(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); - var block = new Blockly.Block(workspace, 'field_variable_test_block'); - var event = new Blockly.Events.BlockDelete(block); - checkDeleteEventValues(event, block, ['1'], 'delete'); - eventTest_tearDownWithMockBlocks(); + try { + var block = createSimpleTestBlock(workspace); + var event = new Blockly.Events.BlockDelete(block); + checkDeleteEventValues(event, block, ['1'], 'delete'); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_change_constructor() { eventTest_setUpWithMockBlocks(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); - var block = new Blockly.Block(workspace, 'field_variable_test_block'); - var event = new Blockly.Events.Change(block, 'field', 'VAR', 'item', 'item2'); - checkExactEventValues(event, {'element': 'field', 'name': 'VAR', - 'oldValue': 'item', 'newValue': 'item2', 'type': 'change'}); - eventTest_tearDownWithMockBlocks(); + try { + Blockly.Events.disable(); + var block = new Blockly.Block(workspace, 'field_variable_test_block'); + Blockly.Events.enable(); + + var event = new Blockly.Events.Change(block, 'field', 'VAR', 'id1', 'id2'); + checkExactEventValues(event, {'element': 'field', 'name': 'VAR', + 'oldValue': 'id1', 'newValue': 'id2', 'type': 'change'}); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_blockChange_constructor() { eventTest_setUpWithMockBlocks(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1']); - var block = new Blockly.Block(workspace, 'field_variable_test_block'); - var event = new Blockly.Events.BlockChange(block, 'field', 'VAR', 'item', - 'item2'); - checkExactEventValues(event, {'element': 'field', 'name': 'VAR', - 'oldValue': 'item', 'newValue': 'item2', 'type': 'change'}); - eventTest_tearDownWithMockBlocks(); + try { + Blockly.Events.disable(); + var block = new Blockly.Block(workspace, 'field_variable_test_block'); + Blockly.Events.enable(); + + var event = new Blockly.Events.BlockChange(block, 'field', 'VAR', 'id1', + 'id2'); + checkExactEventValues(event, {'element': 'field', 'name': 'VAR', + 'oldValue': 'id1', 'newValue': 'id2', 'type': 'change'}); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_move_constructorCoordinate() { // Expect the oldCoordinate to be set. eventTest_setUpWithMockBlocks(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1', '2']); - var block1 = new Blockly.Block(workspace, 'field_variable_test_block'); - var coordinate = new goog.math.Coordinate(3,4); - block1.xy_ = coordinate; + try { + var block1 = createSimpleTestBlock(workspace); + var coordinate = new goog.math.Coordinate(3,4); + block1.xy_ = coordinate; - var event = new Blockly.Events.Move(block1); - checkExactEventValues(event, {'oldCoordinate': coordinate, - 'type': 'move'}); - eventTest_tearDownWithMockBlocks(); + var event = new Blockly.Events.Move(block1); + checkExactEventValues(event, {'oldCoordinate': coordinate, + 'type': 'move'}); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_move_constructoroldParentId() { // Expect the oldParentId to be set but not the oldCoordinate to be set. eventTest_setUpWithMockBlocks(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1', '2']); - var block1 = new Blockly.Block(workspace, 'field_variable_test_block'); - var block2 = new Blockly.Block(workspace, 'field_variable_test_block'); - block1.parentBlock_ = block2; - block1.xy_ = new goog.math.Coordinate(3,4); + try { + var block1 = createSimpleTestBlock(workspace); + var block2 = createSimpleTestBlock(workspace); + block1.parentBlock_ = block2; + block1.xy_ = new goog.math.Coordinate(3,4); - var event = new Blockly.Events.Move(block1); - checkExactEventValues(event, {'oldCoordinate': undefined, - 'oldParentId': '2', 'type': 'move'}); - block1.parentBlock_ = null; - eventTest_tearDownWithMockBlocks(); + var event = new Blockly.Events.Move(block1); + checkExactEventValues(event, {'oldCoordinate': undefined, + 'oldParentId': '2', 'type': 'move'}); + block1.parentBlock_ = null; + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_blockMove_constructorCoordinate() { // Expect the oldCoordinate to be set. eventTest_setUpWithMockBlocks(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1', '2']); - var block1 = new Blockly.Block(workspace, 'field_variable_test_block'); - var coordinate = new goog.math.Coordinate(3,4); - block1.xy_ = coordinate; + try { + var block1 = createSimpleTestBlock(workspace); + var coordinate = new goog.math.Coordinate(3,4); + block1.xy_ = coordinate; - var event = new Blockly.Events.BlockMove(block1); - checkExactEventValues(event, {'oldCoordinate': coordinate, - 'type': 'move'}); - eventTest_tearDownWithMockBlocks(); + var event = new Blockly.Events.BlockMove(block1); + checkExactEventValues(event, {'oldCoordinate': coordinate, + 'type': 'move'}); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_blockMove_constructoroldParentId() { // Expect the oldParentId to be set but not the oldCoordinate to be set. eventTest_setUpWithMockBlocks(); setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1', '2']); - var block1 = new Blockly.Block(workspace, 'field_variable_test_block'); - var block2 = new Blockly.Block(workspace, 'field_variable_test_block'); - block1.parentBlock_ = block2; - block1.xy_ = new goog.math.Coordinate(3,4); + try { + var block1 = createSimpleTestBlock(workspace); + var block2 = createSimpleTestBlock(workspace); + block1.parentBlock_ = block2; + block1.xy_ = new goog.math.Coordinate(3,4); - var event = new Blockly.Events.BlockMove(block1); - checkExactEventValues(event, {'oldCoordinate': undefined, - 'oldParentId': '2', 'type': 'move'}); - block1.parentBlock_ = null; - eventTest_tearDownWithMockBlocks(); + var event = new Blockly.Events.BlockMove(block1); + checkExactEventValues(event, {'oldCoordinate': undefined, + 'oldParentId': '2', 'type': 'move'}); + block1.parentBlock_ = null; + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_varCreate_constructor() { eventTest_setUp(); - var variable = workspace.createVariable('name1', 'type1', 'id1'); - var event = new Blockly.Events.VarCreate(variable); - checkExactEventValues(event, {'varName': 'name1', 'varType': 'type1', - 'type': 'var_create'}); - eventTest_tearDown(); + try { + var variable = workspace.createVariable('name1', 'type1', 'id1'); + var event = new Blockly.Events.VarCreate(variable); + checkExactEventValues(event, {'varName': 'name1', 'varType': 'type1', + 'type': 'var_create'}); + } finally { + eventTest_tearDown(); + } } function test_varCreate_toJson() { eventTest_setUp(); - var variable = workspace.createVariable('name1', 'type1', 'id1'); - var event = new Blockly.Events.VarCreate(variable); - var json = event.toJson(); - var expectedJson = ({type: "var_create", varId: "id1", varType: "type1", - varName: "name1"}); + try { + var variable = workspace.createVariable('name1', 'type1', 'id1'); + var event = new Blockly.Events.VarCreate(variable); + var json = event.toJson(); + var expectedJson = ({type: "var_create", varId: "id1", varType: "type1", + varName: "name1"}); - assertEquals(JSON.stringify(expectedJson), JSON.stringify(json)); - eventTest_tearDown(); + assertEquals(JSON.stringify(expectedJson), JSON.stringify(json)); + } finally { + eventTest_tearDown(); + } } function test_varCreate_fromJson() { eventTest_setUp(); - var variable = workspace.createVariable('name1', 'type1', 'id1'); - var event = new Blockly.Events.VarCreate(variable); - var event2 = new Blockly.Events.VarCreate(null); - var json = event.toJson(); - event2.fromJson(json); + try { + var variable = workspace.createVariable('name1', 'type1', 'id1'); + var event = new Blockly.Events.VarCreate(variable); + var event2 = new Blockly.Events.VarCreate(null); + var json = event.toJson(); + event2.fromJson(json); - assertEquals(JSON.stringify(json), JSON.stringify(event2.toJson())); - eventTest_tearDown(); + assertEquals(JSON.stringify(json), JSON.stringify(event2.toJson())); + } finally { + eventTest_tearDown(); + } } function test_varCreate_runForward() { @@ -395,58 +474,68 @@ function test_varBackard_runForward() { function test_events_filter() { eventTest_setUpWithMockBlocks(); - var block1 = workspace.newBlock('field_variable_test_block', '1'); - var events = [ - new Blockly.Events.BlockCreate(block1), - new Blockly.Events.BlockMove(block1), - new Blockly.Events.BlockChange(block1, 'field', 'VAR', 'item', 'item1'), - new Blockly.Events.Ui(block1, 'click') - ]; - var filteredEvents = Blockly.Events.filter(events, true); - assertEquals(4, filteredEvents.length); // no event should have been removed. - // test that the order hasn't changed - assertTrue(filteredEvents[0] instanceof Blockly.Events.BlockCreate); - assertTrue(filteredEvents[1] instanceof Blockly.Events.BlockMove); - assertTrue(filteredEvents[2] instanceof Blockly.Events.BlockChange); - assertTrue(filteredEvents[3] instanceof Blockly.Events.Ui); + try { + var block1 = workspace.newBlock('field_variable_test_block', '1'); + var events = [ + new Blockly.Events.BlockCreate(block1), + new Blockly.Events.BlockMove(block1), + new Blockly.Events.BlockChange(block1, 'field', 'VAR', 'id1', 'id2'), + new Blockly.Events.Ui(block1, 'click') + ]; + var filteredEvents = Blockly.Events.filter(events, true); + assertEquals(4, filteredEvents.length); // no event should have been removed. + // test that the order hasn't changed + assertTrue(filteredEvents[0] instanceof Blockly.Events.BlockCreate); + assertTrue(filteredEvents[1] instanceof Blockly.Events.BlockMove); + assertTrue(filteredEvents[2] instanceof Blockly.Events.BlockChange); + assertTrue(filteredEvents[3] instanceof Blockly.Events.Ui); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_events_filterForward() { eventTest_setUpWithMockBlocks(); - var block1 = workspace.newBlock('field_variable_test_block', '1'); - var events = [ - new Blockly.Events.BlockCreate(block1), - ]; - helper_addMoveEvent(events, block1, 1, 1); - helper_addMoveEvent(events, block1, 2, 2); - helper_addMoveEvent(events, block1, 3, 3); - var filteredEvents = Blockly.Events.filter(events, true); - assertEquals(2, filteredEvents.length); // duplicate moves should have been removed. - // test that the order hasn't changed - assertTrue(filteredEvents[0] instanceof Blockly.Events.BlockCreate); - assertTrue(filteredEvents[1] instanceof Blockly.Events.BlockMove); - assertEquals(3, filteredEvents[1].newCoordinate.x); - assertEquals(3, filteredEvents[1].newCoordinate.y); - eventTest_tearDownWithMockBlocks(); + try { + var block1 = workspace.newBlock('field_variable_test_block', '1'); + var events = [ + new Blockly.Events.BlockCreate(block1), + ]; + helper_addMoveEvent(events, block1, 1, 1); + helper_addMoveEvent(events, block1, 2, 2); + helper_addMoveEvent(events, block1, 3, 3); + var filteredEvents = Blockly.Events.filter(events, true); + assertEquals(2, filteredEvents.length); // duplicate moves should have been removed. + // test that the order hasn't changed + assertTrue(filteredEvents[0] instanceof Blockly.Events.BlockCreate); + assertTrue(filteredEvents[1] instanceof Blockly.Events.BlockMove); + assertEquals(3, filteredEvents[1].newCoordinate.x); + assertEquals(3, filteredEvents[1].newCoordinate.y); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_events_filterBackward() { eventTest_setUpWithMockBlocks(); - var block1 = workspace.newBlock('field_variable_test_block', '1'); - var events = [ - new Blockly.Events.BlockCreate(block1), - ]; - helper_addMoveEvent(events, block1, 1, 1); - helper_addMoveEvent(events, block1, 2, 2); - helper_addMoveEvent(events, block1, 3, 3); - var filteredEvents = Blockly.Events.filter(events, false); - assertEquals(2, filteredEvents.length); // duplicate event should have been removed. - // test that the order hasn't changed - assertTrue(filteredEvents[0] instanceof Blockly.Events.BlockCreate); - assertTrue(filteredEvents[1] instanceof Blockly.Events.BlockMove); - assertEquals(1, filteredEvents[1].newCoordinate.x); - assertEquals(1, filteredEvents[1].newCoordinate.y); - eventTest_tearDownWithMockBlocks(); + try { + var block1 = workspace.newBlock('field_variable_test_block', '1'); + var events = [ + new Blockly.Events.BlockCreate(block1), + ]; + helper_addMoveEvent(events, block1, 1, 1); + helper_addMoveEvent(events, block1, 2, 2); + helper_addMoveEvent(events, block1, 3, 3); + var filteredEvents = Blockly.Events.filter(events, false); + assertEquals(2, filteredEvents.length); // duplicate event should have been removed. + // test that the order hasn't changed + assertTrue(filteredEvents[0] instanceof Blockly.Events.BlockCreate); + assertTrue(filteredEvents[1] instanceof Blockly.Events.BlockMove); + assertEquals(1, filteredEvents[1].newCoordinate.x); + assertEquals(1, filteredEvents[1].newCoordinate.y); + } finally { + eventTest_tearDownWithMockBlocks(); + } } function test_events_filterDifferentBlocks() { diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index 20421d711..4dc107d54 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -160,14 +160,15 @@ function test_deleteVariable_InternalTrivial() { function test_addTopBlock_TrivialFlyoutIsTrue() { workspaceTest_setUp(); - workspace.isFlyout = true; var targetWorkspace = new Blockly.Workspace(); + workspace.isFlyout = true; workspace.targetWorkspace = targetWorkspace; targetWorkspace.createVariable('name1', '', '1'); // Flyout.init usually does this binding. - workspace.getVariableById = - targetWorkspace.getVariableById.bind(targetWorkspace); + workspace.variableMap_ = targetWorkspace.getVariableMap(); + // workspace.getVariableById = + // targetWorkspace.getVariableById.bind(targetWorkspace); try { var block = createMockBlock('1'); @@ -175,8 +176,11 @@ function test_addTopBlock_TrivialFlyoutIsTrue() { workspace.addTopBlock(block); checkVariableValues(workspace, 'name1', '', '1'); } finally { - targetWorkspace.dispose(); workspaceTest_tearDown(); + // Have to dispose of the main workspace after the flyout workspace, because + // it holds the variable map. + // Normally the main workspace disposes of the flyout workspace. + targetWorkspace.dispose(); } } diff --git a/tests/jsunit/xml_test.js b/tests/jsunit/xml_test.js index a2f929e51..9c3da790b 100644 --- a/tests/jsunit/xml_test.js +++ b/tests/jsunit/xml_test.js @@ -299,13 +299,20 @@ function test_blockToDom_fieldToDom_trivial() { function test_blockToDom_fieldToDom_defaultCase() { xmlTest_setUpWithMockBlocks(); 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'); - 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'); - xmlTest_tearDownWithMockBlocks(); + try { + workspace.createVariable('name1'); + + Blockly.Events.disable(); + var block = new Blockly.Block(workspace, 'field_variable_test_block'); + block.inputList[0].fieldRow[0].setValue('1'); + Blockly.Events.enable(); + + 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'); + } finally { + xmlTest_tearDownWithMockBlocks(); + } } function test_blockToDom_fieldToDom_notAFieldVariable() { @@ -347,8 +354,12 @@ function test_variablesToDom_twoVariables_oneBlock() { workspace.createVariable('name1', '', 'id1'); workspace.createVariable('name2', 'type2', 'id2'); + // If events are enabled during block construction, it will create a default + // variable. + Blockly.Events.disable(); var block = new Blockly.Block(workspace, 'field_variable_test_block'); block.inputList[0].fieldRow[0].setValue('id1'); + Blockly.Events.enable(); var resultDom = Blockly.Xml.variablesToDom(workspace.getAllVariables()); assertEquals(2, resultDom.children.length); From 4cd0b369446728b05f8553ae0693b333b2f8f434 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Tue, 19 Dec 2017 14:25:52 -0800 Subject: [PATCH 2/2] Fix event ordering for variables created from XML implicitly --- core/xml.js | 4 +- tests/jsunit/event_test.js | 81 ++++++++++++++++++++++++++++++++++ tests/jsunit/workspace_test.js | 2 - 3 files changed, 84 insertions(+), 3 deletions(-) diff --git a/core/xml.js b/core/xml.js index 248e8c1c6..08f2fa37e 100644 --- a/core/xml.js +++ b/core/xml.js @@ -531,7 +531,6 @@ Blockly.Xml.domToBlock = function(xmlBlock, workspace) { Blockly.Events.enable(); } if (Blockly.Events.isEnabled()) { - Blockly.Events.fire(new Blockly.Events.BlockCreate(topBlock)); var newVariables = Blockly.Variables.getAddedVariables(workspace, variablesBeforeCreation); // Fire a VarCreate event for each (if any) new variable created. @@ -539,6 +538,9 @@ Blockly.Xml.domToBlock = function(xmlBlock, workspace) { var thisVariable = newVariables[i]; Blockly.Events.fire(new Blockly.Events.VarCreate(thisVariable)); } + // Block events come after var events, in case they refer to newly created + // variables. + Blockly.Events.fire(new Blockly.Events.BlockCreate(topBlock)); } return topBlock; }; diff --git a/tests/jsunit/event_test.js b/tests/jsunit/event_test.js index 8bb485712..742fec938 100644 --- a/tests/jsunit/event_test.js +++ b/tests/jsunit/event_test.js @@ -29,6 +29,15 @@ goog.require('goog.testing.MockControl'); var mockControl_; var workspace; +var savedFireFunc = Blockly.Events.fire; + +function temporary_fireEvent(event) { + if (!Blockly.Events.isEnabled()) { + return; + } + Blockly.Events.FIRE_QUEUE_.push(event); + Blockly.Events.fireNow_(); +} function eventTest_setUp() { workspace = new Blockly.Workspace(); @@ -662,3 +671,75 @@ function helper_addMoveEventParent(events, block, parent) { block.setParent(parent); events[events.length-1].recordNew(); } + +function test_events_newblock_newvar() { + eventTest_setUpWithMockBlocks(); + + Blockly.Events.fire = temporary_fireEvent; + temporary_fireEvent.firedEvents_ = []; + // Expect three calls to genUid: one to set the block's ID, one for the event + // group's id, and one for the variable's ID. + setUpMockMethod(mockControl_, Blockly.utils, 'genUid', null, ['1', '2', '3']); + try { + var block = workspace.newBlock('field_variable_test_block'); + + var firedEvents = workspace.undoStack_; + // Expect two events: varCreate and block create. + assertEquals(2, firedEvents.length); + + var event0 = firedEvents[0]; + var event1 = firedEvents[1]; + assertEquals('var_create', event0.type); + assertEquals('create', event1.type); + + // Expect the events to have the same group ID. + assertEquals(event0.group, event1.group); + + // Expect the group ID to be the result of the second call to genUid. + assertEquals('2', event0.group); + + // Expect the workspace to have a variable with ID '3'. + assertNotNull(workspace.getVariableById('3')); + assertEquals('3', event0.varId); + } finally { + eventTest_tearDownWithMockBlocks(); + Blockly.Events.fire = savedFireFunc; + } +} + +// The sequence of events should be the same whether the block was created from +// XML or directly. +function test_events_newblock_newvar_xml() { + eventTest_setUpWithMockBlocks(); + + Blockly.Events.fire = temporary_fireEvent; + temporary_fireEvent.firedEvents_ = []; + try { + var dom = Blockly.Xml.textToDom( + '' + + ' ' + + ' name1' + + ' ' + + ''); + Blockly.Xml.domToWorkspace(dom, workspace); + + var firedEvents = workspace.undoStack_; + // Expect two events: varCreate and block create. + assertEquals(2, firedEvents.length); + + var event0 = firedEvents[0]; + var event1 = firedEvents[1]; + assertEquals('var_create', event0.type); + assertEquals('create', event1.type); + + // Expect the events to have the same group ID. + assertEquals(event0.group, event1.group); + + // Expect the workspace to have a variable with ID 'id1'. + assertNotNull(workspace.getVariableById('id1')); + assertEquals('id1', event0.varId); + } finally { + eventTest_tearDownWithMockBlocks(); + Blockly.Events.fire = savedFireFunc; + } +} diff --git a/tests/jsunit/workspace_test.js b/tests/jsunit/workspace_test.js index 4dc107d54..1aaca1895 100644 --- a/tests/jsunit/workspace_test.js +++ b/tests/jsunit/workspace_test.js @@ -167,8 +167,6 @@ function test_addTopBlock_TrivialFlyoutIsTrue() { // Flyout.init usually does this binding. workspace.variableMap_ = targetWorkspace.getVariableMap(); - // workspace.getVariableById = - // targetWorkspace.getVariableById.bind(targetWorkspace); try { var block = createMockBlock('1');