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.tv/membership' capability for Twitch.tv #296

Closed
wants to merge 1 commit into from

Conversation

Orphis
Copy link
Contributor

@Orphis Orphis commented Jun 22, 2017

Twitch.tv implementation doesn't send NAMES list automatically
unless supported by the client as they can be quite big.

See: https://dev.twitch.tv/docs/v5/guides/irc/#twitch-irc-capability-membership

Twitch.tv implementation doesn't send NAMES list automatically
unless supported by the client as they can be quite big.

See: https://dev.twitch.tv/docs/v5/guides/irc/#twitch-irc-capability-membership
Copy link
Contributor

@digitalcircuit digitalcircuit left a comment

Choose a reason for hiding this comment

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

Not personally tested (no Twitch account), but this seems fine and the attention to documentation is nice!

@digitalcircuit
Copy link
Contributor

Furthering the pull request review (I'm not sure if I'm supposed to use that? I don't have merge/manage powers), this looks good. I appreciate including the documentation so others in the future can see why Twitch is included in Quassel :)

Not tested, but if it works for you it should be fine! If you can, try this with very large chats to make sure Quassel won't break. Large user counts are a bottleneck for logging in and might be a cause for concern. E.g. magne4000/quassel-webserver#238 and sandsmark/QuasselDroid#60 (comment) .

Eventually this'll need fixed properly via a protocol change, of course. Large IRC channels should be handled by an IRC client, and it's on the Big Three list of things to make Quasseldroid (and Quassel) quick and lightweight.

In short, this seems fine, just might want to see who all will be affected.

@Orphis
Copy link
Contributor Author

Orphis commented Jun 23, 2017

So, I tested it in large-ish (600 viewers) channel and it didn't cause any issue. In huge channels (larger than 1000), as the API says, you only get the channel administrators listed, so they are even easier to handle :)

And well, when you have like 50k viewers, you don't really care about any specific individual anyway.

@digitalcircuit
Copy link
Contributor

In that case, I'd agree with merging this change. No problems for you, and as you've stated the API won't let it exceed 1,000 users anyways - large IRC channels already go above that.

@Sput42 Sput42 closed this in e90ec7a Dec 20, 2017
@Orphis Orphis deleted the twitch_membership branch May 23, 2019 20:08
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.

2 participants