-
Notifications
You must be signed in to change notification settings - Fork 63
Description
Components are responsible for all commandline argument parsing except 'preset' and 'help'.
This is problematic because it results in commits like 0c01325 to fix regressions. Of course we should have tests to catch that too, but it still creates future confusion when the logic is spread around a bunch of files.
Solution idea
Since component settings are automatically handled using the trackedWidgets system already, I think a good fix for this would be to handle these known settings at the core level and only pass through to the component for special cases. This is exactly how the GUI mode works.
For example, Image component with trackedWidgets={'path': self.page.lineEdit_imagePath} would tell core that avp -c 0 image path=test.jpg should set the value of the widget (lineEdit_imagePath) to 'test.jpg'. Therefore it should use the same logic flow to set this setting that would happen in any other context, making differences between CLI and GUI mode less likely.
Implementation of solution
How the program works now
Currently the way commandline arg parsing is handled by components goes like this:
- The BaseComponent metaclass wraps the Component's
command()method using this decorator which checks for apresetarg - If there is no
presetarg, the Component'scommand()method is called (for example) to parse anything else - These methods "should" call super() at the end to run BaseComponent.command() which checks for the
helparg
I wrote this code 9 years ago, and I would not have designed it this way today.
How the program should work
- The BaseComponent metaclass should be reduced to error-checking decorators (but only
commandWrapperis in scope for this issue). - super() should be called at the very top of methods. Components shouldn't have to understand the exact flow of logic through the core systems just to parse commandline arguments.
The trackedWidgets system sets attrs using the values of tracked widgets here (when it's an "automatic" (not undoable) update)
Something like that needs to happen in BaseComponent.command() with the end-goal of having Component objects call super().command() at the top of their methods instead of the bottom.
Benefits of solving this
- Less confusing code, hopefully
- More consistent program behaviour, less possibility for bugs
- Most CLI argument names would be known by the core, so we could automate the testing of them easier
- Allows for new features such as loading a preset and modifying a setting at the same time