Clean up CompletionModel (#4240)

* Refactor CompletionModel

Do some clang-tidy cleanup

* Use a shared mutex for CompletionModel's items mutex

src/common/CompletionModel.cpp

@@ -2,10 +2,8 @@
 
 #include "Application.hpp"
 #include "common/ChatterSet.hpp"
-#include "common/Common.hpp"
 #include "controllers/accounts/AccountController.hpp"
 #include "controllers/commands/CommandController.hpp"
-#include "debug/Benchmark.hpp"
 #include "providers/twitch/TwitchChannel.hpp"
 #include "providers/twitch/TwitchCommon.hpp"
 #include "providers/twitch/TwitchIrcServer.hpp"
@@ -24,8 +22,8 @@ namespace chatterino {
 // TaggedString
 //
 
-CompletionModel::TaggedString::TaggedString(const QString &_string, Type _type)
+CompletionModel::TaggedString::TaggedString(QString _string, Type _type)
-    : string(_string)
+    : string(std::move(_string))
     , type(_type)
 {
 }
@@ -53,30 +51,37 @@ CompletionModel::CompletionModel(Channel &channel)
 {
 }
 
-int CompletionModel::columnCount(const QModelIndex &) const
+int CompletionModel::columnCount(const QModelIndex &parent) const
 {
+    (void)parent;  // unused
+
     return 1;
 }
 
-QVariant CompletionModel::data(const QModelIndex &index, int) const
+QVariant CompletionModel::data(const QModelIndex &index, int role) const
 {
-    std::lock_guard<std::mutex> lock(this->itemsMutex_);
+    (void)role;  // unused
+
+    std::shared_lock lock(this->itemsMutex_);
 
     auto it = this->items_.begin();
     std::advance(it, index.row());
-    return QVariant(it->string);
+    return {it->string};
 }
 
-int CompletionModel::rowCount(const QModelIndex &) const
+int CompletionModel::rowCount(const QModelIndex &parent) const
 {
-    std::lock_guard<std::mutex> lock(this->itemsMutex_);
+    (void)parent;  // unused
+
+    std::shared_lock lock(this->itemsMutex_);
 
     return this->items_.size();
 }
 
 void CompletionModel::refresh(const QString &prefix, bool isFirstWord)
 {
-    std::lock_guard<std::mutex> guard(this->itemsMutex_);
+    std::unique_lock lock(this->itemsMutex_);
+
     this->items_.clear();
 
     if (prefix.length() < 2 || !this->channel_.isTwitchChannel())
@@ -85,7 +90,7 @@ void CompletionModel::refresh(const QString &prefix, bool isFirstWord)
     }
 
     // Twitch channel
-    auto tc = dynamic_cast<TwitchChannel *>(&this->channel_);
+    auto *tc = dynamic_cast<TwitchChannel *>(&this->channel_);
 
     auto addString = [=](const QString &str, TaggedString::Type type) {
         // Special case for handling default Twitch commands
@@ -132,7 +137,8 @@ void CompletionModel::refresh(const QString &prefix, bool isFirstWord)
 
         // Twitch Emotes available locally
         auto localEmoteData = account->accessLocalEmotes();
-        if (tc && localEmoteData->find(tc->roomId()) != localEmoteData->end())
+        if (tc != nullptr &&
+            localEmoteData->find(tc->roomId()) != localEmoteData->end())
         {
             for (const auto &emote : localEmoteData->at(tc->roomId()))
             {
@@ -143,18 +149,19 @@ void CompletionModel::refresh(const QString &prefix, bool isFirstWord)
     }
 
     // 7TV Global
-    for (auto &emote : *getApp()->twitch->getSeventvEmotes().globalEmotes())
+    for (const auto &emote :
+         *getApp()->twitch->getSeventvEmotes().globalEmotes())
     {
         addString(emote.first.string, TaggedString::Type::SeventvGlobalEmote);
     }
     // Bttv Global
-    for (auto &emote : *getApp()->twitch->getBttvEmotes().emotes())
+    for (const auto &emote : *getApp()->twitch->getBttvEmotes().emotes())
     {
         addString(emote.first.string, TaggedString::Type::BTTVChannelEmote);
     }
 
     // Ffz Global
-    for (auto &emote : *getApp()->twitch->getFfzEmotes().emotes())
+    for (const auto &emote : *getApp()->twitch->getFfzEmotes().emotes())
     {
         addString(emote.first.string, TaggedString::Type::FFZChannelEmote);
     }
@@ -163,7 +170,7 @@ void CompletionModel::refresh(const QString &prefix, bool isFirstWord)
     if (prefix.startsWith(":"))
     {
         const auto &emojiShortCodes = getApp()->emotes->emojis.shortCodes;
-        for (auto &m : emojiShortCodes)
+        for (const auto &m : emojiShortCodes)
         {
             addString(QString(":%1:").arg(m), TaggedString::Type::Emoji);
         }
@@ -171,7 +178,7 @@ void CompletionModel::refresh(const QString &prefix, bool isFirstWord)
 
     //
     // Stuff below is available only in regular Twitch channels
-    if (!tc)
+    if (tc == nullptr)
     {
         return;
     }
@@ -205,36 +212,37 @@ void CompletionModel::refresh(const QString &prefix, bool isFirstWord)
     }
 
     // 7TV Channel
-    for (auto &emote : *tc->seventvEmotes())
+    for (const auto &emote : *tc->seventvEmotes())
     {
         addString(emote.first.string, TaggedString::Type::SeventvChannelEmote);
     }
     // Bttv Channel
-    for (auto &emote : *tc->bttvEmotes())
+    for (const auto &emote : *tc->bttvEmotes())
     {
         addString(emote.first.string, TaggedString::Type::BTTVGlobalEmote);
     }
 
     // Ffz Channel
-    for (auto &emote : *tc->ffzEmotes())
+    for (const auto &emote : *tc->ffzEmotes())
     {
         addString(emote.first.string, TaggedString::Type::BTTVGlobalEmote);
     }
 
     // Custom Chatterino commands
-    for (auto &command : getApp()->commands->items)
+    for (const auto &command : getApp()->commands->items)
     {
         addString(command.name, TaggedString::CustomCommand);
     }
 
     // Default Chatterino commands
-    for (auto &command : getApp()->commands->getDefaultChatterinoCommandList())
+    for (const auto &command :
+         getApp()->commands->getDefaultChatterinoCommandList())
     {
         addString(command, TaggedString::ChatterinoCommand);
     }
 
     // Default Twitch commands
-    for (auto &command : TWITCH_DEFAULT_COMMANDS)
+    for (const auto &command : TWITCH_DEFAULT_COMMANDS)
     {
         addString(command, TaggedString::TwitchCommand);
     }
@@ -246,7 +254,9 @@ bool CompletionModel::compareStrings(const QString &a, const QString &b)
     // (fixes order of LuL and LUL)
     int k = QString::compare(a, b, Qt::CaseInsensitive);
     if (k == 0)
+    {
         return a > b;
+    }
 
     return k < 0;
 }

src/common/CompletionModel.hpp

@@ -3,8 +3,8 @@
 #include <QAbstractListModel>
 
 #include <chrono>
-#include <mutex>
 #include <set>
+#include <shared_mutex>
 
 namespace chatterino {
 
@@ -36,29 +36,30 @@ class CompletionModel : public QAbstractListModel
             TwitchCommand,
         };
 
-        TaggedString(const QString &string, Type type);
+        TaggedString(QString _string, Type type);
 
         bool isEmote() const;
         bool operator<(const TaggedString &that) const;
 
-        QString string;
+        const QString string;
-        Type type;
+        const Type type;
     };
 
 public:
     CompletionModel(Channel &channel);
 
-    virtual int columnCount(const QModelIndex &) const override;
+    int columnCount(const QModelIndex &parent) const override;
-    virtual QVariant data(const QModelIndex &index, int) const override;
+    QVariant data(const QModelIndex &index, int role) const override;
-    virtual int rowCount(const QModelIndex &) const override;
+    int rowCount(const QModelIndex &parent) const override;
 
     void refresh(const QString &prefix, bool isFirstWord = false);
 
     static bool compareStrings(const QString &a, const QString &b);
 
 private:
+    mutable std::shared_mutex itemsMutex_;
     std::set<TaggedString> items_;
-    mutable std::mutex itemsMutex_;
+
     Channel &channel_;
 };