jamulussoftware
Adding global commandline parameters
Short description of changes
Commandline parsing is now implemented in a separate class Commandline parameters now accessible anywhere in the code.
CHANGELOG: Refactoring: Commandline parameters are now accessible anywhere in the code via the CCommandline class.
Context: Fixes an issue?
General improvement and preparation for sound-redesign.
Does this change need documentation? What needs to be documented and how?
No documentation changes required.
Status of this Pull Request
What is missing until this pull request can be merged?
Checklist
- [x] I've verified that this Pull Request follows the general code principles
- [x] I tested my code and it does what I want
- [x] My code follows the style guide
- [x] I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
- [x] I've filled all the content above
Better(?) implementation of the Commandline parameters part of #2538 / #2541
@pljones and others of the Jamulus team
Could we please have some reviews here?
Also my new settings implementation is now waiting on this one.
@ann0see @pljones @hoffie
I also have a CCommandlineOptions class ready that reads all commandline options, and checks client/server validity and ranges, But also that one depends on this PR and #2598 and #2620. And I'm also working on a better implementation of Settings handling and that one, again, depends on CCommandlineOptions, and so also on all my other PR's I'm aware of #2617, but could we please get things going ASAP so I can proceed while I still have the time?
@pljones could you review these PRs please, (I think we're missing a C++ developer/reviewer at the moment)
Not sure how to deal with this. I think this is still valid. NOT closing for now.
My view remains that settings and command line handling need to be unified and that all the handling for client and server properties (i.e. the values configured either by settings or command line) should be handled through calls to the client or server instances.
This approach enables a consistent, UI-independent, development basis going forward. The CLI and GUI - and RPC interface - would be sharing common code for client and server. Where client and server needed common code, that would be separated out. It does mean a lot more work rearchitecting Jamulus, however.
I still think that this PR is a good starting point. We'd include the config file parsing in the provided class.
My view remains that settings and command line handling need to be unified and that all the handling for client and server properties (i.e. the values configured either by settings or command line) should be handled through calls to the client or server instances.
This approach enables a consistent, UI-independent, development basis going forward. The CLI and GUI - and RPC interface - would be sharing common code for client and server. Where client and server needed common code, that would be separated out. It does mean a lot more work rearchitecting Jamulus, however.
I must disagree, since the inifile is read before any module is initialized there should be no calls to the modules from reading the inifile but the modules should request the inifile values at their initialization!
I must disagree, since the inifile is read before any module is initialized there should be no calls to the modules from reading the inifile but the modules should request the inifile values at their initialization!
This requires the client and server to know about the inifile, if you're explaining yourself correctly.
I'm suggesting that the client and server know about settings and they "own" their own settings in the sense that any read or update goes through a client or server call. The implementation details (getting those changes to backing store) should, naturally, be independent.
Naming the inifile would come from parsing the command line. Loading the inifile would come next. Instantiating the client or server would come next, passing the settings (the combined inifile and command line values).
Updates to settings would not immediately trigger an update to the inifile, because we don't want interupts when in real time flows, so any disruption should be minimized. We should honour any request to save state from Qt and make sure that's definitely happened on exit. (This remains subject to:
- if an inifile was supplied, in my view it should always be written - Volker thinks a headless server should never write an inifile, IIRC;
- any inifile value where a command line option was used to override the value, in headless mode, should not get written - I agree with Volker on that one, again IIRC;
- any inifile value in GUI, whether overridden by the command line or not, should be written with its current value on close - which could cause a command line option to change the inifile... this is current behaviour and I'm not sure I'm happy with it - but should the behaviour depend on whether an inifile was specified or the default used?)
I'm suggesting that the client and server know about settings and they "own" their own settings in the sense that any read or update goes through a client or server call. The implementation details (getting those changes to backing store) should, naturally, be independent.
I suggest we have a commandline class that reads commandline values and an inifile class containing 3 classes: app_settings, server_settings and client_settings.
First the commandline class parses the commandline and store these values. This comnandline class is passed to the inifile class which then reads the inifile values
Then the app is initialised using app_settings and client/server is initialised using their setting classes
app, client and server retrieve their settings by calls to their settings class, which will apply any commandline overrides.
app, client and server can update their settings by calls to the matching settings class during runtime.
At app closing the settings class writes out the new inifile using the stored settings if there where any changes.
I'd rename the commandline class to config as it's not just saving the CLI.
I'd rename the commandline class to config as it's not just saving the CLI.
It should just parse the CLI!
And config is way more than the commandline so it would be more logical if "inifile" was named config as it should contain all configurable settings (inifile as well as commandline).
The commandline can also contain commandline only (i.e. server/client, gui/nogui, logging, debugging) parameters and, as so, it should be a separate class accessible anywhere.
Hmm. I mean in the end having one class serving the whole configuration state/providing access to CLI/GUI/Commandline would be beneficial as this allows logical separation.
Do you think that's an issue?
Hmm. I mean in the end having one class serving the whole configuration state/providing access to CLI/GUI/Commandline would be beneficial as this allows logical separation.
Do you think that's an issue?
Why putting everything in one class?
I think separate classes are the perfect way to keep things organised, also they can be defined at different, logical locations. Nevertheless don't confuse classes and class instances. The instances of multiple classes can still be located in one class instance.
Though I still think "commandline" and "inifile" should be different instances since the commandline should be parsed before the other instances are created, but even then "config" could be a class containing commandline and inifile...
P.S. Shouldn't we discuss this in another topic ?