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

Add extra context to messages that are added to channels, allowing the logging controller to take more responsibility in what messages to log #5499

Merged
merged 22 commits into from
Jul 13, 2024

Conversation

pajlada
Copy link
Member

@pajlada pajlada commented Jul 7, 2024

I have removed the DoNotLog flags from UserInfoPopup on purpose since all messages added from it are "reposts", i.e. messages that wouldn't be logged anyway

Currently it's the logger that checks whether or not to log reposts - this should just be moved to Channel's addMessage function instead

Fixes #5470

@pajlada pajlada force-pushed the fix/prevent-bad-logging branch from a55a6e4 to 684e93e Compare July 7, 2024 21:24
@pajlada pajlada changed the title fix/prevent bad logging Add extra context to when messages are added to channels, allowing logging controller to take more responsibility in what messages to log Jul 7, 2024
@pajlada pajlada changed the title Add extra context to when messages are added to channels, allowing logging controller to take more responsibility in what messages to log Add extra context to messages that are added to channels, allowing the logging controller to take more responsibility in what messages to log Jul 7, 2024
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/common/Channel.hpp Show resolved Hide resolved
src/controllers/commands/builtin/twitch/SendWhisper.cpp Outdated Show resolved Hide resolved
@pajlada pajlada marked this pull request as ready for review July 13, 2024 09:57
@pajlada
Copy link
Member Author

pajlada commented Jul 13, 2024

image

@pajlada pajlada enabled auto-merge (squash) July 13, 2024 10:49
@pajlada pajlada disabled auto-merge July 13, 2024 10:49
@pajlada pajlada enabled auto-merge (squash) July 13, 2024 10:50
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

@@ -28,6 +28,14 @@ enum class TimeoutStackStyle : int {
Default = DontStackBeyondUserMessage,
};

/// Context of the message being added to a channel
enum class MessageContext {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: enum 'MessageContext' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]

enum class MessageContext {
           ^


if (getSettings()->inlineWhispers &&
!(getSettings()->streamerModeSuppressInlineWhispers &&
getIApp()->getStreamerMode()->isEnabled()))
{
app->getTwitchAbstract()->forEachChannel(
[&messagexD, overrideFlags](ChannelPtr _channel) {
_channel->addMessage(messagexD, overrideFlags);
[&messagexD](ChannelPtr _channel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: the parameter '_channel' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

Suggested change
[&messagexD](ChannelPtr _channel) {
[&messagexD](const ChannelPtr& _channel) {

@pajlada pajlada merged commit 973b7a3 into master Jul 13, 2024
17 checks passed
@pajlada pajlada deleted the fix/prevent-bad-logging branch July 13, 2024 11:15
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.

Opening the usercard logs all of the users messages again
2 participants