Call the steps method after super.loop (#311)

* Let the user delete the loop method in an OpMode.

Removed code from opmode.py that calls the steps method.

In mrc_class_method_def.ts:
Renamed classSpecific and getClassSpecificForInit to superInitParameters and getSuperInitParameters.

In mrc_steps.ts:
Added constant STEPS_METHOD_NAME and changed the steps method name to _steps.

In python.ts:
Added constant PERIODIC_METHOD_NAME.

In extended_python_generator.ts:
Changed finish method to add the code for calling the steps method to the end of the loop method.
Renamed getClassSpecificForInit to getSuperInitParameters.

* Don't generated python code that checks whether _steps method exists.

* If the workspace doesn't have steps and doesn't overide the loop method, show a warning on the opmode details block.

* Fixed misspelling.

* Changed the value of the warning id so it doesn't seem like a string that needs to be translated.

Author: lizlooney

docs/testing_before_pr_checklist.md

@@ -13,7 +13,7 @@ For each PR, make sure each of these works
 * [ ] Add an event to the Mechanism
 * [ ] In Robot, make sure you can see the event handler in the toolbox Robot -> Mechanisms -> my_arm -> Events
 * [ ] In Opmode, make sure you can see the event handler in the toolbox Robot -> Mechanisms -> my_arm -> Events
-# Mechanims
+# Mechanisms
 * [ ] Add a public component to the mechanism
 * [ ] Add a private component to the mechanism
 * [ ] Make sure that in the Robot you can see the public component (and not the private one) in the toolbox

server_python_scripts/blocks_base_classes/opmode.py

@@ -7,9 +7,6 @@ def __init__(self, robot: RobotBase):
     def start(self) -> None:
         self.robot.start()
     def loop(self) -> None:
-        # Call steps method if it exists in the derived class
-        if hasattr(self, 'steps') and callable(self.steps):
-            self.steps()
         self.robot.update()
     def stop(self) -> None:
         self.robot.stop()

src/blocks/mrc_class_method_def.ts

@@ -457,8 +457,8 @@ export const pythonFromBlock = function (
     xfix2 = xfix1;
   }
   if (block.mrcPythonMethodName === '__init__') {
-    const classSpecific = generator.getClassSpecificForInit();
-    branch = generator.INDENT + 'super().__init__(' + classSpecific + ')\n' +
+    const superInitParameters = generator.getSuperInitParameters();
+    branch = generator.INDENT + 'super().__init__(' + superInitParameters + ')\n' +
         generator.generateInitStatements() + branch;
   }
   else if (generator.inBaseClassMethod(blocklyName)) {

src/blocks/mrc_opmode_details.ts

@@ -43,14 +43,19 @@
  */
 import * as Blockly from 'blockly';
 
+import { PERIODIC_METHOD_NAME } from './utils/python';
+import { Editor } from '../editor/editor';
 import { ExtendedPythonGenerator, OpModeDetails } from '../editor/extended_python_generator';
 import { createFieldDropdown } from '../fields/FieldDropdown';
 import { MRC_STYLE_CLASS_BLOCKS } from '../themes/styles';
 
 export const BLOCK_NAME = 'mrc_opmode_details';
 
+const WARNING_ID_STEPS_OR_PERIODIC_REQUIRED = 'id_steps_or_periodic_required';
+
 type OpmodeDetailsBlock = Blockly.Block & OpmodeDetailsMixin;
 interface OpmodeDetailsMixin extends OpmodeDetailsMixinType {
+  mrcHasStepsOrPeriodicRequiredWarning: boolean,
 }
 type OpmodeDetailsMixinType = typeof OPMODE_DETAILS;
 
@@ -59,6 +64,7 @@ const OPMODE_DETAILS = {
     * Block initialization.
     */
   init: function (this: OpmodeDetailsBlock): void {
+    this.mrcHasStepsOrPeriodicRequiredWarning = false;
     this.setStyle(MRC_STYLE_CLASS_BLOCKS);
     this.appendDummyInput()
       .appendField(Blockly.Msg.TYPE)
@@ -80,6 +86,24 @@ const OPMODE_DETAILS = {
     this.getField('NAME')?.setTooltip(Blockly.Msg.OPMODE_NAME_TOOLTIP);
     this.getField('GROUP')?.setTooltip(Blockly.Msg.OPMODE_GROUP_TOOLTIP);
   },
+  checkOpMode(this: OpmodeDetailsBlock, editor: Editor): void {
+    if (editor.isStepsInWorkspace() ||
+        editor.getMethodNamesAlreadyOverriddenInWorkspace().includes(PERIODIC_METHOD_NAME)) {
+      // Remove the previous warning.
+      this.setWarningText(null, WARNING_ID_STEPS_OR_PERIODIC_REQUIRED);
+      this.mrcHasStepsOrPeriodicRequiredWarning = false;
+    } else {
+      // Otherwise, add a warning to the block.
+      if (!this.mrcHasStepsOrPeriodicRequiredWarning) {
+        this.setWarningText(Blockly.Msg.WARNING_OPMODE_STEPS_OR_PERIODIC_REQUIRED, WARNING_ID_STEPS_OR_PERIODIC_REQUIRED);
+        const icon = this.getIcon(Blockly.icons.IconType.WARNING);
+        if (icon) {
+          icon.setBubbleVisible(true);
+        }
+        this.mrcHasStepsOrPeriodicRequiredWarning = true;
+      }
+    }
+  }
 }
 
 export const setup = function () {
@@ -98,3 +122,11 @@ export const pythonFromBlock = function (
                               ));
     return '';
 }
+
+// Misc
+
+export function checkOpMode(workspace: Blockly.Workspace, editor: Editor) {
+  workspace.getBlocksByType(BLOCK_NAME).forEach(block => {
+    (block as OpmodeDetailsBlock).checkOpMode(editor);
+  });
+}

src/blocks/mrc_steps.ts

@@ -32,6 +32,8 @@ import * as toolboxItems from '../toolbox/items';
 
 export const BLOCK_NAME = 'mrc_steps';
 
+export const STEPS_METHOD_NAME = '_steps';
+
 const INPUT_CONDITION_PREFIX = 'CONDITION_';
 const INPUT_STATEMENT_PREFIX = 'STATEMENT_';
 
@@ -219,7 +221,7 @@ export const pythonFromBlock = function (
   block: StepsBlock,
   generator: ExtendedPythonGenerator,
 ) {
-  let code = 'def steps(self):\n';
+  let code = 'def ' + STEPS_METHOD_NAME + '(self):\n';
   code += generator.INDENT + 'if not hasattr(self, \'_initialized_steps\'):\n';
   code += generator.INDENT.repeat(2) + 'self._current_step = \'' + block.mrcStepNames[0] + '\'\n';
   code += generator.INDENT.repeat(2) + 'self._initialized_steps = True\n\n';
@@ -243,7 +245,7 @@ export const pythonFromBlock = function (
     }
   });
 
-  generator.addClassMethodDefinition('steps', code);
+  generator.addClassMethodDefinition(STEPS_METHOD_NAME, code);
 
   return ''
 }

src/blocks/tokens.ts

@@ -106,6 +106,7 @@ export function customTokens(t: (key: string) => string): typeof Blockly.Msg {
     WARNING_EVENT_NOT_IN_HOLDER: t('BLOCKLY.WARNING.EVENT_NOT_IN_HOLDER'),
     WARNING_COMPONENT_NOT_IN_HOLDER: t('BLOCKLY.WARNING.COMPONENT_NOT_IN_HOLDER'),
     WARNING_MECHANISM_NOT_IN_HOLDER: t('BLOCKLY.WARNING.MECHANISM_NOT_IN_HOLDER'),
+    WARNING_OPMODE_STEPS_OR_PERIODIC_REQUIRED: t('BLOCKLY.WARNING.OPMODE_STEPS_OR_PERIODIC_REQUIRED'),
     MRC_CATEGORY_HARDWARE: t('BLOCKLY.CATEGORY.HARDWARE'),
     MRC_CATEGORY_ROBOT: t('BLOCKLY.CATEGORY.ROBOT'),
     MRC_CATEGORY_COMPONENTS: t('BLOCKLY.CATEGORY.COMPONENTS'),

src/blocks/utils/python.ts

@@ -33,9 +33,12 @@ import * as SetPythonVariable from "../mrc_set_python_variable";
 
 export const MODULE_NAME_BLOCKS_BASE_CLASSES = 'blocks_base_classes';
 export const CLASS_NAME_ROBOT_BASE = MODULE_NAME_BLOCKS_BASE_CLASSES + '.RobotBase';
-export const CLASS_NAME_OPMODE = MODULE_NAME_BLOCKS_BASE_CLASSES + '.OpMode';
 export const CLASS_NAME_MECHANISM = MODULE_NAME_BLOCKS_BASE_CLASSES + '.Mechanism';
 
+// TODO(lizlooney): what about PeriodicOpMode and LinearOpMode?
+export const CLASS_NAME_OPMODE = MODULE_NAME_BLOCKS_BASE_CLASSES + '.OpMode';
+// TODO(lizlooney): Make sure to update the value of PERIODIC_METHOD_NAME when we update wpilib.
+export const PERIODIC_METHOD_NAME = 'loop';
 
 export const robotPyData = generatedRobotPyData as PythonData;
 const externalSamplesData = generatedExternalSamplesData as PythonData

src/editor/editor.ts

@@ -29,6 +29,7 @@ import * as storageNames from '../storage/names';
 import * as storageProject from '../storage/project';
 import * as eventHandler from '../blocks/mrc_event_handler';
 import * as classMethodDef from '../blocks/mrc_class_method_def';
+import * as opmodeDetails from '../blocks/mrc_opmode_details';
 import * as blockSteps from '../blocks/mrc_steps';
 import * as mechanismComponentHolder from '../blocks/mrc_mechanism_component_holder';
 import * as workspaces from '../blocks/utils/workspaces';
@@ -90,12 +91,7 @@ export class Editor {
       return;
     }
     if (event.type === Blockly.Events.FINISHED_LOADING) {
-      // Remove the while-loading listener.
-      this.blocklyWorkspace.removeChangeListener(this.bindedOnChange);
-
-      // Add the after-loading listener.
-      this.bindedOnChange = this.onChangeAfterLoading.bind(this);
-      this.blocklyWorkspace.addChangeListener(this.bindedOnChange);
+      this.onFinishedLoading();
       return;
     }
 
@@ -116,6 +112,19 @@ export class Editor {
     }
   }
 
+  private onFinishedLoading(): void {
+    // Remove the while-loading listener.
+    this.blocklyWorkspace.removeChangeListener(this.bindedOnChange);
+
+    // Add the after-loading listener.
+    this.bindedOnChange = this.onChangeAfterLoading.bind(this);
+    this.blocklyWorkspace.addChangeListener(this.bindedOnChange);
+
+    if (this.module.moduleType === storageModule.ModuleType.OPMODE) {
+      opmodeDetails.checkOpMode(this.blocklyWorkspace, this);
+    }
+  }
+
   private onChangeAfterLoading(event: Blockly.Events.Abstract) {
     if (!this.blocklyWorkspace.rendered) {
       // This editor has been abandoned.
@@ -169,6 +178,10 @@ export class Editor {
         }
       }
     }
+
+    if (this.module.moduleType === storageModule.ModuleType.OPMODE) {
+      opmodeDetails.checkOpMode(this.blocklyWorkspace, this);
+    }
   }
 
   public makeCurrent(

src/editor/extended_python_generator.ts

@@ -24,9 +24,12 @@ import { PythonGenerator } from 'blockly/python';
 import { createGeneratorContext, GeneratorContext } from './generator_context';
 import * as mechanismContainerHolder from '../blocks/mrc_mechanism_component_holder';
 import * as eventHandler from '../blocks/mrc_event_handler';
+import { STEPS_METHOD_NAME } from '../blocks/mrc_steps';
+
 import {
     MODULE_NAME_BLOCKS_BASE_CLASSES,
     CLASS_NAME_OPMODE,
+    PERIODIC_METHOD_NAME,
     getClassData,
 } from '../blocks/utils/python';
 import * as storageModule from '../storage/module';
@@ -248,6 +251,21 @@ export class ExtendedPythonGenerator extends PythonGenerator {
 
       code = decorators + 'class ' + className + '(' + baseClassName + '):\n';
 
+      if (this.getModuleType() ===  storageModule.ModuleType.OPMODE) {
+        // If the user has a steps method, we need to generate code to call it from the periodic method.
+        if (STEPS_METHOD_NAME in this.classMethods) {
+          let periodicCode: string;
+          if (PERIODIC_METHOD_NAME in this.classMethods) {
+            periodicCode = this.classMethods[PERIODIC_METHOD_NAME];
+          } else {
+            periodicCode = `def ${PERIODIC_METHOD_NAME}(self):\n`;
+            periodicCode += this.INDENT + `super().${PERIODIC_METHOD_NAME}()\n`;
+          }
+          periodicCode += this.INDENT + `self.${STEPS_METHOD_NAME}()\n`;
+          this.classMethods[PERIODIC_METHOD_NAME] = periodicCode;
+        }
+      }
+
       const classMethods = [];
 
       // Generate the __init__ method first.
@@ -295,7 +313,7 @@ export class ExtendedPythonGenerator extends PythonGenerator {
     this.opModeDetails = opModeDetails;
   }
 
-  getClassSpecificForInit(): string {
+  getSuperInitParameters(): string {
     if (this.context?.getBaseClassName() == CLASS_NAME_OPMODE) {
       return 'robot'
     }

src/i18n/locales/en/translation.json

@@ -217,6 +217,7 @@
             "EVENT_NOT_IN_HOLDER": "This block can only go in the events section of the robot or mechanism.",
             "COMPONENT_NOT_IN_HOLDER": "This block can only go in the components section of the robot or mechanism.",
             "MECHANISM_NOT_IN_HOLDER": "This block can only go in the mechanisms section of the robot.",
+            "OPMODE_STEPS_OR_PERIODIC_REQUIRED": "A PeriodicOpMode should have steps, the periodic method, or both.",
             "MECHANISM_NOT_FOUND": "This block refers to a mechanism that no longer exists."
         },
         "ERROR":{