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 highlight color and show in mentions to automod messages #5215

Merged
merged 7 commits into from
Mar 9, 2024

Conversation

KleberPF
Copy link
Contributor

@KleberPF KleberPF commented Feb 28, 2024

For now, the added/removed term messages don't show up in /mentions, but I'll add a commit doing that if we decide to do it.

Previous messages change color when the user changes settings

image
image

Scrollbar color

image

Scrollbar color also changes when settings are changed

image

Message showing up in mentions

image

Message doesn't show up in mentions if setting is disabled

image
image

I was hitting an assert when allowing/denying a message, but I tested in master and it also happens there. It seems to be caused by the request created in Helix::manageAutoModMessages not being concurrent, but I didn't look too much into it. This was the call stack when the assert triggered

image

Fixes: #5206

@Nerixyz
Copy link
Contributor

Nerixyz commented Feb 29, 2024

You didn't cause the assertion issue: isInStreamerMode is called in a PubSub handler:

isInStreamerMode())

The PubSub handler is running in it's own thread - not the GUI thread. isInStreamerMode isn't annotated, but has to run on a single thread as it accesses cache. This thread must be the GUI thread as it emits streamerModeChanged.

@KleberPF
Copy link
Contributor Author

Oh, nice. I think I mixed some things up. I put a breakpoint in isGuiThread for when the result was false, but this function isn't called just inside assertInGuiThread, and it ends up begin called by some functions in NetworkData. Ended up posting the correct call stack

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

I think the "header", i.e. message that contains the Allow & Deny buttons, should be colored based on the AutomodHighlight

@KleberPF
Copy link
Contributor Author

KleberPF commented Mar 3, 2024

I think the "header", i.e. message that contains the Allow & Deny buttons, should be colored based on the AutomodHighlight

Is it ok to set MessageFlag::AutoModOffendingMessage for that message or should I make a new flag?

@Nerixyz
Copy link
Contributor

Nerixyz commented Mar 3, 2024

Is it ok to set MessageFlag::AutoModOffendingMessage for that message or should I make a new flag?

The issue with reusing the flag is that you'd create two highlights. You can avoid creating a new flag by checking for MessageFlag::Timeout, which is only set for offending messages, and the "header" in combination with AutoMod.

@KleberPF
Copy link
Contributor Author

KleberPF commented Mar 5, 2024

The issue with reusing the flag is that you'd create two highlights. You can avoid creating a new flag by checking for MessageFlag::Timeout, which is only set for offending messages, and the "header" in combination with AutoMod.

Something like this?

if (ctx.preferences.enableAutomodHighlight && 
    (this->message_->flags.has(MessageFlag::AutoModOffendingMessage) || 
     this->message_->flags.has(MessageFlag::Timeout))

checking for MessageFlag::Timeout, which is only set for offending messages

Is it a good idea to rely on this? I feel like it could break something in the future (I guess it's unlikely because we would need the same combination of flags)

@Nerixyz
Copy link
Contributor

Nerixyz commented Mar 5, 2024

Is it a good idea to rely on this? I feel like it could break something in the future (I guess it's unlikely because we would the same combination of flags)

Yea, it's probably better to have a new flag.

@KleberPF
Copy link
Contributor Author

KleberPF commented Mar 6, 2024

Added coloring for the message header
image
in mentions
image

@Mm2PL
Copy link
Collaborator

Mm2PL commented Mar 6, 2024

Is it intended that AutoMod's "Hey! Your message is being checked" is not customizable with your changes? Those messages only have the PubSub flag set.
https://github.com/Chatterino/chatterino2/blob/master/src/providers/twitch/TwitchMessageBuilder.cpp#L4964

@KleberPF
Copy link
Contributor Author

KleberPF commented Mar 6, 2024

Is it intended that AutoMod's "Hey! Your message is being checked" is not customizable with your changes?

Good question. I interpreted the issue as "highlight that automod caught someone else's message", not "highlight that automod caught your message", but I'm not sure about the original intent.

@Mm2PL
Copy link
Collaborator

Mm2PL commented Mar 6, 2024

They look really similar right now on master and are from the same source. I am of the opinion that they should be thrown into the same bag as mod-side automod messages. I don't think the issue author even considered their existence. Interestingly enough currently these messages don't show up in /automod.

@pajlada
Copy link
Member

pajlada commented Mar 6, 2024

I interpreted this as a moderator/broadcaster tool, so that's what I think is important

@Mm2PL Mm2PL added this to the Investigate before 2.4.7 milestone Mar 7, 2024
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Thank you @KleberPF !
Please don't forget to make a separate PR adding yourself into the About page :)

@pajlada pajlada enabled auto-merge (squash) March 9, 2024 10:41
@pajlada pajlada merged commit c507919 into Chatterino:master Mar 9, 2024
17 checks passed
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.

Make "AutoMod Caught Messages" highlight color & "show in mentions" configurable
4 participants