GH-992 Add a transaction/locking mechanism to settings objects

This can cut the FTB loading by ~66% - worth it, but not ideal.
Real solution will have to be implemented later.

application/ConsoleWindow.cpp

@@ -138,7 +138,7 @@ ConsoleWindow::ConsoleWindow(BaseProcess *process, QWidget *parent)
 
 	setMayClose(false);
 
-	if (m_proc->instance()->settings().get("ShowConsole").toBool())
+	if (m_proc->instance()->settings()->get("ShowConsole").toBool())
 	{
 		show();
 	}
@@ -225,7 +225,7 @@ void ConsoleWindow::onEnded(InstancePtr instance, int code, QProcess::ExitStatus
 	bool peacefulExit = code == 0 && status != QProcess::CrashExit;
 	m_killButton->setEnabled(false);
 	setMayClose(true);
-	if (instance->settings().get("AutoCloseConsole").toBool())
+	if (instance->settings()->get("AutoCloseConsole").toBool())
 	{
 		if (peacefulExit)
 		{

application/MainWindow.cpp

@@ -1548,7 +1548,7 @@ void MainWindow::instanceActivated(QModelIndex index)
 	if (!inst)
 		return;
 
-	JavaCommon::checkJVMArgs(inst->settings().get("JvmArgs").toString(), this);
+	JavaCommon::checkJVMArgs(inst->settings()->get("JvmArgs").toString(), this);
 
 	doLaunch();
 }
@@ -1557,7 +1557,7 @@ void MainWindow::on_actionLaunchInstance_triggered()
 {
 	if (m_selectedInstance)
 	{
-		JavaCommon::checkJVMArgs(m_selectedInstance->settings().get("JvmArgs").toString(), this);
+		JavaCommon::checkJVMArgs(m_selectedInstance->settings()->get("JvmArgs").toString(), this);
 		doLaunch();
 	}
 }
@@ -1566,7 +1566,7 @@ void MainWindow::on_actionLaunchInstanceOffline_triggered()
 {
 	if (m_selectedInstance)
 	{
-		JavaCommon::checkJVMArgs(m_selectedInstance->settings().get("JvmArgs").toString(), this);
+		JavaCommon::checkJVMArgs(m_selectedInstance->settings()->get("JvmArgs").toString(), this);
 		doLaunch(false);
 	}
 }

application/pages/InstanceSettingsPage.cpp

@@ -14,7 +14,7 @@
 InstanceSettingsPage::InstanceSettingsPage(BaseInstance *inst, QWidget *parent)
 	: QWidget(parent), ui(new Ui::InstanceSettingsPage), m_instance(inst)
 {
-	m_settings = &(inst->settings());
+	m_settings = inst->settings();
 	ui->setupUi(this);
 	loadSettings();
 }

application/pages/InstanceSettingsPage.h

@@ -69,6 +69,6 @@ private slots:
 private:
 	Ui::InstanceSettingsPage *ui;
 	BaseInstance *m_instance;
-	SettingsObject *m_settings;
+	SettingsObjectPtr m_settings;
 	QObjectPtr<JavaCommon::TestCheck> checker;
 };

logic/BaseInstance.cpp

@@ -95,9 +95,9 @@ InstancePtr BaseInstance::getSharedPtr()
 	return shared_from_this();
 }
 
-SettingsObject &BaseInstance::settings() const
+SettingsObjectPtr BaseInstance::settings() const
 {
-	return *m_settings;
+	return m_settings;
 }
 
 BaseInstance::InstanceFlags BaseInstance::flags() const
@@ -212,5 +212,5 @@ QString BaseInstance::windowTitle() const
 
 QStringList BaseInstance::extraArguments() const
 {
-	return Util::Commandline::splitArgs(settings().get("JvmArgs").toString());
+	return Util::Commandline::splitArgs(settings()->get("JvmArgs").toString());
 }

logic/BaseInstance.h

@@ -132,7 +132,7 @@ public:
 	 * This settings object stores instance-specific settings.
 	 * \return A pointer to this instance's settings object.
 	 */
-	virtual SettingsObject &settings() const;
+	virtual SettingsObjectPtr settings() const;
 
 	/// returns a valid update task
 	virtual std::shared_ptr<Task> doUpdate() = 0;

logic/BaseProcess.cpp

@@ -120,7 +120,7 @@ void BaseProcess::init()
 	connect(this, SIGNAL(readyReadStandardOutput()), SLOT(on_stdOut()));
 
 	// Log prepost launch command output (can be disabled.)
-	if (m_instance->settings().get("LogPrePostOutput").toBool())
+	if (m_instance->settings()->get("LogPrePostOutput").toBool())
 	{
 		connect(&m_prepostlaunchprocess, &QProcess::readyReadStandardError, this,
 				&BaseProcess::on_prepost_stdErr);
@@ -285,7 +285,7 @@ void BaseProcess::killProcess()
 
 bool BaseProcess::preLaunch()
 {
-	QString prelaunch_cmd = m_instance->settings().get("PreLaunchCommand").toString();
+	QString prelaunch_cmd = m_instance->settings()->get("PreLaunchCommand").toString();
 	if (!prelaunch_cmd.isEmpty())
 	{
 		prelaunch_cmd = substituteVariables(prelaunch_cmd);
@@ -330,7 +330,7 @@ bool BaseProcess::preLaunch()
 }
 bool BaseProcess::postLaunch()
 {
-	QString postlaunch_cmd = m_instance->settings().get("PostExitCommand").toString();
+	QString postlaunch_cmd = m_instance->settings()->get("PostExitCommand").toString();
 	if (!postlaunch_cmd.isEmpty())
 	{
 		postlaunch_cmd = substituteVariables(postlaunch_cmd);

logic/ftb/FTBPlugin.cpp

@@ -155,21 +155,29 @@ InstancePtr loadInstance(SettingsObjectPtr globalSettings, QMap<QString, QString
 	{
 		return nullptr;
 	}
+	qDebug() << "Construction " << record.instanceDir;
 
+	SettingsObject::Lock lock(inst->settings());
 	inst->init();
+	qDebug() << "Init " << record.instanceDir;
 	inst->setGroupInitial("FTB");
+	qDebug() << "A " << record.instanceDir;
 	inst->setName(record.name);
+	qDebug() << "B " << record.instanceDir;
 	inst->setIconKey(record.iconKey);
+	qDebug() << "C " << record.instanceDir;
 	if (inst->intendedVersionId() != record.mcVersion)
 	{
 		inst->setIntendedVersionId(record.mcVersion);
 	}
+	qDebug() << "D " << record.instanceDir;
 	inst->setNotes(record.description);
+	qDebug() << "Post-Process " << record.instanceDir;
 	if (!InstanceList::continueProcessInstance(inst, InstanceList::NoCreateError, record.instanceDir, groupMap))
 	{
 		return nullptr;
 	}
-
+	qDebug() << "Final " << record.instanceDir;
 	return inst;
 }
 
@@ -217,6 +225,7 @@ InstancePtr createInstance(SettingsObjectPtr globalSettings, QMap<QString, QStri
 	inst->setIconKey(record.logo);
 	inst->setIntendedVersionId(record.mcVersion);
 	inst->setNotes(record.description);
+	qDebug() << "Post-Process " << record.instanceDir;
 	if (!InstanceList::continueProcessInstance(inst, InstanceList::NoCreateError, record.instanceDir, groupMap))
 	{
 		return nullptr;
@@ -247,6 +256,7 @@ void FTBPlugin::loadInstances(SettingsObjectPtr globalSettings, QMap<QString, QS
 		QString iconKey = record.iconKey;
 		ENV.icons()->addIcon(iconKey, iconKey, PathCombine(record.templateDir, record.logo), MMCIcon::Transient);
 		auto settingsFilePath = PathCombine(record.instanceDir, "instance.cfg");
+		qDebug() << "ICON get!";
 
 		if (QFileInfo(settingsFilePath).exists())
 		{

logic/minecraft/LegacyInstance.cpp

@@ -106,12 +106,12 @@ BaseProcess *LegacyInstance::prepareForLaunch(AuthSessionPtr account)
 	{
 		// window size
 		QString windowParams;
-		if (settings().get("LaunchMaximized").toBool())
+		if (settings()->get("LaunchMaximized").toBool())
 			windowParams = "max";
 		else
 			windowParams = QString("%1x%2")
-							   .arg(settings().get("MinecraftWinWidth").toInt())
+							   .arg(settings()->get("MinecraftWinWidth").toInt())
-							   .arg(settings().get("MinecraftWinHeight").toInt());
+							   .arg(settings()->get("MinecraftWinHeight").toInt());
 
 		QString lwjgl = QDir(m_lwjglFolderSetting->get().toString() + "/" + lwjglVersion())
 							.absolutePath();
@@ -273,14 +273,14 @@ QString LegacyInstance::intendedVersionId() const
 
 bool LegacyInstance::setIntendedVersionId(QString version)
 {
-	settings().set("IntendedJarVersion", version);
+	settings()->set("IntendedJarVersion", version);
 	setShouldUpdate(true);
 	return true;
 }
 
 bool LegacyInstance::shouldUpdate() const
 {
-	QVariant var = settings().get("ShouldUpdate");
+	QVariant var = settings()->get("ShouldUpdate");
 	if (!var.isValid() || var.toBool() == false)
 	{
 		return intendedVersionId() != currentVersionId();
@@ -290,7 +290,7 @@ bool LegacyInstance::shouldUpdate() const
 
 void LegacyInstance::setShouldUpdate(bool val)
 {
-	settings().set("ShouldUpdate", val);
+	settings()->set("ShouldUpdate", val);
 }
 
 QString LegacyInstance::defaultBaseJar() const

logic/minecraft/MinecraftProcess.cpp

@@ -117,7 +117,7 @@ QMap<QString, QString> MinecraftProcess::getVariables() const
 	out.insert("INST_ID", mcInstance->id());
 	out.insert("INST_DIR", QDir(mcInstance->instanceRoot()).absolutePath());
 	out.insert("INST_MC_DIR", QDir(mcInstance->minecraftRoot()).absolutePath());
-	out.insert("INST_JAVA", mcInstance->settings().get("JavaPath").toString());
+	out.insert("INST_JAVA", mcInstance->settings()->get("JavaPath").toString());
 	out.insert("INST_JAVA_ARGS", javaArguments().join(' '));
 	return out;
 }
@@ -141,14 +141,14 @@ QStringList MinecraftProcess::javaArguments() const
 					"minecraft.exe.heapdump");
 #endif
 
-	args << QString("-Xms%1m").arg(m_instance->settings().get("MinMemAlloc").toInt());
+	args << QString("-Xms%1m").arg(m_instance->settings()->get("MinMemAlloc").toInt());
-	args << QString("-Xmx%1m").arg(m_instance->settings().get("MaxMemAlloc").toInt());
+	args << QString("-Xmx%1m").arg(m_instance->settings()->get("MaxMemAlloc").toInt());
 
 	// No PermGen in newer java.
-	auto javaVersion = m_instance->settings().get("JavaVersion");
+	auto javaVersion = m_instance->settings()->get("JavaVersion");
 	if(Strings::naturalCompare(javaVersion.toString(), "1.8.0", Qt::CaseInsensitive) < 0)
 	{
-		auto permgen = m_instance->settings().get("PermGen").toInt();
+		auto permgen = m_instance->settings()->get("PermGen").toInt();
 		if (permgen != 64)
 		{
 			args << QString("-XX:PermSize=%1m").arg(permgen);
@@ -176,7 +176,7 @@ void MinecraftProcess::arm()
 
 	m_instance->setLastLaunch();
 
-	QString JavaPath = m_instance->settings().get("JavaPath").toString();
+	QString JavaPath = m_instance->settings()->get("JavaPath").toString();
 	emit log("Java path is:\n" + JavaPath + "\n\n");
 
 	auto realJavaPath = QStandardPaths::findExecutable(JavaPath);
@@ -191,7 +191,7 @@ void MinecraftProcess::arm()
 	{
 		QFileInfo javaInfo(realJavaPath);
 		qlonglong javaUnixTime = javaInfo.lastModified().toMSecsSinceEpoch();
-		auto storedUnixTime = m_instance->settings().get("JavaTimestamp").toLongLong();
+		auto storedUnixTime = m_instance->settings()->get("JavaTimestamp").toLongLong();
 		// if they are not the same, check!
 		if(javaUnixTime != storedUnixTime)
 		{
@@ -229,8 +229,8 @@ void MinecraftProcess::arm()
 				return;
 			}
 			emit log(tr("Java version is %1!\n").arg(version), MessageLevel::MultiMC);
-			m_instance->settings().set("JavaVersion", version);
+			m_instance->settings()->set("JavaVersion", version);
-			m_instance->settings().set("JavaTimestamp", javaUnixTime);
+			m_instance->settings()->set("JavaTimestamp", javaUnixTime);
 		}
 	}
 

logic/minecraft/OneSixInstance.cpp

@@ -170,12 +170,12 @@ BaseProcess *OneSixInstance::prepareForLaunch(AuthSessionPtr session)
 	// window size, title and state, legacy
 	{
 		QString windowParams;
-		if (settings().get("LaunchMaximized").toBool())
+		if (settings()->get("LaunchMaximized").toBool())
 			windowParams = "max";
 		else
 			windowParams = QString("%1x%2")
-							   .arg(settings().get("MinecraftWinWidth").toInt())
+							   .arg(settings()->get("MinecraftWinWidth").toInt())
-							   .arg(settings().get("MinecraftWinHeight").toInt());
+							   .arg(settings()->get("MinecraftWinHeight").toInt());
 		launchScript += "windowTitle " + windowTitle() + "\n";
 		launchScript += "windowParams " + windowParams + "\n";
 	}
@@ -260,7 +260,7 @@ std::shared_ptr<ModList> OneSixInstance::texturePackList() const
 
 bool OneSixInstance::setIntendedVersionId(QString version)
 {
-	settings().set("IntendedVersion", version);
+	settings()->set("IntendedVersion", version);
 	if(getMinecraftProfile())
 	{
 		clearProfile();
@@ -283,7 +283,7 @@ QList< Mod > OneSixInstance::getJarMods() const
 
 QString OneSixInstance::intendedVersionId() const
 {
-	return settings().get("IntendedVersion").toString();
+	return settings()->get("IntendedVersion").toString();
 }
 
 void OneSixInstance::setShouldUpdate(bool)

logic/settings/INISettingsObject.cpp

@@ -33,6 +33,20 @@ bool INISettingsObject::reload()
 	return m_ini.loadFile(m_filePath) && SettingsObject::reload();
 }
 
+void INISettingsObject::suspendSave()
+{
+	m_suspendSave = true;
+}
+
+void INISettingsObject::resumeSave()
+{
+	m_suspendSave = false;
+	if(m_doSave)
+	{
+		m_ini.saveFile(m_filePath);
+	}
+}
+
 void INISettingsObject::changeSetting(const Setting &setting, QVariant value)
 {
 	if (contains(setting.id()))
@@ -51,6 +65,18 @@ void INISettingsObject::changeSetting(const Setting &setting, QVariant value)
 			for(auto iter: setting.configKeys())
 				m_ini.remove(iter);
 		}
+		doSave();
+	}
+}
+
+void INISettingsObject::doSave()
+{
+	if(m_suspendSave)
+	{
+		m_doSave = true;
+	}
+	else
+	{
 		m_ini.saveFile(m_filePath);
 	}
 }
@@ -62,7 +88,7 @@ void INISettingsObject::resetSetting(const Setting &setting)
 	{
 		for(auto iter: setting.configKeys())
 			m_ini.remove(iter);
-		m_ini.saveFile(m_filePath);
+		doSave();
 	}
 }
 

logic/settings/INISettingsObject.h

@@ -47,15 +47,18 @@ public:
 
 	bool reload() override;
 
-protected
+	void suspendSave();
-slots:
+	void resumeSave();
+
+protected slots:
 	virtual void changeSetting(const Setting &setting, QVariant value);
 	virtual void resetSetting(const Setting &setting);
 
 protected:
 	virtual QVariant retrieveValue(const Setting &setting);
+	void doSave();
 
+protected:
 	INIFile m_ini;
-
 	QString m_filePath;
 };

logic/settings/SettingsObject.h

@@ -22,6 +22,9 @@
 #include <memory>
 
 class Setting;
+class SettingsObject;
+
+typedef std::shared_ptr<SettingsObject> SettingsObjectPtr;
 
 /*!
  * \brief The SettingsObject handles communicating settings between the application and a
@@ -38,6 +41,22 @@ class Setting;
 class SettingsObject : public QObject
 {
 	Q_OBJECT
+public:
+	class Lock
+	{
+	public:
+		Lock(SettingsObjectPtr locked)
+			:m_locked(locked)
+		{
+			m_locked->suspendSave();
+		}
+		~Lock()
+		{
+			m_locked->resumeSave();
+		}
+	private:
+		SettingsObjectPtr m_locked;
+	};
 public:
 	explicit SettingsObject(QObject *parent = 0);
 	virtual ~SettingsObject();
@@ -127,6 +146,8 @@ public:
 	 */
 	virtual bool reload();
 
+	virtual void suspendSave() = 0;
+	virtual void resumeSave() = 0;
 signals:
 	/*!
 	 * \brief Signal emitted when one of this SettingsObject object's settings changes.
@@ -184,6 +205,7 @@ protected:
 
 private:
 	QMap<QString, std::shared_ptr<Setting>> m_settings;
+protected:
+	bool m_suspendSave = false;
+	bool m_doSave = false;
 };
-
-typedef std::shared_ptr<SettingsObject> SettingsObjectPtr;