Fix signal connection nodiscard warnings

CHANGELOG.md

@@ -17,6 +17,7 @@
 - Dev: Fix clang-tidy `cppcoreguidelines-pro-type-member-init` warnings. (#4426)
 - Dev: Immediate layout for invisible `ChannelView`s is skipped. (#4811)
 - Dev: Refactor `Image` & Image's `Frames`. (#4773)
+- Dev: Clarify signal connection lifetimes where applicable. (#4818)
 
 ## 2.4.5
 

src/common/Channel.cpp

@@ -106,7 +106,7 @@ void Channel::addMessage(MessagePtr message,
 
     if (this->messages_.pushBack(message, deleted))
     {
-        this->messageRemovedFromStart.invoke(deleted);
+        this->messageRemovedFromStart(deleted);
     }
 
     this->messageAppended.invoke(message, overridingFlags);
@@ -353,6 +353,10 @@ void Channel::onConnected()
 {
 }
 
+void Channel::messageRemovedFromStart(const MessagePtr &msg)
+{
+}
+
 //
 // Indirect channel
 //

src/common/Channel.hpp

@@ -52,7 +52,6 @@ class Channel : public std::enable_shared_from_this<Channel>
     pajlada::Signals::Signal<const QString &, const QString &, const QString &,
                              bool &>
         sendReplySignal;
-    pajlada::Signals::Signal<MessagePtr &> messageRemovedFromStart;
     pajlada::Signals::Signal<MessagePtr &, boost::optional<MessageFlags>>
         messageAppended;
     pajlada::Signals::Signal<std::vector<MessagePtr> &> messagesAddedAtStart;
@@ -114,6 +113,7 @@ class Channel : public std::enable_shared_from_this<Channel>
 
 protected:
     virtual void onConnected();
+    virtual void messageRemovedFromStart(const MessagePtr &msg);
 
 private:
     const QString name_;

src/controllers/accounts/AccountController.cpp

@@ -1,4 +1,4 @@
-#include "AccountController.hpp"
+#include "controllers/accounts/AccountController.hpp"
 
 #include "controllers/accounts/Account.hpp"
 #include "controllers/accounts/AccountModel.hpp"
@@ -10,22 +10,27 @@ namespace chatterino {
 AccountController::AccountController()
     : accounts_(SharedPtrElementLess<Account>{})
 {
-    this->twitch.accounts.itemInserted.connect([this](const auto &args) {
-        this->accounts_.insert(std::dynamic_pointer_cast<Account>(args.item));
-    });
-
-    this->twitch.accounts.itemRemoved.connect([this](const auto &args) {
-        if (args.caller != this)
-        {
-            auto &accs = this->twitch.accounts.raw();
-            auto it = std::find(accs.begin(), accs.end(), args.item);
-            assert(it != accs.end());
-
-            this->accounts_.removeAt(it - accs.begin(), this);
-        }
-    });
+    // These signal connections can safely be ignored since the twitch object
+    // will always be destroyed before the AccountController
+    std::ignore =
+        this->twitch.accounts.itemInserted.connect([this](const auto &args) {
+            this->accounts_.insert(
+                std::dynamic_pointer_cast<Account>(args.item));
+        });
+
+    std::ignore =
+        this->twitch.accounts.itemRemoved.connect([this](const auto &args) {
+            if (args.caller != this)
+            {
+                auto &accs = this->twitch.accounts.raw();
+                auto it = std::find(accs.begin(), accs.end(), args.item);
+                assert(it != accs.end());
+
+                this->accounts_.removeAt(it - accs.begin(), this);
+            }
+        });
 
-    this->accounts_.itemRemoved.connect([this](const auto &args) {
+    std::ignore = this->accounts_.itemRemoved.connect([this](const auto &args) {
         switch (args.item->getProviderId())
         {
             case ProviderId::Twitch: {

src/controllers/commands/CommandController.cpp

@@ -587,8 +587,10 @@ void CommandController::initialize(Settings &, Paths &paths)
 
         this->maxSpaces_ = maxSpaces;
     };
-    this->items.itemInserted.connect(addFirstMatchToMap);
-    this->items.itemRemoved.connect(addFirstMatchToMap);
+    // We can safely ignore these signal connections since items will be destroyed
+    // before CommandController
+    std::ignore = this->items.itemInserted.connect(addFirstMatchToMap);
+    std::ignore = this->items.itemRemoved.connect(addFirstMatchToMap);
 
     // Initialize setting manager for commands.json
     auto path = combinePath(paths.settingsDirectory, "commands.json");
@@ -604,7 +606,7 @@ void CommandController::initialize(Settings &, Paths &paths)
 
     // Update the setting when the vector of commands has been updated (most
     // likely from the settings dialog)
-    this->items.delayedItemsChanged.connect([this] {
+    std::ignore = this->items.delayedItemsChanged.connect([this] {
         this->commandsSetting_->setValue(this->items.raw());
     });
 

src/controllers/notifications/NotificationController.cpp

@@ -36,9 +36,13 @@ void NotificationController::initialize(Settings &settings, Paths &paths)
         this->channelMap[Platform::Twitch].append(channelName);
     }
 
-    this->channelMap[Platform::Twitch].delayedItemsChanged.connect([this] {
-        this->twitchSetting_.setValue(this->channelMap[Platform::Twitch].raw());
-    });
+    // We can safely ignore this signal connection since channelMap will always be destroyed
+    // before the NotificationController
+    std::ignore =
+        this->channelMap[Platform::Twitch].delayedItemsChanged.connect([this] {
+            this->twitchSetting_.setValue(
+                this->channelMap[Platform::Twitch].raw());
+        });
 
     liveStatusTimer_ = new QTimer();
 

src/providers/irc/AbstractIrcServer.cpp

@@ -50,7 +50,7 @@ AbstractIrcServer::AbstractIrcServer()
         this->writeConnection_->connectionLost, [this](bool timeout) {
             qCDebug(chatterinoIrc)
                 << "Write connection reconnect requested. Timeout:" << timeout;
-            this->writeConnection_->smartReconnect.invoke();
+            this->writeConnection_->smartReconnect();
         });
 
     // Listen to read connection message signals
@@ -86,7 +86,7 @@ AbstractIrcServer::AbstractIrcServer()
                 this->addGlobalSystemMessage(
                     "Server connection timed out, reconnecting");
             }
-            this->readConnection_->smartReconnect.invoke();
+            this->readConnection_->smartReconnect();
         });
 }
 

src/providers/irc/Irc2.cpp

@@ -88,7 +88,9 @@ void IrcServerData::setPassword(const QString &password)
 
 Irc::Irc()
 {
-    this->connections.itemInserted.connect([this](auto &&args) {
+    // We can safely ignore this signal connection since `connections` will always
+    // be destroyed before the Irc object
+    std::ignore = this->connections.itemInserted.connect([this](auto &&args) {
         // make sure only one id can only exist for one server
         assert(this->servers_.find(args.item.id) == this->servers_.end());
 
@@ -117,7 +119,9 @@ Irc::Irc()
         }
     });
 
-    this->connections.itemRemoved.connect([this](auto &&args) {
+    // We can safely ignore this signal connection since `connections` will always
+    // be destroyed before the Irc object
+    std::ignore = this->connections.itemRemoved.connect([this](auto &&args) {
         // restore
         if (auto server = this->servers_.find(args.item.id);
             server != this->servers_.end())
@@ -141,7 +145,9 @@ Irc::Irc()
         }
     });
 
-    this->connections.delayedItemsChanged.connect([this] {
+    // We can safely ignore this signal connection since `connections` will always
+    // be destroyed before the Irc object
+    std::ignore = this->connections.delayedItemsChanged.connect([this] {
         this->save();
     });
 }

src/providers/irc/IrcConnection2.cpp

@@ -38,18 +38,6 @@ IrcConnection::IrcConnection(QObject *parent)
             }
         });
 
-    // Schedule a reconnect that won't violate RECONNECT_MIN_INTERVAL
-    this->smartReconnect.connect([this] {
-        if (this->reconnectTimer_.isActive())
-        {
-            return;
-        }
-
-        auto delay = this->reconnectBackoff_.next();
-        qCDebug(chatterinoIrc) << "Reconnecting in" << delay.count() << "ms";
-        this->reconnectTimer_.start(delay);
-    });
-
     this->reconnectTimer_.setSingleShot(true);
     QObject::connect(&this->reconnectTimer_, &QTimer::timeout, [this] {
         if (this->isConnected())
@@ -123,6 +111,19 @@ IrcConnection::~IrcConnection()
     this->disconnect();
 }
 
+void IrcConnection::smartReconnect()
+{
+    if (this->reconnectTimer_.isActive())
+    {
+        // Ignore this reconnect request, we already have a reconnect request queued up
+        return;
+    }
+
+    auto delay = this->reconnectBackoff_.next();
+    qCDebug(chatterinoIrc) << "Reconnecting in" << delay.count() << "ms";
+    this->reconnectTimer_.start(delay);
+}
+
 void IrcConnection::open()
 {
     this->expectConnectionLoss_ = false;

src/providers/irc/IrcConnection2.hpp

@@ -20,7 +20,8 @@ class IrcConnection : public Communi::IrcConnection
     pajlada::Signals::Signal<bool> connectionLost;
 
     // Request a reconnect with a minimum interval between attempts.
-    pajlada::Signals::NoArgSignal smartReconnect;
+    // This won't violate RECONNECT_MIN_INTERVAL
+    void smartReconnect();
 
     virtual void open();
     virtual void close();

src/providers/twitch/TwitchAccountManager.cpp

@@ -20,7 +20,9 @@ TwitchAccountManager::TwitchAccountManager()
         currentUser->loadSeventvUserID();
     });
 
-    this->accounts.itemRemoved.connect([this](const auto &acc) {
+    // We can safely ignore this signal connection since accounts will always be removed
+    // before TwitchAccountManager
+    std::ignore = this->accounts.itemRemoved.connect([this](const auto &acc) {
         this->removeUser(acc.item.get());
     });
 }

src/providers/twitch/TwitchChannel.cpp

@@ -94,25 +94,15 @@ TwitchChannel::TwitchChannel(const QString &name)
         }));
 
     this->refreshPubSub();
-    this->userStateChanged.connect([this] {
+    // We can safely ignore this signal connection since it's a private signal, meaning
+    // it will only ever be invoked by TwitchChannel itself
+    std::ignore = this->userStateChanged.connect([this] {
         this->refreshPubSub();
     });
 
-    // room id loaded -> refresh live status
-    this->roomIdChanged.connect([this]() {
-        this->refreshPubSub();
-        this->refreshBadges();
-        this->refreshCheerEmotes();
-        this->refreshFFZChannelEmotes(false);
-        this->refreshBTTVChannelEmotes(false);
-        this->refreshSevenTVChannelEmotes(false);
-        this->joinBttvChannel();
-        this->listenSevenTVCosmetics();
-        getIApp()->getTwitchLiveController()->add(
-            std::dynamic_pointer_cast<TwitchChannel>(shared_from_this()));
-    });
-
-    this->connected.connect([this]() {
+    // We can safely ignore this signal connection this has no external dependencies - once the signal
+    // is destroyed, it will no longer be able to fire
+    std::ignore = this->connected.connect([this]() {
         if (this->roomId().isEmpty())
         {
             // If we get a reconnected event when the room id is not set, we
@@ -125,18 +115,7 @@ TwitchChannel::TwitchChannel(const QString &name)
         this->loadRecentMessagesReconnect();
     });
 
-    this->messageRemovedFromStart.connect([this](MessagePtr &msg) {
-        if (msg->replyThread)
-        {
-            if (msg->replyThread->liveCount(msg) == 0)
-            {
-                this->threads_.erase(msg->replyThread->rootId());
-            }
-        }
-    });
-
     // timers
-
     QObject::connect(&this->chattersListTimer_, &QTimer::timeout, [this] {
         this->refreshChatters();
     });
@@ -538,6 +517,20 @@ void TwitchChannel::showLoginMessage()
     this->addMessage(builder.release());
 }
 
+void TwitchChannel::roomIdChanged()
+{
+    this->refreshPubSub();
+    this->refreshBadges();
+    this->refreshCheerEmotes();
+    this->refreshFFZChannelEmotes(false);
+    this->refreshBTTVChannelEmotes(false);
+    this->refreshSevenTVChannelEmotes(false);
+    this->joinBttvChannel();
+    this->listenSevenTVCosmetics();
+    getIApp()->getTwitchLiveController()->add(
+        std::dynamic_pointer_cast<TwitchChannel>(shared_from_this()));
+}
+
 QString TwitchChannel::prepareMessage(const QString &message) const
 {
     auto app = getApp();
@@ -729,7 +722,7 @@ void TwitchChannel::setRoomId(const QString &id)
     if (*this->roomID_.accessConst() != id)
     {
         *this->roomID_.access() = id;
-        this->roomIdChanged.invoke();
+        this->roomIdChanged();
         this->loadRecentMessages();
     }
 }
@@ -1063,6 +1056,17 @@ bool TwitchChannel::tryReplaceLastLiveUpdateAddOrRemove(
     return true;
 }
 
+void TwitchChannel::messageRemovedFromStart(const MessagePtr &msg)
+{
+    if (msg->replyThread)
+    {
+        if (msg->replyThread->liveCount(msg) == 0)
+        {
+            this->threads_.erase(msg->replyThread->rootId());
+        }
+    }
+}
+
 const QString &TwitchChannel::subscriptionUrl()
 {
     return this->subscriptionUrl_;

src/providers/twitch/TwitchChannel.hpp

@@ -191,8 +191,7 @@ class TwitchChannel : public Channel, public ChannelChatters
     const std::unordered_map<QString, std::weak_ptr<MessageThread>> &threads()
         const;
 
-    // Signals
-    pajlada::Signals::NoArgSignal roomIdChanged;
+    // Only TwitchChannel may invoke this signal
     pajlada::Signals::NoArgSignal userStateChanged;
 
     /**
@@ -242,8 +241,6 @@ class TwitchChannel : public Channel, public ChannelChatters
         QString actualDisplayName;
     } nameOptions;
 
-private:
-    // Methods
     void refreshPubSub();
     void refreshChatters();
     void refreshBadges();
@@ -252,6 +249,11 @@ class TwitchChannel : public Channel, public ChannelChatters
     void loadRecentMessagesReconnect();
     void cleanUpReplyThreads();
     void showLoginMessage();
+
+    /// roomIdChanged is called whenever this channel's ID has been changed
+    /// This should only happen once per channel, whenever the ID goes from unset to set
+    void roomIdChanged();
+
     /** Joins (subscribes to) a Twitch channel for updates on BTTV. */
     void joinBttvChannel() const;
     /**
@@ -335,6 +337,8 @@ class TwitchChannel : public Channel, public ChannelChatters
     std::unordered_map<QString, std::weak_ptr<MessageThread>> threads_;
 
 protected:
+    void messageRemovedFromStart(const MessagePtr &msg) override;
+
     Atomic<std::shared_ptr<const EmoteMap>> bttvEmotes_;
     Atomic<std::shared_ptr<const EmoteMap>> ffzEmotes_;
     Atomic<std::shared_ptr<const EmoteMap>> seventvEmotes_;