Settings: Ensure INI gets saved in logical section order

Author: stenzek

src/core/core.cpp

@@ -251,7 +251,7 @@ bool Core::InitializeBaseSettingsLayer(std::string settings_path, Error* error)
     else
     {
       SetDefaultSettings(si, true, true, true);
-      if (!si.Save(error))
+      if (!si.Save(error, Settings::GetSectionSaveOrder()))
       {
         Error::AddPrefix(error, "Failed to save settings: ");
         return false;
@@ -277,7 +277,7 @@ bool Core::InitializeBaseSettingsLayer(std::string settings_path, Error* error)
 bool Core::SaveBaseSettingsLayer(Error* error)
 {
   INISettingsInterface& si = s_locals.base_settings_interface;
-  if (si.IsDirty() && !si.Save(error))
+  if (si.IsDirty() && !si.Save(error, Settings::GetSectionSaveOrder()))
     return false;
 
   return true;

src/core/fullscreenui_settings.cpp

@@ -3159,7 +3159,7 @@ void FullscreenUI::DoSaveInputProfile(const std::string& name)
   const auto lock = Core::GetSettingsLock();
   SettingsInterface* ssi = GetEditingSettingsInterface();
   InputManager::CopyConfiguration(&dsi, *ssi, true, false, true, false);
-  if (dsi.Save())
+  if (dsi.Save(nullptr, Settings::GetSectionSaveOrder()))
     ShowToast(OSDMessageType::Quick, {}, fmt::format(FSUI_FSTR("Controller preset '{}' saved."), name));
   else
     ShowToast(OSDMessageType::Info, {}, fmt::format(FSUI_FSTR("Failed to save controller preset '{}'."), name));

src/core/settings.cpp

@@ -155,6 +155,59 @@ const MediaCaptureBackend Settings::DEFAULT_MEDIA_CAPTURE_BACKEND = MediaCapture
 const MediaCaptureBackend Settings::DEFAULT_MEDIA_CAPTURE_BACKEND = MediaCaptureBackend::MaxCount;
 #endif
 
+std::span<const char* const> Settings::GetSectionSaveOrder()
+{
+  static constexpr std::array order = {
+    // clang-format off
+    "Patches",
+    "Cheats",
+    "Main",
+    "UI",
+    "GameListTableView",
+    "AutoUpdater",
+    "Folders",
+    "GameList",
+    "Cheevos",
+    "Logging",
+    "BIOS",
+    "Console",
+    "CPU",
+    "GPU",
+    "Display",
+    "CDROM",
+    "Audio",
+    "MemoryCards",
+    "TextureReplacements",
+    "MediaCapture",
+    "InternalPostProcessing",
+    "PostProcessing",
+    "BorderOverlay",
+    "InputSources",
+#ifdef ENABLE_SDL
+    "SDLExtra",
+#endif
+    "ControllerPorts",
+    "Pad1",
+    "Pad2",
+    "Pad3",
+    "Pad4",
+    "Pad5",
+    "Pad6",
+    "Pad7",
+    "Pad8",
+    "Hotkeys",
+    "PIO",
+    "SIO",
+    "PCDrv",
+    "Debug",
+    "DebugWindows",
+    "Hacks",
+    // clang-format on
+  };
+
+  return order;
+}
+
 Settings::Settings()
 {
   display_osd_margin = ImGuiManager::DEFAULT_SCREEN_MARGIN;

src/core/settings.h

@@ -471,6 +471,8 @@ struct Settings : public GPUSettings
 
   bool AreGPUDeviceSettingsChanged(const Settings& old_settings) const;
 
+  static std::span<const char* const> GetSectionSaveOrder();
+
   /// Initializes configuration.
   static void SetDefaultLogConfig(SettingsInterface& si);
   static void UpdateLogConfig(const SettingsInterface& si);

src/duckstation-qt/controllersettingswindow.cpp

@@ -15,6 +15,7 @@
 #include "util/input_manager.h"
 
 #include "common/assert.h"
+#include "common/error.h"
 #include "common/file_system.h"
 
 #include <QtWidgets/QInputDialog>
@@ -227,11 +228,13 @@ void ControllerSettingsWindow::onNewProfileClicked()
     }
   }
 
-  if (!temp_si.Save())
+  Error error;
+  if (!temp_si.Save(&error, Settings::GetSectionSaveOrder()))
   {
-    QtUtils::AsyncMessageBox(
-      this, QMessageBox::Critical, tr("Error"),
-      tr("Failed to save the new preset to '%1'.").arg(QString::fromStdString(temp_si.GetPath())));
+    QtUtils::AsyncMessageBox(this, QMessageBox::Critical, tr("Error"),
+                             tr("Failed to save the new preset to '%1':\n%2")
+                               .arg(QString::fromStdString(temp_si.GetPath()))
+                               .arg(QString::fromStdString(error.GetDescription())));
     return;
   }
 
@@ -316,7 +319,14 @@ void ControllerSettingsWindow::onCopyGlobalSettingsClicked()
                                     false);
   }
 
-  m_editing_settings_interface->Save();
+  Error error;
+  if (!m_editing_settings_interface->Save(&error, Settings::GetSectionSaveOrder()))
+  {
+    QtUtils::AsyncMessageBox(this, QMessageBox::Critical, tr("Error"),
+                             tr("Failed to save the preset:\n%1").arg(QString::fromStdString(error.GetDescription())));
+    return;
+  }
+
   g_core_thread->reloadGameSettings();
   createWidgets();
 
@@ -409,7 +419,7 @@ void ControllerSettingsWindow::clearSettingValue(const char* section, const char
   if (m_editing_settings_interface)
   {
     m_editing_settings_interface->DeleteValue(section, key);
-    m_editing_settings_interface->Save();
+    m_editing_settings_interface->Save(nullptr, Settings::GetSectionSaveOrder());
     g_core_thread->reloadGameSettings();
   }
   else

src/duckstation-qt/qthost.cpp

@@ -697,7 +697,7 @@ bool QtHost::SaveGameSettings(SettingsInterface* sif, bool delete_if_empty)
   // see above
   const auto lock = Core::GetSettingsLock();
 
-  if (!ini->Save(&error))
+  if (!ini->Save(&error, Settings::GetSectionSaveOrder()))
   {
     Host::ReportErrorAsync(
       TRANSLATE_SV("QtHost", "Error"),

src/util-tests/ini_settings_interface_tests.cpp

@@ -846,3 +846,198 @@ TEST(INISettingsInterface, SetPathDirtyFlag)
   si.SetPath("/tmp/new_path.ini");
   EXPECT_TRUE(si.IsDirty());
 }
+
+// ---- Ordered Save ----
+
+TEST(INISettingsInterface, OrderedSaveEmptyOrder)
+{
+  INISettingsInterface si;
+  si.LoadFromString("[B]\nb = 2\n\n[A]\na = 1\n");
+
+  const std::string without_order = si.SaveToString();
+  const std::string with_empty_order = si.SaveToString({});
+  EXPECT_EQ(without_order, with_empty_order);
+}
+
+TEST(INISettingsInterface, OrderedSaveBasic)
+{
+  INISettingsInterface si;
+  si.LoadFromString("[Alpha]\na = 1\n\n[Beta]\nb = 2\n\n[Gamma]\ng = 3\n");
+
+  // Request Gamma first, then Alpha.
+  const char* const order[] = {"Gamma", "Alpha"};
+  const std::string output = si.SaveToString(order);
+  EXPECT_EQ(output,
+            "[Gamma]\ng = 3\n\n"
+            "[Alpha]\na = 1\n\n"
+            "[Beta]\nb = 2\n");
+}
+
+TEST(INISettingsInterface, OrderedSaveAllSections)
+{
+  INISettingsInterface si;
+  si.LoadFromString("[A]\na = 1\n\n[B]\nb = 2\n\n[C]\nc = 3\n");
+
+  // Reverse the natural alphabetical order.
+  const char* const order[] = {"C", "B", "A"};
+  const std::string output = si.SaveToString(order);
+  EXPECT_EQ(output,
+            "[C]\nc = 3\n\n"
+            "[B]\nb = 2\n\n"
+            "[A]\na = 1\n");
+}
+
+TEST(INISettingsInterface, OrderedSaveNonExistentSections)
+{
+  INISettingsInterface si;
+  si.LoadFromString("[A]\na = 1\n\n[B]\nb = 2\n");
+
+  // "Missing" doesn't exist; should be silently skipped.
+  const char* const order[] = {"Missing", "B"};
+  const std::string output = si.SaveToString(order);
+  EXPECT_EQ(output,
+            "[B]\nb = 2\n\n"
+            "[A]\na = 1\n");
+}
+
+TEST(INISettingsInterface, OrderedSaveRemainingInAlphabeticalOrder)
+{
+  INISettingsInterface si;
+  si.LoadFromString("[Delta]\nd = 4\n\n[Alpha]\na = 1\n\n[Charlie]\nc = 3\n\n[Bravo]\nb = 2\n");
+
+  // Only specify Charlie; rest should come after in alphabetical order.
+  const char* const order[] = {"Charlie"};
+  const std::string output = si.SaveToString(order);
+  EXPECT_EQ(output,
+            "[Charlie]\nc = 3\n\n"
+            "[Alpha]\na = 1\n\n"
+            "[Bravo]\nb = 2\n\n"
+            "[Delta]\nd = 4\n");
+}
+
+TEST(INISettingsInterface, OrderedSaveSingleSection)
+{
+  INISettingsInterface si;
+  si.LoadFromString("[Only]\nkey = val\n");
+
+  const char* const order[] = {"Only"};
+  const std::string output = si.SaveToString(order);
+  EXPECT_EQ(output, "[Only]\nkey = val\n");
+}
+
+TEST(INISettingsInterface, OrderedSaveEmptyINI)
+{
+  INISettingsInterface si;
+  si.LoadFromString("");
+
+  const char* const order[] = {"A", "B"};
+  const std::string output = si.SaveToString(order);
+  EXPECT_TRUE(output.empty());
+}
+
+TEST(INISettingsInterface, OrderedSaveDuplicateOrderEntries)
+{
+  INISettingsInterface si;
+  si.LoadFromString("[A]\na = 1\n\n[B]\nb = 2\n");
+
+  // "A" appears twice; section A should only be written once.
+  const char* const order[] = {"A", "B", "A"};
+  const std::string output = si.SaveToString(order);
+  EXPECT_EQ(output,
+            "[A]\na = 1\n\n"
+            "[B]\nb = 2\n");
+}
+
+TEST(INISettingsInterface, OrderedSaveContentPreserved)
+{
+  INISettingsInterface si;
+  si.LoadFromString("[Z]\n"
+                    "plain = hello\n"
+                    "quoted = \"value ; with # chars\"\n\n"
+                    "[A]\n"
+                    "num = 42\n");
+
+  const char* const order[] = {"Z"};
+  const std::string output = si.SaveToString(order);
+
+  // Z comes first (as ordered), A follows. Quoting is preserved on save.
+  EXPECT_EQ(output,
+            "[Z]\nplain = hello\nquoted = \"value ; with # chars\"\n\n"
+            "[A]\nnum = 42\n");
+}
+
+TEST(INISettingsInterface, OrderedSavePrefixMatching)
+{
+  INISettingsInterface si;
+  si.LoadFromString("[Other]\no = 0\n\n"
+                    "[Pad]\np = 1\n\n"
+                    "[Pad/1]\np1 = 2\n\n"
+                    "[Pad/2]\np2 = 3\n");
+
+  // "Pad" should match "Pad" (exact) and "Pad/1", "Pad/2" (prefix with /).
+  const char* const order[] = {"Pad"};
+  const std::string output = si.SaveToString(order);
+  EXPECT_EQ(output,
+            "[Pad]\np = 1\n\n"
+            "[Pad/1]\np1 = 2\n\n"
+            "[Pad/2]\np2 = 3\n\n"
+            "[Other]\no = 0\n");
+}
+
+TEST(INISettingsInterface, OrderedSavePrefixBoundary)
+{
+  INISettingsInterface si;
+  si.LoadFromString("[Pad]\np = 1\n\n"
+                    "[Pad/1]\np1 = 2\n\n"
+                    "[Pad2]\np2 = 3\n\n"
+                    "[Padding]\npd = 4\n");
+
+  // "Pad" should match "Pad" and "Pad/1", but NOT "Pad2" or "Padding".
+  const char* const order[] = {"Pad"};
+  const std::string output = si.SaveToString(order);
+  EXPECT_EQ(output,
+            "[Pad]\np = 1\n\n"
+            "[Pad/1]\np1 = 2\n\n"
+            "[Pad2]\np2 = 3\n\n"
+            "[Padding]\npd = 4\n");
+}
+
+TEST(INISettingsInterface, OrderedSavePrefixGroupsPreserveOrder)
+{
+  INISettingsInterface si;
+  si.LoadFromString("[Pad/3]\np3 = 3\n\n"
+                    "[Pad/1]\np1 = 1\n\n"
+                    "[Pad/2]\np2 = 2\n\n"
+                    "[Other]\no = 0\n");
+
+  // Sub-sections should appear in their natural (alphabetical) order within the prefix group.
+  const char* const order[] = {"Pad"};
+  const std::string output = si.SaveToString(order);
+  EXPECT_EQ(output,
+            "[Pad/1]\np1 = 1\n\n"
+            "[Pad/2]\np2 = 2\n\n"
+            "[Pad/3]\np3 = 3\n\n"
+            "[Other]\no = 0\n");
+}
+
+TEST(INISettingsInterface, OrderedSaveMultiplePrefixes)
+{
+  INISettingsInterface si;
+  si.LoadFromString("[Hotkey]\nh = 0\n\n"
+                    "[Hotkey/1]\nh1 = 1\n\n"
+                    "[Other]\no = 0\n\n"
+                    "[Pad]\np = 0\n\n"
+                    "[Pad/1]\np1 = 1\n\n"
+                    "[Pad/2]\np2 = 2\n");
+
+  // Pad group first, then Hotkey group, then remaining.
+  const char* const order[] = {"Pad", "Hotkey"};
+  const std::string output = si.SaveToString(order);
+  EXPECT_EQ(output,
+            "[Pad]\np = 0\n\n"
+            "[Pad/1]\np1 = 1\n\n"
+            "[Pad/2]\np2 = 2\n\n"
+            "[Hotkey]\nh = 0\n\n"
+            "[Hotkey/1]\nh1 = 1\n\n"
+            "[Other]\no = 0\n");
+}