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

@@ -80,14 +80,14 @@ class MediaFactory
     #endif
     }
 
-    static unique_ptr<Settings> createSettings(OSystem& osystem)
+    static unique_ptr<Settings> createSettings()
     {
     #if defined(BSPF_UNIX)
-      return make_unique<SettingsUNIX>(osystem);
+      return make_unique<SettingsUNIX>();
     #elif defined(BSPF_WINDOWS)
-      return make_unique<SettingsWINDOWS>(osystem);
+      return make_unique<SettingsWINDOWS>();
     #elif defined(BSPF_MACOS)
-      return make_unique<SettingsMACOS>(osystem);
+      return make_unique<SettingsMACOS>();
     #else
       #error Unsupported platform for Settings!
     #endif

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();
+  mySettings->load(configFile(), options);
-
-  logMessage("Applying commandline config options ...", 2);
-  for(const auto& opt: options)
-    mySettings->setValue(opt.first, opt.second);
-
-  logMessage("Validating config options ...", 2);
-  mySettings->validate();
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
@@ -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)
+  logMessage("Saving config options ...", 2);
-    mySettings->saveConfig();
+  mySettings->save(configFile());
 
   if(myPropSet)
+  {
+    logMessage("Saving properties set ...", 2);
     myPropSet->save(myPropertiesFile);
+  }
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
@@ -469,7 +468,7 @@ unique_ptr<Console> 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

src/emucore/Settings.cxx

@@ -28,8 +28,7 @@
 #include "Settings.hxx"
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-Settings::Settings(OSystem& osystem)
+Settings::Settings()
-  : myOSystem(osystem)
 {
   // 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;
+  // First load from the platform-specific config file
-  string::size_type equalPos, garbage;
+  // Different ports may override this functionality
-
+  if(!loadConfigFile(cfgfile))
-  ifstream in(myOSystem.configFile());
-  if(!in || !in.is_open())
   {
-    myOSystem.logMessage("ERROR: Couldn't load settings file", 0);
+    // FIXME - make logger available everywhere
-    return;
+    // 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
+    // FIXME - make logger available everywhere
-    while((garbage = line.find("\t")) != string::npos)
+    // myOSystem.logMessage("ERROR: Couldn't save settings file", 0);
-      line.erase(garbage, 1);
+    cout << "ERROR: Couldn't save settings file" << endl;
-
-    // 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);
   }
 }
 
@@ -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())
-  {
+    return false;
-    myOSystem.logMessage("ERROR: Couldn't save settings file", 0);
-    return;
-  }
 
   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;
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

src/emucore/Settings.hxx

@@ -18,8 +18,6 @@
 #ifndef SETTINGS_HXX
 #define SETTINGS_HXX
 
-class OSystem;
-
 #include <map>
 
 #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<string, Variant>;
@@ -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.
+      platform-specific settings file.  Since different ports can have
-    */
+      different behaviour here, we mark it as virtual so derived
-    virtual void saveConfig();
+      classes can override as needed.
 
-    /**
+      @param cfgfile  The full path to the configuration file
-      This method must be called *after* settings have been fully loaded
+      @return  False on any error, else true
-      to validate (and change, if necessary) any improper settings.
     */
-    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;

src/macos/SettingsMACOS.cxx

@@ -24,13 +24,13 @@ extern "C" {
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-SettingsMACOS::SettingsMACOS(OSystem& osystem)
+SettingsMACOS::SettingsMACOS()
-  : Settings(osystem)
+  : 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;
 }

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;

src/unix/SettingsUNIX.cxx

@@ -18,7 +18,7 @@
 #include "SettingsUNIX.hxx"
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-SettingsUNIX::SettingsUNIX(OSystem& osystem)
+SettingsUNIX::SettingsUNIX()
-  : Settings(osystem)
+  : Settings()
 {
 }

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;

src/windows/SettingsWINDOWS.cxx

@@ -18,6 +18,7 @@
 #include "SettingsWINDOWS.hxx"
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-SettingsWINDOWS::SettingsWINDOWS(OSystem& osystem)
+SettingsWINDOWS::SettingsWINDOWS()
-  : Settings(osystem)
+  : Settings()
-{}
+{
+}

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;