common: Make SyncableObject non-copyable

QObjects are non-copyable for good reasons. SyncableObject, although
deriving from QObject, was made sort-of copyable by adding custom copy
ctor and copy assignment operator that did not make a full copy of the
underlying QObject, but just copied (most) properties. Several classes
deriving from SyncableObject then implemented their own versions of
those special member functions on top of that. This was not only
rather unexpected and intransparent behavior due to the incomplete
copy functionality, but also a most fragile hack since one had to
remember to update those functions when adding or modifying
properties.

In addition, newer compilers apply a somewhat stricter interpretation
of the C++ standard, basically enforcing the Rule of Three by omitting
implicit generation of copy ctor and/or copy assignment operating
in case certain user-defined special member functions exist. In
particular, Clang 10 started to emit several warnings (and we already
worked around a similar warning from GCC 9+ in cc21148).

Instead of adding more workarounds further obfuscating matters, remove
the relevant special member functions altogether and thus make
SyncableObject and its derivatives non-copyable as they should be.
Modify affected code to cope with this cleanly, and without abusing
unexpected behavior.

diff --git a/src/common/aliasmanager.cpp b/src/common/aliasmanager.cpp
index a439fb9..f87cd58 100644 (file)
--- a/src/common/aliasmanager.cpp
+++ b/src/common/aliasmanager.cpp
@@ -25,16 +25,6 @@
 
 #include "network.h"
 
-AliasManager& AliasManager::operator=(const AliasManager& other)
-{
-    if (this == &other)
-        return *this;
-
-    SyncableObject::operator=(other);
-    _aliases = other._aliases;
-    return *this;
-}
-
 int AliasManager::indexOf(const QString& name) const
 {
     for (int i = 0; i < _aliases.count(); i++) {

diff --git a/src/common/aliasmanager.h b/src/common/aliasmanager.h
index 937a5ab..2ae16cc 100644 (file)
--- a/src/common/aliasmanager.h
+++ b/src/common/aliasmanager.h
@@ -42,7 +42,6 @@ public:
     {
         setAllowClientUpdates(true);
     }
-    AliasManager& operator=(const AliasManager& other);
 
     struct Alias
     {

diff --git a/src/common/highlightrulemanager.cpp b/src/common/highlightrulemanager.cpp
index 37e2d86..5a62f9e 100644 (file)
--- a/src/common/highlightrulemanager.cpp
+++ b/src/common/highlightrulemanager.cpp
@@ -25,18 +25,6 @@
 #include "expressionmatch.h"
 #include "util.h"
 
-HighlightRuleManager& HighlightRuleManager::operator=(const HighlightRuleManager& other)
-{
-    if (this == &other)
-        return *this;
-
-    SyncableObject::operator=(other);
-    _highlightRuleList = other._highlightRuleList;
-    _nicksCaseSensitive = other._nicksCaseSensitive;
-    _highlightNick = other._highlightNick;
-    return *this;
-}
-
 int HighlightRuleManager::indexOf(int id) const
 {
     for (int i = 0; i < _highlightRuleList.count(); i++) {

diff --git a/src/common/highlightrulemanager.h b/src/common/highlightrulemanager.h
index 47be30a..8f9a381 100644 (file)
--- a/src/common/highlightrulemanager.h
+++ b/src/common/highlightrulemanager.h
@@ -55,7 +55,6 @@ public:
     {
         setAllowClientUpdates(true);
     }
-    HighlightRuleManager& operator=(const HighlightRuleManager& other);
 
     /**
      * Individual highlight rule

diff --git a/src/common/ignorelistmanager.cpp b/src/common/ignorelistmanager.cpp
index 83add35..d0ab71f 100644 (file)
--- a/src/common/ignorelistmanager.cpp
+++ b/src/common/ignorelistmanager.cpp
@@ -23,16 +23,6 @@
 #include <QDebug>
 #include <QStringList>
 
-IgnoreListManager& IgnoreListManager::operator=(const IgnoreListManager& other)
-{
-    if (this == &other)
-        return *this;
-
-    SyncableObject::operator=(other);
-    _ignoreList = other._ignoreList;
-    return *this;
-}
-
 int IgnoreListManager::indexOf(const QString& ignore) const
 {
     for (int i = 0; i < _ignoreList.count(); i++) {

diff --git a/src/common/ignorelistmanager.h b/src/common/ignorelistmanager.h
index c1dbf10..ecb56fb 100644 (file)
--- a/src/common/ignorelistmanager.h
+++ b/src/common/ignorelistmanager.h
@@ -43,7 +43,6 @@ public:
     {
         setAllowClientUpdates(true);
     }
-    IgnoreListManager& operator=(const IgnoreListManager& other);
 
     enum IgnoreType
     {

diff --git a/src/common/syncableobject.cpp b/src/common/syncableobject.cpp
index 22ebec2..825b97d 100644 (file)
--- a/src/common/syncableobject.cpp
+++ b/src/common/syncableobject.cpp
@@ -44,13 +44,6 @@ SyncableObject::SyncableObject(const QString& objectName, QObject* parent)
     });
 }
 
-SyncableObject::SyncableObject(const SyncableObject& other, QObject* parent)
-    : SyncableObject(QString{}, parent)
-{
-    _initialized = other._initialized;
-    _allowClientUpdates = other._allowClientUpdates;
-}
-
 SyncableObject::~SyncableObject()
 {
     QList<SignalProxy*>::iterator proxyIter = _signalProxies.begin();
@@ -61,16 +54,6 @@ SyncableObject::~SyncableObject()
     }
 }
 
-SyncableObject& SyncableObject::operator=(const SyncableObject& other)
-{
-    if (this == &other)
-        return *this;
-
-    _initialized = other._initialized;
-    _allowClientUpdates = other._allowClientUpdates;
-    return *this;
-}
-
 bool SyncableObject::isInitialized() const
 {
     return _initialized;

diff --git a/src/common/syncableobject.h b/src/common/syncableobject.h
index 3e371c2..c216302 100644 (file)
--- a/src/common/syncableobject.h
+++ b/src/common/syncableobject.h
@@ -58,7 +58,6 @@ class COMMON_EXPORT SyncableObject : public QObject
 public:
     SyncableObject(QObject* parent = nullptr);
     SyncableObject(const QString& objectName, QObject* parent = nullptr);
-    SyncableObject(const SyncableObject& other, QObject* parent = nullptr);
     ~SyncableObject() override;
 
     //! Stores the object's state into a QVariantMap.
@@ -94,8 +93,6 @@ public slots:
 protected:
     void sync_call__(SignalProxy::ProxyMode modeType, const char* funcname, ...) const;
 
-    SyncableObject& operator=(const SyncableObject& other);
-
 signals:
     void initDone();
     void updatedRemotely();

diff --git a/src/qtui/settingspages/aliasesmodel.cpp b/src/qtui/settingspages/aliasesmodel.cpp
index 4271614..780b152 100644 (file)
--- a/src/qtui/settingspages/aliasesmodel.cpp
+++ b/src/qtui/settingspages/aliasesmodel.cpp
@@ -244,47 +244,41 @@ QModelIndex AliasesModel::index(int row, int column, const QModelIndex& parent)
 
 const AliasManager& AliasesModel::aliasManager() const
 {
-    if (_configChanged)
-        return _clonedAliasManager;
-    else
-        return *Client::aliasManager();
+    return _clonedAliasManager ? *_clonedAliasManager : *Client::aliasManager();
 }
 
 AliasManager& AliasesModel::aliasManager()
 {
-    if (_configChanged)
-        return _clonedAliasManager;
-    else
-        return *Client::aliasManager();
+    return _clonedAliasManager ? *_clonedAliasManager : *Client::aliasManager();
 }
 
 AliasManager& AliasesModel::cloneAliasManager()
 {
-    if (!_configChanged) {
-        _clonedAliasManager = *Client::aliasManager();
-        _configChanged = true;
+    if (!_clonedAliasManager) {
+        _clonedAliasManager = std::make_unique<ClientAliasManager>();
+        _clonedAliasManager->fromVariantMap(Client::aliasManager()->toVariantMap());
         emit configChanged(true);
     }
-    return _clonedAliasManager;
+    return *_clonedAliasManager;
 }
 
 void AliasesModel::revert()
 {
-    if (!_configChanged)
+    if (!_clonedAliasManager)
         return;
 
-    _configChanged = false;
-    emit configChanged(false);
     beginResetModel();
+    _clonedAliasManager.reset();
     endResetModel();
+    emit configChanged(false);
 }
 
 void AliasesModel::commit()
 {
-    if (!_configChanged)
+    if (!_clonedAliasManager)
         return;
 
-    Client::aliasManager()->requestUpdate(_clonedAliasManager.toVariantMap());
+    Client::aliasManager()->requestUpdate(_clonedAliasManager->toVariantMap());
     revert();
 }
 
@@ -307,10 +301,9 @@ void AliasesModel::clientConnected()
 
 void AliasesModel::clientDisconnected()
 {
-    // clear
-    _clonedAliasManager = ClientAliasManager();
     _modelReady = false;
     beginResetModel();
+    _clonedAliasManager.reset();
     endResetModel();
     emit modelReady(false);
 }

diff --git a/src/qtui/settingspages/aliasesmodel.h b/src/qtui/settingspages/aliasesmodel.h
index ba0674b..3dc7f0d 100644 (file)
--- a/src/qtui/settingspages/aliasesmodel.h
+++ b/src/qtui/settingspages/aliasesmodel.h
@@ -18,11 +18,11 @@
  *   51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.         *
  ***************************************************************************/
 
-#ifndef ALIASESMODEL_H
-#define ALIASESMODEL_H
+#pragma once
+
+#include <memory>
 
 #include <QAbstractItemModel>
-#include <QPointer>
 
 #include "clientaliasmanager.h"
 
@@ -47,7 +47,7 @@ public:
     inline int rowCount(const QModelIndex& parent = QModelIndex()) const override;
     inline int columnCount(const QModelIndex& parent = QModelIndex()) const override;
 
-    inline bool hasConfigChanged() const { return _configChanged; }
+    inline bool hasConfigChanged() const { return static_cast<bool>(_clonedAliasManager); }
     inline bool isReady() const { return _modelReady; }
 
 public slots:
@@ -62,8 +62,7 @@ signals:
     void modelReady(bool);
 
 private:
-    ClientAliasManager _clonedAliasManager;
-    bool _configChanged{false};
+    std::unique_ptr<ClientAliasManager> _clonedAliasManager;
     bool _modelReady{false};
 
     const AliasManager& aliasManager() const;
@@ -88,5 +87,3 @@ int AliasesModel::columnCount(const QModelIndex& parent) const
     Q_UNUSED(parent);
     return isReady() ? 2 : 0;
 }
-
-#endif  // ALIASESMODEL_H

diff --git a/src/qtui/settingspages/corehighlightsettingspage.cpp b/src/qtui/settingspages/corehighlightsettingspage.cpp
index 9e968ee..5a53b4a 100644 (file)
--- a/src/qtui/settingspages/corehighlightsettingspage.cpp
+++ b/src/qtui/settingspages/corehighlightsettingspage.cpp
@@ -620,7 +620,7 @@ void CoreHighlightSettingsPage::save()
     if (ruleManager == nullptr)
         return;
 
-    auto clonedManager = HighlightRuleManager();
+    HighlightRuleManager clonedManager;
     clonedManager.fromVariantMap(ruleManager->toVariantMap());
     clonedManager.clear();
 
@@ -734,7 +734,7 @@ void CoreHighlightSettingsPage::importRules()
         save();
     }
 
-    auto clonedManager = HighlightRuleManager();
+    HighlightRuleManager clonedManager;
     clonedManager.fromVariantMap(Client::highlightRuleManager()->toVariantMap());
 
     for (const auto& variant : notificationSettings.highlightList()) {

diff --git a/src/qtui/settingspages/dccsettingspage.cpp b/src/qtui/settingspages/dccsettingspage.cpp
index 56d22ca..3d74449 100644 (file)
--- a/src/qtui/settingspages/dccsettingspage.cpp
+++ b/src/qtui/settingspages/dccsettingspage.cpp
@@ -81,14 +81,14 @@ bool DccSettingsPage::hasDefaults() const
 
 void DccSettingsPage::defaults()
 {
-    _localConfig = DccConfig();
+    _localConfig.fromVariantMap(DccConfig{}.toVariantMap());
     SettingsPage::load();
     widgetHasChanged();
 }
 
 void DccSettingsPage::load()
 {
-    _localConfig = isClientConfigValid() ? *_clientConfig : DccConfig{};
+    _localConfig.fromVariantMap(isClientConfigValid() ? _clientConfig->toVariantMap() : DccConfig{}.toVariantMap());
     SettingsPage::load();
     widgetHasChanged();
 }

diff --git a/src/qtui/settingspages/ignorelistmodel.cpp b/src/qtui/settingspages/ignorelistmodel.cpp
index 3d14ddc..d08a280 100644 (file)
--- a/src/qtui/settingspages/ignorelistmodel.cpp
+++ b/src/qtui/settingspages/ignorelistmodel.cpp
@@ -144,7 +144,6 @@ bool IgnoreListModel::newIgnoreRule(const IgnoreListManager::IgnoreListItem& ite
     if (manager.contains(item.contents()))
         return false;
     beginInsertRows(QModelIndex(), rowCount(), rowCount());
-    // manager.addIgnoreListItem(item);
     manager.addIgnoreListItem(item.type(), item.contents(), item.isRegEx(), item.strictness(), item.scope(), item.scopeRule(), item.isEnabled());
     endInsertRows();
     return true;
@@ -216,47 +215,41 @@ QModelIndex IgnoreListModel::index(int row, int column, const QModelIndex& paren
 
 const IgnoreListManager& IgnoreListModel::ignoreListManager() const
 {
-    if (_configChanged)
-        return _clonedIgnoreListManager;
-    else
-        return *Client::ignoreListManager();
+    return _clonedIgnoreListManager ? *_clonedIgnoreListManager : *Client::ignoreListManager();
 }
 
 IgnoreListManager& IgnoreListModel::ignoreListManager()
 {
-    if (_configChanged)
-        return _clonedIgnoreListManager;
-    else
-        return *Client::ignoreListManager();
+    return _clonedIgnoreListManager ? *_clonedIgnoreListManager : *Client::ignoreListManager();
 }
 
 IgnoreListManager& IgnoreListModel::cloneIgnoreListManager()
 {
-    if (!_configChanged) {
-        _clonedIgnoreListManager = *Client::ignoreListManager();
-        _configChanged = true;
+    if (!_clonedIgnoreListManager) {
+        _clonedIgnoreListManager = std::make_unique<ClientIgnoreListManager>();
+        _clonedIgnoreListManager->fromVariantMap(Client::ignoreListManager()->toVariantMap());
         emit configChanged(true);
     }
-    return _clonedIgnoreListManager;
+    return *_clonedIgnoreListManager;
 }
 
 void IgnoreListModel::revert()
 {
-    if (!_configChanged)
+    if (!_clonedIgnoreListManager)
         return;
 
-    _configChanged = false;
-    emit configChanged(false);
     beginResetModel();
+    _clonedIgnoreListManager.reset();
     endResetModel();
+    emit configChanged(false);
 }
 
 void IgnoreListModel::commit()
 {
-    if (!_configChanged)
+    if (!_clonedIgnoreListManager)
         return;
 
-    Client::ignoreListManager()->requestUpdate(_clonedIgnoreListManager.toVariantMap());
+    Client::ignoreListManager()->requestUpdate(_clonedIgnoreListManager->toVariantMap());
     revert();
 }
 
@@ -279,10 +272,9 @@ void IgnoreListModel::clientConnected()
 
 void IgnoreListModel::clientDisconnected()
 {
-    // clear
-    _clonedIgnoreListManager = ClientIgnoreListManager();
     _modelReady = false;
     beginResetModel();
+    _clonedIgnoreListManager.reset();
     endResetModel();
     emit modelReady(false);
 }

diff --git a/src/qtui/settingspages/ignorelistmodel.h b/src/qtui/settingspages/ignorelistmodel.h
index 3b27a1c..262358b 100644 (file)
--- a/src/qtui/settingspages/ignorelistmodel.h
+++ b/src/qtui/settingspages/ignorelistmodel.h
@@ -18,11 +18,11 @@
  *   51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.         *
  ***************************************************************************/
 
-#ifndef IGNORELISTMODEL_H
-#define IGNORELISTMODEL_H
+#pragma once
+
+#include <memory>
 
 #include <QAbstractItemModel>
-#include <QPointer>
 
 #include "clientignorelistmanager.h"
 
@@ -47,7 +47,7 @@ public:
     inline int rowCount(const QModelIndex& parent = QModelIndex()) const override;
     inline int columnCount(const QModelIndex& parent = QModelIndex()) const override;
 
-    inline bool hasConfigChanged() const { return _configChanged; }
+    inline bool hasConfigChanged() const { return static_cast<bool>(_clonedIgnoreListManager); }
     inline bool isReady() const { return _modelReady; }
 
     const IgnoreListManager::IgnoreListItem& ignoreListItemAt(int row) const;
@@ -66,8 +66,7 @@ signals:
     void modelReady(bool);
 
 private:
-    ClientIgnoreListManager _clonedIgnoreListManager;
-    bool _configChanged{false};
+    std::unique_ptr<ClientIgnoreListManager> _clonedIgnoreListManager;
     bool _modelReady{false};
 
     const IgnoreListManager& ignoreListManager() const;
@@ -92,5 +91,3 @@ int IgnoreListModel::columnCount(const QModelIndex& parent) const
     Q_UNUSED(parent);
     return isReady() ? 3 : 0;
 }
-
-#endif  // IGNORELISTMODEL_H