-
Notifications
You must be signed in to change notification settings - Fork 41
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 support of animated stickers and emojis #429
Conversation
Looks like the first two commits need to be squashed? |
Oops, looks like it's better to squash all commits |
I mean, having 1 commit per feature/change is usually what I prefer and it makes reviewing PRs much easier. |
Last commit also change sticker. And I don't sure how to separate them |
ca3b686
to
6f3e651
Compare
You need to use |
c9de911
to
668435d
Compare
I think features are separated ok now |
595aa2f
to
0b910fc
Compare
2d3d01d
to
4c4445c
Compare
938b3a6
to
edf97cd
Compare
bc79aae
to
550b4e3
Compare
This PR does a lot of stuff and the way you organized the commits makes it hard to review (you do one thing in one and then change everything in the other). For what I can see this PR does the following:
This is great, but I think that this is too much for a single PR. I would be glad if you split it at least in 2 parts (with good organized commits too). Since the title of the PR is "Add support of animated stickers and emojis", do you mind only leaving the TGS support for stickers and MessageAnimatedEmoji support here, please? I think the rest can go in a separate PR for now. |
e2cc14b
to
c11f6b0
Compare
src/components/sticker.rs
Outdated
|
||
#[derive(Default)] | ||
pub struct Sticker { | ||
pub(super) message_id: Cell<i64>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I wrong or this property seems completely useless? Also, it doesn't make sense to have it anyway because if I understand correctly we're gonna use this widget outside of the messages as well (like for a sticker picker).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to check if widget isn't recycled after sticker is downloaded IIRC you have a similar check in MessageDocument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah k, yeah that check is important. The problem is that I don't like the fact that it's placed in the generic component. Maybe it's better having a sync variant to load the sticker and still do the check in MessageSticker?
match &sticker.full_type { | ||
StickerFullType::CustomEmoji(data) if data.needs_repainting => { | ||
self.add_css_class("needs-repainting") | ||
} | ||
_ => self.remove_css_class("needs-repainting"), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh, I don't really get what this css class is for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check one of those packs: Emoticon Emoji, Outline Emoji, Animal Emoji
Those emojis can be either black or white dependent on theme, currently I'm using css to recolor them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screencast.from.2023-03-24.09-14-40.webm
690463d
to
1594cd5
Compare
0ba99ce
to
cf568ed
Compare
animatons.webm
Features: