From d74ae463f840aed2ceb4126fef66d6e21c371b70 Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Fri, 13 Sep 2024 22:08:58 +0200 Subject: [PATCH 1/9] fix: close logging-channels when closing channels Co-authored-by: kornes <28986062+kornes@users.noreply.github.com> --- mocks/include/mocks/Logging.hpp | 10 +++++++++ src/Application.cpp | 5 +++++ src/Application.hpp | 3 +++ src/common/Channel.cpp | 5 +++++ src/singletons/Logging.cpp | 11 ++++++++++ src/singletons/Logging.hpp | 6 +++++ tests/src/ChannelChatters.cpp | 39 ++++++++++++++++++++++++++++----- tests/src/Filters.cpp | 7 ++++++ tests/src/InputCompletion.cpp | 7 ++++++ tests/src/MessageBuilder.cpp | 7 ++++++ 10 files changed, 95 insertions(+), 5 deletions(-) diff --git a/mocks/include/mocks/Logging.hpp b/mocks/include/mocks/Logging.hpp index 0f4dba38a18..78c77ef0c6b 100644 --- a/mocks/include/mocks/Logging.hpp +++ b/mocks/include/mocks/Logging.hpp @@ -18,6 +18,10 @@ class Logging : public ILogging (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 @@ class EmptyLogging : public ILogging { // } + + void closeChannel(const QString &channelName, + const QString &platformName) override + { + // + } }; } // namespace chatterino::mock diff --git a/src/Application.cpp b/src/Application.cpp index d781dbac7de..fd14b103511 100644 --- a/src/Application.cpp +++ b/src/Application.cpp @@ -697,4 +697,9 @@ IApplication *getApp() return INSTANCE; } +IApplication *tryGetApp() +{ + return INSTANCE; +} + } // namespace chatterino diff --git a/src/Application.hpp b/src/Application.hpp index 2f6b11a3f33..1c3c5aad6ed 100644 --- a/src/Application.hpp +++ b/src/Application.hpp @@ -239,4 +239,7 @@ class Application : public IApplication IApplication *getApp(); +/// Might return `nullptr` if the app is being destroyed +IApplication *tryGetApp(); + } // namespace chatterino diff --git a/src/common/Channel.cpp b/src/common/Channel.cpp index a74aa32c49e..ef778bad15a 100644 --- a/src/common/Channel.cpp +++ b/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(); } diff --git a/src/singletons/Logging.cpp b/src/singletons/Logging.cpp index 138efd38a4c..859f1d7beb9 100644 --- a/src/singletons/Logging.cpp +++ b/src/singletons/Logging.cpp @@ -74,4 +74,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 diff --git a/src/singletons/Logging.hpp b/src/singletons/Logging.hpp index 984932ad328..0af72386ee2 100644 --- a/src/singletons/Logging.hpp +++ b/src/singletons/Logging.hpp @@ -24,6 +24,9 @@ class ILogging 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 @@ class Logging : public ILogging const QString &platformName, const QString &streamID) override; + void closeChannel(const QString &channelName, + const QString &platformName) override; + private: using PlatformName = QString; using ChannelName = QString; diff --git a/tests/src/ChannelChatters.cpp b/tests/src/ChannelChatters.cpp index 79711ce15af..f3975ec1653 100644 --- a/tests/src/ChannelChatters.cpp +++ b/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 @@ -9,9 +11,28 @@ using namespace chatterino; using chatterino::mock::MockChannel; +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(ChatterChatters, insertSameUser) +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); diff --git a/tests/src/Filters.cpp b/tests/src/Filters.cpp index dd592b3b6ee..7504c393b2f 100644 --- a/tests/src/Filters.cpp +++ b/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,11 @@ class MockApplication : public mock::BaseApplication return &this->highlights; } + ILogging *getChatLogger() override + { + return &this->logging; + } + AccountController accounts; Emotes emotes; mock::UserDataController userData; @@ -83,6 +89,7 @@ class MockApplication : public mock::BaseApplication FfzBadges ffzBadges; SeventvBadges seventvBadges; HighlightController highlights; + mock::EmptyLogging logging; }; class FiltersF : public ::testing::Test diff --git a/tests/src/InputCompletion.cpp b/tests/src/InputCompletion.cpp index 460fb152630..7fc970b6c53 100644 --- a/tests/src/InputCompletion.cpp +++ b/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,12 +70,18 @@ class MockApplication : public mock::BaseApplication return &this->seventvEmotes; } + ILogging *getChatLogger() override + { + return &this->logging; + } + AccountController accounts; mock::MockTwitchIrcServer twitch; Emotes emotes; BttvEmotes bttvEmotes; FfzEmotes ffzEmotes; SeventvEmotes seventvEmotes; + mock::EmptyLogging logging; }; } // namespace diff --git a/tests/src/MessageBuilder.cpp b/tests/src/MessageBuilder.cpp index 39f01f196a4..5b8a9290eac 100644 --- a/tests/src/MessageBuilder.cpp +++ b/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,11 @@ class MockApplication : public mock::BaseApplication return &this->seventvEmotes; } + ILogging *getChatLogger() override + { + return &this->logging; + } + AccountController accounts; Emotes emotes; mock::UserDataController userData; @@ -102,6 +108,7 @@ class MockApplication : public mock::BaseApplication BttvEmotes bttvEmotes; FfzEmotes ffzEmotes; SeventvEmotes seventvEmotes; + mock::EmptyLogging logging; }; } // namespace From 09591926cecc01f260d768f81a5f2deac23b4ef3 Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Fri, 13 Sep 2024 22:12:27 +0200 Subject: [PATCH 2/9] fix: add missing space on stop message --- src/singletons/helper/LoggingChannel.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/singletons/helper/LoggingChannel.cpp b/src/singletons/helper/LoggingChannel.cpp index dcce92f234d..c27cb4c3a9c 100644 --- a/src/singletons/helper/LoggingChannel.cpp +++ b/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); From 6e57e547dc172540c6dba76d372a55fba3b7ba22 Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Fri, 13 Sep 2024 22:12:43 +0200 Subject: [PATCH 3/9] fix: remove `std::move` of pointers --- src/singletons/Logging.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/singletons/Logging.cpp b/src/singletons/Logging.cpp index 859f1d7beb9..7fef6c299e0 100644 --- a/src/singletons/Logging.cpp +++ b/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 @@ -58,7 +57,7 @@ void Logging::addMessage(const QString &channelName, MessagePtr message, auto map = std::map>(); 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 { From 09859faaef0042ec2a8e9d073a34620a6805e968 Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Fri, 13 Sep 2024 22:21:58 +0200 Subject: [PATCH 4/9] chore: add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11cd233788d..7cee048904a 100644 --- a/CHANGELOG.md +++ b/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) From 5802dc74c69369dae6f42f1f39456a2c8c45f5ac Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Sat, 14 Sep 2024 11:00:18 +0200 Subject: [PATCH 5/9] let's print the stack! --- src/singletons/Logging.cpp | 12 ++++++++++++ src/singletons/Logging.hpp | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/singletons/Logging.cpp b/src/singletons/Logging.cpp index 7fef6c299e0..0d576b857cf 100644 --- a/src/singletons/Logging.cpp +++ b/src/singletons/Logging.cpp @@ -4,6 +4,7 @@ #include "singletons/helper/LoggingChannel.hpp" #include "singletons/Settings.hpp" +#include #include #include @@ -12,6 +13,17 @@ namespace chatterino { +void ILogging::closeChannel(const QString & /*channelName*/, + const QString & /*platform*/) +{ + auto st = boost::stacktrace::stacktrace(); + qFatal() << "closeChannel called without ILogging instance"; +#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0) // idc about qt 5 + qFatal() << boost::stacktrace::to_string(st); +#endif + assert(false); +} + Logging::Logging(Settings &settings) { // We can safely ignore this signal connection since settings are only-ever destroyed diff --git a/src/singletons/Logging.hpp b/src/singletons/Logging.hpp index 0af72386ee2..66333907db5 100644 --- a/src/singletons/Logging.hpp +++ b/src/singletons/Logging.hpp @@ -26,7 +26,7 @@ class ILogging const QString &streamID) = 0; virtual void closeChannel(const QString &channelName, - const QString &platformName) = 0; + const QString &platformName); }; class Logging : public ILogging From eb589c1f3741c6a31a2cb30282c04ab3569b70f2 Mon Sep 17 00:00:00 2001 From: Rasmus Karlsson Date: Sat, 14 Sep 2024 11:31:28 +0200 Subject: [PATCH 6/9] move Logging initialization higher up in Application --- src/Application.cpp | 2 +- src/Application.hpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Application.cpp b/src/Application.cpp index fd14b103511..4ff800ab9d2 100644 --- a/src/Application.cpp +++ b/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) diff --git a/src/Application.hpp b/src/Application.hpp index 1c3c5aad6ed..71145b162a4 100644 --- a/src/Application.hpp +++ b/src/Application.hpp @@ -144,6 +144,7 @@ class Application : public IApplication private: std::unique_ptr themes; std::unique_ptr fonts; + const std::unique_ptr logging; std::unique_ptr emotes; std::unique_ptr accounts; std::unique_ptr hotkeys; @@ -169,7 +170,6 @@ class Application : public IApplication std::unique_ptr ffzEmotes; std::unique_ptr seventvEmotes; std::unique_ptr seventvEventAPI; - const std::unique_ptr logging; std::unique_ptr linkResolver; std::unique_ptr streamerMode; std::unique_ptr twitchUsers; From 53d74cd7af0509f567447a5f2e8089b4334230f3 Mon Sep 17 00:00:00 2001 From: Rasmus Karlsson Date: Sat, 14 Sep 2024 11:31:47 +0200 Subject: [PATCH 7/9] fix(tests): always initialize logger before anything that might hold a channel --- tests/src/Commands.cpp | 2 +- tests/src/Filters.cpp | 2 +- tests/src/InputCompletion.cpp | 6 +++--- tests/src/MessageBuilder.cpp | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/src/Commands.cpp b/tests/src/Commands.cpp index 420f71fdf5c..f1b872b7dc1 100644 --- a/tests/src/Commands.cpp +++ b/tests/src/Commands.cpp @@ -55,11 +55,11 @@ class MockApplication : public mock::BaseApplication return &this->chatLogger; } + mock::EmptyLogging chatLogger; AccountController accounts; CommandController commands; mock::MockTwitchIrcServer twitch; Emotes emotes; - mock::EmptyLogging chatLogger; }; } // namespace diff --git a/tests/src/Filters.cpp b/tests/src/Filters.cpp index 7504c393b2f..af230454619 100644 --- a/tests/src/Filters.cpp +++ b/tests/src/Filters.cpp @@ -81,6 +81,7 @@ class MockApplication : public mock::BaseApplication return &this->logging; } + mock::EmptyLogging logging; AccountController accounts; Emotes emotes; mock::UserDataController userData; @@ -89,7 +90,6 @@ class MockApplication : public mock::BaseApplication FfzBadges ffzBadges; SeventvBadges seventvBadges; HighlightController highlights; - mock::EmptyLogging logging; }; class FiltersF : public ::testing::Test diff --git a/tests/src/InputCompletion.cpp b/tests/src/InputCompletion.cpp index 7fc970b6c53..1f219570c49 100644 --- a/tests/src/InputCompletion.cpp +++ b/tests/src/InputCompletion.cpp @@ -75,13 +75,13 @@ class MockApplication : public mock::BaseApplication return &this->logging; } + mock::EmptyLogging logging; AccountController accounts; mock::MockTwitchIrcServer twitch; Emotes emotes; BttvEmotes bttvEmotes; FfzEmotes ffzEmotes; SeventvEmotes seventvEmotes; - mock::EmptyLogging logging; }; } // namespace @@ -140,9 +140,9 @@ class InputCompletionTest : public ::testing::Test void TearDown() override { - this->mockApplication.reset(); - this->mockHelix.reset(); this->channelPtr.reset(); + this->mockHelix.reset(); + this->mockApplication.reset(); } std::unique_ptr mockApplication; diff --git a/tests/src/MessageBuilder.cpp b/tests/src/MessageBuilder.cpp index 5b8a9290eac..9a3030822fd 100644 --- a/tests/src/MessageBuilder.cpp +++ b/tests/src/MessageBuilder.cpp @@ -97,6 +97,7 @@ class MockApplication : public mock::BaseApplication return &this->logging; } + mock::EmptyLogging logging; AccountController accounts; Emotes emotes; mock::UserDataController userData; @@ -108,7 +109,6 @@ class MockApplication : public mock::BaseApplication BttvEmotes bttvEmotes; FfzEmotes ffzEmotes; SeventvEmotes seventvEmotes; - mock::EmptyLogging logging; }; } // namespace From a0bada9357c1834013e1a04a33fc6955d6348779 Mon Sep 17 00:00:00 2001 From: Rasmus Karlsson Date: Sat, 14 Sep 2024 11:32:49 +0200 Subject: [PATCH 8/9] revert closeChannel logging --- src/singletons/Logging.cpp | 12 ------------ src/singletons/Logging.hpp | 2 +- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/src/singletons/Logging.cpp b/src/singletons/Logging.cpp index 0d576b857cf..7fef6c299e0 100644 --- a/src/singletons/Logging.cpp +++ b/src/singletons/Logging.cpp @@ -4,7 +4,6 @@ #include "singletons/helper/LoggingChannel.hpp" #include "singletons/Settings.hpp" -#include #include #include @@ -13,17 +12,6 @@ namespace chatterino { -void ILogging::closeChannel(const QString & /*channelName*/, - const QString & /*platform*/) -{ - auto st = boost::stacktrace::stacktrace(); - qFatal() << "closeChannel called without ILogging instance"; -#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0) // idc about qt 5 - qFatal() << boost::stacktrace::to_string(st); -#endif - assert(false); -} - Logging::Logging(Settings &settings) { // We can safely ignore this signal connection since settings are only-ever destroyed diff --git a/src/singletons/Logging.hpp b/src/singletons/Logging.hpp index 66333907db5..0af72386ee2 100644 --- a/src/singletons/Logging.hpp +++ b/src/singletons/Logging.hpp @@ -26,7 +26,7 @@ class ILogging const QString &streamID) = 0; virtual void closeChannel(const QString &channelName, - const QString &platformName); + const QString &platformName) = 0; }; class Logging : public ILogging From 506ade63e250365cd600273f563a0fe1016a5d87 Mon Sep 17 00:00:00 2001 From: Rasmus Karlsson Date: Sat, 14 Sep 2024 11:43:24 +0200 Subject: [PATCH 9/9] assert logging isn't nullptr --- src/Application.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Application.cpp b/src/Application.cpp index 4ff800ab9d2..8cb2211015b 100644 --- a/src/Application.cpp +++ b/src/Application.cpp @@ -503,6 +503,7 @@ PubSub *Application::getTwitchPubSub() ILogging *Application::getChatLogger() { assertInGuiThread(); + assert(this->logging); return this->logging.get(); }