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

Migrate viewer list to helix #4117

Merged
merged 26 commits into from
Mar 27, 2023

Conversation

cbclemmer
Copy link
Contributor

@cbclemmer cbclemmer commented Nov 5, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

This uses the new helix endpoints for mods, vips, and chatters to get the viewer list via the helix api instead of the tmi endpoint which may go out of date whenever the irc commands are deprecated. It basically future proofs the viewer list so that if the tmi endpoint is ever randomly deprecated the user can change a setting and it will work for mods and the broadcaster. This is opt in only and will still use the tmi endpoint by default using the same time gate functionality as we use for the irc commands. The setting is located with the other time gate settings at the bottom of the general settings tab. I intentionally copied and pasted most of the Split::showViewerList function instead of making a better integrated function for both use cases so that whenever it is deprecated the transition to delete the old code will be as painless as possible.

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/widgets/settingspages/GeneralPage.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp 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.

The approach you've gone with is fair, but it's not the intention of the viewer list, which is to show the people currently(within reason) in chat.

Due to the restrictions of the response we are given, this is the approach I have imagined:

  1. Get the chatters list
  2. Get the mods list
  3. Get the vips list

// for broadcasters
If any of the users from the chatters response are also in the mods list, they are listed in the mods section
If any of the users from the chatters response are also in the vip list, they are listed in the vips section
Everyone else from the chatters response in the Viewers section

// for mods
If the broadcaster is in the Chatters response, place them at the top
The mods section should note that Chatterino can no longer check who is a moderator
The vips section should note Chatterino can no longer check who is a VIP
(or remove these sections when it's a mod)
Everyone else from the chatters response in the Viewers section

// for plebs
Due to Twitch restrictions, this feature is only available for moderators. If you would like to see the Viewer list, you must use the Twitch website.

image

{"data":[{"user_login":"felanbird"},{"user_login":"fossabot"},{"user_login":"kla
un85"},{"user_login":"ohbot"},{"user_login":"scriptorex"},{"user_login":"streame
lements"}],"pagination":{},"total":6}

@cbclemmer cbclemmer force-pushed the cc/helix-viewer-list branch from bbb5fac to 0892ba2 Compare November 6, 2022 14:36
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/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
@cbclemmer
Copy link
Contributor Author

@Felanbird New stuff:

  • The viewer list will now only show mods and vips returned from the chatter list.
  • The chatters section will not contain mods and vips.
  • Warning text that you outlined are now visible.

@cbclemmer
Copy link
Contributor Author

I'm not a mod in any large channel so you probably want to test this yourself.

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/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
@Felanbird
Copy link
Collaborator

@Felanbird New stuff:

* The viewer list will now only show mods and vips returned from the chatter list.
* The chatters section will not contain mods and vips.

image

* Warning text that you outlined are now visible.


Implementation as of this comment loads fairly fast as a mod for 3,701 chatters

Additional:

Mods should be lowercase, display name capitalization looks out of place with the rest of the list being lowercase.
Broadcaster should not be listed if they are not in the chatters response
All sections should be alphabetized

src/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/GeneralPage.cpp Outdated Show resolved Hide resolved
@cbclemmer
Copy link
Contributor Author

@Felanbird I changed the vectors to QStringLists which had better functionality.

  • All username should now be in lower case
  • Everything should be sorted

@cbclemmer
Copy link
Contributor Author

cbclemmer commented Nov 7, 2022

All sections should be alphabetized

@Felanbird I'm not sure what you mean

@cbclemmer
Copy link
Contributor Author

And broadcaster shouldn't be in list now if it wasn't returned from the chatters response.

@Felanbird
Copy link
Collaborator

Felanbird commented Nov 7, 2022

All sections should be alphabetized

@Felanbird I'm not sure what you mean

If possible the sections should be displayed 0-9 A-Z, to match the old TMI list
https://user-images.githubusercontent.com/41973452/200418308-e9fc9366-9fca-484d-8c3c-3d71c730c735.png
You have already done this

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/widgets/settingspages/GeneralPage.cpp Outdated Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Outdated Show resolved Hide resolved
@Felanbird
Copy link
Collaborator

Felanbird commented Nov 7, 2022

  • All usernames are lowercase
  • Broadcaster should not be listed if they are not in the chatters response
  • All sections should be alphabetized
  • The viewer list only shows mods and VIPs returned from the chatter list.
    (ignore the lack of broadcaster, it does work, Twitch was just having an an xd at the moment of testing)
    image

@cbclemmer
Copy link
Contributor Author

@Felanbird I missed the excluding vips and mods from the main viewer list. That should be fixed now. Also:

  • I changed the "Viewers" section to "Chatters" to better reflect what is actually happening
  • If a chatter is both a mod and a vip, they only show up in the mods list. I can change that if need be.

@cbclemmer
Copy link
Contributor Author

Is there anything still waiting to be done for this PR?

@Felanbird
Copy link
Collaborator

the longer he doesn't remember to review this the longer we get to keep the viewer list 👺

🙂

@pajlada
Copy link
Member

pajlada commented Nov 15, 2022

Is there anything still waiting to be done for this PR?

No - everything in this PR seems good. I'm holding off on it a bit to do a proper review/test & evaluation

@cbclemmer
Copy link
Contributor Author

fwiw @Felanbird I agree with you lol

@goldbattle
Copy link
Contributor

Is there a reason that some moderators, vips, and chatters can't be directly filled in based on the people that have talked in the last x minutes in the chat? This is information that is already provided by just being in the chat room. Maybe displaying the username (last chatted X minutes ago) in the list would be really nice (maybe this is worth just doing a separate PR on?).

@Felanbird
Copy link
Collaborator

Is there a reason that some moderators, vips, and chatters can't be directly filled in based on the people that have talked in the last x minutes in the chat? This is information that is already provided by just being in the chat room. Maybe displaying the username (last chatted X minutes ago) in the list would be really nice (maybe this is worth just doing a separate PR on?).

It would definitely be a separate PR thing, if done.

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.

Seems to still work as I would expect it to 👍

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/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
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/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
src/widgets/splits/Split.cpp Show resolved Hide resolved
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.

Migrate the Viewer List and the /chatters command to Helix API
5 participants