chatterino2 icon indicating copy to clipboard operation
chatterino2 copied to clipboard

7TV Emote Provider Implementation

Open AnatoleAM opened this issue 3 years ago • 11 comments

Pull request checklist:

  • [ ] CHANGELOG.md was updated, if applicable
  • [x] Add an option to hide unlisted emotes
  • [x] Adjust scaling of emotes
  • [x] Support zero-width emotes
  • [x] Animated Avatars (chatterino7#69)
  • [x] Add the 4x CDN link in the right click context (chatterino7#62)
  • [ ] Migrate to 7TV API v3
  • [ ] Append instructions to building docs regarding WebP
  • [ ] Use data-driven CDN URLs
  • [ ] Use the REST API instead of GQL for fetching emotes
  • [ ] (Optional) Implement the 7TV EventAPI
  • [ ] (Optional) Implement Nametag Paints
  • [ ] (Optional) Add support for images in AVIF format

Description

Implementing 7TV (https://7tv.app) as a new Emote Provider in Chatterino2. The changes include adding a new provider to fetch global emotes, channel emotes and user badges from the 7TV API.

This pull request mirrors the SevenTV/chatterino7 fork maintained by myself and @TroyKomodo, with contributions by @zneix, @goldbattle, @MrAuro, @mmattbtw and others, while backtracking a few changes (such as versioning and workflows) to upstream.

AnatoleAM avatar Jul 25 '21 16:07 AnatoleAM

Hi, I was wondering what is preventing this minimal support from being merged into master?

goldbattle avatar Jan 05 '22 07:01 goldbattle

Hi, I was wondering what is preventing this minimal support from being merged into master?

Most of the list that you see in the pr description. As far as I'm aware, anatole still needs to add REST to 7tv (which I believe is coming in v3).

ALazyMeme avatar Jan 05 '22 09:01 ALazyMeme

Hi, I was wondering what is preventing this minimal support from being merged into master?

I believe since minimal support already exists, the currently intended approach is to only merge full support into master.

Felanbird avatar Jan 05 '22 13:01 Felanbird

@Felanbird Correct me if I am wrong, but currently there is no support for seeing or emote completing 7tv emotes in the master branch.

Besides the above improvement todos I don't see why the current code would not be great to have already merged so people can see these emotes. If you could expand what needs to be performed in the todo list that would be great. I know the websocket / event api allows for live changes instead of having to reload emotes correct? And most other things could be done in a follow up PR as people have time.

goldbattle avatar Jan 05 '22 16:01 goldbattle

Correct me if I am wrong, but currently there is no support for seeing or emote completing 7tv emotes in the master branch.

Just use the 7TV fork if you want the emotes in chatterino, there is no reason to implement rudimentary support for a soon deprecated API in the main version until 7TV itself has released their v3 API, which if I understood everything correctly is basically a full overhaul and supposed to be a definitive and long term version where implementing proper support for it is actually worth the effort.

LosFarmosCTL avatar Jan 05 '22 17:01 LosFarmosCTL

Thanks for the context. I am always for upstreaming so that things are easier down the line. In terms of the V3 api you mean this GQL work in progress one and REST work in progress one? The chatterino7 also uses the V2 GQL so the same update to V3 would be require / applicable to both.

My specific questions about the todo list:

  • Add an option to hide unlisted emotes - This refers to checking is the visibility_simple has UNLISTED in it, if so then don't show it. Would need to be implemented correct?
  • Implement the 7TV EventAPI - Isn't necessary to have support but is instead a more ideal implementation? Not sure about benefit over GQL or REST.
  • Adjust scaling of emotes - When I built I didn't see any issue, is this for non-regular sizes?
  • Append instructions to building docs regarding WebP - What details need to be added?
  • Add the 4x CDN link in the right click context - https://github.com/SevenTV/chatterino7/pull/62
  • Use data-driven CDN URLs - The current url https://cdn.7tv.app/emote/%1/%2 matches what the chrome client does. Not sure what this refers to.
  • Use the REST API instead of GQL for fetching emote - Isn't applicable in this PR any more since the new api is GQL based, and the chatterino7 uses the GQLv2.

goldbattle avatar Jan 05 '22 19:01 goldbattle

I'm just too lazy to work on this, that's all.

zneix avatar Jan 05 '22 20:01 zneix

Thanks for the context. I am always for upstreaming so that things are easier down the line. In terms of the V3 api you mean this GQL work in progress one and REST work in progress one? The chatterino7 also uses the V2 GQL so the same update to V3 would be require / applicable to both.

My specific questions about the todo list:

  • Add an option to hide unlisted emotes - This refers to checking is the visibility_simple has UNLISTED in it, if so then don't show it. Would need to be implemented correct?
  • Implement the 7TV EventAPI - Isn't necessary to have support but is instead a more ideal implementation? Not sure about benefit over GQL or REST.
  • Adjust scaling of emotes - When I built I didn't see any issue, is this for non-regular sizes?
  • Append instructions to building docs regarding WebP - What details need to be added?
  • Add the 4x CDN link in the right click context - Added 4x links to open/copy emote link menu. SevenTV/chatterino7#62
  • Use data-driven CDN URLs - The current url https://cdn.7tv.app/emote/%1/%2 matches what the chrome client does. Not sure what this refers to.
  • Use the REST API instead of GQL for fetching emote - Isn't applicable in this PR any more since the new api is GQL based, and the chatterino7 uses the GQLv2.

The current status for upstreaming is pending at least some of the tasks on this PR draft.

  • Unlisted emotes are those that don't quite pass standards for safety, but aren't bad enough to be completely deleted. (i.e sexually suggestive, but not porn); these can still be added to channels, so there could be situations where a streamer open a chat on screen and show potentially tos content. Ideally the option to hide these can be auto-enabled with streamer mode as well.
  • I marked the EventAPI implementation as optional, since it is more QoL than required. It means chatterino users wouldn't need to F5 when an emote is added/removed in channels
  • I believe this was mostly about missing the 4x size (though there is also the issue that 2x and 3x sizes are not correct on the 7TV cdn, which is something that will be fixed alongside API V3)
  • Data-driven CDN URLs means using the urls provided by the API response, rather than hardcoding the cdn's base url. This is mainly for future proofing for some planned features.
  • The usage of GQL in chat client applications isn't recommended, as it lacks caching and is typically slower in v2. The plan has been to switch SevenTV/chatterino7 to the REST API, though no one has taken on that task as of yet.

Also to emphasize this, the lack of progress on upstreaming isn't the fault of the c2 maintainers. This is a draft, because it's uncomplete, and a draft doesn't get reviewed.

AnatoleAM avatar Jan 09 '22 18:01 AnatoleAM

Great, thanks so much for all the clarifications. To be upfront, I wanted to spend time to help, thus I simply wanted clarification on what I can do to complete the PR, and your explanations really give exactly what needs to be done, so thanks so much for that. I have started working on the changes you have mentioned on this branch: https://github.com/goldbattle/chatterino2/tree/to-upstream

What is included so far:

  • Handle zero width emotes https://github.com/SevenTV/chatterino7/pull/36
  • Emote searching from https://github.com/SevenTV/chatterino7/pull/64
  • Initial 4x code from https://github.com/SevenTV/chatterino7/pull/62
  • Fixed scaling of emotes and properly handle 4x emotes https://github.com/goldbattle/chatterino2/commit/403b947b6351f2c6bc6cf3f756e536cdfc00ab96
  • Handle unapproved / hidden emotes from displaying https://github.com/goldbattle/chatterino2/commit/125efefb6cea6a745206cdf469aef707cd19ab75 and https://github.com/goldbattle/chatterino2/commit/42c9ff02d464479dfed438a03c8a303ec65aad9e

From your description I plan to work on the following:

  • (DONE via https://github.com/goldbattle/chatterino2/commit/f5c4d51635c12c8ff02d8d9a486c41cc21b65e8e): Add to my current unlisted emote logic to allow for seeing of unlisted emotes (default is not seeing them)
  • (DONE via https://github.com/goldbattle/chatterino2/commit/835e51adf90c2c21cefc657daf86ef23b2ffb258): Switch to the REST api endpoint instead of the GQL one
  • (DONE via https://github.com/goldbattle/chatterino2/commit/835e51adf90c2c21cefc657daf86ef23b2ffb258): Use urls from the API endpoint (once REST is used, this seems straight forward)

Questions:

  • Should global emotes always be shown (my current logic)? or should their visibility also be checked?
  • Should I create another PR to this repo once these three points are completed, or try to merge to the fork repo? (my initial PR addressing the above was closed on the c7 repo)

EDIT: here is the git comparison to the master branch: https://github.com/Chatterino/chatterino2/compare/master...goldbattle:to-upstream?expand=1

goldbattle avatar Jan 09 '22 20:01 goldbattle

You should PR those changes to the chatterino7 branch on the fork, the to-upstream branch is heavily cherrypicked and require some additional care to merge. (i.e ensuring that the versioning & other changes are not tracked)

Should global emotes always be shown (my current logic)? or should their visibility also be checked?

Global emotes would never also be unlisted, so you can leave whatever behavior is produced by your implementation - it won't matter.

@goldbattle

AnatoleAM avatar Jan 10 '22 00:01 AnatoleAM

You should PR those changes to the chatterino7 branch on the fork, the to-upstream branch is heavily cherrypicked and require some additional care to merge. (i.e ensuring that the versioning & other changes are not tracked)

I pulled and implemented everything required for minimal support for upstreaming 7tv integration into my up-stream branch since upstreaming support is my goal. I can make some PRs to the chatterino7 so they can get those changes also, but this doesn't address the former. Do you have a recommendation for this?

Edit: Done https://github.com/SevenTV/chatterino7/pull/67 , https://github.com/SevenTV/chatterino7/pull/68, with my x4 in https://github.com/SevenTV/chatterino7/pull/62

@AnatoleAM

goldbattle avatar Jan 10 '22 00:01 goldbattle