Adding global commandline parameters #2599

pgScorpio · 2022-04-16T16:04:43Z

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

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

pgScorpio · 2022-04-16T16:08:10Z

Better(?) implementation of the Commandline parameters part of #2538 / #2541

pgScorpio · 2022-04-16T16:11:35Z

src/global.h

-
-/* Prototypes for global functions ********************************************/
-// command line parsing, TODO do not declare functions globally but in a class
-QString UsageArguments ( char** argv );
-
-bool GetFlagArgument ( char** argv, int& i, QString strShortOpt, QString strLongOpt );
-
-bool GetStringArgument ( int argc, char** argv, int& i, QString strShortOpt, QString strLongOpt, QString& strArg );
-
-bool GetNumericArgument ( int     argc,
-                          char**  argv,
-                          int&    i,
-                          QString strShortOpt,
-                          QString strLongOpt,
-                          double  rRangeStart,
-                          double  rRangeStop,
-                          double& rValue );


From now on include "cmdline.h" to access commandline parameters via the CCommandline class

pgScorpio · 2022-04-16T16:14:01Z

src/main.cpp

+void OnFatalError ( QString errMsg )
+{
+    qCritical() << qUtf8Printable ( errMsg );
+    exit ( 1 );
+}
+


Note that OnFatalError could be used on many places in main.cpp.

qCritical() << qUtf8Printable ( ... ); exit ( 1 );

Is used in many places...

pgScorpio · 2022-04-16T16:29:44Z

src/main.cpp

-
-bool GetFlagArgument ( char** argv, int& i, QString strShortOpt, QString strLongOpt )
-{
-    if ( ( !strShortOpt.compare ( argv[i] ) ) || ( !strLongOpt.compare ( argv[i] ) ) )
-    {
-        return true;
-    }
-    else
-    {
-        return false;
-    }
-}
-
-bool GetStringArgument ( int argc, char** argv, int& i, QString strShortOpt, QString strLongOpt, QString& strArg )
-{
-    if ( ( !strShortOpt.compare ( argv[i] ) ) || ( !strLongOpt.compare ( argv[i] ) ) )
-    {
-        if ( ++i >= argc )
-        {
-            qCritical() << qUtf8Printable ( QString ( "%1: '%2' needs a string argument." ).arg ( argv[0] ).arg ( argv[i - 1] ) );
-            exit ( 1 );
-        }
-
-        strArg = argv[i];
-
-        return true;
-    }
-    else
-    {
-        return false;
-    }
-}
-
-bool GetNumericArgument ( int     argc,
-                          char**  argv,
-                          int&    i,
-                          QString strShortOpt,
-                          QString strLongOpt,
-                          double  rRangeStart,
-                          double  rRangeStop,
-                          double& rValue )
-{
-    if ( ( !strShortOpt.compare ( argv[i] ) ) || ( !strLongOpt.compare ( argv[i] ) ) )
-    {
-        QString errmsg = "%1: '%2' needs a numeric argument from '%3' to '%4'.";
-        if ( ++i >= argc )
-        {
-            qCritical() << qUtf8Printable ( errmsg.arg ( argv[0] ).arg ( argv[i - 1] ).arg ( rRangeStart ).arg ( rRangeStop ) );
-            exit ( 1 );
-        }
-
-        char* p;
-        rValue = strtod ( argv[i], &p );
-        if ( *p || ( rValue < rRangeStart ) || ( rValue > rRangeStop ) )
-        {
-            qCritical() << qUtf8Printable ( errmsg.arg ( argv[0] ).arg ( argv[i - 1] ).arg ( rRangeStart ).arg ( rRangeStop ) );
-            exit ( 1 );
-        }
-
-        return true;
-    }
-    else
-    {
-        return false;
-    }
-}


These functions are now implemented in CCommandline.

Note that in the new implementation the

qCritical() << qUtf8Printable ( ... ); exit ( 1 );

parts are replaced by the CCommandline::onArgumentError function which can, optionally, be passed in the CCommandLine constructor, since we probably don't want the program to exit on parsing wrong (--special?) parameters.

pgScorpio · 2022-04-16T16:31:02Z

src/main.cpp

 // clicking on the Mac application bundle, the actual application
 // is called with weird command line args -> do not exit on these


If this is true, how about all other places in main using almost the same code ?

qCritical() << qUtf8Printable ( QString (...) ); exit (1);

OnFatalError uses this code now too.

pgScorpio · 2022-04-23T22:23:53Z

@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.

pgScorpio · 2022-04-28T20:46:56Z

@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?

ann0see · 2022-05-06T14:39:34Z

@pljones could you review these PRs please, (I think we're missing a C++ developer/reviewer at the moment)

Commandline parsing is now implemented in a separate class Commandline parameters now accessible anywhere in the code.

NOTE: These are needed for the upcomming CCmdlnOptions class that will read and check all commandline parameters.

ann0see · 2023-07-01T19:45:58Z

Not sure how to deal with this. I think this is still valid. NOT closing for now.

pljones · 2023-07-02T09:33:13Z

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.

ann0see · 2023-07-02T20:36:41Z

I still think that this PR is a good starting point. We'd include the config file parsing in the provided class.

pgScorpio · 2023-07-02T21:52:39Z

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!

pgScorpio · 2023-07-02T21:58:15Z

oops accidentally closed...

pljones · 2023-07-03T17:45:55Z

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?)

pgScorpio · 2023-07-03T18:30:44Z

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.

ann0see · 2023-07-04T06:36:21Z

I'd rename the commandline class to config as it's not just saving the CLI.

pgScorpio · 2023-07-04T12:41:18Z

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.

ann0see · 2023-07-04T17:41:11Z

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?

pgScorpio · 2023-07-04T18:17:58Z

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 ?

ann0see · 2023-07-17T21:07:41Z

Yes. Probably in #609

pljones · 2023-09-18T18:30:41Z

Setting to draft as branch needs conflict resolution.

pgScorpio commented Apr 16, 2022

View reviewed changes

hoffie added this to Triage in Tracking (old) via automation Apr 16, 2022

pgScorpio commented Apr 16, 2022

View reviewed changes

hoffie requested a review from pljones April 16, 2022 16:14

pgScorpio commented Apr 16, 2022

View reviewed changes

pgScorpio mentioned this pull request Apr 16, 2022

Global MsgBoxes and Commandline #2583

Closed

5 tasks

ann0see added the refactoring Non-behavioural changes, Code cleanup label Apr 16, 2022

pgScorpio mentioned this pull request Apr 28, 2022

Fixing memory leaks. #2618

Closed

4 tasks

ann0see moved this from Triage to Waiting on Team in Tracking (old) Apr 29, 2022

pgScorpio added 2 commits May 6, 2022 17:07

Adding global commandline parameters

72586d0

Commandline parsing is now implemented in a separate class Commandline parameters now accessible anywhere in the code.

Added missing parameter definitions and a GetArgumentList function

3f47e54

NOTE: These are needed for the upcomming CCmdlnOptions class that will read and check all commandline parameters.

pgScorpio force-pushed the global-commandline branch from 47a6c66 to 3f47e54 Compare May 6, 2022 15:07

pljones moved this from Waiting on Team to Triage in Tracking (old) Oct 5, 2022

ann0see changed the base branch from master to main December 26, 2022 19:09

ann0see mentioned this pull request Jul 1, 2023

Server: Don't warn when no --serverinfo is provided #2951

Merged

5 tasks

pgScorpio closed this Jul 2, 2023

pgScorpio reopened this Jul 2, 2023

pljones removed this from Triage in Tracking (old) Jul 28, 2023

pljones marked this pull request as draft September 18, 2023 18:30

pljones assigned pgScorpio May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding global commandline parameters #2599

Adding global commandline parameters #2599

pgScorpio commented Apr 16, 2022 •

edited

pgScorpio commented Apr 16, 2022

pgScorpio Apr 16, 2022

pgScorpio Apr 16, 2022

pgScorpio Apr 16, 2022

pgScorpio Apr 16, 2022

pgScorpio commented Apr 23, 2022

pgScorpio commented Apr 28, 2022

ann0see commented May 6, 2022 •

edited

ann0see commented Jul 1, 2023

pljones commented Jul 2, 2023 •

edited

ann0see commented Jul 2, 2023

pgScorpio commented Jul 2, 2023 •

edited

pgScorpio commented Jul 2, 2023

pljones commented Jul 3, 2023 •

edited

pgScorpio commented Jul 3, 2023 •

edited

ann0see commented Jul 4, 2023

pgScorpio commented Jul 4, 2023 •

edited

ann0see commented Jul 4, 2023

pgScorpio commented Jul 4, 2023

ann0see commented Jul 17, 2023

pljones commented Sep 18, 2023

		// clicking on the Mac application bundle, the actual application
		// is called with weird command line args -> do not exit on these

Adding global commandline parameters #2599

Are you sure you want to change the base?

Adding global commandline parameters #2599

Conversation

pgScorpio commented Apr 16, 2022 • edited

Checklist

pgScorpio commented Apr 16, 2022

pgScorpio Apr 16, 2022

Choose a reason for hiding this comment

pgScorpio Apr 16, 2022

Choose a reason for hiding this comment

pgScorpio Apr 16, 2022

Choose a reason for hiding this comment

pgScorpio Apr 16, 2022

Choose a reason for hiding this comment

pgScorpio commented Apr 23, 2022

pgScorpio commented Apr 28, 2022

ann0see commented May 6, 2022 • edited

ann0see commented Jul 1, 2023

pljones commented Jul 2, 2023 • edited

ann0see commented Jul 2, 2023

pgScorpio commented Jul 2, 2023 • edited

pgScorpio commented Jul 2, 2023

pljones commented Jul 3, 2023 • edited

pgScorpio commented Jul 3, 2023 • edited

ann0see commented Jul 4, 2023

pgScorpio commented Jul 4, 2023 • edited

ann0see commented Jul 4, 2023

pgScorpio commented Jul 4, 2023

ann0see commented Jul 17, 2023

pljones commented Sep 18, 2023

pgScorpio commented Apr 16, 2022 •

edited

ann0see commented May 6, 2022 •

edited

pljones commented Jul 2, 2023 •

edited

pgScorpio commented Jul 2, 2023 •

edited

pljones commented Jul 3, 2023 •

edited

pgScorpio commented Jul 3, 2023 •

edited

pgScorpio commented Jul 4, 2023 •

edited