fix: close logging-channels when closing channels (#5592)

Co-authored-by: kornes <28986062+kornes@users.noreply.github.com>
Co-authored-by: Rasmus Karlsson <rasmus.karlsson@pajlada.com>

CHANGELOG.md

@@ -48,6 +48,7 @@
 - Bugfix: Fixed tab visibility being controllable in the emote popup. (#5530)
 - Bugfix: Fixed account switch not being saved if no other settings were changed. (#5558)
 - Bugfix: Fixed some tooltips not being readable. (#5578)
+- Bugfix: Fixed log files being locked longer than needed. (#5592)
 - Dev: Update Windows build from Qt 6.5.0 to Qt 6.7.1. (#5420)
 - Dev: Update vcpkg build Qt from 6.5.0 to 6.7.0, boost from 1.83.0 to 1.85.0, openssl from 3.1.3 to 3.3.0. (#5422)
 - Dev: Unsingletonize `ISoundController`. (#5462)

mocks/include/mocks/Logging.hpp

@@ -18,6 +18,10 @@ public:
                 (const QString &channelName, MessagePtr message,
                  const QString &platformName, const QString &streamID),
                 (override));
+
+    MOCK_METHOD(void, closeChannel,
+                (const QString &channelName, const QString &platformName),
+                (override));
 };
 
 class EmptyLogging : public ILogging
@@ -32,6 +36,12 @@ public:
     {
         //
     }
+
+    void closeChannel(const QString &channelName,
+                      const QString &platformName) override
+    {
+        //
+    }
 };
 
 }  // namespace chatterino::mock

src/Application.cpp

@@ -149,6 +149,7 @@ Application::Application(Settings &_settings, const Paths &paths,
     , args_(_args)
     , themes(new Theme(paths))
     , fonts(new Fonts(_settings))
+    , logging(new Logging(_settings))
     , emotes(new Emotes)
     , accounts(new AccountController)
     , hotkeys(new HotkeyController)
@@ -175,7 +176,6 @@ Application::Application(Settings &_settings, const Paths &paths,
     , ffzEmotes(new FfzEmotes)
     , seventvEmotes(new SeventvEmotes)
     , seventvEventAPI(makeSeventvEventAPI(_settings))
-    , logging(new Logging(_settings))
     , linkResolver(new LinkResolver)
     , streamerMode(new StreamerMode)
     , twitchUsers(new TwitchUsers)
@@ -503,6 +503,7 @@ PubSub *Application::getTwitchPubSub()
 ILogging *Application::getChatLogger()
 {
     assertInGuiThread();
+    assert(this->logging);
 
     return this->logging.get();
 }
@@ -697,4 +698,9 @@ IApplication *getApp()
     return INSTANCE;
 }
 
+IApplication *tryGetApp()
+{
+    return INSTANCE;
+}
+
 }  // namespace chatterino

src/Application.hpp

@@ -144,6 +144,7 @@ public:
 private:
     std::unique_ptr<Theme> themes;
     std::unique_ptr<Fonts> fonts;
+    const std::unique_ptr<Logging> logging;
     std::unique_ptr<Emotes> emotes;
     std::unique_ptr<AccountController> accounts;
     std::unique_ptr<HotkeyController> hotkeys;
@@ -169,7 +170,6 @@ private:
     std::unique_ptr<FfzEmotes> ffzEmotes;
     std::unique_ptr<SeventvEmotes> seventvEmotes;
     std::unique_ptr<SeventvEventAPI> seventvEventAPI;
-    const std::unique_ptr<Logging> logging;
     std::unique_ptr<ILinkResolver> linkResolver;
     std::unique_ptr<IStreamerMode> streamerMode;
     std::unique_ptr<ITwitchUsers> twitchUsers;
@@ -239,4 +239,7 @@ private:
 
 IApplication *getApp();
 
+/// Might return `nullptr` if the app is being destroyed
+IApplication *tryGetApp();
+
 }  // namespace chatterino

src/common/Channel.cpp

@@ -38,6 +38,11 @@ Channel::Channel(const QString &name, Type type)
 
 Channel::~Channel()
 {
+    auto *app = tryGetApp();
+    if (app)
+    {
+        app->getChatLogger()->closeChannel(this->name_, this->platform_);
+    }
     this->destroyed.invoke();
 }
 

src/singletons/Logging.cpp

@@ -2,7 +2,6 @@
 
 #include "messages/Message.hpp"
 #include "singletons/helper/LoggingChannel.hpp"
-#include "singletons/Paths.hpp"
 #include "singletons/Settings.hpp"
 
 #include <QDir>
@@ -58,7 +57,7 @@ void Logging::addMessage(const QString &channelName, MessagePtr message,
         auto map = std::map<QString, std::unique_ptr<LoggingChannel>>();
         this->loggingChannels_[platformName] = std::move(map);
         auto &ref = this->loggingChannels_.at(platformName);
-        ref.emplace(channelName, std::move(channel));
+        ref.emplace(channelName, channel);
         return;
     }
     auto chanIt = platIt->second.find(channelName);
@@ -66,7 +65,7 @@ void Logging::addMessage(const QString &channelName, MessagePtr message,
     {
         auto *channel = new LoggingChannel(channelName, platformName);
         channel->addMessage(message, streamID);
-        platIt->second.emplace(channelName, std::move(channel));
+        platIt->second.emplace(channelName, channel);
     }
     else
     {
@@ -74,4 +73,15 @@ void Logging::addMessage(const QString &channelName, MessagePtr message,
     }
 }
 
+void Logging::closeChannel(const QString &channelName,
+                           const QString &platformName)
+{
+    auto platIt = this->loggingChannels_.find(platformName);
+    if (platIt == this->loggingChannels_.end())
+    {
+        return;
+    }
+    platIt->second.erase(channelName);
+}
+
 }  // namespace chatterino

src/singletons/Logging.hpp

@@ -24,6 +24,9 @@ public:
     virtual void addMessage(const QString &channelName, MessagePtr message,
                             const QString &platformName,
                             const QString &streamID) = 0;
+
+    virtual void closeChannel(const QString &channelName,
+                              const QString &platformName) = 0;
 };
 
 class Logging : public ILogging
@@ -35,6 +38,9 @@ public:
                     const QString &platformName,
                     const QString &streamID) override;
 
+    void closeChannel(const QString &channelName,
+                      const QString &platformName) override;
+
 private:
     using PlatformName = QString;
     using ChannelName = QString;

src/singletons/helper/LoggingChannel.cpp

@@ -40,7 +40,7 @@ QString generateClosingString(
 {
     QString ret("# Stop logging at ");
 
-    ret.append(now.toString("yyyy-MM-dd HH:mm:ss"));
+    ret.append(now.toString("yyyy-MM-dd HH:mm:ss "));
     ret.append(now.timeZoneAbbreviation());
     ret.append(ENDLINE);
 

tests/src/ChannelChatters.cpp

@@ -1,6 +1,8 @@
 #include "common/ChannelChatters.hpp"
 
+#include "mocks/BaseApplication.hpp"
 #include "mocks/Channel.hpp"
+#include "mocks/Logging.hpp"
 #include "Test.hpp"
 
 #include <QColor>
@@ -9,9 +11,28 @@
 using namespace chatterino;
 using chatterino::mock::MockChannel;
 
-// Ensure inserting the same user does not increase the size of the stored colors
-TEST(ChatterChatters, insertSameUser)
+namespace {
+
+class MockApplication : public mock::BaseApplication
 {
+public:
+    MockApplication() = default;
+
+    ILogging *getChatLogger() override
+    {
+        return &this->logging;
+    }
+
+    mock::EmptyLogging logging;
+};
+
+}  // namespace
+
+// Ensure inserting the same user does not increase the size of the stored colors
+TEST(ChannelChatters, insertSameUser)
+{
+    MockApplication app;
+
     MockChannel channel("test");
 
     ChannelChatters chatters(channel);
@@ -24,8 +45,10 @@ TEST(ChatterChatters, insertSameUser)
 }
 
 // Ensure we can update a chatters color
-TEST(ChatterChatters, insertSameUserUpdatesColor)
+TEST(ChannelChatters, insertSameUserUpdatesColor)
 {
+    MockApplication app;
+
     MockChannel channel("test");
 
     ChannelChatters chatters(channel);
@@ -37,8 +60,10 @@ TEST(ChatterChatters, insertSameUserUpdatesColor)
 }
 
 // Ensure getting a non-existant users color returns an invalid QColor
-TEST(ChatterChatters, getNonExistantUser)
+TEST(ChannelChatters, getNonExistantUser)
 {
+    MockApplication app;
+
     MockChannel channel("test");
 
     ChannelChatters chatters(channel);
@@ -47,8 +72,10 @@ TEST(ChatterChatters, getNonExistantUser)
 }
 
 // Ensure getting a user doesn't create an entry
-TEST(ChatterChatters, getDoesNotCreate)
+TEST(ChannelChatters, getDoesNotCreate)
 {
+    MockApplication app;
+
     MockChannel channel("test");
 
     ChannelChatters chatters(channel);
@@ -59,8 +86,10 @@ TEST(ChatterChatters, getDoesNotCreate)
 }
 
 // Ensure the least recently used entry is purged when we reach MAX_SIZE
-TEST(ChatterChatters, insertMaxSize)
+TEST(ChannelChatters, insertMaxSize)
 {
+    MockApplication app;
+
     MockChannel channel("test");
 
     ChannelChatters chatters(channel);

tests/src/Commands.cpp

@@ -55,11 +55,11 @@ public:
         return &this->chatLogger;
     }
 
+    mock::EmptyLogging chatLogger;
     AccountController accounts;
     CommandController commands;
     mock::MockTwitchIrcServer twitch;
     Emotes emotes;
-    mock::EmptyLogging chatLogger;
 };
 
 }  // namespace

tests/src/Filters.cpp

@@ -8,6 +8,7 @@
 #include "mocks/Channel.hpp"
 #include "mocks/ChatterinoBadges.hpp"
 #include "mocks/EmptyApplication.hpp"
+#include "mocks/Logging.hpp"
 #include "mocks/TwitchIrcServer.hpp"
 #include "mocks/UserData.hpp"
 #include "providers/ffz/FfzBadges.hpp"
@@ -75,6 +76,12 @@ public:
         return &this->highlights;
     }
 
+    ILogging *getChatLogger() override
+    {
+        return &this->logging;
+    }
+
+    mock::EmptyLogging logging;
     AccountController accounts;
     Emotes emotes;
     mock::UserDataController userData;

tests/src/InputCompletion.cpp

@@ -8,6 +8,7 @@
 #include "mocks/BaseApplication.hpp"
 #include "mocks/Channel.hpp"
 #include "mocks/Helix.hpp"
+#include "mocks/Logging.hpp"
 #include "mocks/TwitchIrcServer.hpp"
 #include "singletons/Emotes.hpp"
 #include "singletons/Paths.hpp"
@@ -69,6 +70,12 @@ public:
         return &this->seventvEmotes;
     }
 
+    ILogging *getChatLogger() override
+    {
+        return &this->logging;
+    }
+
+    mock::EmptyLogging logging;
     AccountController accounts;
     mock::MockTwitchIrcServer twitch;
     Emotes emotes;
@@ -133,9 +140,9 @@ protected:
 
     void TearDown() override
     {
-        this->mockApplication.reset();
-        this->mockHelix.reset();
         this->channelPtr.reset();
+        this->mockHelix.reset();
+        this->mockApplication.reset();
     }
 
     std::unique_ptr<MockApplication> mockApplication;

tests/src/MessageBuilder.cpp

@@ -7,6 +7,7 @@
 #include "mocks/Channel.hpp"
 #include "mocks/ChatterinoBadges.hpp"
 #include "mocks/DisabledStreamerMode.hpp"
+#include "mocks/Logging.hpp"
 #include "mocks/TwitchIrcServer.hpp"
 #include "mocks/UserData.hpp"
 #include "providers/ffz/FfzBadges.hpp"
@@ -91,6 +92,12 @@ public:
         return &this->seventvEmotes;
     }
 
+    ILogging *getChatLogger() override
+    {
+        return &this->logging;
+    }
+
+    mock::EmptyLogging logging;
     AccountController accounts;
     Emotes emotes;
     mock::UserDataController userData;