From 904821cff97ce0f58b166074bcdf9dc6f396e11e Mon Sep 17 00:00:00 2001 From: Stephen Anthony Date: Wed, 20 Feb 2019 23:43:29 -0330 Subject: [PATCH] Refactoring of settings load/save functionality - commandline parsing is now done in main function - loading of settings is handled by OSystem - settings that are used only in main are not accessible globally - beginnings of converting Settings class to use map instead of linear arrays --- src/common/Variant.hxx | 10 +-- src/common/main.cxx | 149 +++++++++++++++++++++++++++++---------- src/emucore/OSystem.cxx | 15 +++- src/emucore/OSystem.hxx | 37 +++++++--- src/emucore/Settings.cxx | 62 ---------------- src/emucore/Settings.hxx | 29 ++++---- 6 files changed, 173 insertions(+), 129 deletions(-) diff --git a/src/common/Variant.hxx b/src/common/Variant.hxx index d963a264c..61182f783 100644 --- a/src/common/Variant.hxx +++ b/src/common/Variant.hxx @@ -55,16 +55,16 @@ class Variant Variant(const GUI::Size& s) { buf().str(""); buf() << s; data = buf().str(); } // Conversion methods - const string& toString() const { return data; } - const char* const toCString() const { return data.c_str(); } - const Int32 toInt() const { + const string& toString() const { return data; } + const char* const toCString() const { return data.c_str(); } + const Int32 toInt() const { istringstream ss(data); Int32 parsed; ss >> parsed; return parsed; } - const float toFloat() const { + const float toFloat() const { istringstream ss(data); float parsed; ss >> parsed; @@ -72,7 +72,7 @@ class Variant return parsed; } const bool toBool() const { return data == "1" || data == "true"; } - const GUI::Size toSize() const { return GUI::Size(data); } + const GUI::Size toSize() const { return GUI::Size(data); } // Comparison bool operator==(const Variant& v) const { return data == v.data; } diff --git a/src/common/main.cxx b/src/common/main.cxx index 7d373f7af..8cf932e09 100644 --- a/src/common/main.cxx +++ b/src/common/main.cxx @@ -42,24 +42,103 @@ #include "CheatManager.hxx" #endif +/** + Parse the commandline arguments and store into the appropriate hashmap. + + Keys without a corresponding value are assumed to be boolean, and set to true. + Some keys are used only by the main function; these are placed in localOpts. + The rest are needed globally, and are placed in globalOpts. +*/ +void parseCommandLine(int ac, char* av[], + Settings::Options& globalOpts, Settings::Options& localOpts); + /** Checks the commandline for special settings that are used by various ports to use a specific 'base directory'. - This needs to be done separately from the main commandline and settings - functionality, since they both depend on the settings file being already - available. However, since a variabe basedir implies a different location - for the settings file, it *must* be processed first. + This needs to be done separately, before either an OSystem or Settings + object can be created, since they both depend on each other, and a + variable basedir implies a different location for the settings file. - This function will set the environment variables STELLA_BASEDIR and - STELLA_USECURRENTASBASEDIR; it is up to each port to use these as they wish. + This function will call OSystem::overrideBaseDir() when either of the + applicable arguments are found, and then remove them from the argument + list. */ -void checkForCustomBaseDir(int ac, char* av[]); +void checkForCustomBaseDir(Settings::Options& options); // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -void checkForCustomBaseDir(int ac, char* av[]) +void parseCommandLine(int ac, char* av[], + Settings::Options& globalOpts, Settings::Options& localOpts) { + localOpts["ROMFILE"] = ""; // make sure we have an entry for this + + for(int i = 1; i < ac; ++i) + { + string key = av[i]; + if(key[0] == '-') + { + key = key.substr(1); + + // Certain options are used only in the main function + // We detect these now, and remove them from further consideration + if(key == "help" || key == "listrominfo" || key == "rominfo" || key == "takesnapshot") + { + localOpts[key] = true; + continue; + } + // Take care of arguments without an option that are needed outside + // be saved to the config file + if(key == "debug" || key == "holdselect" || key == "holdreset") + { + globalOpts[key] = true; + continue; + } + // Some ports have the ability to override the base directory where all + // configuration files are stored; we check for those next + if(key == "baseinappdir") + { + globalOpts[key] = true; + continue; + } + + if(++i >= ac) + { + cerr << "Missing argument for '" << key << "'" << endl; + continue; + } + if(key == "basedir" || key == "break") + localOpts[key] = av[i]; + else + globalOpts[key] = av[i]; + } + else + localOpts["ROMFILE"] = key; + } + +#if 0 + cout << "Global opts:" << endl; + for(const auto& x: globalOpts) + cout << " -> " << x.first << ": " << x.second << endl; + cout << "Local opts:" << endl; + for(const auto& x: localOpts) + cout << " -> " << x.first << ": " << x.second << endl; +#endif +} + +// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - +void checkForCustomBaseDir(Settings::Options& options) +{ + // If both of these are activated, the 'base in app dir' takes precedence + auto it = options.find("baseinappdir"); + if(it != options.end()) + OSystem::overrideBaseDir(); + else + { + it = options.find("basedir"); + if(it != options.end()) + OSystem::overrideBaseDir(it->second.toString()); + } } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - @@ -73,34 +152,32 @@ int main(int ac, char* av[]) std::ios_base::sync_with_stdio(false); - // Check for custom base directory - // This needs to be done separately from the normal startup, since it - // queries information from the commandline that both OSystem and Settings - // need *before* they are created - checkForCustomBaseDir(ac, av); - - // Create the parent OSystem object - unique_ptr theOSystem = MediaFactory::createOSystem(); - theOSystem->loadConfig(); - theOSystem->logMessage("Loading config options ...", 2); + unique_ptr theOSystem; auto Cleanup = [&theOSystem]() { - theOSystem->logMessage("Cleanup from main", 2); - theOSystem->saveConfig(); - theOSystem.reset(); // Force delete of object + if(theOSystem) + { + theOSystem->logMessage("Cleanup from main", 2); + theOSystem->saveConfig(); + theOSystem.reset(); // Force delete of object + } MediaFactory::cleanUp(); // Finish any remaining cleanup return 0; }; - // Take care of commandline arguments - theOSystem->logMessage("Loading commandline arguments ...", 2); - string romfile = theOSystem->settings().loadCommandLine(ac, av); + // Parse the commandline arguments + // They are placed in different maps depending on whether they're used + // locally or globally + Settings::Options globalOpts, localOpts; + parseCommandLine(ac, av, globalOpts, localOpts); - // Finally, make sure the settings are valid - // We do it once here, so the rest of the program can assume valid settings - theOSystem->logMessage("Validating config options ...", 2); - theOSystem->settings().validate(); + // Check for custom base directory; some ports make use of this + checkForCustomBaseDir(localOpts); + + // Create the parent OSystem object and initialize settings + theOSystem = MediaFactory::createOSystem(); + theOSystem->loadConfig(globalOpts); // Create the full OSystem after the settings, since settings are // probably needed for defaults @@ -114,13 +191,14 @@ int main(int ac, char* av[]) // Check to see if the user requested info about a specific ROM, // or the list of internal ROMs // If so, show the information and immediately exit - if(theOSystem->settings().getBool("listrominfo")) + string romfile = localOpts["ROMFILE"].toString(); + if(localOpts["listrominfo"].toBool()) { theOSystem->logMessage("Showing output from 'listrominfo' ...", 2); theOSystem->propSet().print(); return Cleanup(); } - else if(theOSystem->settings().getBool("rominfo")) + else if(localOpts["rominfo"].toBool()) { theOSystem->logMessage("Showing output from 'rominfo' ...", 2); FilesystemNode romnode(romfile); @@ -128,7 +206,7 @@ int main(int ac, char* av[]) return Cleanup(); } - else if(theOSystem->settings().getBool("help")) + else if(localOpts["help"].toBool()) { theOSystem->logMessage("Displaying usage", 2); theOSystem->settings().usage(); @@ -165,7 +243,7 @@ int main(int ac, char* av[]) #if 0 TODO: Fix this to use functionality from OSystem::mainLoop - if(theOSystem->settings().getBool("takesnapshot")) + if(localOpts["takesnapshot"].toBool()) { for(int i = 0; i < 30; ++i) theOSystem->frameBuffer().update(); // theOSystem->frameBuffer().tiaSurface().saveSnapShot(); @@ -182,14 +260,11 @@ int main(int ac, char* av[]) #ifdef DEBUGGER_SUPPORT // Set up any breakpoint that was on the command line - // (and remove the key from the settings, so they won't get set again) - const string& initBreak = theOSystem->settings().getString("break"); - if(initBreak != "") + if(localOpts["break"].toString() != "") { Debugger& dbg = theOSystem->debugger(); - uInt16 bp = uInt16(dbg.stringToValue(initBreak)); + uInt16 bp = uInt16(dbg.stringToValue(localOpts["break"].toString())); dbg.setBreakPoint(bp, true); - theOSystem->settings().setValue("break", ""); } #endif } diff --git a/src/emucore/OSystem.cxx b/src/emucore/OSystem.cxx index 6d6488e0f..fa1e8dec6 100644 --- a/src/emucore/OSystem.cxx +++ b/src/emucore/OSystem.cxx @@ -167,9 +167,17 @@ bool OSystem::create() } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -void OSystem::loadConfig() +void OSystem::loadConfig(const Settings::Options& options) { + logMessage("Loading config options ...", 2); mySettings->loadConfig(); + + logMessage("Applying commandline config options ...", 2); + for(const auto& opt: options) + mySettings->setValue(opt.first, opt.second); + + logMessage("Validating config options ...", 2); + mySettings->validate(); } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - @@ -215,6 +223,7 @@ void OSystem::setConfigPaths() myPaletteFile = FilesystemNode(myBaseDir + "stella.pal").getPath(); myPropertiesFile = FilesystemNode(myBaseDir + "stella.pro").getPath(); +#if 0 // TODO - remove this auto dbgPath = [](const string& desc, const string& location) { @@ -230,6 +239,7 @@ void OSystem::setConfigPaths() dbgPath("pal file ", myPaletteFile); dbgPath("pro file ", myPropertiesFile); dbgPath("INI file ", myConfigFile); +#endif } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - @@ -722,3 +732,6 @@ void OSystem::mainLoop() myCheatManager->saveCheatDatabase(); #endif } + +// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - +string OSystem::ourOverrideBaseDir = ""; diff --git a/src/emucore/OSystem.hxx b/src/emucore/OSystem.hxx index 420b18039..37da00d17 100644 --- a/src/emucore/OSystem.hxx +++ b/src/emucore/OSystem.hxx @@ -34,7 +34,6 @@ class Properties; class PropertiesSet; class Random; class SerialPort; -class Settings; class Sound; class StateManager; class TimerManager; @@ -48,6 +47,7 @@ class AudioSettings; #include "FrameBufferConstants.hxx" #include "EventHandlerConstants.hxx" #include "FpsMeter.hxx" +#include "Settings.hxx" #include "bspf.hxx" /** @@ -188,16 +188,16 @@ class OSystem PNGLibrary& png() const { return *myPNGLib; } /** - This method should be called to load the current settings from an rc file. - It first loads the settings from the config file, then informs subsystems - about the new settings. + This method should be called to initiate the process of loading settings + from the config file. It takes care of loading settings, applying + commandline overrides, and finally validating all settings. */ - void loadConfig(); + void loadConfig(const Settings::Options& options); /** - This method should be called to save the current settings to an rc file. - It first asks each subsystem to update its settings, then it saves all - settings to the config file. + This method should be called to save the current settings. It first asks + each subsystem to update its settings, then it saves all settings to the + config file. */ void saveConfig(); @@ -382,9 +382,23 @@ class OSystem /** Reset FPS measurement. - */ + */ void resetFps(); + /** + Attempt to override the base directory that will be used by derived + classes, and use this one instead. Note that this is only a hint; + derived classes are free to ignore this, as some can't use an + alternate base directory. + + If the default path is used, this indicates to attempt to use the + application directory directly. Again, this is not supported on + all systems, so it may be simply ignored. + */ + static void overrideBaseDir(const string& path = "USE_APP_DIR") { + ourOverrideBaseDir = path; + } + public: ////////////////////////////////////////////////////////////////////// // The following methods are system-specific and can be overrided in @@ -505,6 +519,11 @@ class OSystem // Indicates whether to stop the main loop bool myQuitLoop; + // If not empty, a hint for derived classes to use this as the + // base directory (where all settings are stored) + // Derived classes are free to ignore it and use their own defaults + static string ourOverrideBaseDir; + private: string myBaseDir; string myStateDir; diff --git a/src/emucore/Settings.cxx b/src/emucore/Settings.cxx index 4cab419b9..47fa40224 100644 --- a/src/emucore/Settings.cxx +++ b/src/emucore/Settings.cxx @@ -232,68 +232,6 @@ void Settings::loadConfig() } } -// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -string Settings::loadCommandLine(int argc, char** argv) -{ - for(int i = 1; i < argc; ++i) - { - // strip off the '-' character - string key = argv[i]; - if(key[0] == '-') - { - key = key.substr(1, key.length()); - - // Take care of the arguments which are meant to be executed immediately - // (and then Stella should exit) - if(key == "help" || key == "listrominfo") - { - setExternal(key, "true"); - return ""; - } - - // Take care of arguments without an option or ones that shouldn't - // be saved to the config file - if(key == "rominfo" || key == "debug" || key == "holdreset" || - key == "holdselect" || key == "takesnapshot") - { - setExternal(key, "true"); - continue; - } - - ostringstream buf; - if(++i >= argc) - { - buf << "Missing argument for '" << key << "'" << endl; - myOSystem.logMessage(buf.str(), 0); - return ""; - } - string value = argv[i]; - - buf.str(""); - buf << " key = '" << key << "', value = '" << value << "'"; - - // Settings read from the commandline must not be saved to - // the rc-file, unless they were previously set - if(int idx = getInternalPos(key) != -1) - { - setInternal(key, value, idx); // don't set initialValue here - buf << "(I)\n"; - } - else - { - setExternal(key, value); - buf << "(E)\n"; - } - - myOSystem.logMessage(buf.str(), 2); - } - else - return key; - } - - return EmptyString; -} - // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - void Settings::validate() { diff --git a/src/emucore/Settings.hxx b/src/emucore/Settings.hxx index 8465db3fb..f175c0d60 100644 --- a/src/emucore/Settings.hxx +++ b/src/emucore/Settings.hxx @@ -20,6 +20,8 @@ class OSystem; +#include + #include "Variant.hxx" #include "bspf.hxx" @@ -39,20 +41,9 @@ class Settings explicit Settings(OSystem& osystem); virtual ~Settings() = default; + using Options = std::map; + public: - /** - This method should be called to load the arguments from the commandline. - - @return Name of the ROM to load, otherwise empty string - */ - string loadCommandLine(int argc, char** argv); - - /** - This method should be called *after* settings have been read, - to validate (and change, if necessary) any improper settings. - */ - void validate(); - /** This method should be called to display usage information. */ @@ -88,15 +79,23 @@ class Settings protected: /** - This method will be called to load the current settings from an rc file. + This method will be called to load the settings from the + platform-specific settings file. */ virtual void loadConfig(); /** - This method will be called to save the current settings to an rc file. + This method will be called to save the current settings to the + platform-specific settings file. */ virtual void saveConfig(); + /** + This method must be called *after* settings have been fully loaded + to validate (and change, if necessary) any improper settings. + */ + void validate(); + // Trim leading and following whitespace from a string static string trim(const string& str) {