From 4cd0b369446728b05f8553ae0693b333b2f8f434 Mon Sep 17 00:00:00 2001
From: Rachel Fenichel <fenichel@google.com>
Date: Tue, 19 Dec 2017 14:25:52 -0800
Subject: [PATCH] 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(
+        '<xml>' +
+        '  <block type="field_variable_test_block" id="block1">' +
+        '    <field name="VAR" id="id1" variabletype="">name1</field>' +
+        '  </block>' +
+        '</xml>');
+    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');