Fix some bugs around game-specific config load/save (#21033)

Core/Config.cpp

@@ -1166,9 +1166,9 @@ Config::~Config() {
 }
 
 void Config::Reload() {
-	reload_ = true;
+	inReload_ = true;
 	Load();
-	reload_ = false;
+	inReload_ = false;
 }
 
 // Call this if you change the search path (such as when changing memstick directory. can't
@@ -1351,15 +1351,16 @@ void Config::Load(const char *iniFileName, const char *controllerIniFilename) {
 	// So this is all the way down here to overwrite the controller settings
 	// sadly it won't benefit from all the "version conversion" going on up-above
 	// but these configs shouldn't contain older versions anyhow
-	if (gameSpecific_) {
-		LoadGameConfig(gameId_);
-	}
+	_dbg_assert_(!IsGameSpecific());
 
 	PostLoadCleanup();
 
 	INFO_LOG(Log::Loader, "Config loaded: '%s' (%0.1f ms)", iniFilename_.c_str(), (time_now_d() - startTime) * 1000.0);
 }
 
+// If we're in game specific mode, we need to:
+// * Save the game-specific settings to the game-specific ini file.
+// * Then, save the NON-game-specific settings ONLY to the regular ini file!
 bool Config::Save(const char *saveReason) {
 	double startTime = time_now_d();
 	if (!IsFirstInstance()) {
@@ -1371,7 +1372,10 @@ bool Config::Save(const char *saveReason) {
 	}
 
 	if (!iniFilename_.empty() && g_Config.bSaveSettings) {
-		SaveGameConfig(gameId_, "");  // we don't pass a title, it was stored in the ini the first time.
+		if (IsGameSpecific()) {
+			// Save just the game-specific settings to the game-specific ini.
+			SaveGameConfig(gameId_, "");  // we don't pass a title, it was stored in the ini the first time.
+		}
 
 		PreSaveCleanup();
 
@@ -1389,6 +1393,10 @@ bool Config::Save(const char *saveReason) {
 			Section *section = iniFile.GetOrCreateSection(meta.section);
 			ConfigBlock *configBlock = meta.configBlock;
 			for (size_t j = 0; j < meta.settingsCount; j++) {
+				if (IsGameSpecific() && (meta.settings[j].Flags() & CfgFlag::PER_GAME)) {
+					// Skip per-game settings in non-game-specific ini.
+					continue;
+				}
 				meta.settings[j].WriteToIniSection(configBlock, section);
 			}
 		}
@@ -1405,7 +1413,9 @@ bool Config::Save(const char *saveReason) {
 			pinnedPaths->Set(keyName, vPinnedPaths[i]);
 		}
 
-		if (!gameSpecific_) {
+		if (!IsGameSpecific()) {
+			// These settings can be game specific, and so are handled in SaveGameConfig().
+
 			Section *postShaderSetting = iniFile.GetOrCreateSection("PostShaderSetting");
 			postShaderSetting->Clear();
 			for (const auto &[k, v] : mPostShaderSetting) {
@@ -1443,8 +1453,8 @@ bool Config::Save(const char *saveReason) {
 		}
 		INFO_LOG(Log::Loader, "Config saved (%s): '%s' (%0.1f ms)", saveReason, iniFilename_.c_str(), (time_now_d() - startTime) * 1000.0);
 
-		if (!gameSpecific_) //otherwise we already did this in SaveGameConfig()
-		{
+		if (!IsGameSpecific()) {
+			// These settings can be game specific, and so are handled in SaveGameConfig().
 			IniFile controllerIniFile;
 			if (!controllerIniFile.Load(controllerIniFilename_)) {
 				ERROR_LOG(Log::Loader, "Error saving controller config - can't read ini first '%s'", controllerIniFilename_.c_str());
@@ -1593,8 +1603,8 @@ Path Config::FindConfigFile(std::string_view baseFilename, bool *exists) const {
 }
 
 void Config::RestoreDefaults(RestoreSettingsBits whatToRestore, bool log) {
-	if (gameSpecific_) {
-		// TODO: This should be possible to do in a cleaner way.
+	if (IsGameSpecific()) {
+		// TODO: This could be done in a cleaner way.
 		DeleteGameConfig(gameId_);
 		CreateGameConfig(gameId_);
 		Load();
@@ -1630,15 +1640,6 @@ bool Config::HasGameConfig(std::string_view gameId) {
 	return exists;
 }
 
-void Config::ChangeGameSpecific(const std::string &pGameId, std::string_view title) {
-	if (!reload_) {
-		Save("changeGameSpecific");
-	}
-
-	gameId_ = pGameId;
-	gameSpecific_ = !pGameId.empty();
-}
-
 bool Config::CreateGameConfig(std::string_view gameId) {
 	bool exists;
 	Path fullIniFilePath = GetGameConfigFilePath(gameId, &exists);
@@ -1678,6 +1679,11 @@ bool Config::SaveGameConfig(const std::string &gameId, std::string_view titleFor
 		return false;
 	}
 
+	if (gameId_.empty()) {
+		INFO_LOG(Log::G3D, "Switching to game-specific mode for saving config: %s", gameId.c_str());
+		gameId_ = gameId;
+	}
+
 	bool exists;
 	Path fullIniFilePath = GetGameConfigFilePath(gameId, &exists);
 
@@ -1693,6 +1699,7 @@ bool Config::SaveGameConfig(const std::string &gameId, std::string_view titleFor
 
 	PreSaveCleanup();
 
+	// Do all the actual saving.
 	for (const ConfigSectionMeta &meta : g_sectionMeta) {
 		Section *section = iniFile.GetOrCreateSection(meta.section);
 		ConfigBlock *configBlock = meta.configBlock;
@@ -1727,6 +1734,12 @@ bool Config::SaveGameConfig(const std::string &gameId, std::string_view titleFor
 }
 
 bool Config::LoadGameConfig(const std::string &gameId) {
+	// Switch to game specific mode, if we're not in it.
+	if (gameId_.empty()) {
+		INFO_LOG(Log::Loader, "Switching to game specific mode before load: %s", gameId.c_str());
+		gameId_ = gameId;
+	}
+
 	bool exists;
 	Path iniFileNameFull = GetGameConfigFilePath(gameId, &exists);
 	if (!exists) {
@@ -1734,10 +1747,6 @@ bool Config::LoadGameConfig(const std::string &gameId) {
 		return false;
 	}
 
-	_dbg_assert_(!gameId.empty());
-
-	ChangeGameSpecific(gameId);
-
 	IniFile iniFile;
 	iniFile.Load(iniFileNameFull);
 
@@ -1784,16 +1793,16 @@ bool Config::LoadGameConfig(const std::string &gameId) {
 	}
 
 	PostLoadCleanup();
+
+	DEBUG_LOG(Log::Loader, "Game-specific config loaded: %s", gameId_.c_str());
 	return true;
 }
 
 void Config::UnloadGameConfig() {
-	if (!gameSpecific_) {
-		return;
-	}
+	_dbg_assert_(IsGameSpecific());
 
+	// Leave game-specific mode.
 	gameId_.clear();
-	gameSpecific_ = false;
 
 	// Reload all settings from the main ini file.
 	IniFile iniFile;

Core/Config.h

@@ -665,9 +665,6 @@ public:
 	void Reload();
 	void RestoreDefaults(RestoreSettingsBits whatToRestore, bool log = false);
 
-	// Per-game config management.
-	void ChangeGameSpecific(const std::string &gameId = "", std::string_view title = "");
-
 	// Note: This doesn't switch to the config, just creates it.
 	bool CreateGameConfig(std::string_view gameId);
 	bool DeleteGameConfig(std::string_view gameId);
@@ -677,7 +674,7 @@ public:
 
 	Path GetGameConfigFilePath(std::string_view gameId, bool *exists);
 	bool HasGameConfig(std::string_view gameId);
-	bool IsGameSpecific() const { return gameSpecific_; }
+	bool IsGameSpecific() const { return !gameId_.empty(); }
 
 	void SetSearchPath(const Path &path);
 	Path FindConfigFile(std::string_view baseFilename, bool *exists) const;
@@ -729,15 +726,17 @@ private:
 	// Applies defaults for missing settings.
 	void ReadAllSettings(const IniFile &iniFile);
 
-	bool reload_ = false;
+	bool inReload_ = false;
 
-	bool gameSpecific_ = false;
+	// If not empty, we're using a game-specific config.
 	std::string gameId_;
 
 	PlayTimeTracker playTimeTracker_;
 
+	// Always the paths to the main configs, doesn't change with game-specific overlay.
 	Path iniFilename_;
 	Path controllerIniFilename_;
+
 	Path searchPath_;
 	Path appendedConfigFileName_;
 	// A set make more sense, but won't have many entry, and I dont want to include the whole std::set header here

Core/ConfigSettings.h

@@ -202,6 +202,7 @@ struct ConfigSetting {
 	void ReportSetting(const ConfigBlock *configBlock, UrlEncoder &data, const std::string &prefix) const;
 
 	// Easy flag accessors.
+	CfgFlag Flags() const { return flags_; }
 	bool PerGame() const { return flags_ & CfgFlag::PER_GAME; }
 	bool SaveSetting() const { return !(flags_ & CfgFlag::DONT_SAVE); }
 	bool Report() const { return flags_ & CfgFlag::REPORT; }

Core/System.cpp

@@ -735,7 +735,9 @@ void PSP_Shutdown(bool success) {
 
 	currentMIPS = nullptr;
 
-	g_Config.UnloadGameConfig();
+	if (g_Config.IsGameSpecific()) {
+		g_Config.UnloadGameConfig();
+	}
 
 	Core_NotifyLifecycle(CoreLifecycle::STOPPED);
 

UI/GameSettingsScreen.cpp

@@ -132,9 +132,45 @@ public:
 };
 
 GameSettingsScreen::GameSettingsScreen(const Path &gamePath, std::string gameID, bool editThenRestore)
-	: UITabbedBaseDialogScreen(gamePath, TabDialogFlags::HorizontalOnlyIcons | TabDialogFlags::VerticalShowIcons), gameID_(gameID), editThenRestore_(editThenRestore) {
+	: UITabbedBaseDialogScreen(gamePath, TabDialogFlags::HorizontalOnlyIcons | TabDialogFlags::VerticalShowIcons), gameID_(gameID), editGameSpecificThenRestore_(editThenRestore) {
 	prevInflightFrames_ = g_Config.iInflightFrames;
 	analogSpeedMapped_ = KeyMap::InputMappingsFromPspButton(VIRTKEY_SPEED_ANALOG, nullptr, true);
+
+	if (editGameSpecificThenRestore_) {
+		std::shared_ptr<GameInfo> info = g_gameInfoCache->GetInfo(nullptr, gamePath_, GameInfoFlags::PARAM_SFO);
+		g_Config.LoadGameConfig(gameID_);
+	}
+
+	iAlternateSpeedPercent1_ = g_Config.iFpsLimit1 < 0 ? -1 : (g_Config.iFpsLimit1 * 100) / 60;
+	iAlternateSpeedPercent2_ = g_Config.iFpsLimit2 < 0 ? -1 : (g_Config.iFpsLimit2 * 100) / 60;
+	iAlternateSpeedPercentAnalog_ = (g_Config.iAnalogFpsLimit * 100) / 60;
+}
+
+GameSettingsScreen::~GameSettingsScreen() {
+	Reporting::Enable(enableReports_, "report.ppsspp.org");
+	Reporting::UpdateConfig();
+	if (!g_Config.Save("GameSettingsScreen::onFinish")) {
+		System_Toast("Failed to save settings!\nCheck permissions, or try to restart the device.");
+	}
+
+	if (editGameSpecificThenRestore_) {
+		// We already saved above. Just unload the game config.
+		// Note that we are leaving the screen here in practice, we don't need to reset the bool.
+		g_Config.UnloadGameConfig();
+	}
+
+	System_Notify(SystemNotification::UI);
+
+	KeyMap::UpdateNativeMenuKeys();
+
+	// Wipe some caches after potentially changing settings.
+	// Let's not send resize messages here, handled elsewhere.
+	System_PostUIMessage(UIMessage::GPU_CONFIG_CHANGED);
+}
+
+void GameSettingsScreen::PreCreateViews() {
+	ReloadAllPostShaderInfo(screenManager()->getDrawContext());
+	ReloadAllThemeInfo();
 }
 
 // This needs before run CheckGPUFeatures()
@@ -218,20 +254,6 @@ static bool PathToVisualUsbPath(Path path, std::string &outPath) {
 	return false;
 }
 
-void GameSettingsScreen::PreCreateViews() {
-	ReloadAllPostShaderInfo(screenManager()->getDrawContext());
-	ReloadAllThemeInfo();
-
-	if (editThenRestore_) {
-		std::shared_ptr<GameInfo> info = g_gameInfoCache->GetInfo(nullptr, gamePath_, GameInfoFlags::PARAM_SFO);
-		g_Config.LoadGameConfig(gameID_);
-	}
-
-	iAlternateSpeedPercent1_ = g_Config.iFpsLimit1 < 0 ? -1 : (g_Config.iFpsLimit1 * 100) / 60;
-	iAlternateSpeedPercent2_ = g_Config.iFpsLimit2 < 0 ? -1 : (g_Config.iFpsLimit2 * 100) / 60;
-	iAlternateSpeedPercentAnalog_ = (g_Config.iAnalogFpsLimit * 100) / 60;
-}
-
 void GameSettingsScreen::CreateTabs() {
 	using namespace UI;
 	auto ms = GetI18NCategory(I18NCat::MAINSETTINGS);
@@ -1613,29 +1635,6 @@ void GameSettingsScreen::OnFullscreenMultiChange(UI::EventParams &e) {
 	System_ToggleFullscreenState(g_Config.UseFullScreen() ? "1" : "0");
 }
 
-void GameSettingsScreen::onFinish(DialogResult result) {
-	Reporting::Enable(enableReports_, "report.ppsspp.org");
-	Reporting::UpdateConfig();
-	if (!g_Config.Save("GameSettingsScreen::onFinish")) {
-		System_Toast("Failed to save settings!\nCheck permissions, or try to restart the device.");
-	}
-
-	if (editThenRestore_) {
-		// In case we didn't have the title yet before, try again.
-		std::shared_ptr<GameInfo> info = g_gameInfoCache->GetInfo(nullptr, gamePath_, GameInfoFlags::PARAM_SFO);
-		g_Config.ChangeGameSpecific(gameID_, info->GetTitle());
-		g_Config.UnloadGameConfig();
-	}
-
-	System_Notify(SystemNotification::UI);
-
-	KeyMap::UpdateNativeMenuKeys();
-
-	// Wipe some caches after potentially changing settings.
-	// Let's not send resize messages here, handled elsewhere.
-	System_PostUIMessage(UIMessage::GPU_CONFIG_CHANGED);
-}
-
 void GameSettingsScreen::dialogFinished(const Screen *dialog, DialogResult result) {
 	if (result == DialogResult::DR_OK) {
 		g_Config.iFpsLimit1 = iAlternateSpeedPercent1_ < 0 ? -1 : (iAlternateSpeedPercent1_ * 60) / 100;
@@ -1700,7 +1699,7 @@ void GameSettingsScreen::TriggerRestartOrDo(std::function<void()> callback) {
 	auto di = GetI18NCategory(I18NCat::DIALOG);
 	screenManager()->push(new UI::MessagePopupScreen(di->T("Restart"), di->T("Changing this setting requires PPSSPP to restart."), di->T("Restart"), di->T("Cancel"), [=](bool yes) {
 		if (yes) {
-			TriggerRestart("GameSettingsScreen::RenderingBackendYes", editThenRestore_, gamePath_);
+			TriggerRestart("GameSettingsScreen::RenderingBackendYes", editGameSpecificThenRestore_, gamePath_);
 		} else {
 			callback();
 		}

UI/GameSettingsScreen.h

@@ -35,8 +35,8 @@ class Path;
 class GameSettingsScreen : public UITabbedBaseDialogScreen {
 public:
 	GameSettingsScreen(const Path &gamePath, std::string gameID = "", bool editThenRestore = false);
+	~GameSettingsScreen();
 
-	void onFinish(DialogResult result) override;
 	const char *tag() const override { return "GameSettings"; }
 
 protected:
@@ -118,7 +118,7 @@ private:
 	bool analogSpeedMapped_ = false;
 
 	// edit the game-specific settings and restore the global settings after exiting
-	bool editThenRestore_ = false;
+	bool editGameSpecificThenRestore_ = false;
 
 	// Android-only
 	std::string pendingMemstickFolder_;

UI/PauseScreen.cpp

@@ -808,7 +808,6 @@ void GamePauseScreen::OnCreateConfig(UI::EventParams &e) {
 	if (info->Ready(GameInfoFlags::PARAM_SFO)) {
 		std::string gameId = info->id;
 		g_Config.CreateGameConfig(gameId);
-		g_Config.ChangeGameSpecific(gameId, info->GetTitle());
 		g_Config.SaveGameConfig(gameId, info->GetTitle());
 		if (info) {
 			info->hasConfig = true;

UI/SimpleDialogScreen.cpp

@@ -52,7 +52,7 @@ void UITwoPaneBaseDialogScreen::CreateViews() {
 		LinearLayout *root = new LinearLayout(ORIENT_VERTICAL, new LayoutParams(FILL_PARENT, FILL_PARENT));
 
 		TopBarFlags topBarFlags = TopBarFlags::Portrait;
-		if (flags_ & TwoPaneFlags::SettingsInContextMenu) {
+		if (flags_ & (TwoPaneFlags::SettingsInContextMenu | TwoPaneFlags::CustomContextMenu)) {
 			topBarFlags |= TopBarFlags::ContextMenuButton;
 		}
 		TopBar *topBar = root->Add(new TopBar(*screenManager()->getUIContext(), topBarFlags, GetTitle()));
@@ -62,7 +62,11 @@ void UITwoPaneBaseDialogScreen::CreateViews() {
 		if (flags_ & TwoPaneFlags::SettingsInContextMenu) {
 			topBar->OnContextMenuClick.Add([this](UI::EventParams &e) {
 				this->screenManager()->push(new PopupCallbackScreen([this](UI::ViewGroup *parent) {
-					CreateSettingsViews(parent);
+					if (flags_ & TwoPaneFlags::CustomContextMenu) {
+						CreateContextMenu(parent);
+					} else {
+						CreateSettingsViews(parent);
+					}
 				}, nullptr));
 			});
 		} else {

UI/SimpleDialogScreen.h

@@ -30,6 +30,7 @@ enum class TwoPaneFlags {
 	SettingsInContextMenu = 2,
 	SettingsCanScroll = 4,
 	ContentsCanScroll = 8,
+	CustomContextMenu = 16,
 };
 ENUM_CLASS_BITOPS(TwoPaneFlags);
 
@@ -48,6 +49,7 @@ public:
 	virtual void BeforeCreateViews() {}  // If something needs to happen before both settings and contents, this is a good place.
 	virtual void CreateSettingsViews(UI::ViewGroup *parent) = 0;
 	virtual void CreateContentViews(UI::ViewGroup *parent) = 0;
+	virtual void CreateContextMenu(UI::ViewGroup *parent) {}  // only called if CustomContextMenu is set in flags.
 	virtual std::string_view GetTitle() const { return ""; }
 	virtual float SettingsWidth() const { return 350.0f; }