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

Handle follower emotes differently if subscribed #4922

Merged
merged 11 commits into from
Nov 4, 2023

Conversation

baines
Copy link
Contributor

@baines baines commented Oct 28, 2023

If we also have a subscriber emote set for a channel, then don't treat a follower emote set as local to that channel.

Description

On Twitch it's possible to use a channel's follower emotes everywhere if also subscribed to the followed channel.
But Chatterino did not show the emotes as available in other channels or allow them to be auto-completed in this case.

This PR makes a small change so that if we also have a subscriber emote set for a channel, then the follower emote set is not treated as local.

I know there's another PR to implement this behaviour (#4281) but I'm not confident enough with the changes in there to pick that one up.

This code doesn't attempt to convert an existing local set into a non-local one or vice versa (when subscription is gained/lost). I don't know if twitch sends another GLOBALUSERSTATE with the emote sets when that happens or what.

It's also making an assumption that loadUserstateEmotes is called only once after all the emote sets are known. If they can come in "piecemeal" then it might not find the subscriber set if it's in a later batch than the follower one.

(felanbird): Closes #4233

If we also have a subscriber emote set for a channel, then don't treat
a follower emote set as local to that channel.
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/twitch/TwitchAccount.cpp Show resolved Hide resolved
src/providers/twitch/TwitchAccount.cpp Outdated Show resolved Hide resolved
src/providers/twitch/TwitchAccount.cpp Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Felanbird
Copy link
Collaborator

Those suggestions are all shit you can go ahead and revert them, the bot doesn't always get it right

Copy link
Contributor

@iProdigy iProdigy left a comment

Choose a reason for hiding this comment

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

nice work

src/providers/twitch/TwitchAccount.cpp Show resolved Hide resolved
src/providers/twitch/TwitchAccount.cpp Show resolved Hide resolved
@iProdigy
Copy link
Contributor

Also, we could hook on subscription event to update local status for the relevant follower emotes (but this can happen in a separate PR)

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.

Seems good. Just a note: Twitch announced a Get User Emotes endpoint in their Standard Output stream. However, it isn't clear how this will handle follower emotes.

src/providers/twitch/TwitchAccount.cpp Outdated Show resolved Hide resolved
src/providers/twitch/TwitchAccount.cpp Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Felanbird Felanbird left a comment

Choose a reason for hiding this comment

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

I'm happy with this change, prevent the double serialization on merge if anyone really cares (they probably do) but I'm glad to have a working solution to this.

Copy link
Contributor

@iProdigy iProdigy left a comment

Choose a reason for hiding this comment

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

Great first contribution!

src/providers/twitch/TwitchAccount.cpp Show resolved Hide resolved
Co-authored-by: iProdigy <8106344+iProdigy@users.noreply.github.com>
@Mm2PL Mm2PL self-requested a review October 31, 2023 15:59
@pajlada
Copy link
Member

pajlada commented Nov 4, 2023

@Mm2PL want to take a peek?

Copy link
Collaborator

@Mm2PL Mm2PL left a comment

Choose a reason for hiding this comment

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

LGTM, works as expected. Thank you for your contribution.

@pajlada pajlada enabled auto-merge (squash) November 4, 2023 17:19
@pajlada pajlada merged commit 879a63e into Chatterino:master Nov 4, 2023
15 checks passed
@pajlada
Copy link
Member

pajlada commented Nov 4, 2023

Thank you @baines! As a first-time contributor, you can now add yourself to the contributors list that's shown inside the About page in Chatterino.

If you want this, you can open a new PR where you modify the resources/contributors.txt file and add yourself to the list. Make sure to read the comments at the top for instructions.

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.

Update Follower Emote Handling
6 participants