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/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
+  This needs to be done separately, before either an OSystem or Settings
-  functionality, since they both depend on the settings file being already
+  object can be created, since they both depend on each other, and a
-  available.  However, since a variabe basedir implies a different location
+  variable basedir implies a different location for the settings file.
-  for the settings file, it *must* be processed first.
 
-  This function will set the environment variables STELLA_BASEDIR and
+  This function will call OSystem::overrideBaseDir() when either of the
-  STELLA_USECURRENTASBASEDIR; it is up to each port to use these as they wish.
+  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
+  unique_ptr<OSystem> theOSystem;
-  // 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<OSystem> theOSystem = MediaFactory::createOSystem();
-  theOSystem->loadConfig();
-  theOSystem->logMessage("Loading config options ...", 2);
 
   auto Cleanup = [&theOSystem]() {
+    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
+  // Parse the commandline arguments
-  theOSystem->logMessage("Loading commandline arguments ...", 2);
+  // They are placed in different maps depending on whether they're used
-  string romfile = theOSystem->settings().loadCommandLine(ac, av);
+  // locally or globally
+  Settings::Options globalOpts, localOpts;
+  parseCommandLine(ac, av, globalOpts, localOpts);
 
-  // Finally, make sure the settings are valid
+  // Check for custom base directory; some ports make use of this
-  // We do it once here, so the rest of the program can assume valid settings
+  checkForCustomBaseDir(localOpts);
-  theOSystem->logMessage("Validating config options ...", 2);
+
-  theOSystem->settings().validate();
+  // 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)
+    if(localOpts["break"].toString() != "")
-    const string& initBreak = theOSystem->settings().getString("break");
-    if(initBreak != "")
     {
       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
   }

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 = "";

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.
+      This method should be called to initiate the process of loading settings
-      It first loads the settings from the config file, then informs subsystems
+      from the config file.  It takes care of loading settings, applying
-      about the new settings.
+      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.
+      This method should be called to save the current settings. It first asks
-      It first asks each subsystem to update its settings, then it saves all
+      each subsystem to update its settings, then it saves all settings to the
-      settings to the config file.
+      config file.
     */
     void saveConfig();
 
@@ -385,6 +385,20 @@ class OSystem
     */
     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;

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()
 {

src/emucore/Settings.hxx

@@ -20,6 +20,8 @@
 
 class OSystem;
 
+#include <map>
+
 #include "Variant.hxx"
 #include "bspf.hxx"
 
@@ -39,20 +41,9 @@ class Settings
     explicit Settings(OSystem& osystem);
     virtual ~Settings() = default;
 
+    using Options = std::map<string, Variant>;
+
   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)
     {