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

Communicate streams that went offline through /live #2880

Merged

Conversation

Mm2PL
Copy link
Collaborator

@Mm2PL Mm2PL commented Jun 10, 2021

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Adds channel is now offline. messages to the /live channel. Closes #2799. Fixes #2791

@Mm2PL Mm2PL requested review from zneix, pajlada and apa420 June 10, 2021 21:17
Copy link
Collaborator

@apa420 apa420 left a comment

Choose a reason for hiding this comment

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

LGTM, but wouldn't it be better to just gray out the previous channel is online message instead maybe (in the /live split)?
I feel like that would make more sense in a /live tab since you'll have a lot of channels going live or offline so it might be hard to track if a channel went offline or is still live.

Copy link
Collaborator

@zneix zneix left a comment

Choose a reason for hiding this comment

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

Overall the PR looks good and does its job well 👍

wouldn't it be better to just gray out the previous channel is online message instead maybe

I agree with apa on this one though. it will get very unclear and hard to follow/connect messages related to one channel and another.

@Mm2PL
Copy link
Collaborator Author

Mm2PL commented Jun 10, 2021

LGTM, but wouldn't it be better to just gray out the previous channel is online message instead maybe (in the /live split)?

I really like this idea. I didn't even think of such a possibility. Thank you for pointing this out. I'll be sure to add this.

@Mm2PL Mm2PL marked this pull request as draft June 10, 2021 21:31
@apa420
Copy link
Collaborator

apa420 commented Jun 10, 2021

Only con with the approach I suggested is you won't know when they went offline.
I'm unsure how relevant that is to people.

But to be fair this is called a /live split so I think it's fine it's left out.

@Mm2PL Mm2PL marked this pull request as ready for review June 10, 2021 21:53
@Mm2PL Mm2PL changed the title Add "channel is now offline." messages to /live Communicate streams that went offline through /live Jun 10, 2021
@leon-richardt
Copy link
Collaborator

Just spitballing some ideas, maybe striking through the messages (instead of)|(in addition to) graying them out could also be a possibility? Might make it a little more noticeable

@Mm2PL
Copy link
Collaborator Author

Mm2PL commented Jun 11, 2021

Also accidentally fixes #2791.

@Mm2PL
Copy link
Collaborator Author

Mm2PL commented Jun 11, 2021

@leon-richardt

Just spitballing some ideas, maybe striking through the messages (instead of)|(in addition to) graying them out could also be a possibility? Might make it a little more noticeable

I've looked into this. This requires a bunch of changes to rendering fonts. Currently the function that renders text doesn't need to know anything about the outside world like should this message have text be struck-through.

@goldbattle
Copy link
Contributor

It would also be nice if when a stream goes offline, it reports the up time so the user can know how long of a stream they missed.

@pajlada
Copy link
Member

pajlada commented Jun 19, 2021

It would also be nice if when a stream goes offline, it reports the up time so the user can know how long of a stream they missed.

This is not trivial to achieve since the easily accessible timestamp of "going live" we have might be the same time we started Chatterino.
Would need to make the /live tab a bit more smart and add metadata received by the API request to each Streamer is live! message.

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.

LGTM 👍

@pajlada pajlada enabled auto-merge (squash) June 19, 2021 13:36
@pajlada pajlada merged commit 74960bf into Chatterino:master Jun 19, 2021
@pajlada pajlada deleted the feature/offline_messages_in_the_/live_channel branch June 19, 2021 17:27
zneix added a commit to SevenTV/chatterino7 that referenced this pull request Jun 19, 2021
Now we're on commit d6b5921; Changes from upstream we pulled:

- Major: Added username autocompletion popup menu when typing usernames with an @ prefix. (Chatterino#1979, Chatterino#2866)
- Minor: The /live split now shows channels going offline. (Chatterino#2880)
- Minor: Now shows deletions of messages like timeouts (Chatterino#1155, Chatterino#2841, Chatterino#2867, Chatterino#2874)
- Bugfix: Moderation buttons now show the correct time unit when using units other than seconds. (Chatterino#1719, Chatterino#2864)
- Bugfix: Fixed bit and new subscriber emotes not (re)loading in some rare cases. (Chatterino#2856, Chatterino#2857)
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.

unsure if people in /live already went offline /live doesn't log properly
6 participants