Implement motor test plugin#1016
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a plugin architecture to the parameter editor, enabling step-specific UI extensions to be displayed alongside the parameter table. The first plugin implementation is a motor test UI that appears during the motor configuration step.
Key changes:
- Introduced a plugin configuration schema allowing steps to declare UI plugins with placement options (left/top)
- Implemented dynamic layout switching to accommodate plugins alongside the parameter table
- Added motor test plugin integration for the "15_motor.param" configuration step
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
configuration_steps_schema.json |
Added plugin schema definition with name and placement properties |
configuration_steps_ArduCopter.json |
Configured motor_test plugin for the motor configuration step |
backend_filesystem_configuration_steps.py |
Added get_plugin() method to retrieve plugin configuration for a file |
configuration_manager.py |
Added plugin API methods: get_plugin() and create_plugin_data_model() |
frontend_tkinter_parameter_editor.py |
Implemented dynamic layout switching and plugin loading logic |
test_configuration_manager.py |
Added tests for create_plugin_data_model() with various scenarios |
test_frontend_tkinter_parameter_editor.py |
Added tests for __load_plugin() method covering success and error cases |
| ) | ||
|
|
||
| def __update_plugin_layout(self, plugin: Optional[dict]) -> None: | ||
| """Update the plugin layout based on the current plugin.""" |
There was a problem hiding this comment.
The __update_plugin_layout method lacks a docstring explaining its parameters, return value, and behavior. Add a docstring describing the plugin parameter (dict with 'name' and 'placement' keys or None) and explaining that it recreates the layout when the plugin changes.
| """Update the plugin layout based on the current plugin.""" | |
| """ | |
| Update the plugin layout based on the current plugin. | |
| This method destroys and recreates the parameter area container and its contents | |
| depending on the provided plugin configuration. | |
| Parameters: | |
| plugin (Optional[dict]): Either None, or a dictionary with at least the keys | |
| 'name' (str) and 'placement' (str, either 'left' or 'top'). If None, no plugin | |
| is loaded and the default layout is used. | |
| Returns: | |
| None | |
| Side Effects: | |
| Destroys and recreates the parameter area container and its child widgets. | |
| Recreates the parameter table in the new layout. | |
| """ |
| self.parameter_editor_table = ParameterEditorTable(self.parameter_container, self.configuration_manager, self) | ||
| self.parameter_editor_table.pack(side="top", fill="both", expand=True) |
There was a problem hiding this comment.
The parameter table is recreated every time the plugin layout changes. This may cause issues if the table holds any state (like scroll position or user selections). Consider whether the table state needs to be preserved and restored, or document that state is intentionally reset.
| ttk.Label(parent_frame, text="Motor test requires flight controller connection").pack() | ||
| return | ||
|
|
||
| MotorTestView(parent_frame, model, self) # type: ignore[arg-type] |
There was a problem hiding this comment.
The type: ignore[arg-type] comment suggests a type mismatch. The issue is that ParameterEditorWindow is passed where BaseWindow is expected. Since ParameterEditorWindow inherits from BaseWindow, this should type-check correctly. Verify that the type hint on MotorTestView.__init__ is correct and remove the type ignore comment if possible.
| MotorTestView(parent_frame, model, self) # type: ignore[arg-type] | |
| MotorTestView(parent_frame, model, self) |
| def get_plugin(self, filename: str) -> Optional[dict]: | ||
| return self._local_filesystem.get_plugin(filename) | ||
|
|
||
| def create_plugin_data_model(self, plugin_name: str) -> Optional[object]: |
There was a problem hiding this comment.
The return type Optional[object] is too generic and provides no type safety for callers. Consider using a Union type of specific model classes (e.g., Optional[MotorTestDataModel]) or defining a base class/protocol for plugin data models.
| def get_plugin(self, filename: str) -> Optional[dict]: | ||
| return self._local_filesystem.get_plugin(filename) | ||
|
|
||
| def create_plugin_data_model(self, plugin_name: str) -> Optional[object]: |
There was a problem hiding this comment.
The plugin_name parameter should be validated. Consider adding type checking or documentation about what happens when plugin_name is None or an empty string, since the tests check these cases but the implementation may not handle them explicitly.
amilcarlucas
force-pushed
the
motor_test_plugin
branch
4 times, most recently
from
fd8ea91 to
57309f5
Compare
Uses dependency injection via plugin factory to avoid circular dependencies Adds tests
amilcarlucas
force-pushed
the
motor_test_plugin
branch
from
57309f5 to
5b9d1c6
Compare
Adds tests