Merge branch 'config-fixes' into 'master'

Config fixes

See merge request OpenMW/openmw!367

CHANGELOG.md

@@ -7,6 +7,7 @@
     Bug #2311: Targeted scripts are not properly supported on non-unique RefIDs
     Bug #2473: Unable to overstock merchants
     Bug #2798: Mutable ESM records
+    Bug #2976 [reopened]: Issues combining settings from the command line and both config files
     Bug #3676: NiParticleColorModifier isn't applied properly
     Bug #3714: Savegame fails to load due to conflict between SpellState and MagicEffects
     Bug #3862: Random container contents behave differently than vanilla

apps/opencs/editor.cpp

@@ -89,10 +89,10 @@ std::pair<Files::PathContainer, std::vector<std::string> > CS::Editor::readConfi
 
     desc.add_options()
     ("data", boost::program_options::value<Files::EscapePathContainer>()->default_value(Files::EscapePathContainer(), "data")->multitoken()->composing())
-    ("data-local", boost::program_options::value<Files::EscapeHashString>()->default_value(""))
+    ("data-local", boost::program_options::value<Files::EscapePath>()->default_value(Files::EscapePath(), ""))
     ("fs-strict", boost::program_options::value<bool>()->implicit_value(true)->default_value(false))
     ("encoding", boost::program_options::value<Files::EscapeHashString>()->default_value("win1252"))
-    ("resources", boost::program_options::value<Files::EscapeHashString>()->default_value("resources"))
+    ("resources", boost::program_options::value<Files::EscapePath>()->default_value(Files::EscapePath(), "resources"))
     ("fallback-archive", boost::program_options::value<Files::EscapeStringVector>()->
         default_value(Files::EscapeStringVector(), "fallback-archive")->multitoken())
     ("fallback", boost::program_options::value<FallbackMap>()->default_value(FallbackMap(), "")
@@ -112,7 +112,7 @@ std::pair<Files::PathContainer, std::vector<std::string> > CS::Editor::readConfi
     mDocumentManager.setEncoding(ToUTF8::calculateEncoding(mEncodingName));
     mFileDialog.setEncoding (QString::fromUtf8(mEncodingName.c_str()));
 
-    mDocumentManager.setResourceDir (mResources = variables["resources"].as<Files::EscapeHashString>().toStdString());
+    mDocumentManager.setResourceDir (mResources = variables["resources"].as<Files::EscapePath>().mPath);
 
     if (variables["script-blacklist-use"].as<bool>())
         mDocumentManager.setBlacklistedScripts (
@@ -125,14 +125,9 @@ std::pair<Files::PathContainer, std::vector<std::string> > CS::Editor::readConfi
         dataDirs = Files::PathContainer(Files::EscapePath::toPathContainer(variables["data"].as<Files::EscapePathContainer>()));
     }
 
-    std::string local = variables["data-local"].as<Files::EscapeHashString>().toStdString();
+    Files::PathContainer::value_type local(variables["data-local"].as<Files::EscapePath>().mPath);
     if (!local.empty())
-    {
+        dataLocal.push_back(local);
-        if (local.front() == '\"')
-            local = local.substr(1, local.length() - 2);
-
-        dataLocal.push_back(Files::PathContainer::value_type(local));
-    }
 
     mCfgMgr.processPaths (dataDirs);
     mCfgMgr.processPaths (dataLocal, true);

apps/openmw/main.cpp

@@ -63,20 +63,20 @@ bool parseOptions (int argc, char** argv, OMW::Engine& engine, Files::Configurat
         ("data", bpo::value<Files::EscapePathContainer>()->default_value(Files::EscapePathContainer(), "data")
             ->multitoken()->composing(), "set data directories (later directories have higher priority)")
 
-            ("data-local", bpo::value<Files::EscapeHashString>()->default_value(""),
+        ("data-local", bpo::value<Files::EscapePath>()->default_value(Files::EscapePath(), ""),
             "set local data directory (highest priority)")
 
         ("fallback-archive", bpo::value<Files::EscapeStringVector>()->default_value(Files::EscapeStringVector(), "fallback-archive")
-            ->multitoken(), "set fallback BSA archives (later archives have higher priority)")
+            ->multitoken()->composing(), "set fallback BSA archives (later archives have higher priority)")
 
-            ("resources", bpo::value<Files::EscapeHashString>()->default_value("resources"),
+        ("resources", bpo::value<Files::EscapePath>()->default_value(Files::EscapePath(), "resources"),
             "set resources directory")
 
         ("start", bpo::value<Files::EscapeHashString>()->default_value(""),
             "set initial cell")
 
         ("content", bpo::value<Files::EscapeStringVector>()->default_value(Files::EscapeStringVector(), "")
-            ->multitoken(), "content file(s): esm/esp, or omwgame/omwaddon")
+            ->multitoken()->composing(), "content file(s): esm/esp, or omwgame/omwaddon")
 
         ("no-sound", bpo::value<bool>()->implicit_value(true)
             ->default_value(false), "disable all sounds")
@@ -101,12 +101,12 @@ bool parseOptions (int argc, char** argv, OMW::Engine& engine, Files::Configurat
             "\t2 - treat warnings as errors")
 
         ("script-blacklist", bpo::value<Files::EscapeStringVector>()->default_value(Files::EscapeStringVector(), "")
-            ->multitoken(), "ignore the specified script (if the use of the blacklist is enabled)")
+            ->multitoken()->composing(), "ignore the specified script (if the use of the blacklist is enabled)")
 
         ("script-blacklist-use", bpo::value<bool>()->implicit_value(true)
             ->default_value(true), "enable script blacklisting")
 
-            ("load-savegame", bpo::value<Files::EscapeHashString>()->default_value(""),
+        ("load-savegame", bpo::value<Files::EscapePath>()->default_value(Files::EscapePath(), ""),
             "load a save game file on game startup (specify an absolute filename or a filename relative to the current working directory)")
 
         ("skip-menu", bpo::value<bool>()->implicit_value(true)
@@ -159,14 +159,16 @@ bool parseOptions (int argc, char** argv, OMW::Engine& engine, Files::Configurat
     {
         cfgMgr.readConfiguration(variables, desc, true);
 
-        Version::Version v = Version::getOpenmwVersion(variables["resources"].as<Files::EscapeHashString>().toStdString());
+        Version::Version v = Version::getOpenmwVersion(variables["resources"].as<Files::EscapePath>().mPath.string());
         std::cout << v.describe() << std::endl;
         return false;
     }
 
+    bpo::variables_map composingVariables = cfgMgr.separateComposingVariables(variables, desc);
     cfgMgr.readConfiguration(variables, desc);
+    cfgMgr.mergeComposingVariables(variables, composingVariables, desc);
 
-    Version::Version v = Version::getOpenmwVersion(variables["resources"].as<Files::EscapeHashString>().toStdString());
+    Version::Version v = Version::getOpenmwVersion(variables["resources"].as<Files::EscapePath>().mPath.string());
     std::cout << v.describe() << std::endl;
 
     engine.setGrabMouse(!variables["no-grab"].as<bool>());
@@ -181,18 +183,13 @@ bool parseOptions (int argc, char** argv, OMW::Engine& engine, Files::Configurat
 
     Files::PathContainer dataDirs(Files::EscapePath::toPathContainer(variables["data"].as<Files::EscapePathContainer>()));
 
-    std::string local(variables["data-local"].as<Files::EscapeHashString>().toStdString());
+    Files::PathContainer::value_type local(variables["data-local"].as<Files::EscapePath>().mPath);
     if (!local.empty())
-    {
+        dataDirs.push_back(local);
-        if (local.front() == '\"')
-            local = local.substr(1, local.length() - 2);
-
-        dataDirs.push_back(Files::PathContainer::value_type(local));
-    }
 
     cfgMgr.processPaths(dataDirs);
 
-    engine.setResourceDir(variables["resources"].as<Files::EscapeHashString>().toStdString());
+    engine.setResourceDir(variables["resources"].as<Files::EscapePath>().mPath);
     engine.setDataDirs(dataDirs);
 
     // fallback archives
@@ -230,7 +227,7 @@ bool parseOptions (int argc, char** argv, OMW::Engine& engine, Files::Configurat
     engine.setWarningsMode (variables["script-warn"].as<int>());
     engine.setScriptBlacklist (variables["script-blacklist"].as<Files::EscapeStringVector>().toStdStringVector());
     engine.setScriptBlacklistUse (variables["script-blacklist-use"].as<bool>());
-    engine.setSaveGameFile (variables["load-savegame"].as<Files::EscapeHashString>().toStdString());
+    engine.setSaveGameFile (variables["load-savegame"].as<Files::EscapePath>().mPath.string());
 
     // other settings
     Fallback::Map::init(variables["fallback"].as<FallbackMap>().mMap);

components/config/gamesettings.cpp

@@ -118,12 +118,19 @@ bool Config::GameSettings::readFile(QTextStream &stream, QMultiMap<QString, QStr
             QString key = keyRe.cap(1).trimmed();
             QString value = keyRe.cap(2).trimmed();
 
-            // Don't remove existing data entries
+            // Don't remove composing entries
-            if (key != QLatin1String("data"))
+            if (key != QLatin1String("data")
+                && key != QLatin1String("fallback-archive")
+                && key != QLatin1String("content")
+                && key != QLatin1String("script-blacklist"))
                 settings.remove(key);
-            else
+
+            if (key == QLatin1String("data")
+                || key == QLatin1String("data-local")
+                || key == QLatin1String("resources")
+                || key == QLatin1String("load-savegame"))
             {
-                // 'data=...' line, so needs processing to deal with ampersands and quotes
+                // Path line (e.g. 'data=...'), so needs processing to deal with ampersands and quotes
                 // The following is based on boost::io::detail::quoted_manip.hpp, but calling those functions did not work as there are too may QStrings involved
                 QChar delim = '\"';
                 QChar escape = '&';
@@ -175,8 +182,11 @@ bool Config::GameSettings::writeFile(QTextStream &stream)
     while (i.hasPrevious()) {
         i.previous();
 
-        // 'data=...' lines need quotes and ampersands escaping to match how boost::filesystem::path uses boost::io::quoted
+        // path lines (e.g. 'data=...') need quotes and ampersands escaping to match how boost::filesystem::path uses boost::io::quoted
-        if (i.key() == QLatin1String("data"))
+        if (i.key() == QLatin1String("data")
+            || i.key() == QLatin1String("data-local")
+            || i.key() == QLatin1String("resources")
+            || i.key() == QLatin1String("load-savegame"))
         {
             stream << i.key() << "=";
 
@@ -198,20 +208,6 @@ bool Config::GameSettings::writeFile(QTextStream &stream)
             continue;
         }
 
-        // Quote paths with spaces
-        if (i.key() == QLatin1String("data-local")
-            || i.key() == QLatin1String("resources"))
-        {
-            if (i.value().contains(QChar(' ')))
-            {
-                QString stripped = i.value();
-                stripped.remove(QChar('\"')); // Remove quotes
-
-                stream << i.key() << "=\"" << stripped << "\"\n";
-                continue;
-            }
-        }
-
         stream << i.key() << "=" << i.value() << "\n";
 
     }
@@ -406,7 +402,10 @@ bool Config::GameSettings::writeFileWithComments(QFile &file)
     {
         it.previous();
 
-        if (it.key() == QLatin1String("data"))
+        if (it.key() == QLatin1String("data")
+            || it.key() == QLatin1String("data-local")
+            || it.key() == QLatin1String("resources")
+            || it.key() == QLatin1String("load-savegame"))
         {
             settingLine = it.key() + "=";
 
@@ -424,15 +423,6 @@ bool Config::GameSettings::writeFileWithComments(QFile &file)
             }
             settingLine += delim;
         }
-        // Quote paths with spaces
-        else if ((it.key() == QLatin1String("data-local")
-             || it.key() == QLatin1String("resources")) && it.value().contains(QChar(' ')))
-        {
-            QString stripped = it.value();
-            stripped.remove(QChar('\"')); // Remove quotes
-
-            settingLine = it.key() + "=\"" + stripped + "\"";
-        }
         else
             settingLine = it.key() + "=" + it.value();
 

components/fallback/validate.cpp

@@ -0,0 +1,28 @@
+#include "validate.hpp"
+
+void Fallback::validate(boost::any& v, std::vector<std::string> const& tokens, FallbackMap*, int)
+{
+    if (v.empty())
+    {
+        v = boost::any(FallbackMap());
+    }
+
+    FallbackMap *map = boost::any_cast<FallbackMap>(&v);
+
+    for (const auto& token : tokens)
+    {
+        std::string temp = Files::EscapeHashString::processString(token);
+        size_t sep = temp.find(",");
+        if (sep < 1 || sep == temp.length() - 1 || sep == std::string::npos)
+#if (BOOST_VERSION < 104200)
+            throw boost::program_options::validation_error("invalid value");
+#else
+            throw boost::program_options::validation_error(boost::program_options::validation_error::invalid_option_value);
+#endif
+
+        std::string key(temp.substr(0, sep));
+        std::string value(temp.substr(sep + 1));
+
+        map->mMap[key] = value;
+    }
+}

components/fallback/validate.hpp

@@ -16,32 +16,7 @@ namespace Fallback
         std::map<std::string, std::string> mMap;
     };
 
-    void validate(boost::any &v, std::vector<std::string> const &tokens, FallbackMap*, int)
+    void validate(boost::any &v, std::vector<std::string> const &tokens, FallbackMap*, int);
-    {
-        if (v.empty())
-        {
-            v = boost::any(FallbackMap());
-        }
-
-        FallbackMap *map = boost::any_cast<FallbackMap>(&v);
-
-        for (std::vector<std::string>::const_iterator it = tokens.begin(); it != tokens.end(); ++it)
-        {
-            std::string temp = Files::EscapeHashString::processString(*it);
-            int sep = temp.find(",");
-            if (sep < 1 || sep == (int)temp.length() - 1)
-#if (BOOST_VERSION < 104200)
-                throw boost::program_options::validation_error("invalid value");
-#else
-                throw boost::program_options::validation_error(boost::program_options::validation_error::invalid_option_value);
-#endif
-
-            std::string key(temp.substr(0, sep));
-            std::string value(temp.substr(sep + 1));
-
-            map->mMap[key] = value;
-        }
-    }
 }
 
 #endif

components/files/configurationmanager.cpp

@@ -2,6 +2,7 @@
 
 #include <components/debug/debuglog.hpp>
 #include <components/files/escape.hpp>
+#include <components/fallback/validate.hpp>
 
 #include <boost/filesystem/fstream.hpp>
 /**
@@ -59,22 +60,86 @@ void ConfigurationManager::readConfiguration(boost::program_options::variables_m
     bool silent = mSilent;
     mSilent = quiet;
     
+    // User config has the highest priority.
+    auto composingVariables = separateComposingVariables(variables, description);
+    loadConfig(mFixedPath.getUserConfigPath(), variables, description);
+    mergeComposingVariables(variables, composingVariables, description);
+    boost::program_options::notify(variables);
+
     // read either local or global config depending on type of installation
+    composingVariables = separateComposingVariables(variables, description);
     bool loaded = loadConfig(mFixedPath.getLocalPath(), variables, description);
+    mergeComposingVariables(variables, composingVariables, description);
     boost::program_options::notify(variables);
     if (!loaded)
     {
+        composingVariables = separateComposingVariables(variables, description);
         loadConfig(mFixedPath.getGlobalConfigPath(), variables, description);
+        mergeComposingVariables(variables, composingVariables, description);
         boost::program_options::notify(variables);
     }
 
-    // User config has the highest priority.
-    loadConfig(mFixedPath.getUserConfigPath(), variables, description);
-    boost::program_options::notify(variables);
-
     mSilent = silent;
 }
 
+boost::program_options::variables_map ConfigurationManager::separateComposingVariables(boost::program_options::variables_map & variables, boost::program_options::options_description& description)
+{
+    boost::program_options::variables_map composingVariables;
+    for (auto itr = variables.begin(); itr != variables.end();)
+    {
+        if (description.find((*itr).first, false).semantic()->is_composing())
+        {
+            composingVariables.emplace(*itr);
+            itr = variables.erase(itr);
+        }
+        else
+            ++itr;
+    }
+    return composingVariables;
+}
+
+void ConfigurationManager::mergeComposingVariables(boost::program_options::variables_map & first, boost::program_options::variables_map & second, boost::program_options::options_description& description)
+{
+    for (auto& [name, variableValue] : first)
+    {
+        if (description.find(name, false).semantic()->is_composing())
+        {
+            if (second[name].defaulted() || second[name].empty())
+                continue;
+
+            boost::any& firstValue = variableValue.value();
+            const boost::any& secondValue = second[name].value();
+            
+            if (firstValue.type() == typeid(Files::EscapePathContainer))
+            {
+                auto& firstPathContainer = boost::any_cast<Files::EscapePathContainer&>(firstValue);
+                const auto& secondPathContainer = boost::any_cast<const Files::EscapePathContainer&>(secondValue);
+
+                firstPathContainer.insert(firstPathContainer.end(), secondPathContainer.begin(), secondPathContainer.end());
+            }
+            else if (firstValue.type() == typeid(Files::EscapeStringVector))
+            {
+                auto& firstVector = boost::any_cast<Files::EscapeStringVector&>(firstValue);
+                const auto& secondVector = boost::any_cast<const Files::EscapeStringVector&>(secondValue);
+
+                firstVector.mVector.insert(firstVector.mVector.end(), secondVector.mVector.begin(), secondVector.mVector.end());
+            }
+            else if (firstValue.type() == typeid(Fallback::FallbackMap))
+            {
+                auto& firstMap = boost::any_cast<Fallback::FallbackMap&>(firstValue);
+                const auto& secondMap = boost::any_cast<const Fallback::FallbackMap&>(secondValue);
+
+                std::map<std::string, std::string> tempMap(secondMap.mMap);
+                tempMap.merge(firstMap.mMap);
+                firstMap.mMap.swap(tempMap);
+            }
+            else
+                Log(Debug::Error) << "Unexpected composing variable type. Curse boost and their blasted arcane templates.";
+        }
+    }
+
+}
+
 void ConfigurationManager::processPaths(Files::PathContainer& dataDirs, bool create)
 {
     std::string path;

components/files/configurationmanager.hpp

@@ -25,6 +25,10 @@ struct ConfigurationManager
     void readConfiguration(boost::program_options::variables_map& variables,
         boost::program_options::options_description& description, bool quiet=false);
 
+    boost::program_options::variables_map separateComposingVariables(boost::program_options::variables_map& variables, boost::program_options::options_description& description);
+
+    void mergeComposingVariables(boost::program_options::variables_map& first, boost::program_options::variables_map& second, boost::program_options::options_description& description);
+
     void processPaths(Files::PathContainer& dataDirs, bool create = false);
     ///< \param create Try creating the directory, if it does not exist.