Fixed mrc_class_method_def blocks for methods that return a value (#299)

* In mrc_class_method_def.ts:
Added constants for NO_RETURN_VALUE and UNTYPED_RETURN_VALUE.
Added upgrade_004_to_005 to change mrcReturnType from 'Any' to UNTYPED_RETURN_VALUE.
Changed createCustomMethodBlockWithReturn to use UNTYPED_RETURN_VALUE instead of 'Any'.

In upgrade_project.ts:
Incremented CURRENT_VERSION to 0.0.5.
Changed switch statement in upgradeProjectIfNecessary to have the default case at the top.
All other cases have @ts-ignore and blank lines so it is more clear that the @ts-ignore goes with the case after it.
Added upgradeBlocksFiles function that can be called from other upgradeFrom_..._to_... functions.
Added upgradeFrom_004_to_005.

* In mrc_class_method_def.ts:
Added upgrade_002_to_003 function.

In upgrade_project.ts:
Modified upgradeBlocksFiles to take a predicate function.
Added anyModuleType and isOpMode functions that can be passed to upgradeBlocksFiles as the predicate function.
Modified upgradeFrom_002_to_003 to call upgradeBlocksFiles.
Modified upgradeFrom_004_to_005 to pass anyModuleType to upgradeBlocksFiles.

* Added preupgradePredicate and preupgradeFunc parameters to upgradeBlocksFiles.
Modified upgradeFrom_001_to_002 to call upgradeBlocksFiles.

* In mrc_mechanism_component_holder.ts:
Renamed function hidePrivateComponents to upgrade_001_to_002 and added code to check that the module type is ROBOT.

In module_content.ts:
Renamed addPrivateComponents to preupgrade_001_to_002.

In upgrade_project.ts:
Updated upgradeFrom_001_to_002.

* Removed code that checks that the workspace is headless in upgrade functions.

Author: lizlooney

src/blocks/mrc_class_method_def.ts

@@ -37,6 +37,9 @@ import * as paramContainer from './mrc_param_container'
 
 export const BLOCK_NAME = 'mrc_class_method_def';
 
+const NO_RETURN_VALUE = 'None';
+const UNTYPED_RETURN_VALUE = '';
+
 const INPUT_TITLE = 'TITLE';
 export const FIELD_METHOD_NAME = 'NAME';
 const FIELD_PARAM_PREFIX = 'PARAM_';
@@ -82,8 +85,8 @@ type ClassMethodDefExtraState = {
   canBeCalledOutsideClass: boolean,
   /**
    * The return type of the function.
-   * Use 'None' for no return value.
-   * Use '' for an untyped return value.
+   * Use NO_RETURN_VALUE for no return value.
+   * Use UNTYPED_RETURN_VALUE for an untyped return value.
    */
   returnType: string,
   /**
@@ -252,8 +255,8 @@ const CLASS_METHOD_DEF = {
       this.removeInput(INPUT_RETURN);
     }
 
-    // Add return input if return type is not 'None'
-    if (this.mrcReturnType && this.mrcReturnType !== 'None') {
+    // Add return input if return type is not NO_RETURN_VALUE
+    if (this.mrcReturnType !== undefined && this.mrcReturnType !== NO_RETURN_VALUE) {
       this.appendValueInput(INPUT_RETURN)
           .setAlign(Blockly.inputs.Align.RIGHT)
           .appendField(Blockly.Msg.PROCEDURES_DEFRETURN_RETURN);
@@ -348,6 +351,11 @@ const CLASS_METHOD_DEF = {
       methodBlock.mrcParameters = filteredParams;
     }
   },
+  upgrade_004_to_005: function(this: ClassMethodDefBlock) {
+    if (this.mrcReturnType === 'Any') {
+      this.mrcReturnType = UNTYPED_RETURN_VALUE;
+    }
+  },
 };
 
 /**
@@ -508,7 +516,7 @@ export function createCustomMethodBlock(): toolboxItems.Block {
     canChangeSignature: true,
     canBeCalledWithinClass: true,
     canBeCalledOutsideClass: true,
-    returnType: 'None',
+    returnType: NO_RETURN_VALUE,
     params: [],
   };
   const fields: {[key: string]: any} = {};
@@ -521,7 +529,7 @@ export function createCustomMethodBlockWithReturn(): toolboxItems.Block {
       canChangeSignature: true,
       canBeCalledWithinClass: true,
       canBeCalledOutsideClass: true,
-      returnType: 'Any',
+      returnType: UNTYPED_RETURN_VALUE,
       params: [],
   };
   const fields: {[key: string]: any} = {};
@@ -609,3 +617,23 @@ export function getMethodNamesAlreadyOverriddenInWorkspace(
     }
   });
 }
+
+/**
+ * Upgrades the ClassMethodDefBlocks in the given workspace from version 002 to 003.
+ * This function should only be called when upgrading old projects.
+ */
+export function upgrade_002_to_003(workspace: Blockly.Workspace): void {
+  workspace.getBlocksByType(BLOCK_NAME).forEach(block => {
+    (block as ClassMethodDefBlock).upgrade_002_to_003();
+  });
+}
+
+/**
+ * Upgrades the ClassMethodDefBlocks in the given workspace from version 004 to 005.
+ * This function should only be called when upgrading old projects.
+ */
+export function upgrade_004_to_005(workspace: Blockly.Workspace): void {
+  workspace.getBlocksByType(BLOCK_NAME).forEach(block => {
+    (block as ClassMethodDefBlock).upgrade_004_to_005();
+  });
+}

src/blocks/mrc_mechanism_component_holder.ts

@@ -36,6 +36,7 @@ import { ComponentBlock } from './mrc_component';
 import { BLOCK_NAME as  MRC_EVENT_NAME } from './mrc_event';
 import { OUTPUT_NAME as EVENT_OUTPUT } from './mrc_event';
 import { EventBlock } from './mrc_event';
+import { getModuleTypeForWorkspace } from './utils/workspaces';
 
 export const BLOCK_NAME = 'mrc_mechanism_component_holder';
 
@@ -520,13 +521,14 @@ export function mrcDescendantsMayHaveChanged(workspace: Blockly.Workspace): void
 }
 
 /**
- * Hide private components.
+ * Upgrades the MechanismComponentHolderBlock in the given workspace from version 001 to 002 by
+ * setting mrcHidePrivateComponents to true.
  * This function should only be called when upgrading old projects.
  */
-export function hidePrivateComponents(workspace: Blockly.Workspace) {
-  // Make sure the workspace is headless.
-  if (workspace.rendered) {
-    throw new Error('hidePrivateComponents should never be called with a rendered workspace.');
+export function upgrade_001_to_002(workspace: Blockly.Workspace) {
+  // Make sure the module type is ROBOT.
+  if (getModuleTypeForWorkspace(workspace) !== storageModule.ModuleType.ROBOT) {
+    throw new Error('upgrade_001_to_002 should only be called for a robot module.');
   }
   workspace.getBlocksByType(BLOCK_NAME).forEach(block => {
     (block as MechanismComponentHolderBlock).mrcHidePrivateComponents = true;

src/storage/module_content.ts

@@ -270,10 +270,10 @@ export class ModuleContent {
 }
 
 /**
- * Add privateComponents field.
- * This function should only called when upgrading old projects.
+ * Preupgrades the module content text by adding the privateComponents field.
+ * This function should only be called when upgrading old projects.
  */
-export function addPrivateComponents(moduleContentText: string): string {
+export function preupgrade_001_to_002(moduleContentText: string): string {
   const parsedContent = JSON.parse(moduleContentText);
   if (!('privateComponents' in parsedContent)) {
     parsedContent.privateComponents = [];

src/storage/upgrade_project.ts

@@ -22,130 +22,170 @@
 import * as semver from 'semver';
 import * as Blockly from 'blockly/core';
 
-import * as mechanismComponentHolder from '../blocks/mrc_mechanism_component_holder';
 import * as commonStorage from './common_storage';
 import * as storageModule from './module';
 import * as storageModuleContent from './module_content';
 import * as storageNames from './names';
 import * as storageProject from './project';
-import { ClassMethodDefBlock, BLOCK_NAME as MRC_CLASS_METHOD_DEF_BLOCK_NAME } from '../blocks/mrc_class_method_def';
+import { upgrade_001_to_002 } from '../blocks/mrc_mechanism_component_holder';
+import { upgrade_002_to_003, upgrade_004_to_005 } from '../blocks/mrc_class_method_def';
 import * as workspaces from '../blocks/utils/workspaces';
 
 export const NO_VERSION = '0.0.0';
-export const CURRENT_VERSION = '0.0.4';
+export const CURRENT_VERSION = '0.0.5';
 
 export async function upgradeProjectIfNecessary(
     storage: commonStorage.Storage, projectName: string): Promise<void> {
   const projectInfo = await storageProject.fetchProjectInfo(storage, projectName);
   if (semver.lt(projectInfo.version, CURRENT_VERSION)) {
     switch (projectInfo.version) {
+      default:
+        throw new Error('Unrecognized project version: ' + projectInfo.version);
+
+      // Intentional fallthrough after case '0.0.0'
       // @ts-ignore
       case '0.0.0':
         upgradeFrom_000_to_001(storage, projectName, projectInfo)
-      // Intentional fallthrough
+
+      // Intentional fallthrough after case '0.0.1'
       // @ts-ignore
       case '0.0.1':
         upgradeFrom_001_to_002(storage, projectName, projectInfo);
-      // Intentional fallthrough
+
+      // Intentional fallthrough after case '0.0.2'
       // @ts-ignore
       case '0.0.2':
-        upgradeFrom_002_to_003(storage, projectName, projectInfo);   
+        upgradeFrom_002_to_003(storage, projectName, projectInfo);
+
+      // Intentional fallthrough after case '0.0.3'
+      // @ts-ignore
       case '0.0.3':
         upgradeFrom_003_to_004(storage, projectName, projectInfo);
-        break;
-      default:
-        throw new Error('Unrecognized project version: ' + projectInfo.version);
 
+      // Intentional fallthrough after case '0.0.4'
+      // @ts-ignore
+      case '0.0.4':
+        upgradeFrom_004_to_005(storage, projectName, projectInfo);
     }
     await storageProject.saveProjectInfo(storage, projectName);
   }
 }
 
-async function upgradeFrom_000_to_001(
-    _storage: commonStorage.Storage,
-    _projectName: string,
-    projectInfo: storageProject.ProjectInfo): Promise<void> {
-  // Project was saved without a project.info.json file.
-  // Nothing needs to be done to upgrade to '0.0.1';
-  projectInfo.version = '0.0.1';
-}
-
-async function upgradeFrom_001_to_002(
+async function upgradeBlocksFiles(
     storage: commonStorage.Storage,
     projectName: string,
-    projectInfo: storageProject.ProjectInfo): Promise<void> {
-  // Modules were saved without private components.
-  // The Robot's mrc_mechanism_component_holder block was saved without hidePrivateComponents.
+    preupgradePredicate: (moduleType: storageModule.ModuleType) => boolean,
+    preupgradeFunc: (moduleContentText: string) => string,
+    upgradePredicate: (moduleType: storageModule.ModuleType) => boolean,
+    upgradeFunc: (w: Blockly.Workspace) => void
+): Promise<void> {
   const projectFileNames: string[] = await storage.list(
     storageNames.makeProjectDirectoryPath(projectName));
   for (const projectFileName of projectFileNames) {
     const modulePath = storageNames.makeFilePath(projectName, projectFileName);
-    let moduleContentText = await storage.fetchFileContentText(modulePath);
+    const moduleType = storageNames.getModuleType(modulePath);
+    const originalModuleContentText = await storage.fetchFileContentText(modulePath);
+    let moduleContentText = originalModuleContentText;
 
-    // Add private components to the module content.
-    moduleContentText = storageModuleContent.addPrivateComponents(moduleContentText);
+    if (preupgradePredicate(moduleType)) {
+      moduleContentText = preupgradeFunc(moduleContentText);
+    }
 
-    if (storageNames.getModuleType(modulePath) === storageModule.ModuleType.ROBOT) {
-      // If this module is the robot, hide the private components part of the
-      // mrc_mechanism_component_holder block.
+    if (upgradePredicate(moduleType)) {
       const moduleContent = storageModuleContent.parseModuleContentText(moduleContentText);
       let blocks = moduleContent.getBlocks();
 
       // Create a temporary workspace to upgrade the blocks.
-      const headlessWorkspace = workspaces.createHeadlessWorkspace(storageModule.ModuleType.ROBOT);
+      const headlessWorkspace = workspaces.createHeadlessWorkspace(moduleType);
 
       try {
         Blockly.serialization.workspaces.load(blocks, headlessWorkspace);
-        mechanismComponentHolder.hidePrivateComponents(headlessWorkspace);
+        upgradeFunc(headlessWorkspace);
         blocks = Blockly.serialization.workspaces.save(headlessWorkspace);
       } finally {
         workspaces.destroyHeadlessWorkspace(headlessWorkspace);
       }
+
       moduleContent.setBlocks(blocks);
       moduleContentText = moduleContent.getModuleContentText();
     }
 
-    await storage.saveFile(modulePath, moduleContentText);
+    if (moduleContentText !== originalModuleContentText) {
+      await storage.saveFile(modulePath, moduleContentText);
+    }
   }
-  projectInfo.version = '0.0.2';
 }
 
-async function upgradeFrom_002_to_003(
-    storage: commonStorage.Storage,
-    projectName: string,
-    projectInfo: storageProject.ProjectInfo): Promise<void> {
-  // Opmodes had robot as a parameter to init method
-  const projectFileNames: string[] = await storage.list(
-    storageNames.makeProjectDirectoryPath(projectName));
+/**
+ * Predicate function that can be passed to upgradeBlocksFiles indicating that all modules should be
+ * affected.
+ */
+function anyModuleType(_moduleType: storageModule.ModuleType): boolean {
+  return true;
+}
 
-  for (const projectFileName of projectFileNames) {
-    const modulePath = storageNames.makeFilePath(projectName, projectFileName);
+/**
+ * Predicate function that can be passed to upgradeBlocksFiles indicating that only OpMode modules
+ * should be affected.
+ */
+function isOpMode(moduleType: storageModule.ModuleType): boolean {
+  return moduleType === storageModule.ModuleType.OPMODE;
+}
 
-    if (storageNames.getModuleType(modulePath) === storageModule.ModuleType.OPMODE) {
-      let moduleContentText = await storage.fetchFileContentText(modulePath);
-      const moduleContent = storageModuleContent.parseModuleContentText(moduleContentText);
-      let blocks = moduleContent.getBlocks();
+/**
+ * Predicate function that can be passed to upgradeBlocksFiles indicating that only Robot modules
+ * should be affected.
+ */
+function isRobot(moduleType: storageModule.ModuleType): boolean {
+  return moduleType === storageModule.ModuleType.ROBOT;
+}
 
-      // Create a temporary workspace to upgrade the blocks.
-      const headlessWorkspace = workspaces.createHeadlessWorkspace(storageModule.ModuleType.ROBOT);
+/**
+ * Predicate function that can be passed to upgradeBlocksFiles indicating that no modules should be
+ * affected.
+ */
+function noModuleTypes(_moduleType: storageModule.ModuleType): boolean {
+  return false;
+}
 
-      try {
-        Blockly.serialization.workspaces.load(blocks, headlessWorkspace);
+/**
+ * Preupgrade function that makes no changes to moduleContentText.
+ */
+function noPreupgrade(moduleContentText: string): string {
+  return moduleContentText;
+}
 
-        // Method blocks need to be upgraded
-        headlessWorkspace.getBlocksByType(MRC_CLASS_METHOD_DEF_BLOCK_NAME, false).forEach(block => {
-          (block as ClassMethodDefBlock).upgrade_002_to_003();
-        });
-        blocks = Blockly.serialization.workspaces.save(headlessWorkspace);
-      } finally {
-        workspaces.destroyHeadlessWorkspace(headlessWorkspace);
-      }
+async function upgradeFrom_000_to_001(
+    _storage: commonStorage.Storage,
+    _projectName: string,
+    projectInfo: storageProject.ProjectInfo): Promise<void> {
+  // Project was saved without a project.info.json file.
+  // Nothing needs to be done to upgrade to '0.0.1';
+  projectInfo.version = '0.0.1';
+}
 
-      moduleContent.setBlocks(blocks);
-      moduleContentText = moduleContent.getModuleContentText();
-      await storage.saveFile(modulePath, moduleContentText);
-    }
-  }
+async function upgradeFrom_001_to_002(
+    storage: commonStorage.Storage,
+    projectName: string,
+    projectInfo: storageProject.ProjectInfo): Promise<void> {
+  // Modules were saved without private components.
+  // The Robot's mrc_mechanism_component_holder block was saved without hidePrivateComponents.
+  await upgradeBlocksFiles(
+      storage, projectName,
+      anyModuleType, storageModuleContent.preupgrade_001_to_002,
+      isRobot, upgrade_001_to_002);
+  projectInfo.version = '0.0.2';
+}
+
+async function upgradeFrom_002_to_003(
+    storage: commonStorage.Storage,
+    projectName: string,
+    projectInfo: storageProject.ProjectInfo): Promise<void> {
+  // OpModes had robot as a parameter to init method.
+  await upgradeBlocksFiles(
+      storage, projectName,
+      noModuleTypes, noPreupgrade,
+      isOpMode, upgrade_002_to_003);
   projectInfo.version = '0.0.3';
 }
 
@@ -157,3 +197,15 @@ async function upgradeFrom_003_to_004(
   // from loading a project with an older version of software.
   projectInfo.version = '0.0.4';
 }
+
+async function upgradeFrom_004_to_005(
+    storage: commonStorage.Storage,
+    projectName: string,
+    projectInfo: storageProject.ProjectInfo): Promise<void> {
+  // mrc_class_method_def blocks that return a value need to have returnType changed from 'Any' to ''.
+  await upgradeBlocksFiles(
+      storage, projectName,
+      noModuleTypes, noPreupgrade,
+      anyModuleType, upgrade_004_to_005);
+  projectInfo.version = '0.0.5';
+}