From 61f316aebf71a751f80d4fcacdf862768f2a8992 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacio=20Rodr=C3=ADguez?= Date: Tue, 2 Feb 2021 02:21:46 +0700 Subject: [PATCH] Save configuration on apply (#6907) * Save configuration on apply * obtained PR number * SYNERGY-579 Save settings during modification * SYNERGY-579 Save configuration on apply * SYNERGY-579 Fix sonar code smell Co-authored-by: Serhii Hadzhilov Co-authored-by: SerhiiGadzhilov <71632867+SerhiiGadzhilov@users.noreply.github.com> --- ChangeLog | 2 + src/gui/src/AppConfig.cpp | 63 +++++++++++----------- src/gui/src/AppConfig.h | 10 ++-- src/gui/src/ConfigWriter.cpp | 54 ++----------------- src/gui/src/ConfigWriter.h | 12 ----- src/gui/src/MainWindow.cpp | 9 ++-- src/gui/src/SettingsDialog.cpp | 88 +++++++++++++++++++++++-------- src/gui/src/SettingsDialog.h | 18 +++++-- src/gui/src/SettingsDialogBase.ui | 8 +-- src/gui/src/SslCertificate.cpp | 3 +- 10 files changed, 138 insertions(+), 129 deletions(-) diff --git a/ChangeLog b/ChangeLog index 12700031f..7fa224860 100644 --- a/ChangeLog +++ b/ChangeLog @@ -4,6 +4,7 @@ Bug fixes: - #6900 Remaining SonarCloud reported bug items - #6903 Save global settings - #6889 Systray Icon on Ubuntu Auto Start (take 2) +- #6907 Saving configuration on apply - #6914 Fix for Qt Word Wrap on Mac - #6920 Windows Installer checksums - #6921 Handling pre-main window creation status notifications @@ -17,6 +18,7 @@ Enhancements: - #6910 Don't use the word “Version” for release names because it can lead to errors during update checking. - #6918 Removing positional union initialisation + v1.13.0-stable =========== Bug fixes: diff --git a/src/gui/src/AppConfig.cpp b/src/gui/src/AppConfig.cpp index 8c1810a2a..99ba5aa8a 100644 --- a/src/gui/src/AppConfig.cpp +++ b/src/gui/src/AppConfig.cpp @@ -137,11 +137,6 @@ AppConfig::AppConfig() : } -AppConfig::~AppConfig() -{ - saveSettings(); -} - const QString &AppConfig::screenName() const { return m_ScreenName; } int AppConfig::port() const { return m_Port; } @@ -423,6 +418,9 @@ void AppConfig::setCryptoEnabled(bool newValue) { if (m_CryptoEnabled != newValue && newValue){ generateCertificate(); } + else { + emit sslToggled(); + } setSettingModified(m_CryptoEnabled, newValue); } @@ -467,35 +465,40 @@ QVariant AppConfig::loadSetting(AppConfig::Setting name, const QVariant& default return ConfigWriter::make()->loadSetting(settingName(name), defaultValue); } +void AppConfig::loadScope(GUI::Config::ConfigWriter::Scope scope) const { + auto writer = GUI::Config::ConfigWriter::make(); + + if (writer->getScope() != scope) { + writer->setScope(scope); + if (writer->hasSetting(settingName(kScreenName), writer->getScope())) { + //If the user already has settings, then load them up now. + writer->globalLoad(); + } + } +} void AppConfig::setLoadFromSystemScope(bool value) { - using GUI::Config::ConfigWriter; - auto writer = ConfigWriter::make(); + if (value) { + /* Before switching to system scope we should store + * m_LoadFromSystemScope with the new value into user scope. + * It's neccessary because constructor of ConfigWriter + * loads user scope by default and we should know in this + * scope if we should switch to system scope. + */ + m_LoadFromSystemScope = value; + saveSettings(); + loadScope(GUI::Config::ConfigWriter::kSystem); + } + else { + loadScope(GUI::Config::ConfigWriter::kUser); + } - if (value && writer->getScope() != ConfigWriter::kSystem) - { - m_LoadFromSystemScope = value; - m_unsavedChanges = true; - writer->globalSave(); //Save user prefs - writer->setScope(ConfigWriter::kSystem); //Switch the the System Scope - //If the system scope has settings, trigger a global reload, otherwise keep the current users settings - if (writer->hasSetting(settingName(kScreenName), ConfigWriter::kUser)) { - // If the system already has settings, then load them up now. - writer->globalLoad(); - } - } - else if (!value && writer->getScope() == ConfigWriter::kSystem) - { - writer->setScope(ConfigWriter::kUser); // Switch to UserScope - if (writer->hasSetting(settingName(kScreenName), ConfigWriter::kUser)) { - // If the user already has settings, then load them up now. - writer->globalLoad(); - } - m_LoadFromSystemScope = value; - m_unsavedChanges = true; - writer->globalSave(); // Save user prefs - } + /* + * It's very imprortant to set this variable after loadScope + * because during scope loading this variable can be rewritten with old value + */ + m_LoadFromSystemScope = value; } bool AppConfig::isSystemScoped() const { diff --git a/src/gui/src/AppConfig.h b/src/gui/src/AppConfig.h index c7e388825..809d17c7f 100644 --- a/src/gui/src/AppConfig.h +++ b/src/gui/src/AppConfig.h @@ -27,6 +27,7 @@ #include #include #include "ConfigBase.h" +#include "ConfigWriter.h" #include "CoreInterface.h" // this should be incremented each time a new page is added. this is @@ -64,7 +65,6 @@ class AppConfig: public QObject, public GUI::Config::ConfigBase public: AppConfig(); - ~AppConfig() override; public: @@ -116,8 +116,8 @@ class AppConfig: public QObject, public GUI::Config::ConfigBase #endif /// @brief Sets the user preference to load from SystemScope. /// @param [in] value - /// True - This will set the variable, and save the user settings before loading the global scope settings - /// False - This will load the UserScope then set the variable and save. + /// True - This will set the variable and load the global scope settings. + /// False - This will set the variable and load the user scope settings. void setLoadFromSystemScope(bool value); @@ -290,6 +290,10 @@ protected: template void setSettingModified(T& variable,const T& newValue); + /// @brief This method loads config from specified scope + /// @param [in] scope which should be loaded. + void loadScope(GUI::Config::ConfigWriter::Scope scope) const; + signals: void sslToggled() const; void zeroConfToggled(); diff --git a/src/gui/src/ConfigWriter.cpp b/src/gui/src/ConfigWriter.cpp index f85f35c6d..8ba75af5f 100644 --- a/src/gui/src/ConfigWriter.cpp +++ b/src/gui/src/ConfigWriter.cpp @@ -131,22 +131,10 @@ namespace GUI { //Save if there are any unsaved changes otherwise skip if (unsavedChanges()) { - auto choice = checkSystemSave(); - - switch (choice) { - case kSaveToUser: - //Switch to local and overrun into the save case without reloading - m_CurrentScope = kUser; - m_pSettingsCurrent = m_pSettingsUser; - case kSave: - for (auto &i : m_pCallerList) { - i->saveSettings(); - } - save(); - break; - default: - break; - } + for (auto &i : m_pCallerList) { + i->saveSettings(); + } + m_unsavedChanges = false; } } @@ -196,37 +184,5 @@ namespace GUI { void ConfigWriter::markUnsaved() { m_unsavedChanges = true; } - - ConfigWriter::SaveChoice ConfigWriter::checkSystemSave() const { - if (m_CurrentScope == kSystem) { - - QMessageBox query; - query.setWindowTitle(tr("Save global settings.")); - query.setText(tr("This will overwrite the settings of anybody else that uses this computer.")); - - query.addButton(tr("Save for all users"), QMessageBox::ActionRole); - const auto* pBtnCancel = query.addButton(tr("Do not save"), QMessageBox::ActionRole); - const auto* pBtnSaveLocal = query.addButton(tr("Save to user"), QMessageBox::ActionRole); - - query.setDefaultButton(QMessageBox::Cancel); - - query.exec(); - - if(query.clickedButton() == pBtnSaveLocal) - { - return kSaveToUser; - } - else if(query.clickedButton() == pBtnCancel) - { - return kCancel; - } - } - return kSave; - } - - void ConfigWriter::save() { - m_pSettingsCurrent->sync(); - m_unsavedChanges = false; - } } -} \ No newline at end of file +} diff --git a/src/gui/src/ConfigWriter.h b/src/gui/src/ConfigWriter.h index f7817ef18..94da69f9e 100644 --- a/src/gui/src/ConfigWriter.h +++ b/src/gui/src/ConfigWriter.h @@ -45,9 +45,6 @@ namespace GUI { ///@brief An Enumeration of all the scopes available enum Scope { kCurrent, kSystem, kUser}; - /// @brief The choice selected when saving. - enum SaveChoice { kSave, kCancel, kSaveToUser}; - /// @brief Checks if the setting exists /// @param [in] name The name of the setting to check /// @param [in] scope The scope to search in @@ -81,9 +78,6 @@ namespace GUI { /// @brief trigger a config save across all registered classes void globalSave(); - /// @brief Saves the settings to file - void save(); - /// @brief Returns the current scopes settings object /// If more specialize control into the settings is needed this can provide /// direct access to the settings file handler @@ -101,12 +95,6 @@ namespace GUI { /// @return bool True if any registered class has unsaved changes bool unsavedChanges() const; - /// @brief If the scope is set to system, this function will query the user - /// if they want to continue saving to global scope or switch to user scope - /// if the scope is set to User the function will just return Save - /// @return SaveChoice The choice that was selected, or Save if the scope is user already - SaveChoice checkSystemSave() const; - protected: Scope m_CurrentScope = kUser; /// @brief The current scope of the settings diff --git a/src/gui/src/MainWindow.cpp b/src/gui/src/MainWindow.cpp index 128b5a92e..a582a873d 100644 --- a/src/gui/src/MainWindow.cpp +++ b/src/gui/src/MainWindow.cpp @@ -339,10 +339,13 @@ void MainWindow::saveSettings() appConfig().setConfigFile(m_pLineEditConfigFile->text()); appConfig().setServerHostname(m_pLineEditHostname->text()); - - //Save everything + /* Save everything + * ConfigWriter is a singlethon hence we should call destroy + * During destroy the ConfigWriter destroys QSetting which saves all settings to file. + * Before destroy all settings are stored only in memmory. + */ GUI::Config::ConfigWriter::make()->globalSave(); - + GUI::Config::ConfigWriter::destroy(); } void MainWindow::zeroConfToggled() { diff --git a/src/gui/src/SettingsDialog.cpp b/src/gui/src/SettingsDialog.cpp index 8adc714a9..d69a3a37f 100644 --- a/src/gui/src/SettingsDialog.cpp +++ b/src/gui/src/SettingsDialog.cpp @@ -15,7 +15,6 @@ * You should have received a copy of the GNU General Public License * along with this program. If not, see . */ - #include "SettingsDialog.h" #include "CoreInterface.h" @@ -34,8 +33,6 @@ #include #include -static const char networkSecurity[] = "ns"; - SettingsDialog::SettingsDialog(QWidget* parent, AppConfig& config) : QDialog(parent, Qt::WindowTitleHint | Qt::WindowSystemMenuHint), Ui::SettingsDialogBase(), @@ -48,31 +45,42 @@ SettingsDialog::SettingsDialog(QWidget* parent, AppConfig& config) : m_pMainWindow = dynamic_cast(parent); m_Locale.fillLanguageComboBox(m_pComboLanguage); + m_isSystemAtStart = appConfig().isSystemScoped(); + buttonBox->button(QDialogButtonBox::Save)->setEnabled(false); loadFromConfig(); + + connect(m_pLineEditLogFilename, SIGNAL(textChanged(const QString&)), this, SLOT(onChange())); + connect(m_pComboLogLevel, SIGNAL(currentIndexChanged(int)), this, SLOT(onChange())); + connect(m_pLineEditCertificatePath, SIGNAL(textChanged(const QString&)), this, SLOT(onChange())); + connect(m_pCheckBoxAutoConfig, SIGNAL(clicked()), this, SLOT(onChange())); + connect(m_pCheckBoxMinimizeToTray, SIGNAL(clicked()), this, SLOT(onChange())); + connect(m_pCheckBoxAutoHide, SIGNAL(clicked()), this, SLOT(onChange())); + connect(m_pLineEditInterface, SIGNAL(textEdited(const QString&)), this, SLOT(onChange())); + connect(m_pSpinBoxPort, SIGNAL(valueChanged(int)), this, SLOT(onChange())); + connect(m_pLineEditScreenName, SIGNAL(textEdited(const QString&)), this, SLOT(onChange())); } void SettingsDialog::accept() { - appConfig().setScreenName(m_pLineEditScreenName->text()); - appConfig().setPort(m_pSpinBoxPort->value()); - appConfig().setNetworkInterface(m_pLineEditInterface->text()); - appConfig().setLogLevel(m_pComboLogLevel->currentIndex()); - appConfig().setLogToFile(m_pCheckBoxLogToFile->isChecked()); - appConfig().setLogFilename(m_pLineEditLogFilename->text()); - appConfig().setLanguage(m_pComboLanguage->itemData(m_pComboLanguage->currentIndex()).toString()); - appConfig().setElevateMode(static_cast(m_pComboElevate->currentIndex())); - appConfig().setAutoHide(m_pCheckBoxAutoHide->isChecked()); - appConfig().setAutoConfig(m_pCheckBoxAutoConfig->isChecked()); - appConfig().setMinimizeToTray(m_pCheckBoxMinimizeToTray->isChecked()); - appConfig().setTLSCertPath(m_pLineEditCertificatePath->text()); - appConfig().setTLSKeyLength(m_pComboBoxKeyLength->currentText()); + appConfig().setLoadFromSystemScope(m_pRadioSystemScope->isChecked()); + appConfig().setScreenName(m_pLineEditScreenName->text()); + appConfig().setPort(m_pSpinBoxPort->value()); + appConfig().setNetworkInterface(m_pLineEditInterface->text()); + appConfig().setLogLevel(m_pComboLogLevel->currentIndex()); + appConfig().setLogToFile(m_pCheckBoxLogToFile->isChecked()); + appConfig().setLogFilename(m_pLineEditLogFilename->text()); + appConfig().setLanguage(m_pComboLanguage->itemData(m_pComboLanguage->currentIndex()).toString()); + appConfig().setElevateMode(static_cast(m_pComboElevate->currentIndex())); + appConfig().setAutoHide(m_pCheckBoxAutoHide->isChecked()); + appConfig().setAutoConfig(m_pCheckBoxAutoConfig->isChecked()); + appConfig().setMinimizeToTray(m_pCheckBoxMinimizeToTray->isChecked()); + appConfig().setTLSCertPath(m_pLineEditCertificatePath->text()); + appConfig().setTLSKeyLength(m_pComboBoxKeyLength->currentText()); + appConfig().setCryptoEnabled(m_pCheckBoxEnableCrypto->isChecked()); - //We only need to test the System scoped Radio as they are connected - appConfig().setLoadFromSystemScope(m_pRadioSystemScope->isChecked()); - m_appConfig.setCryptoEnabled(m_pCheckBoxEnableCrypto->isChecked()); - - QDialog::accept(); + appConfig().saveSettings(); + QDialog::accept(); } void SettingsDialog::reject() @@ -81,6 +89,11 @@ void SettingsDialog::reject() QSynergyApplication::getInstance()->switchTranslator(appConfig().language()); } + // We should restore scope at start if the user rejects changes. + if (appConfig().isSystemScoped() != m_isSystemAtStart) { + appConfig().setLoadFromSystemScope(m_isSystemAtStart); + } + QDialog::reject(); } @@ -176,7 +189,6 @@ void SettingsDialog::loadFromConfig() { adjustSize(); } - void SettingsDialog::allowAutoConfig() { m_pLabelInstallBonjour->hide(); @@ -190,6 +202,7 @@ void SettingsDialog::on_m_pCheckBoxLogToFile_stateChanged(int i) m_pLineEditLogFilename->setEnabled(checked); m_pButtonBrowseLog->setEnabled(checked); + buttonBox->button(QDialogButtonBox::Save)->setEnabled(isModified()); } void SettingsDialog::on_m_pButtonBrowseLog_clicked() @@ -209,11 +222,12 @@ void SettingsDialog::on_m_pComboLanguage_currentIndexChanged(int index) { QString ietfCode = m_pComboLanguage->itemData(index).toString(); QSynergyApplication::getInstance()->switchTranslator(ietfCode); + buttonBox->button(QDialogButtonBox::Save)->setEnabled(isModified()); } void SettingsDialog::on_m_pCheckBoxEnableCrypto_toggled(bool checked) { - m_appConfig.setCryptoEnabled(checked); + buttonBox->button(QDialogButtonBox::Save)->setEnabled(isModified()); if (checked) { verticalSpacer_4->changeSize(10, 10, QSizePolicy::Minimum); } else { @@ -231,8 +245,10 @@ void SettingsDialog::on_m_pLabelInstallBonjour_linkActivated(const QString&) void SettingsDialog::on_m_pRadioSystemScope_toggled(bool checked) { + //We only need to test the System scoped Radio as they are connected appConfig().setLoadFromSystemScope(checked); loadFromConfig(); + buttonBox->button(QDialogButtonBox::Save)->setEnabled(m_isSystemAtStart != checked); } void SettingsDialog::on_m_pPushButtonBrowseCert_clicked() { @@ -254,6 +270,7 @@ void SettingsDialog::on_m_pPushButtonBrowseCert_clicked() { } void SettingsDialog::on_m_pComboBoxKeyLength_currentIndexChanged(int index) { + buttonBox->button(QDialogButtonBox::Save)->setEnabled(isModified()); updateRegenButton(); } @@ -279,3 +296,28 @@ void SettingsDialog::updateKeyLengthOnFile(const QString &path) { //Also update what is in the appconfig to match the file itself appConfig().setTLSKeyLength(length); } + +bool SettingsDialog::isModified() +{ + return ( + appConfig().screenName() != m_pLineEditScreenName->text() + || appConfig().port() != m_pSpinBoxPort->value() + || appConfig().networkInterface() != m_pLineEditInterface->text() + || appConfig().logLevel() != m_pComboLogLevel->currentIndex() + || appConfig().logToFile() != m_pCheckBoxLogToFile->isChecked() + || appConfig().logFilename() != m_pLineEditLogFilename->text() + || appConfig().language() != m_pComboLanguage->itemData(m_pComboLanguage->currentIndex()).toString() + || appConfig().elevateMode() != static_cast(m_pComboElevate->currentIndex()) + || appConfig().getAutoHide() != m_pCheckBoxAutoHide->isChecked() + || appConfig().autoConfig() != m_pCheckBoxAutoConfig->isChecked() + || appConfig().getMinimizeToTray() != m_pCheckBoxMinimizeToTray->isChecked() + || appConfig().getTLSCertPath() != m_pLineEditCertificatePath->text() + || appConfig().getTLSKeyLength() != m_pComboBoxKeyLength->currentText() + || appConfig().getCryptoEnabled() != m_pCheckBoxEnableCrypto->isChecked() + ); +} + +void SettingsDialog::onChange() +{ + buttonBox->button(QDialogButtonBox::Save)->setEnabled(isModified()); +} diff --git a/src/gui/src/SettingsDialog.h b/src/gui/src/SettingsDialog.h index 5c8a0e85e..afdde8a33 100644 --- a/src/gui/src/SettingsDialog.h +++ b/src/gui/src/SettingsDialog.h @@ -40,9 +40,9 @@ class SettingsDialog : public QDialog, public Ui::SettingsDialogBase void allowAutoConfig(); protected: - void accept(); - void reject(); - void changeEvent(QEvent* event); + void accept() override; + void reject() override; + void changeEvent(QEvent* event) override; AppConfig& appConfig() { return m_appConfig; } /// @brief Causes the dialog to load all the settings from m_appConfig @@ -55,6 +55,10 @@ class SettingsDialog : public QDialog, public Ui::SettingsDialogBase /// @param [in] QString path The path to the file to test void updateKeyLengthOnFile(const QString& path); + /// @brief Check if there are modifications. + /// @return true if there are modifications. + bool isModified(); + private: MainWindow* m_pMainWindow; AppConfig& m_appConfig; @@ -62,6 +66,11 @@ class SettingsDialog : public QDialog, public Ui::SettingsDialogBase CoreInterface m_CoreInterface; BonjourWindows* m_pBonjourWindows; + /// @brief Stores settings scope at start of settings dialog + /// This is neccessary to restore state if user changes + /// the scope and doesn't save changes + bool m_isSystemAtStart = false; + private slots: void on_m_pCheckBoxEnableCrypto_toggled(bool checked); void on_m_pComboLanguage_currentIndexChanged(int index); @@ -83,6 +92,9 @@ class SettingsDialog : public QDialog, public Ui::SettingsDialogBase /// @brief handels the regenerate cert button event /// This will regenerate the TLS certificate as long as the settings haven't changed void on_m_pPushButtonRegenCert_clicked(); + + /// @brief This slot handles common functionality for all fields. + void onChange(); }; #endif diff --git a/src/gui/src/SettingsDialogBase.ui b/src/gui/src/SettingsDialogBase.ui index a79c59226..a54d82310 100644 --- a/src/gui/src/SettingsDialogBase.ui +++ b/src/gui/src/SettingsDialogBase.ui @@ -17,13 +17,13 @@ - &Settings Scope + Use &settings profile from: - System + All users true @@ -33,7 +33,7 @@ - User + Current user @@ -489,7 +489,7 @@ Qt::Horizontal - QDialogButtonBox::Cancel|QDialogButtonBox::Ok + QDialogButtonBox::Cancel|QDialogButtonBox::Save diff --git a/src/gui/src/SslCertificate.cpp b/src/gui/src/SslCertificate.cpp index 3e48cd732..636554e3f 100644 --- a/src/gui/src/SslCertificate.cpp +++ b/src/gui/src/SslCertificate.cpp @@ -150,11 +150,10 @@ void SslCertificate::generateCertificate(const QString& path, const QString& key return; } + generateFingerprint(pathToUse); emit info(tr("SSL certificate generated.")); } - generateFingerprint(pathToUse); - emit generateFinished(); }