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 Twitch Chat Pronouns badge provider #3346

Closed
wants to merge 3 commits into from
Closed

Add Twitch Chat Pronouns badge provider #3346

wants to merge 3 commits into from

Conversation

jakemiki
Copy link

@jakemiki jakemiki commented Nov 10, 2021

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Adds badge provider for Twitch Chat Pronouns (https://pronouns.alejo.io/) API.
Pronouns are shown as a small text before username.
Pronoun badges are not displayed for historic messages to limit requests on startup.
Requests are made lazily, once when a user sends a message, so badge won't show for user's first message in current session.

image

This is a very naive implementation, so let me know if anything can be done better.

Gets pronoun information from https://pronouns.alejo.io/ API and shows it as a text badge in chat
@jakemiki jakemiki marked this pull request as ready for review November 10, 2021 20:20
@Felanbird Felanbird self-requested a review November 10, 2021 20:23
@Mm2PL Mm2PL self-requested a review November 10, 2021 20:49
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 would like to approve this PR, the problem is the push-back against this feature is fairly strong.

As I mentioned in #3192 (comment), for the purpose of users this feature is "covered" by nicknames. The only major issue with this approach is nicknames are lost with setting corruption issues on windows (when incorrectly shutting down a PC).

Also as I noted in that comment chain, the API itself doesn't seem like it can keep up with as many requests as we would be sending. When it comes to my own side-by-side testing of this version and another approach at this feature, I'm unable to get consistency between both versions.
image

It's my belief that the only way this will be added to the main branch of Chatterino, it will be via the introduction of a plugin system.

As for coding functionality, I'm unable to give you any tips, all I can do is direct you to the other attempt at the pronoun build, (found here) encase you would like to keep utilizing this feature from your own changes and see a coding practice being used that might spark your interest.

Visual functionality on the other hand, I do like your approach including the pronoun similar to nicknames including the users color, in comparison to the other build which looks like this.
image
One note I can give would be looking into aligning the pronoun with the username.

I will leave the final say to @pajlada - but I would just assume the answer will be a PR denial.

@Felanbird Felanbird requested a review from pajlada November 10, 2021 21:04
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.

These are the issues I found during testing:

  1. IMO It should be an opt-in, not opt-out thing.
  2. This executes LOADS OF HTTP requests even while off. Those requests are using bandwidth which the poor server owner has to pay for. If disabled this effectively wastes that bandwidth.
  3. Please add the new files to src/CMakeLists.txt so CMake can actually compile Chatterino.

As for my opinion on the feature: I dislike having them in chat. I'd honestly prefer having pronouns tucked somewhere in the usercard if anything. While I agree that this may help some people, I don't feel like it would help me personally. When meeting someone new I'd either go with relatively neutral they/them or just type out the username.

@jakemiki
Copy link
Author

Thank you for your responses. I looked through PRs and issues but somehow missed the previous attempt in discussions.

I noticed that the API might be a problem, but figured that if the extension and FFZ addon was designed to work with it in it's current form it could handle the potential load. I guess only hope in this case is some kind of official implementation.

I agree that it would be best suited as an optional plugin due to it being rather heavy and possibly unwanted by a lot of users. My motivation came from watching a few streamers that mentioned the extension and are using chatterino, but in the grand scheme of things it still is a rather niche audience.

I'll make the requested changes, to at least leave it in a better state, even if PR is denied.

@@ -582,6 +582,7 @@ void GeneralPage::initLayout(GeneralPageView &layout)
layout.addCheckbox("Chatterino", s.showBadgesChatterino);
layout.addCheckbox("FrankerFaceZ (Bot, FFZ Supporter, FFZ Developer)",
s.showBadgesFfz);
layout.addCheckbox("Pronouns", s.showBadgesPronouns);
Copy link
Contributor

@TETYYS TETYYS Nov 10, 2021

Choose a reason for hiding this comment

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

Other 3rd party badge settings indicate which service they are from (or badges are named after the service itself), I think it would be good to somehow indicate that Pronouns badges are provided by alejo.io

@pajlada
Copy link
Member

pajlada commented Nov 13, 2021

For this functionality to be part of Chatterino there would need to be a better solution to resolving a User -> Preferred Pronoun.
Realistically, the only way I could see that being done is through native Twitch support. If Twitch had a customizable Preferred Pronoun field and passed it along with incoming IRC messages it would be a lot easier to support.
Because of this, I'll deny this PR from going into mainline Chatterino.

As @Felanbird mentioned, there is a fork already with automated(?) builds over here: https://github.com/GabeEddyT/chatterino2/releases

If a fork needs support in figuring out cross-platform builds, then there's developers able to help in our Discord and in our GitHub issues.

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.

5 participants