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 <serhii-external@symless.com>
Co-authored-by: SerhiiGadzhilov <71632867+SerhiiGadzhilov@users.noreply.github.com>

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:

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)
+   /*
-    {
+    * It's very imprortant to set this variable after loadScope
-        m_LoadFromSystemScope = value;
+    * because during scope loading this variable can be rewritten with old value
-        m_unsavedChanges = true;
+   */
-        writer->globalSave();     //Save user prefs
+   m_LoadFromSystemScope = value;
-        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
-    }
 }
 
 bool AppConfig::isSystemScoped() const {

src/gui/src/AppConfig.h

@@ -27,6 +27,7 @@
 #include <shared/EditionType.h>
 #include <mutex>
 #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
+        ///             True - This will set the variable and load the global scope settings.
-        ///             False - This will load the UserScope then set the variable and save.
+        ///             False - This will set the variable and load the user scope settings.
         void setLoadFromSystemScope(bool value);
 
 
@@ -290,6 +290,10 @@ protected:
         template <typename T>
         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();

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();
+               for (auto &i : m_pCallerList) {
-
+                   i->saveSettings();
-                switch (choice) {
+               }
-                    case kSaveToUser:
+               m_unsavedChanges = false;
-                        //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;
-                }
             }
         }
 
@@ -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;
-        }
     }
 }

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

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() {

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 <http://www.gnu.org/licenses/>.
  */
-
 #include "SettingsDialog.h"
 
 #include "CoreInterface.h"
@@ -34,8 +33,6 @@
 #include <QFileDialog>
 #include <QDir>
 
-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<MainWindow*>(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().setLoadFromSystemScope(m_pRadioSystemScope->isChecked());
-    appConfig().setPort(m_pSpinBoxPort->value());
+   appConfig().setScreenName(m_pLineEditScreenName->text());
-    appConfig().setNetworkInterface(m_pLineEditInterface->text());
+   appConfig().setPort(m_pSpinBoxPort->value());
-    appConfig().setLogLevel(m_pComboLogLevel->currentIndex());
+   appConfig().setNetworkInterface(m_pLineEditInterface->text());
-    appConfig().setLogToFile(m_pCheckBoxLogToFile->isChecked());
+   appConfig().setLogLevel(m_pComboLogLevel->currentIndex());
-    appConfig().setLogFilename(m_pLineEditLogFilename->text());
+   appConfig().setLogToFile(m_pCheckBoxLogToFile->isChecked());
-    appConfig().setLanguage(m_pComboLanguage->itemData(m_pComboLanguage->currentIndex()).toString());
+   appConfig().setLogFilename(m_pLineEditLogFilename->text());
-    appConfig().setElevateMode(static_cast<ElevateMode>(m_pComboElevate->currentIndex()));
+   appConfig().setLanguage(m_pComboLanguage->itemData(m_pComboLanguage->currentIndex()).toString());
-    appConfig().setAutoHide(m_pCheckBoxAutoHide->isChecked());
+   appConfig().setElevateMode(static_cast<ElevateMode>(m_pComboElevate->currentIndex()));
-    appConfig().setAutoConfig(m_pCheckBoxAutoConfig->isChecked());
+   appConfig().setAutoHide(m_pCheckBoxAutoHide->isChecked());
-    appConfig().setMinimizeToTray(m_pCheckBoxMinimizeToTray->isChecked());
+   appConfig().setAutoConfig(m_pCheckBoxAutoConfig->isChecked());
-    appConfig().setTLSCertPath(m_pLineEditCertificatePath->text());
+   appConfig().setMinimizeToTray(m_pCheckBoxMinimizeToTray->isChecked());
-    appConfig().setTLSKeyLength(m_pComboBoxKeyLength->currentText());
+   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().saveSettings();
-    appConfig().setLoadFromSystemScope(m_pRadioSystemScope->isChecked());
+   QDialog::accept();
-    m_appConfig.setCryptoEnabled(m_pCheckBoxEnableCrypto->isChecked());
-
-    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<ElevateMode>(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());
+}

src/gui/src/SettingsDialog.h

@@ -40,9 +40,9 @@ class SettingsDialog : public QDialog, public Ui::SettingsDialogBase
         void allowAutoConfig();
 
     protected:
-        void accept();
+        void accept() override;
-        void reject();
+        void reject() override;
-        void changeEvent(QEvent* event);
+        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

src/gui/src/SettingsDialogBase.ui

@@ -17,13 +17,13 @@
    <item row="0" column="0">
     <widget class="QGroupBox" name="m_pGroupScope">
      <property name="title">
-      <string>&amp;Settings Scope</string>
+      <string>Use &amp;settings profile from:</string>
      </property>
      <layout class="QGridLayout" name="gridLayout_4">
       <item row="1" column="0">
        <widget class="QRadioButton" name="m_pRadioSystemScope">
         <property name="text">
-         <string>System</string>
+         <string>All users</string>
         </property>
         <property name="checked">
          <bool>true</bool>
@@ -33,7 +33,7 @@
       <item row="1" column="1">
        <widget class="QRadioButton" name="m_pRadioUserScope">
         <property name="text">
-         <string>User</string>
+         <string>Current user</string>
         </property>
        </widget>
       </item>
@@ -489,7 +489,7 @@
       <enum>Qt::Horizontal</enum>
      </property>
      <property name="standardButtons">
-      <set>QDialogButtonBox::Cancel|QDialogButtonBox::Ok</set>
+      <set>QDialogButtonBox::Cancel|QDialogButtonBox::Save</set>
      </property>
     </widget>
    </item>

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();
 }