From b3034859ff56d85e1804ab2da465786904873a84 Mon Sep 17 00:00:00 2001 From: SerhiiGadzhilov <71632867+SerhiiGadzhilov@users.noreply.github.com> Date: Thu, 4 Feb 2021 20:10:27 +0300 Subject: [PATCH] SYNERGY-677 Application loses settings (#6933) * SYNERGY-677 Application loses settings * SYNERGY-677 Update ChangeLog * SYNERGY-677 Code smells fix. --- ChangeLog | 1 + src/gui/src/AppConfig.cpp | 91 +++--- src/gui/src/AppConfig.h | 14 +- src/gui/src/ConfigWriter.cpp | 24 +- src/gui/src/ConfigWriter.h | 4 + src/gui/src/MainWindow.cpp | 5 +- src/gui/src/MainWindow.h | 1 - src/gui/src/SettingsDialog.cpp | 37 ++- src/gui/src/SettingsDialog.h | 3 + src/gui/src/SettingsDialogBase.ui | 445 +++++++++++++++--------------- 10 files changed, 355 insertions(+), 270 deletions(-) diff --git a/ChangeLog b/ChangeLog index b1aef048f..af776ff55 100644 --- a/ChangeLog +++ b/ChangeLog @@ -10,6 +10,7 @@ Bug fixes: - #6921 Handling pre-main window creation status notifications - #6922 macOS CI build - #6927 Text copied and pasted between Windows and Mac OS converts to japanese +- #6933 Application loses settings Enhancements: - #6912 Removes UI for Screen Saver Sync and Files Drag and Drop diff --git a/src/gui/src/AppConfig.cpp b/src/gui/src/AppConfig.cpp index 99ba5aa8a..fbc46dea3 100644 --- a/src/gui/src/AppConfig.cpp +++ b/src/gui/src/AppConfig.cpp @@ -28,6 +28,7 @@ #include "ConfigWriter.h" #include "SslCertificate.h" +using GUI::Config::ConfigWriter; #if defined(Q_OS_WIN) const char AppConfig::m_SynergysName[] = "synergys.exe"; const char AppConfig::m_SynergycName[] = "synergyc.exe"; @@ -111,30 +112,20 @@ AppConfig::AppConfig() : m_LoadFromSystemScope() { - using GUI::Config::ConfigWriter; - auto writer = ConfigWriter::make(); //Register this class to receive global load and saves writer->registerClass(this); - //User settings exist and the load from system scope variable is true - if (writer->hasSetting(settingName(kLoadSystemSettings), ConfigWriter::kUser) && - writer->loadSetting(settingName(kLoadSystemSettings), false, ConfigWriter::kUser).toBool()) - { - writer->setScope(ConfigWriter::kSystem); - } - //If user setting don't exist but system ones do, load the system settings - else if (!writer->hasSetting(settingName(kScreenName), ConfigWriter::kUser) && - writer->hasSetting(settingName(kScreenName), ConfigWriter::kSystem)) - { - writer->setScope(ConfigWriter::kSystem); - } else { // Otherwise just load to user scope - writer->setScope(ConfigWriter::kUser); - } - - //Notify registered classes to reload writer->globalLoad(); + //User settings exist and the load from system scope variable is true + if (writer->hasSetting(settingName(kLoadSystemSettings), ConfigWriter::kUser)) { + setLoadFromSystemScope(m_LoadFromSystemScope); + } + //If user setting don't exist but system ones do, load the system settings + else if (writer->hasSetting(settingName(kScreenName), ConfigWriter::kSystem)) { + setLoadFromSystemScope(true); + } } const QString &AppConfig::screenName() const { return m_ScreenName; } @@ -214,7 +205,7 @@ void AppConfig::loadSettings() m_LogLevel = loadSetting(kLogLevel, 0).toInt(); m_LogToFile = loadSetting(kLogToFile, false).toBool(); m_LogFilename = loadSetting(kLogFilename, synergyLogDir() + "synergy.log").toString(); - m_WizardLastRun = loadSetting(kWizardLastRun, 0).toInt(); + m_WizardLastRun = loadCommonSetting(kWizardLastRun, 0).toInt(); m_Language = loadSetting(kLanguage, QLocale::system().name()).toString(); m_StartedBefore = loadSetting(kStartedBefore, false).toBool(); m_AutoConfig = loadSetting(kAutoConfig, false).toBool(); @@ -237,7 +228,7 @@ void AppConfig::loadSettings() m_LastExpiringWarningTime = loadSetting(kLastExpireWarningTime, 0).toInt(); m_ActivationHasRun = loadSetting(kActivationHasRun, false).toBool(); m_MinimizeToTray = loadSetting(kMinimizeToTray, false).toBool(); - m_LoadFromSystemScope = loadSetting(kLoadSystemSettings, false).toBool(); + m_LoadFromSystemScope = loadCommonSetting(kLoadSystemSettings, false).toBool(); m_ServerGroupChecked = loadSetting(kGroupServerCheck, false).toBool(); m_UseExternalConfig = loadSetting(kUseExternalConfig, false).toBool(); m_ConfigFile = loadSetting(kConfigFile, QDir::homePath() + "/" + synergyConfigName).toString(); @@ -246,8 +237,8 @@ void AppConfig::loadSettings() m_ServerHostname = loadSetting(kServerHostname).toString(); //only change the serial key if the settings being loaded contains a key - bool updateSerial = GUI::Config::ConfigWriter::make() - ->hasSetting(settingName(kLoadSystemSettings),GUI::Config::ConfigWriter::kCurrent); + bool updateSerial = ConfigWriter::make() + ->hasSetting(settingName(kLoadSystemSettings),ConfigWriter::kCurrent); //if the setting exists and is not empty updateSerial = updateSerial && !loadSetting(kSerialKey, "").toString().trimmed().isEmpty(); @@ -272,13 +263,14 @@ void AppConfig::loadSettings() void AppConfig::saveSettings() { + setCommonSetting(kWizardLastRun, kWizardVersion); + setCommonSetting(kLoadSystemSettings, m_LoadFromSystemScope); setSetting(kScreenName, m_ScreenName); setSetting(kPort, m_Port); setSetting(kInterfaceSetting, m_Interface); setSetting(kLogLevel, m_LogLevel); setSetting(kLogToFile, m_LogToFile); setSetting(kLogFilename, m_LogFilename); - setSetting(kWizardLastRun, kWizardVersion); setSetting(kLanguage, m_Language); setSetting(kStartedBefore, m_StartedBefore); setSetting(kAutoConfig, m_AutoConfig); @@ -295,7 +287,6 @@ void AppConfig::saveSettings() setSetting(kLastExpireWarningTime, m_LastExpiringWarningTime); setSetting(kActivationHasRun, m_ActivationHasRun); setSetting(kMinimizeToTray, m_MinimizeToTray); - setSetting(kLoadSystemSettings, m_LoadFromSystemScope); setSetting(kGroupServerCheck, m_ServerGroupChecked); setSetting(kUseExternalConfig, m_UseExternalConfig); setSetting(kConfigFile, m_ConfigFile); @@ -383,12 +374,14 @@ void AppConfig::setAutoConfigServer(const QString& autoConfigServer) #ifndef SYNERGY_ENTERPRISE void AppConfig::setEdition(Edition e) { setSettingModified(m_Edition, e); + setCommonSetting(kEditionSetting, m_Edition); } Edition AppConfig::edition() const { return m_Edition; } void AppConfig::setSerialKey(const QString& serial) { setSettingModified(m_Serialkey, serial); + setCommonSetting(kSerialKey, m_Serialkey); } void AppConfig::clearSerialKey() @@ -456,17 +449,41 @@ QString AppConfig::settingName(AppConfig::Setting name) { template void AppConfig::setSetting(AppConfig::Setting name, T value) { - using GUI::Config::ConfigWriter; ConfigWriter::make()->setSetting(settingName(name), value); } +template +void AppConfig::setCommonSetting(AppConfig::Setting name, T value) { + ConfigWriter::make()->setSetting(settingName(name), value, ConfigWriter::kUser); + ConfigWriter::make()->setSetting(settingName(name), value, ConfigWriter::kSystem); +} + QVariant AppConfig::loadSetting(AppConfig::Setting name, const QVariant& defaultValue) { - using GUI::Config::ConfigWriter; return ConfigWriter::make()->loadSetting(settingName(name), defaultValue); } -void AppConfig::loadScope(GUI::Config::ConfigWriter::Scope scope) const { - auto writer = GUI::Config::ConfigWriter::make(); +QVariant AppConfig::loadCommonSetting(AppConfig::Setting name, const QVariant& defaultValue) const { + QVariant result(defaultValue); + QString setting(settingName(name)); + auto& writer = *ConfigWriter::make(); + + if (writer.hasSetting(setting)) { + result = writer.loadSetting(setting, defaultValue); + } + else if (writer.getScope() == ConfigWriter::kSystem ) { + if (writer.hasSetting(setting, ConfigWriter::kUser)) { + result = writer.loadSetting(setting, defaultValue, ConfigWriter::kUser); + } + } + else if (writer.hasSetting(setting, ConfigWriter::kSystem)){ + result = writer.loadSetting(setting, defaultValue, ConfigWriter::kSystem); + } + + return result; +} + +void AppConfig::loadScope(ConfigWriter::Scope scope) const { + auto writer = ConfigWriter::make(); if (writer->getScope() != scope) { writer->setScope(scope); @@ -480,18 +497,10 @@ void AppConfig::loadScope(GUI::Config::ConfigWriter::Scope scope) const { void AppConfig::setLoadFromSystemScope(bool value) { 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); + loadScope(ConfigWriter::kSystem); } else { - loadScope(GUI::Config::ConfigWriter::kUser); + loadScope(ConfigWriter::kUser); } /* @@ -501,8 +510,12 @@ void AppConfig::setLoadFromSystemScope(bool value) { m_LoadFromSystemScope = value; } +bool AppConfig::isWritable() const { + return ConfigWriter::make()->isWritable(); +} + bool AppConfig::isSystemScoped() const { - return GUI::Config::ConfigWriter::make()->getScope() == GUI::Config::ConfigWriter::kSystem; + return ConfigWriter::make()->getScope() == ConfigWriter::kSystem; } bool AppConfig::getServerGroupChecked() const { diff --git a/src/gui/src/AppConfig.h b/src/gui/src/AppConfig.h index 809d17c7f..cba0089b6 100644 --- a/src/gui/src/AppConfig.h +++ b/src/gui/src/AppConfig.h @@ -67,7 +67,7 @@ class AppConfig: public QObject, public GUI::Config::ConfigBase AppConfig(); public: - + bool isWritable() const; bool isSystemScoped() const; const QString& screenName() const; @@ -273,11 +273,23 @@ protected: template void setSetting(AppConfig::Setting name, T value); + /// @brief Sets the value of a common setting + /// which should have the same value for all scopes + /// @param [in] name The Setting to be saved + /// @param [in] value The Value to be saved + template + void setCommonSetting(AppConfig::Setting name, T value); + /// @brief Loads a setting /// @param [in] name The setting to be loaded /// @param [in] defaultValue The default value of the setting QVariant loadSetting(AppConfig::Setting name, const QVariant& defaultValue = QVariant()); + /// @brief Loads a common setting + /// @param [in] name The setting to be loaded + /// @param [in] defaultValue The default value of the setting + QVariant loadCommonSetting(AppConfig::Setting name, const QVariant& defaultValue = QVariant()) const; + /// @brief As the settings will be accessible by multiple objects this lock will ensure that /// it cant be modified by more that one object at a time if the setting is being switched /// from system to user. diff --git a/src/gui/src/ConfigWriter.cpp b/src/gui/src/ConfigWriter.cpp index 8ba75af5f..2ceed673e 100644 --- a/src/gui/src/ConfigWriter.cpp +++ b/src/gui/src/ConfigWriter.cpp @@ -18,8 +18,7 @@ #include #include -#include -#include +#include #include "ConfigWriter.h" #include "ConfigBase.h" @@ -38,6 +37,18 @@ namespace GUI { return s_pConfiguration; } + void loadOldSystemSettings(QSettings& settings) + { + if (!QFile(settings.fileName()).exists()) { + QFile oldSystemSettings("SystemConfig.ini"); + if (oldSystemSettings.exists()) { + QSettings oldSettings(oldSystemSettings.fileName(), QSettings::Format::IniFormat); + for (const auto& key : oldSettings.allKeys()) { + settings.setValue(key, oldSettings.value(key)); + } + } + } + } ConfigWriter::ConfigWriter() { QSettings::setPath(QSettings::Format::IniFormat, @@ -51,6 +62,9 @@ namespace GUI { QCoreApplication::organizationName(), QCoreApplication::applicationName()); + //This call is needed for backwardcapability with old settings. + loadOldSystemSettings(*m_pSettingsSystem); + //defaults to user scope, if we set the scope specifically then we also have to set // the application name and the organisation name which breaks backwards compatibility // See #6730 @@ -86,7 +100,9 @@ namespace GUI { } } - + bool ConfigWriter::isWritable() const { + return m_pSettingsCurrent->isWritable(); + } QVariant ConfigWriter::loadSetting(const QString& name, const QVariant &defaultValue, Scope scope) { switch (scope){ @@ -151,7 +167,7 @@ namespace GUI { QString path; #if defined(Q_OS_WIN) // Program file - path = ""; + path = QCoreApplication::applicationDirPath() + "\\"; #elif defined(Q_OS_DARWIN) //Global preferances dir // Would be nice to use /library, but QT has no elevate system in place diff --git a/src/gui/src/ConfigWriter.h b/src/gui/src/ConfigWriter.h index 94da69f9e..8eae8e73b 100644 --- a/src/gui/src/ConfigWriter.h +++ b/src/gui/src/ConfigWriter.h @@ -51,6 +51,10 @@ namespace GUI { /// @return bool True if the current scope has the named setting bool hasSetting(const QString& name, Scope scope = kCurrent) const; + /// @brief Checks if the current scope settings writable + /// @return bool True if the current scope writable + bool isWritable() const; + /// @brief Sets the value of a setting /// @param [in] name The Setting to be saved /// @param [in] value The Value to be saved (Templated) diff --git a/src/gui/src/MainWindow.cpp b/src/gui/src/MainWindow.cpp index a582a873d..8538e8aa7 100644 --- a/src/gui/src/MainWindow.cpp +++ b/src/gui/src/MainWindow.cpp @@ -120,7 +120,6 @@ MainWindow::MainWindow (AppConfig& appConfig, m_pMenuHelp(NULL), m_pCancelButton(NULL), m_ExpectedRunningState(kStopped), - m_pSslCertificate(NULL), m_SecureSocket(false) { #ifndef SYNERGY_ENTERPRISE @@ -222,8 +221,6 @@ MainWindow::~MainWindow() #ifndef SYNERGY_ENTERPRISE delete m_pZeroconf; #endif - - delete m_pSslCertificate; } void MainWindow::open() @@ -1412,4 +1409,4 @@ void MainWindow::windowStateChanged() void MainWindow::updateScreenName() { m_pLabelScreenName->setText(getScreenName()); -} \ No newline at end of file +} diff --git a/src/gui/src/MainWindow.h b/src/gui/src/MainWindow.h index 15f575b6d..0ef3f2d35 100644 --- a/src/gui/src/MainWindow.h +++ b/src/gui/src/MainWindow.h @@ -236,7 +236,6 @@ public slots: TrayIcon m_trayIcon; qRuningState m_ExpectedRunningState; QMutex m_StopDesktopMutex; - SslCertificate* m_pSslCertificate; bool m_SecureSocket; // brief Is the program running a secure socket protocol (SSL/TLS) QString m_SecureSocketVersion; // brief Contains the version of the Secure Socket currently active diff --git a/src/gui/src/SettingsDialog.cpp b/src/gui/src/SettingsDialog.cpp index a207a1b4c..e25da8c7e 100644 --- a/src/gui/src/SettingsDialog.cpp +++ b/src/gui/src/SettingsDialog.cpp @@ -45,10 +45,11 @@ 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(); + m_isSystemAtStart = appConfig().isSystemScoped(); + buttonBox->button(QDialogButtonBox::Save)->setEnabled(false); + enableControls(appConfig().isWritable()); connect(m_pLineEditLogFilename, SIGNAL(textChanged(const QString&)), this, SLOT(onChange())); connect(m_pComboLogLevel, SIGNAL(currentIndexChanged(int)), this, SLOT(onChange())); @@ -250,6 +251,7 @@ void SettingsDialog::on_m_pRadioSystemScope_toggled(bool checked) appConfig().setLoadFromSystemScope(checked); loadFromConfig(); buttonBox->button(QDialogButtonBox::Save)->setEnabled(m_isSystemAtStart != checked); + enableControls(appConfig().isWritable()); } void SettingsDialog::on_m_pPushButtonBrowseCert_clicked() { @@ -318,6 +320,37 @@ bool SettingsDialog::isModified() ); } +void SettingsDialog::enableControls(bool enable) { + m_pLineEditScreenName->setEnabled(enable); + m_pSpinBoxPort->setEnabled(enable); + m_pLineEditInterface->setEnabled(enable); + m_pComboLogLevel->setEnabled(enable); + m_pCheckBoxLogToFile->setEnabled(enable); + m_pComboLanguage->setEnabled(enable); + m_pComboElevate->setEnabled(enable); + m_pCheckBoxAutoHide->setEnabled(enable); + m_pCheckBoxAutoConfig->setEnabled(enable); + m_pCheckBoxMinimizeToTray->setEnabled(enable); + m_pLineEditCertificatePath->setEnabled(enable); + m_pComboBoxKeyLength->setEnabled(enable); + m_pPushButtonBrowseCert->setEnabled(enable); + m_labelAdminRightsMessage->setVisible(!enable); + + if (enable) { + m_pLineEditLogFilename->setEnabled(m_pCheckBoxLogToFile->isChecked()); + m_pButtonBrowseLog->setEnabled(m_pCheckBoxLogToFile->isChecked()); + m_pCheckBoxEnableCrypto->setEnabled(m_appConfig.isCryptoAvailable()); + updateRegenButton(); + } + else { + m_pLineEditLogFilename->setEnabled(enable); + m_pButtonBrowseLog->setEnabled(enable); + m_pCheckBoxEnableCrypto->setEnabled(enable); + m_pPushButtonRegenCert->setEnabled(enable); + } + +} + void SettingsDialog::onChange() { buttonBox->button(QDialogButtonBox::Save)->setEnabled(isModified()); diff --git a/src/gui/src/SettingsDialog.h b/src/gui/src/SettingsDialog.h index afdde8a33..7345d546a 100644 --- a/src/gui/src/SettingsDialog.h +++ b/src/gui/src/SettingsDialog.h @@ -59,6 +59,9 @@ class SettingsDialog : public QDialog, public Ui::SettingsDialogBase /// @return true if there are modifications. bool isModified(); + /// @brief Enables\disables all controls. + void enableControls(bool enabled); + private: MainWindow* m_pMainWindow; AppConfig& m_appConfig; diff --git a/src/gui/src/SettingsDialogBase.ui b/src/gui/src/SettingsDialogBase.ui index a54d82310..95ad447a4 100644 --- a/src/gui/src/SettingsDialogBase.ui +++ b/src/gui/src/SettingsDialogBase.ui @@ -7,7 +7,7 @@ 0 0 378 - 756 + 770 @@ -40,7 +40,226 @@ - + + + + Qt::Horizontal + + + QDialogButtonBox::Cancel|QDialogButtonBox::Save + + + + + + + TLS/SSL Settings + + + + + + + + + Key length + + + + + + + Certificate Path + + + + + + + Browse + + + + + + + 1024 + + + + 1024 + + + + + 2048 + + + + + 4096 + + + + + + + + Regenerate Cert + + + + + + + + + + Qt::Vertical + + + QSizePolicy::Minimum + + + + 20 + 10 + + + + + + + + Qt::Vertical + + + QSizePolicy::MinimumExpanding + + + + 20 + 10 + + + + + + + + Qt::Vertical + + + QSizePolicy::Minimum + + + + 20 + 10 + + + + + + + + Qt::Vertical + + + QSizePolicy::Minimum + + + + 20 + 10 + + + + + + + + true + + + + 0 + 0 + + + + &Network + + + + 2 + + + 12 + + + 2 + + + 12 + + + + + 0 + + + 12 + + + + + <html><head/><body><p><a href="https://symless.com/account?source=gui&amp;intent=upgrade"><span style=" text-decoration: underline; color:#007af4;">Upgrade to Pro</span></a></p></body></html> + + + Qt::RichText + + + true + + + + + + + <html><head/><body><p><a href="#"><span style=" text-decoration: underline; color:#007af4;">Install Bonjour</span></a></p></body></html> + + + Qt::RichText + + + + + + + false + + + Enable Auto Config + + + + + + + false + + + Enable &TLS Encryption + + + + + + + + + &Miscellaneous @@ -189,200 +408,7 @@ - - - - Qt::Vertical - - - QSizePolicy::Minimum - - - - 20 - 10 - - - - - - - - true - - - - 0 - 0 - - - - &Network - - - - 2 - - - 12 - - - 2 - - - 12 - - - - - 0 - - - 12 - - - - - <html><head/><body><p><a href="https://symless.com/account?source=gui&amp;intent=upgrade"><span style=" text-decoration: underline; color:#007af4;">Upgrade to Pro</span></a></p></body></html> - - - Qt::RichText - - - true - - - - - - - <html><head/><body><p><a href="#"><span style=" text-decoration: underline; color:#007af4;">Install Bonjour</span></a></p></body></html> - - - Qt::RichText - - - - - - - false - - - Enable Auto Config - - - - - - - false - - - Enable &TLS Encryption - - - - - - - - - - - - Qt::Vertical - - - QSizePolicy::Minimum - - - - 20 - 10 - - - - - - - - TLS/SSL Settings - - - - - - - - - Key length - - - - - - - Certificate Path - - - - - - - Browse - - - - - - - 2048 - - - - 1024 - - - - - 2048 - - - - - 4096 - - - - - - - - Regenerate Cert - - - - - - - - - - Qt::Vertical - - - QSizePolicy::Minimum - - - - 20 - 10 - - - - - + @@ -467,29 +493,10 @@ - - - - Qt::Vertical - - - QSizePolicy::MinimumExpanding - - - - 20 - 10 - - - - - - - - Qt::Horizontal - - - QDialogButtonBox::Cancel|QDialogButtonBox::Save + + + + <html><head/><body><p><span style=" font-size:7pt; font-weight:600;">To edit settings for all users, Admin privileges are required.</span></p></body></html>