Replace use of isRenderingOn with Blockly event management

When the 2017 Blockly update was done, it used isRenderingOn to
determine when to position and render the blocks. This was to optimize
the interaction between the load/upgrade process and workspace
resizes. However, in rare instances it seems to result in blocks not
being placed correctly.

This change removes the special handling of upgrading. Instead, it
disables and enables the Blockly event system during a load/upgrade
operation. At the end of the upgrade process (if needed), it will also
force a save of the blocks workspace.

Fixes #1071

Change-Id: I6bec41dd67bc6371e794e93850ea1455b9acf8c7

appinventor/blocklyeditor/src/events.js

@@ -76,6 +76,12 @@ AI.Events.BLOCKS_ARRANGE_END = 'blocks.arrange.end';
  */
 AI.Events.WORKSPACE_VIEWPORT_MOVE = "blocks.workspace.move";
 
+/**
+ * Type identifier used for forcing a workspace save (required after upgrades).
+ * @type {string}
+ */
+AI.Events.FORCE_SAVE = 'blocks.save.force';
+
 /**
  * Abstract class for all App Inventor events.
  * @constructor
@@ -481,6 +487,52 @@ AI.Events.WorkspaceMove.prototype.run = function(forward) {
   workspace.scrollbar.set(x, y);
 };
 
+/**
+ * An event used to trigger a save of the blocks workspace.
+ * @param {Blockly.Workspace=} workspace The workspace to be saved.
+ * @constructor
+ */
+AI.Events.ForceSave = function(workspace) {
+  AI.Events.ForceSave.superClass_.constructor.call(this);
+  if (workspace) {
+    this.workspaceId = workspace.id;
+  }
+  this.recordUndo = false;
+};
+goog.inherits(AI.Events.ForceSave, AI.Events.Abstract);
+
+/**
+ * The type of the event.
+ * @type {string}
+ */
+AI.Events.ForceSave.prototype.type = AI.Events.FORCE_SAVE;
+
+/**
+ * ForceSave must not be transient. The isTransient flag is used to determine whether or not to
+ * save the workspace, so if ForceSave were transient the workspace would not save.
+ * @type {boolean}
+ */
+AI.Events.ForceSave.prototype.isTransient = false;
+
+/**
+ * Serialize the ForceSave event as a JSON object.
+ * @returns {Object}
+ */
+AI.Events.ForceSave.prototype.toJson = function() {
+  var json = AI.Events.ForceSave.superClass_.toJson.call(this);
+  json['workspaceId'] = this.workspaceId;
+  return json;
+};
+
+/**
+ * Deserialize the ForceSave event form a JSON object.
+ * @param {Object} json A JSON object previously created by {@link #toJson()}
+ */
+AI.Events.ForceSave.prototype.fromJson = function(json) {
+  AI.Events.ForceSave.superClass_.fromJson.call(this, json);
+  this.workspaceId = json['workspaceId'];
+};
+
 /**
  * Filter the queued events and merge duplicates. This version is O(n) versus the implementation
  * provided by Blockly that is O(n^2). This improves performance when people perform or undo

appinventor/blocklyeditor/src/savefile.js

@@ -26,36 +26,41 @@ goog.require('AI.Blockly.Instrument');
 if (Blockly.SaveFile === undefined) Blockly.SaveFile = {};
 
 Blockly.SaveFile.load = function(preUpgradeFormJson, blocksContent) {
-  Blockly.Instrument.initializeStats("Blockly.SaveFile.load");
-  Blockly.Instrument.timer(
-  function () {
-    // We leave it to our caller to catch JavaScriptException and deal with
-    // errors loading the block space.
+  try {
+    Blockly.Events.disable();
+    Blockly.Instrument.initializeStats("Blockly.SaveFile.load");
+    Blockly.Instrument.timer(
+      function () {
+        // We leave it to our caller to catch JavaScriptException and deal with
+        // errors loading the block space.
 
-    if (blocksContent.length != 0) {
+        if (blocksContent.length != 0) {
 
-      // Turn rendering off since we may go back and forth between
-      // dom and blocks representations many time, and only want
-      // to render once at the very end.
-      try {
-        Blockly.Block.isRenderingOn = false;
-        // Perform language and component upgrades, and put blocks into Blockly.mainWorkspace
-        Blockly.Versioning.upgrade(preUpgradeFormJson,blocksContent);
-      } finally { // Guarantee that rendering is turned on going forward.
-        Blockly.Block.isRenderingOn = true;
+          // Turn rendering off since we may go back and forth between
+          // dom and blocks representations many time, and only want
+          // to render once at the very end.
+          try {
+            Blockly.Block.isRenderingOn = false;
+            // Perform language and component upgrades, and put blocks into Blockly.mainWorkspace
+            Blockly.Versioning.upgrade(preUpgradeFormJson, blocksContent);
+          } finally { // Guarantee that rendering is turned on going forward.
+            Blockly.Block.isRenderingOn = true;
+          }
+        }
+      },
+      function (result, timeDiff) {
+        Blockly.Instrument.stats.totalTime = timeDiff;
+        Blockly.Instrument.stats.blockCount = Blockly.Instrument.stats.domToBlockInnerCalls;
+        Blockly.Instrument.stats.topBlockCount = Blockly.Instrument.stats.domToBlockCalls;
+        Blockly.Instrument.displayStats("Blockly.SaveFile.load");
+        if (Blockly.mainWorkspace != null && Blockly.mainWorkspace.getCanvas() != null) {
+          Blockly.mainWorkspace.render(); // Save the rendering of the workspace until the very end
+        }
       }
-    }
-  },
-  function (result, timeDiff) {
-    Blockly.Instrument.stats.totalTime = timeDiff;
-    Blockly.Instrument.stats.blockCount = Blockly.Instrument.stats.domToBlockInnerCalls;
-    Blockly.Instrument.stats.topBlockCount = Blockly.Instrument.stats.domToBlockCalls;
-    Blockly.Instrument.displayStats("Blockly.SaveFile.load");
-    if (Blockly.mainWorkspace != null && Blockly.mainWorkspace.getCanvas() != null) {
-      Blockly.mainWorkspace.render(); // Save the rendering of the workspace until the very end
-    }
+    );
+  } finally {
+    Blockly.Events.enable();
   }
-  );
 };
 
 /**

appinventor/blocklyeditor/src/versioning.js

@@ -69,6 +69,7 @@ Blockly.Versioning.upgrade = function (preUpgradeFormJsonString, blocksContent,
   opt_workspace = opt_workspace || Blockly.mainWorkspace;
   var preUpgradeFormJsonObject = JSON.parse(preUpgradeFormJsonString);
   var dom = Blockly.Xml.textToDom(blocksContent); // Initial blocks rep is dom for blocksContent
+  var didUpgrade = false;
 
   /**
    * Upgrade the given componentType. If componentType is "Language", upgrades the blocks language.
@@ -117,6 +118,7 @@ Blockly.Versioning.upgrade = function (preUpgradeFormJsonString, blocksContent,
         // Apply upgrader, possibly mutating rep and changing its dynamic type.
         rep = Blockly.Versioning.applyUpgrader(versionUpgrader, rep, opt_workspace);
       }
+      didUpgrade = true;
     } // otherwise, preUpgradeVersion and systemVersion are equal and no updgrade is necessary
     return rep; // Return final blocks representation, for dynamic typing purposes
   }
@@ -166,6 +168,7 @@ Blockly.Versioning.upgrade = function (preUpgradeFormJsonString, blocksContent,
   Blockly.Versioning.log("Blockly.Versioning.upgrade: Final conversion to Blockly.mainWorkspace");
   Blockly.Versioning.ensureWorkspace(blocksRep, opt_workspace); // No need to use result; does work by side effect on Blockly.mainWorkspace
 
+  return didUpgrade;
 };
 
 /**
@@ -260,7 +263,7 @@ Blockly.Versioning.ensureWorkspace = function (blocksRep, opt_workspace) {
     var workspace = opt_workspace || Blockly.mainWorkspace;
     Blockly.Versioning.log("Blockly.Versioning.ensureWorkspace: converting dom to Blockly.mainWorkspace");
     workspace.clear(); // Remove any existing blocks before we add new ones.
-    Blockly.Xml.domToWorkspaceHeadless(blocksRep, workspace);
+    Blockly.Xml.domToWorkspace(blocksRep, workspace);
     // update top block positions in event of save before rendering.
     var blocks = workspace.getTopBlocks();
     for (var i = 0; i < blocks.length; i++) {

appinventor/blocklyeditor/src/workspace_svg.js

@@ -403,10 +403,15 @@ Blockly.WorkspaceSvg.prototype.populateComponentTypes = function(strComponentInf
 Blockly.WorkspaceSvg.prototype.loadBlocksFile = function(formJson, blocksContent) {
   if (blocksContent.length != 0) {
     try {
-      Blockly.Block.isRenderingOn = false;
-      Blockly.Versioning.upgrade(formJson, blocksContent, this);
+      Blockly.Events.disable();
+      if (Blockly.Versioning.upgrade(formJson, blocksContent, this)) {
+        var self = this;
+        setTimeout(function() {
+          self.fireChangeListener(new AI.Events.ForceSave(self));
+        });
+      }
     } finally {
-      Blockly.Block.isRenderingOn = true;
+      Blockly.Events.enable();
     }
     if (this.getCanvas() != null) {
       this.render();

appinventor/blocklyeditor/src/xml.js

@@ -18,90 +18,6 @@ goog.require('Blockly.Xml');
 // App Inventor extensions to Blockly
 goog.require('AI.Blockly.Instrument');  // lyn's instrumentation code
 
-/**
- * Decode an XML DOM and create blocks on the workspace.
- * @param {!Element} xml XML DOM.
- * @param {!Blockly.Workspace} workspace The workspace.
- */
-Blockly.Xml.domToWorkspaceHeadless = function(xml, workspace) {
-  Blockly.Events.disable();
-  try {
-    if (xml instanceof Blockly.Workspace) {
-      var swap = xml;
-      xml = workspace;
-      workspace = swap;
-      console.warn('Deprecated call to Blockly.Xml.domToWorkspace, ' +
-                   'swap the arguments.');
-    }
-    var width;  // Not used in LTR.
-    workspace.rendered = false;
-    if (workspace.RTL) {
-      width = workspace.getWidth();
-    }
-    Blockly.Field.startCache();
-    // Safari 7.1.3 is known to provide node lists with extra references to
-    // children beyond the lists' length.  Trust the length, do not use the
-    // looping pattern of checking the index for an object.
-    var childCount = xml.childNodes.length;
-    var existingGroup = Blockly.Events.getGroup();
-    if (!existingGroup) {
-      Blockly.Events.setGroup(true);
-    }
-    for (var i = 0; i < childCount; i++) {
-      var xmlChild = xml.childNodes[i];
-      var name = xmlChild.nodeName.toLowerCase();
-      if (name == 'block' ||
-        (name == 'shadow' && !Blockly.Events.recordUndo)) {
-        // Allow top-level shadow blocks if recordUndo is disabled since
-        // that means an undo is in progress.  Such a block is expected
-        // to be moved to a nested destination in the next operation.
-        var block = Blockly.Xml.domToBlockHeadless_(xmlChild, workspace);
-        block.x = parseInt(xmlChild.getAttribute('x'), 10);
-        block.y = parseInt(xmlChild.getAttribute('y'), 10);
-      } else if (name == 'shadow') {
-        goog.asserts.fail('Shadow block cannot be a top-level block.');
-      }
-    }
-    var block, blocks = workspace.getAllBlocks();
-    for (i = 0; block = blocks[i]; i++) {
-      if (block.eventparam) {
-        block.setFieldValue(workspace.getComponentDatabase().getInternationalizedParameterName(block.eventparam), 'VAR');
-      }
-    }
-    if (!existingGroup) {
-      Blockly.Events.setGroup(false);
-    }
-    Blockly.Field.stopCache();
-  } finally {
-    Blockly.Events.enable();
-  }
-
-  workspace.updateVariableList(false);
-};
-
-/**
- * Encode a block subtree as XML with XY coordinates.
- * @param {!Blockly.Block} block The root block to encode.
- * @param {boolean} opt_noId True if the encoder should skip the block id.
- * @return {!Element} Tree of XML elements.
- */
-Blockly.Xml.blockToDomWithXY = (function(f) {
-  return function(block, opt_noId) {
-    var element = f(block, opt_noId);
-    if (!Blockly.Block.isRenderingOn) {
-      // isRenderingOn is off during loading, so we are serializing in the middle of loading a file.
-      // Save the XY coordinate of the block so that we don't end up with all blocks at (0, 0).
-      // App Inventor only positions the blocks at the very end to reduce repositioning churn going
-      // to/from the Blockly workspace representation. Ideally we wouldn't go between the two
-      // representation because DOM manipulations are costly.
-      var width = block.workspace.RTL ? block.workspace.getWidth() : 0;
-      element.setAttribute('x', block.workspace.RTL ? width - block.x : block.x);
-      element.setAttribute('y', block.y);
-    }
-    return element;
-  };
-})(Blockly.Xml.blockToDomWithXY);
-
 if (Blockly.Instrument.isOn) {
 
 Blockly.Xml.domToWorkspace = (function(func) {

appinventor/blocklyeditor/tests/com/google/appinventor/blocklyeditor/moleMashTest.js

@@ -62,7 +62,8 @@ page.open('src/demos/yail/yail_testing_index.html', function(status) {
     for (var i = 0, block; i < topBlocks.length; i++) {
       block = topBlocks[i];
       if (block.type === 'global_declaration') {
-        return block.x === 16 && block.y === 182;
+        var xy = block.getRelativeToSurfaceXY();
+        return xy.x === 16 && xy.y === 182;
       }
     }
     return false;