From 130fcf1dfce560c3144d3d2df406c066c991f658 Mon Sep 17 00:00:00 2001 From: Stephen Anthony Date: Thu, 21 Feb 2019 21:55:08 -0330 Subject: [PATCH] Fairly large refactoring of Settings class - Completely separate Settings and OSystem; the former no longer uses the latter at all - Moved separate Settings methods directly into that class, exposing less info outside the class - Reworked loading/saving config files; this may break macOS port (not tested yet) - Next thing TODO is convert Settings class to use map instead of vectors --- src/common/MediaFactory.hxx | 8 +-- src/emucore/OSystem.cxx | 27 ++++---- src/emucore/Settings.cxx | 112 ++++++++++++++++++++------------ src/emucore/Settings.hxx | 55 +++++++++++----- src/macos/SettingsMACOS.cxx | 11 ++-- src/macos/SettingsMACOS.hxx | 9 +-- src/unix/SettingsUNIX.cxx | 4 +- src/unix/SettingsUNIX.hxx | 5 +- src/windows/SettingsWINDOWS.cxx | 7 +- src/windows/SettingsWINDOWS.hxx | 3 +- 10 files changed, 143 insertions(+), 98 deletions(-) diff --git a/src/common/MediaFactory.hxx b/src/common/MediaFactory.hxx index 94e26b006..057c0df51 100644 --- a/src/common/MediaFactory.hxx +++ b/src/common/MediaFactory.hxx @@ -80,14 +80,14 @@ class MediaFactory #endif } - static unique_ptr createSettings(OSystem& osystem) + static unique_ptr createSettings() { #if defined(BSPF_UNIX) - return make_unique(osystem); + return make_unique(); #elif defined(BSPF_WINDOWS) - return make_unique(osystem); + return make_unique(); #elif defined(BSPF_MACOS) - return make_unique(osystem); + return make_unique(); #else #error Unsupported platform for Settings! #endif diff --git a/src/emucore/OSystem.cxx b/src/emucore/OSystem.cxx index fa1e8dec6..ba568a38c 100644 --- a/src/emucore/OSystem.cxx +++ b/src/emucore/OSystem.cxx @@ -92,7 +92,7 @@ OSystem::OSystem() << " [" << BSPF::ARCH << "]"; myBuildInfo = info.str(); - mySettings = MediaFactory::createSettings(*this); + mySettings = MediaFactory::createSettings(); } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - @@ -149,7 +149,7 @@ bool OSystem::create() // Create the sound object; the sound subsystem isn't actually // opened until needed, so this is non-blocking (on those systems - // that only have a single sound device (no hardware mixing) + // that only have a single sound device (no hardware mixing)) createSound(); // Create the serial port object @@ -170,14 +170,7 @@ bool OSystem::create() 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(); + mySettings->load(configFile(), options); } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - @@ -185,13 +178,19 @@ void OSystem::saveConfig() { // Ask all subsystems to save their settings if(myFrameBuffer) - myFrameBuffer->tiaSurface().ntsc().saveConfig(*mySettings); + { + logMessage("Saving TV effects options ...", 2); + myFrameBuffer->tiaSurface().ntsc().saveConfig(settings()); + } - if(mySettings) - mySettings->saveConfig(); + logMessage("Saving config options ...", 2); + mySettings->save(configFile()); if(myPropSet) + { + logMessage("Saving properties set ...", 2); myPropSet->save(myPropertiesFile); + } } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - @@ -469,7 +468,7 @@ unique_ptr OSystem::openConsole(const FilesystemNode& romfile, string& // Open the cartridge image and read it in BytePtr image; - uInt32 size = 0; + uInt32 size = 0; if((image = openROM(romfile, md5, size)) != nullptr) { // Get a valid set of properties, including any entered on the commandline diff --git a/src/emucore/Settings.cxx b/src/emucore/Settings.cxx index 47fa40224..8a1bb2734 100644 --- a/src/emucore/Settings.cxx +++ b/src/emucore/Settings.cxx @@ -28,8 +28,7 @@ #include "Settings.hxx" // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -Settings::Settings(OSystem& osystem) - : myOSystem(osystem) +Settings::Settings() { // Video-related options setInternal("video", ""); @@ -192,43 +191,34 @@ Settings::Settings(OSystem& osystem) } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -void Settings::loadConfig() +void Settings::load(const string& cfgfile, const Options& options) { - string line, key, value; - string::size_type equalPos, garbage; - - ifstream in(myOSystem.configFile()); - if(!in || !in.is_open()) + // First load from the platform-specific config file + // Different ports may override this functionality + if(!loadConfigFile(cfgfile)) { - myOSystem.logMessage("ERROR: Couldn't load settings file", 0); - return; + // FIXME - make logger available everywhere + // myOSystem.logMessage("ERROR: Couldn't load settings file", 0); + cout << "ERROR: Couldn't load settings file" << endl; } - while(getline(in, line)) + // Apply commandline options, which override those from settings file + for(const auto& opt: options) + setValue(opt.first, opt.second); + + // Finally, validate some settings, so the rest of the codebase + // can assume the values are valid + validate(); +} + +// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - +void Settings::save(const string& cfgfile) const +{ + if(!saveConfigFile(cfgfile)) { - // Strip all whitespace and tabs from the line - while((garbage = line.find("\t")) != string::npos) - line.erase(garbage, 1); - - // Ignore commented and empty lines - if((line.length() == 0) || (line[0] == ';')) - continue; - - // Search for the equal sign and discard the line if its not found - if((equalPos = line.find("=")) == string::npos) - continue; - - // Split the line into key/value pairs and trim any whitespace - key = trim(line.substr(0, equalPos)); - value = trim(line.substr(equalPos + 1, line.length() - key.length() - 1)); - - // Skip absent key - if(key.length() == 0) - continue; - - // Only settings which have been previously set are valid - if(int idx = getInternalPos(key) != -1) - setInternal(key, value, idx, true); + // FIXME - make logger available everywhere + // myOSystem.logMessage("ERROR: Couldn't save settings file", 0); + cout << "ERROR: Couldn't save settings file" << endl; } } @@ -609,7 +599,48 @@ void Settings::setValue(const string& key, const Variant& value) } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -void Settings::saveConfig() +bool Settings::loadConfigFile(const string& cfgfile) +{ + string line, key, value; + string::size_type equalPos, garbage; + + ifstream in(cfgfile); + if(!in || !in.is_open()) + { + return false; + } + + while(getline(in, line)) + { + // Strip all whitespace and tabs from the line + while((garbage = line.find("\t")) != string::npos) + line.erase(garbage, 1); + + // Ignore commented and empty lines + if((line.length() == 0) || (line[0] == ';')) + continue; + + // Search for the equal sign and discard the line if its not found + if((equalPos = line.find("=")) == string::npos) + continue; + + // Split the line into key/value pairs and trim any whitespace + key = trim(line.substr(0, equalPos)); + value = trim(line.substr(equalPos + 1, line.length() - key.length() - 1)); + + // Skip absent key + if(key.length() == 0) + continue; + + // Only settings which have been previously set are valid + if(int idx = getInternalPos(key) != -1) + setInternal(key, value, idx, true); + } + return true; +} + +// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - +bool Settings::saveConfigFile(const string& cfgfile) const { // Do a quick scan of the internal settings to see if any have // changed. If not, we don't need to save them at all. @@ -624,14 +655,11 @@ void Settings::saveConfig() } if(!settingsChanged) - return; + return true; - ofstream out(myOSystem.configFile()); + ofstream out(cfgfile); if(!out || !out.is_open()) - { - myOSystem.logMessage("ERROR: Couldn't save settings file", 0); - return; - } + return false; out << "; Stella configuration file" << endl << ";" << endl @@ -651,6 +679,8 @@ void Settings::saveConfig() // Write out each of the key and value pairs for(const auto& s: myInternalSettings) out << s.key << " = " << s.value << endl; + + return true; } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/src/emucore/Settings.hxx b/src/emucore/Settings.hxx index f175c0d60..d2579a2b0 100644 --- a/src/emucore/Settings.hxx +++ b/src/emucore/Settings.hxx @@ -18,8 +18,6 @@ #ifndef SETTINGS_HXX #define SETTINGS_HXX -class OSystem; - #include #include "Variant.hxx" @@ -32,13 +30,11 @@ class OSystem; */ class Settings { - friend class OSystem; - public: /** Create a new settings abstract class */ - explicit Settings(OSystem& osystem); + explicit Settings(); virtual ~Settings() = default; using Options = std::map; @@ -49,6 +45,22 @@ class Settings */ void usage() const; + /** + This method is called to load settings from the settings file, + and apply commandline options specified by the given parameter. + + @param cfgfile The full path to the configuration file + @param options A list of options that overrides ones in the + settings file + */ + void load(const string& cfgfile, const Options& options); + + /** + This method is called to save the current settings to the + settings file. + */ + void save(const string& cfgfile) const; + /** Get the value assigned to the specified key. @@ -80,21 +92,25 @@ class Settings protected: /** This method will be called to load the settings from the - platform-specific settings file. + platform-specific settings file. Since different ports can have + different behaviour here, we mark it as virtual so derived + classes can override as needed. + + @param cfgfile The full path to the configuration file + @return False on any error, else true */ - virtual void loadConfig(); + virtual bool loadConfigFile(const string& cfgfile); /** This method will be called to save the current settings to the - platform-specific settings file. - */ - virtual void saveConfig(); + platform-specific settings file. Since different ports can have + different behaviour here, we mark it as virtual so derived + classes can override as needed. - /** - This method must be called *after* settings have been fully loaded - to validate (and change, if necessary) any improper settings. + @param cfgfile The full path to the configuration file + @return False on any error, else true */ - void validate(); + virtual bool saveConfigFile(const string& cfgfile) const; // Trim leading and following whitespace from a string static string trim(const string& str) @@ -105,9 +121,6 @@ class Settings } protected: - // The parent OSystem object - OSystem& myOSystem; - // Structure used for storing settings struct Setting { @@ -135,6 +148,13 @@ class Settings int setExternal(const string& key, const Variant& value, int pos = -1, bool useAsInitial = false); + private: + /** + This method must be called *after* settings have been fully loaded + to validate (and change, if necessary) any improper settings. + */ + void validate(); + private: // Holds key,value pairs that are necessary for Stella to // function and must be saved on each program exit. @@ -146,7 +166,6 @@ class Settings private: // Following constructors and assignment operators not supported - Settings() = delete; Settings(const Settings&) = delete; Settings(Settings&&) = delete; Settings& operator=(const Settings&) = delete; diff --git a/src/macos/SettingsMACOS.cxx b/src/macos/SettingsMACOS.cxx index 5b37e0394..6a61cf8a6 100644 --- a/src/macos/SettingsMACOS.cxx +++ b/src/macos/SettingsMACOS.cxx @@ -24,13 +24,13 @@ extern "C" { } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -SettingsMACOS::SettingsMACOS(OSystem& osystem) - : Settings(osystem) +SettingsMACOS::SettingsMACOS() + : Settings() { } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -void SettingsMACOS::loadConfig() +bool SettingsMACOS::loadConfigFile(const string&) { string key, value; char cvalue[4096]; @@ -43,14 +43,17 @@ void SettingsMACOS::loadConfig() if(cvalue[0] != 0) setInternal(settings[i].key, cvalue, i, true); } + return true; } // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -void SettingsMACOS::saveConfig() +bool SettingsMACOS::saveConfigFile(const string&) const { // Write out each of the key and value pairs for(const auto& s: getInternalSettings()) prefsSetString(s.key.c_str(), s.value.toCString()); prefsSave(); + + return true; } diff --git a/src/macos/SettingsMACOS.hxx b/src/macos/SettingsMACOS.hxx index bca439ee2..a963c4254 100644 --- a/src/macos/SettingsMACOS.hxx +++ b/src/macos/SettingsMACOS.hxx @@ -18,8 +18,6 @@ #ifndef SETTINGS_MACOS_HXX #define SETTINGS_MACOS_HXX -class OSystem; - #include "Settings.hxx" /** @@ -33,7 +31,7 @@ class SettingsMACOS : public Settings /** Create a new UNIX settings object */ - explicit SettingsMACOS(OSystem& osystem); + explicit SettingsMACOS(); virtual ~SettingsMACOS() = default; public: @@ -41,17 +39,16 @@ class SettingsMACOS : public Settings This method should be called to load the current settings from the standard Mac preferences. */ - void loadConfig() override; + bool loadConfigFile(const string&) override; /** This method should be called to save the current settings to the standard Mac preferences. */ - void saveConfig() override; + bool saveConfigFile(const string&) const override; private: // Following constructors and assignment operators not supported - SettingsMACOS() = delete; SettingsMACOS(const SettingsMACOS&) = delete; SettingsMACOS(SettingsMACOS&&) = delete; SettingsMACOS& operator=(const SettingsMACOS&) = delete; diff --git a/src/unix/SettingsUNIX.cxx b/src/unix/SettingsUNIX.cxx index b10f72a2a..91e589e68 100644 --- a/src/unix/SettingsUNIX.cxx +++ b/src/unix/SettingsUNIX.cxx @@ -18,7 +18,7 @@ #include "SettingsUNIX.hxx" // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -SettingsUNIX::SettingsUNIX(OSystem& osystem) - : Settings(osystem) +SettingsUNIX::SettingsUNIX() + : Settings() { } diff --git a/src/unix/SettingsUNIX.hxx b/src/unix/SettingsUNIX.hxx index e3409a877..71acbb101 100644 --- a/src/unix/SettingsUNIX.hxx +++ b/src/unix/SettingsUNIX.hxx @@ -18,8 +18,6 @@ #ifndef SETTINGS_UNIX_HXX #define SETTINGS_UNIX_HXX -class OSystem; - #include "Settings.hxx" /** @@ -33,12 +31,11 @@ class SettingsUNIX : public Settings /** Create a new UNIX settings object */ - explicit SettingsUNIX(OSystem& osystem); + explicit SettingsUNIX(); virtual ~SettingsUNIX() = default; private: // Following constructors and assignment operators not supported - SettingsUNIX() = delete; SettingsUNIX(const SettingsUNIX&) = delete; SettingsUNIX(SettingsUNIX&&) = delete; SettingsUNIX& operator=(const SettingsUNIX&) = delete; diff --git a/src/windows/SettingsWINDOWS.cxx b/src/windows/SettingsWINDOWS.cxx index 5910eb817..ec7ac237b 100644 --- a/src/windows/SettingsWINDOWS.cxx +++ b/src/windows/SettingsWINDOWS.cxx @@ -18,6 +18,7 @@ #include "SettingsWINDOWS.hxx" // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -SettingsWINDOWS::SettingsWINDOWS(OSystem& osystem) - : Settings(osystem) -{} +SettingsWINDOWS::SettingsWINDOWS() + : Settings() +{ +} diff --git a/src/windows/SettingsWINDOWS.hxx b/src/windows/SettingsWINDOWS.hxx index 7ed6d6c3d..fd998c491 100644 --- a/src/windows/SettingsWINDOWS.hxx +++ b/src/windows/SettingsWINDOWS.hxx @@ -28,12 +28,11 @@ class SettingsWINDOWS : public Settings /** Create a new UNIX settings object */ - explicit SettingsWINDOWS(OSystem& osystem); + explicit SettingsWINDOWS(); virtual ~SettingsWINDOWS() = default; private: // Following constructors and assignment operators not supported - SettingsWINDOWS() = delete; SettingsWINDOWS(const SettingsWINDOWS&) = delete; SettingsWINDOWS(SettingsWINDOWS&&) = delete; SettingsWINDOWS& operator=(const SettingsWINDOWS&) = delete;