Merge pull request #5413

Mick Sayson (2):
      refactor(settings): Consolidate friendProp insert logic
      fix(settings): Add mutex locks for consistency

src/persistence/settings.cpp

@@ -355,8 +355,7 @@ void Settings::loadPersonal(Profile* profile)
         friendLst.reserve(size);
         for (int i = 0; i < size; i++) {
             ps.setArrayIndex(i);
-            friendProp fp;
-            fp.addr = ps.value("addr").toString();
+            friendProp fp{ps.value("addr").toString()};
             fp.alias = ps.value("alias").toString();
             fp.note = ps.value("note").toString();
             fp.autoAcceptDir = ps.value("autoAcceptDir").toString();
@@ -371,7 +370,7 @@ void Settings::loadPersonal(Profile* profile)
 
             if (getEnableLogging())
                 fp.activity = ps.value("activity", QDate()).toDate();
-            friendLst[ToxId(fp.addr).getPublicKey().getKey()] = fp;
+            friendLst.insert(ToxId(fp.addr).getPublicKey().getKey(), fp);
         }
         ps.endArray();
     }
@@ -1251,6 +1250,8 @@ void Settings::setEnableLanDiscovery(bool enabled)
 
 QNetworkProxy Settings::getProxy() const
 {
+    QMutexLocker locker{&bigLock};
+
     QNetworkProxy proxy;
     switch (Settings::getProxyType()) {
     case ProxyType::ptNone:
@@ -1400,15 +1401,10 @@ void Settings::setAutoAcceptDir(const ToxPk& id, const QString& dir)
 {
     QMutexLocker locker{&bigLock};
 
-    auto it = friendLst.find(id.getKey());
-    if (it == friendLst.end()) {
-        updateFriendAddress(id.toString());
-        setAutoAcceptDir(id, dir);
-        return;
-    }
+    auto& frnd = getOrInsertFriendPropRef(id);
 
-    if (it->autoAcceptDir != dir) {
-        it->autoAcceptDir = dir;
+    if (frnd.autoAcceptDir != dir) {
+        frnd.autoAcceptDir = dir;
         emit autoAcceptDirChanged(id, dir);
     }
 }
@@ -1428,15 +1424,10 @@ void Settings::setAutoAcceptCall(const ToxPk& id, AutoAcceptCallFlags accept)
 {
     QMutexLocker locker{&bigLock};
 
-    auto it = friendLst.find(id.getKey());
-    if (it == friendLst.end()) {
-        updateFriendAddress(id.toString());
-        setAutoAcceptCall(id, accept);
-        return;
-    }
+    auto& frnd = getOrInsertFriendPropRef(id);
 
-    if (it->autoAcceptCall != accept) {
-        it->autoAcceptCall = accept;
+    if (frnd.autoAcceptCall != accept) {
+        frnd.autoAcceptCall = accept;
         emit autoAcceptCallChanged(id, accept);
     }
 }
@@ -1457,15 +1448,10 @@ void Settings::setAutoGroupInvite(const ToxPk& id, bool accept)
 {
     QMutexLocker locker{&bigLock};
 
-    auto it = friendLst.find(id.getKey());
-    if (it == friendLst.end()) {
-        updateFriendAddress(id.toString());
-        setAutoGroupInvite(id, accept);
-        return;
-    }
+    auto& frnd = getOrInsertFriendPropRef(id);
 
-    if (it->autoGroupInvite != accept) {
-        it->autoGroupInvite = accept;
+    if (frnd.autoGroupInvite != accept) {
+        frnd.autoGroupInvite = accept;
         emit autoGroupInviteChanged(id, accept);
     }
 }
@@ -1485,15 +1471,10 @@ void Settings::setContactNote(const ToxPk& id, const QString& note)
 {
     QMutexLocker locker{&bigLock};
 
-    auto it = friendLst.find(id.getKey());
-    if (it == friendLst.end()) {
-        updateFriendAddress(id.toString());
-        setContactNote(id, note);
-        return;
-    }
+    auto& frnd = getOrInsertFriendPropRef(id);
 
-    if (it->note != note) {
-        it->note = note;
+    if (frnd.note != note) {
+        frnd.note = note;
         emit contactNoteChanged(id, note);
     }
 }
@@ -2073,18 +2054,9 @@ void Settings::updateFriendAddress(const QString& newAddr)
 {
     QMutexLocker locker{&bigLock};
     // TODO: using ToxId here is a hack
-    QByteArray key = ToxId(newAddr).getPublicKey().getKey();
-    auto it = friendLst.find(key);
-    if (it != friendLst.end()) {
-        it->addr = newAddr;
-    } else {
-        friendProp fp;
-        fp.addr = newAddr;
-        fp.alias = "";
-        fp.note = "";
-        fp.autoAcceptDir = "";
-        friendLst[key] = fp;
-    }
+    auto key = ToxId(newAddr).getPublicKey();
+    auto& frnd = getOrInsertFriendPropRef(key);
+    frnd.addr = newAddr;
 }
 
 QString Settings::getFriendAlias(const ToxPk& id) const
@@ -2100,21 +2072,13 @@ QString Settings::getFriendAlias(const ToxPk& id) const
 void Settings::setFriendAlias(const ToxPk& id, const QString& alias)
 {
     QMutexLocker locker{&bigLock};
-    auto it = friendLst.find(id.getKey());
-    if (it != friendLst.end()) {
-        it->alias = alias;
-    } else {
-        friendProp fp;
-        fp.addr = id.toString();
-        fp.alias = alias;
-        fp.note = "";
-        fp.autoAcceptDir = "";
-        friendLst[id.getKey()] = fp;
-    }
+    auto& frnd = getOrInsertFriendPropRef(id);
+    frnd.alias = alias;
 }
 
 int Settings::getFriendCircleID(const ToxPk& id) const
 {
+    QMutexLocker locker{&bigLock};
     auto it = friendLst.find(id.getKey());
     if (it != friendLst.end())
         return it->circleID;
@@ -2124,18 +2088,9 @@ int Settings::getFriendCircleID(const ToxPk& id) const
 
 void Settings::setFriendCircleID(const ToxPk& id, int circleID)
 {
-    auto it = friendLst.find(id.getKey());
-    if (it != friendLst.end()) {
-        it->circleID = circleID;
-    } else {
-        friendProp fp;
-        fp.addr = id.toString();
-        fp.alias = "";
-        fp.note = "";
-        fp.autoAcceptDir = "";
-        fp.circleID = circleID;
-        friendLst[id.getKey()] = fp;
-    }
+    QMutexLocker locker{&bigLock};
+    auto& frnd = getOrInsertFriendPropRef(id);
+    frnd.circleID = circleID;
 }
 
 QDate Settings::getFriendActivity(const ToxPk& id) const
@@ -2149,19 +2104,9 @@ QDate Settings::getFriendActivity(const ToxPk& id) const
 
 void Settings::setFriendActivity(const ToxPk& id, const QDate& activity)
 {
-    auto it = friendLst.find(id.getKey());
-    if (it != friendLst.end()) {
-        it->activity = activity;
-    } else {
-        friendProp fp;
-        fp.addr = id.toString();
-        fp.alias = "";
-        fp.note = "";
-        fp.autoAcceptDir = "";
-        fp.circleID = -1;
-        fp.activity = activity;
-        friendLst[id.getKey()] = fp;
-    }
+    QMutexLocker locker{&bigLock};
+    auto& frnd = getOrInsertFriendPropRef(id);
+    frnd.activity = activity;
 }
 
 void Settings::saveFriendSettings(const ToxPk& id)
@@ -2274,22 +2219,27 @@ void Settings::setShowIdenticons(bool value)
 
 int Settings::getCircleCount() const
 {
+    QMutexLocker locker{&bigLock};
     return circleLst.size();
 }
 
 QString Settings::getCircleName(int id) const
 {
+    QMutexLocker locker{&bigLock};
     return circleLst[id].name;
 }
 
 void Settings::setCircleName(int id, const QString& name)
 {
+    QMutexLocker locker{&bigLock};
     circleLst[id].name = name;
     savePersonal();
 }
 
 int Settings::addCircle(const QString& name)
 {
+    QMutexLocker locker{&bigLock};
+
     circleProp cp;
     cp.expanded = false;
 
@@ -2305,11 +2255,13 @@ int Settings::addCircle(const QString& name)
 
 bool Settings::getCircleExpanded(int id) const
 {
+    QMutexLocker locker{&bigLock};
     return circleLst[id].expanded;
 }
 
 void Settings::setCircleExpanded(int id, bool expanded)
 {
+    QMutexLocker locker{&bigLock};
     circleLst[id].expanded = expanded;
 }
 
@@ -2438,6 +2390,8 @@ bool Settings::getEnableGroupChatsColor() const
  */
 void Settings::createPersonal(QString basename)
 {
+    QMutexLocker locker{&bigLock};
+
     QString path = getSettingsDirPath() + QDir::separator() + basename + ".ini";
     qDebug() << "Creating new profile settings in " << path;
 
@@ -2457,6 +2411,8 @@ void Settings::createPersonal(QString basename)
  */
 void Settings::createSettingsDir()
 {
+    QMutexLocker locker{&bigLock};
+
     QString dir = Settings::getSettingsDirPath();
     QDir directory(dir);
     if (!directory.exists() && !directory.mkpath(directory.absolutePath()))
@@ -2476,3 +2432,15 @@ void Settings::sync()
     QMutexLocker locker{&bigLock};
     qApp->processEvents();
 }
+
+Settings::friendProp& Settings::getOrInsertFriendPropRef(const ToxPk& id)
+{
+    // No mutex lock, this is a private fn that should only be called by other
+    // public functions that already locked the mutex
+    auto it = friendLst.find(id.getKey());
+    if (it == friendLst.end()) {
+        it = friendLst.insert(id.getKey(), friendProp{id.toString()});
+    }
+
+    return *it;
+}

src/persistence/settings.h

@@ -570,11 +570,14 @@ public:
     static uint32_t makeProfileId(const QString& profile);
 
 private:
+    struct friendProp;
+
     Settings();
     ~Settings();
     Settings(Settings& settings) = delete;
     Settings& operator=(const Settings&) = delete;
     void savePersonal(QString profileName, const ToxEncrypt* passkey);
+    friendProp& getOrInsertFriendPropRef(const ToxPk& id);
 
 public slots:
     void savePersonal(Profile* profile);
@@ -687,10 +690,14 @@ private:
 
     struct friendProp
     {
-        QString alias;
-        QString addr;
-        QString autoAcceptDir;
-        QString note;
+        friendProp() = delete;
+        friendProp(QString addr)
+            : addr(addr)
+        {}
+        QString alias = "";
+        QString addr = "";
+        QString autoAcceptDir = "";
+        QString note = "";
         int circleID = -1;
         QDate activity = QDate();
         AutoAcceptCallFlags autoAcceptCall;