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

Closed
wants to merge 6 commits into from

Conversation

kornes
Copy link
Contributor

@kornes kornes commented Oct 31, 2023

Issue

Currently log files are never unlocked, to clear logs of closed chat user needs to close chatterino.

Cause

LogginChannel destructor is never called, because channels are currently never removed from map in Logging singleton

Fix

  • add Logging::removeChannel method and call it when TwitchChannel or IrcChannel2 are destructed, which happens when last split of given channel is closed

Other changes

  • move channelPlatform QString creation away from Channel::addMessage (frequent code path) to constructor made logFolderName_
  • add virtual Channel::logFolderName() and override it for IRC channels
  • move map inserting logic from Logging::addMessage to its own Logging::addChannel, and call it once per Channel instance
  • remove unused imports
  • change loggingChannels_ from map to unordered_map

Copy link
Contributor

@Nerixyz Nerixyz left a comment

Choose a reason for hiding this comment

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

Chatterino now creates log files for all open channels, even when logging is disabled.

src/common/Channel.hpp Outdated Show resolved Hide resolved
Comment on lines 49 to 51
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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/providers/irc/IrcChannel2.hpp Outdated Show resolved Hide resolved
src/singletons/Logging.cpp Outdated Show resolved Hide resolved
src/singletons/Logging.cpp Outdated Show resolved Hide resolved
@kornes kornes marked this pull request as draft October 31, 2023 17:13
@kornes
Copy link
Contributor Author

kornes commented Sep 13, 2024

taken over by @Nerixyz

@kornes kornes closed this Sep 13, 2024
@kornes kornes deleted the fix/log-file-locking branch September 13, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants