-
Notifications
You must be signed in to change notification settings - Fork 3
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
import thumbnails; refactor #30
Conversation
@@ -41,6 +41,13 @@ impl Image { | |||
)) | |||
} | |||
|
|||
pub async fn convert_tgs_if_some(self, animation_format: Option<AnimationFormat>) -> anyhow::Result<Self> { |
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.
is this really necessary as a public function?
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.
Probably not. I think the half of everything which is currently pub should not be pub. I should review this everywhere, before the next release.
pub struct Sticker { | ||
/// Emoji associated with the sticker. | ||
pub emoji: Option<String>, | ||
pub struct PhotoSize { |
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.
This struct is poorly named. I would expect it to store the size of a photo. However, it stores the file of the "photo" along with its size. It would probably be better to call this ImageWithSize
or SizedImage
or similar.
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 did not how to name. The project does already have many meta-data struct. So I have simple choose the same name like the telegram api. https://core.telegram.org/bots/api#photosize
Since deserializing is the only usecase for this struct, I am fine with the name.
) | ||
.await?; | ||
let thumb = match self.thumbnail.as_ref() { | ||
None => None, //async map is currently not supported by std |
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 have two questions.
-
What does that comment mean?
-
Do we want to skip adding a thumbnail just because telegram does not have one? Or do we instead want to downsize the sticker ourselves?
Especially for lottie files, I can imagine that telegram doesn't need a thumbnail, as lottie files are vector animations. But converting them to webp/gif makes them size dependent, so adding a thumbnail for matrix would be helpful.
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.
self.thumbnail.as_ref().map(|f| f.foo().await?)
does not work.- I would like to add this later. I think this should be also impl for all sticker not only for telegram imports. Also the most packs seems to have one.
Co-authored-by: msrd0 <msrd0@users.noreply.github.com>
No description provided.