🔧 Added class for parsing command line parameters #941
Conversation
| if (args.LastError() != SO_SUCCESS) | ||
| { | ||
| LOG(LOG_ERROR) << GetLastErrorText(args.LastError()) << " " << args.OptionText(); | ||
| success = false; |
There was a problem hiding this comment.
Should it return false immediately?
|
I think it is better not to write to the setting with cmd line parameters. |
It doesn't get written to the json file |
You have right. |
|
|
||
| // ================================== | ||
| // Command line options | ||
| // ================================== |
There was a problem hiding this comment.
Can you extend the comment, that these settings are special ones and will not be preserved in between runs?
src/Game.cxx
Outdated
| vd = Settings::instance().videoDriver.c_str(); | ||
|
|
||
| if (SDL_VideoInit(videoDriver) != 0) | ||
| if (SDL_VideoInit(vd) != 0) |
There was a problem hiding this comment.
vd is bad name for variable
| // ================================== | ||
|
|
||
| /// Sets a different video driver | ||
| std::string videoDriver = "Default"; |
There was a problem hiding this comment.
do you really need save it all game? it just option on start, dont pay for unused variables
| { | ||
| videoDriver = argv[videoOpt + 1]; | ||
| } | ||
| ParseCli cli; |
src/main.cxx
Outdated
| return EXIT_FAILURE; | ||
|
|
||
| if (!skipMenu) | ||
| bool quitGame = Settings::instance().quitGame; |
There was a problem hiding this comment.
again temporary variable moved to persistant class? did we really need hold this all game?
| if (!skipMenu) | ||
| bool quitGame = Settings::instance().quitGame; | ||
|
|
||
| if (!Settings::instance().skipMenu) |
There was a problem hiding this comment.
again temp var in persisten place
| #include "SimpleOpt.h" | ||
| #include <string> | ||
|
|
||
| class ParseCli |
| // option identifiers | ||
| enum | ||
| { | ||
| OPT_HELP, |
There was a problem hiding this comment.
better user bane enum opt_id {
e_opt_help
....
}
OPT_HELP looks like macros
src/Game.cxx
Outdated
| return false; | ||
| } | ||
| const char *vd = nullptr; | ||
| if(Settings::instance().videoDriver != "Default") |
| * @param SkipMenu if the main menu should be skipped or not | ||
| */ | ||
| virtual void run(bool SkipMenu = false); | ||
| virtual void run(); |
There was a problem hiding this comment.
why it virtual, we planed have same modes in game?
There was a problem hiding this comment.
I don't think that was changed in this PR/commits.
|
|
||
| Cytopia::Game game; | ||
|
|
||
| LOG(LOG_DEBUG) << "Initializing Cytopia"; |
There was a problem hiding this comment.
too much words? debug () << text, debug( "%s", text )?
|
Kudos, SonarCloud Quality Gate passed! |
|
@AnotherFoxGuy pls fix conflicts and nits |
|
Kudos, SonarCloud Quality Gate passed! |
| bool ProcessCommandLine(int argc, char **argv); | ||
|
|
||
| private: | ||
| std::string GetLastErrorText(int a_nError); |
There was a problem hiding this comment.
Because SonarCloud has infiltrated my head: I think this could be made const.
| #include "Settings.hxx" | ||
|
|
||
| // option identifiers | ||
| enum |
There was a problem hiding this comment.
Does this enum have a name?
| // ================================== | ||
|
|
||
| /// Sets a different video driver | ||
| std::string videoDriver = "Default"; |
There was a problem hiding this comment.
Will video driver handling be added later in a different PR? I guess I'm also just wondering why this parameter is necessary, like I don't know how many people use different drivers for different programs. But then again, I don't normally run Cytopia from the command line.
|
Pls fix issues and will add this to repo |
5 participants
Fixes #940