-
-
Notifications
You must be signed in to change notification settings - Fork 689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allowing Config class to resolve isFullscreen flag from CLI arguments #1349
base: main
Are you sure you want to change the base?
Allowing Config class to resolve isFullscreen flag from CLI arguments #1349
Conversation
src/emulator.cpp
Outdated
@@ -256,6 +256,14 @@ void Emulator::Run(const std::filesystem::path& file) { | |||
std::exit(0); | |||
} | |||
|
|||
void Emulator::Run(int& argc, char* argv[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is argc
a reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I don't know. I'll pass by value instead
It might be better to use an arg parsing library like Boost program_options |
linux-sdl appimage works when entering path |
@LNDF I really would like to use a library for this, but I was hesitant to introduce a new dependency to
If that doesn't apply here, then I could rework this to use the Boost library. That would also probably resolve the ordering issue @qurious-pixel mentioned. |
Boost is already a dependency. You could try to open a PR in https://github.com/shadps4-emu/ext-boost adding boost/program_options.hpp using bcp. |
Addresses feature request #1147
Changes:
Config
class-f
and--fullscreen
flags that overrideisFullscreen
in the configpatchFile
member variable to config (but excluding from TOML file, since it's game specific)Emulator::Run
I have unfortunately not been able to properly test fullscreen in my environment due to #1348, but I think the new CLI argument works.