Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix log files unlocking when closing chat #4928

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 18 additions & 17 deletions src/common/Channel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@
#include "Application.hpp"
#include "messages/Message.hpp"
#include "messages/MessageBuilder.hpp"
#include "providers/irc/IrcChannel2.hpp"
#include "providers/irc/IrcServer.hpp"
#include "providers/twitch/IrcMessageHandler.hpp"
#include "singletons/Emotes.hpp"
#include "singletons/Logging.hpp"
#include "singletons/Settings.hpp"
Expand All @@ -32,6 +29,13 @@ Channel::Channel(const QString &name, Type type)
, messages_(getSettings()->scrollbackSplitLimit)
, type_(type)
{
this->logFolderName_ = [this]() -> QString {
if (this->isTwitchChannel())
{
return "twitch";
}
return "other";
}();
}

Channel::~Channel()
Expand Down Expand Up @@ -74,6 +78,11 @@ bool Channel::hasMessages() const
return !this->messages_.empty();
}

QString &Channel::logFolderName()
{
return this->logFolderName_;
}

LimitedQueueSnapshot<MessagePtr> Channel::getMessageSnapshot()
{
return this->messages_.getSnapshot();
Expand All @@ -82,26 +91,18 @@ LimitedQueueSnapshot<MessagePtr> Channel::getMessageSnapshot()
void Channel::addMessage(MessagePtr message,
std::optional<MessageFlags> overridingFlags)
{
auto app = getApp();
MessagePtr deleted;

if (!overridingFlags || !overridingFlags->has(MessageFlag::DoNotLog))
{
QString channelPlatform("other");
if (this->type_ == Type::Irc)
if (!this->isLogInitialized_)
{
auto *irc = dynamic_cast<IrcChannel *>(this);
if (irc != nullptr)
{
channelPlatform = QString("irc-%1").arg(
irc->server()->userFriendlyIdentifier());
}
getApp()->logging->addChannel(this->name_, this->logFolderName());
this->isLogInitialized_ = true;
}
else if (this->isTwitchChannel())
{
channelPlatform = "twitch";
}
app->logging->addMessage(this->name_, message, channelPlatform);

getApp()->logging->addMessage(this->name_, message,
this->logFolderName());
}

if (this->messages_.pushBack(message, deleted))
Expand Down
3 changes: 3 additions & 0 deletions src/common/Channel.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ class Channel : public std::enable_shared_from_this<Channel>
virtual bool shouldIgnoreHighlights() const;
virtual bool canReconnect() const;
virtual void reconnect();
virtual QString &logFolderName();
kornes marked this conversation as resolved.
Show resolved Hide resolved

static std::shared_ptr<Channel> getEmpty();

Expand All @@ -118,6 +119,8 @@ class Channel : public std::enable_shared_from_this<Channel>
const QString name_;
LimitedQueue<MessagePtr> messages_;
Type type_;
bool isLogInitialized_{false};
QString logFolderName_;
QTimer clearCompletionModelTimer_;
};

Expand Down
13 changes: 13 additions & 0 deletions src/providers/irc/IrcChannel2.cpp
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
#include "IrcChannel2.hpp"

#include "Application.hpp"
#include "debug/AssertInGuiThread.hpp"
#include "messages/Message.hpp"
#include "messages/MessageBuilder.hpp"
#include "messages/MessageElement.hpp"
#include "providers/irc/IrcCommands.hpp"
#include "providers/irc/IrcMessageBuilder.hpp"
#include "providers/irc/IrcServer.hpp"
#include "singletons/Logging.hpp"
#include "util/Helpers.hpp"

namespace chatterino {
Expand All @@ -15,9 +17,15 @@ IrcChannel::IrcChannel(const QString &name, IrcServer *server)
: Channel(name, Channel::Type::Irc)
, ChannelChatters(*static_cast<Channel *>(this))
, server_(server)
, logFolderName_(QString("irc-%1").arg(server->userFriendlyIdentifier()))
{
}

IrcChannel::~IrcChannel()
{
getApp()->logging->removeChannel(this->getName(), this->logFolderName());
}

void IrcChannel::sendMessage(const QString &message)
{
assertInGuiThread();
Expand Down Expand Up @@ -97,6 +105,11 @@ bool IrcChannel::canReconnect() const
return true;
}

QString &IrcChannel::logFolderName()
{
return this->logFolderName_;
}

void IrcChannel::reconnect()
{
if (this->server())
Expand Down
3 changes: 3 additions & 0 deletions src/providers/irc/IrcChannel2.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class IrcChannel final : public Channel, public ChannelChatters
{
public:
explicit IrcChannel(const QString &name, IrcServer *server);
~IrcChannel();
kornes marked this conversation as resolved.
Show resolved Hide resolved

void sendMessage(const QString &message) override;

Expand All @@ -20,12 +21,14 @@ class IrcChannel final : public Channel, public ChannelChatters

// Channel methods
bool canReconnect() const override;
QString &logFolderName() override;
void reconnect() override;

private:
void setServer(IrcServer *server);

IrcServer *server_;
QString logFolderName_;

friend class Irc;
};
Expand Down
6 changes: 4 additions & 2 deletions src/providers/twitch/TwitchChannel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "providers/twitch/TwitchIrcServer.hpp"
#include "providers/twitch/TwitchMessageBuilder.hpp"
#include "singletons/Emotes.hpp"
#include "singletons/Logging.hpp"
#include "singletons/Settings.hpp"
#include "singletons/Toasts.hpp"
#include "singletons/WindowManager.hpp"
Expand Down Expand Up @@ -237,6 +238,7 @@ TwitchChannel::~TwitchChannel()
getApp()->twitch->seventvEventAPI->unsubscribeTwitchChannel(
this->roomId());
}
getApp()->logging->removeChannel(this->getName(), this->logFolderName());
}

void TwitchChannel::initialize()
Expand Down Expand Up @@ -1251,8 +1253,8 @@ void TwitchChannel::addReplyThread(const std::shared_ptr<MessageThread> &thread)
this->threads_[thread->rootId()] = thread;
}

const std::unordered_map<QString, std::weak_ptr<MessageThread>>
&TwitchChannel::threads() const
const std::unordered_map<QString, std::weak_ptr<MessageThread>> &
TwitchChannel::threads() const
{
return this->threads_;
}
Expand Down
42 changes: 24 additions & 18 deletions src/singletons/Logging.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@
#include "singletons/Paths.hpp"
#include "singletons/Settings.hpp"

#include <QDir>
#include <QStandardPaths>

#include <memory>
#include <utility>

Expand Down Expand Up @@ -49,27 +46,36 @@ void Logging::addMessage(const QString &channelName, MessagePtr message,
}
}

auto platIt = this->loggingChannels_.find(platformName);
if (platIt == this->loggingChannels_.end())
this->loggingChannels_.at(platformName)
.at(channelName)
->addMessage(message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels dangerous. Although all channels initialize themselves, it would be nice if this didn't throw if the platform or the channel can't be found.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw should never happen, because addMessage is called only after loggingChannels_ was populated with same set of args, i would argue it failing instantly is better than hiding potential problems

kornes marked this conversation as resolved.
Show resolved Hide resolved
}

void Logging::addChannel(const QString &channelName,
const QString &platformName)
{
if (!this->loggingChannels_.contains(platformName))
{
auto channel = new LoggingChannel(channelName, platformName);
channel->addMessage(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));
return;
this->loggingChannels_.emplace(
platformName,
std::unordered_map<ChannelName, std::unique_ptr<LoggingChannel>>());
}
auto chanIt = platIt->second.find(channelName);
if (chanIt == platIt->second.end())

if (!this->loggingChannels_.at(platformName).contains(channelName))
{
auto channel = new LoggingChannel(channelName, platformName);
channel->addMessage(message);
platIt->second.emplace(channelName, std::move(channel));
this->loggingChannels_.at(platformName)
.emplace(channelName,
std::unique_ptr<LoggingChannel>(std::move(channel)));
kornes marked this conversation as resolved.
Show resolved Hide resolved
}
else
}

void Logging::removeChannel(const QString &channelName,
const QString &platformName)
{
if (this->loggingChannels_.contains(platformName))
{
chanIt->second->addMessage(message);
this->loggingChannels_.at(platformName).erase(channelName);
}
}

Expand Down
9 changes: 6 additions & 3 deletions src/singletons/Logging.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@

#include <QString>

#include <map>
#include <memory>
#include <unordered_map>
#include <unordered_set>

namespace chatterino {
Expand All @@ -28,12 +28,15 @@ class Logging : public Singleton

void addMessage(const QString &channelName, MessagePtr message,
const QString &platformName);
void addChannel(const QString &channelName, const QString &platformName);
void removeChannel(const QString &channelName, const QString &platformName);

private:
using PlatformName = QString;
using ChannelName = QString;
std::map<PlatformName,
std::map<ChannelName, std::unique_ptr<LoggingChannel>>>
std::unordered_map<
PlatformName,
std::unordered_map<ChannelName, std::unique_ptr<LoggingChannel>>>
loggingChannels_;

// Keeps the value of the `loggedChannels` settings
Expand Down
Loading