-
-
Notifications
You must be signed in to change notification settings - Fork 29
Add support for custom avatars #60
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| use std::sync::Arc; | ||
| use std::fs; | ||
| use std::{future::Future, pin::Pin}; | ||
|
|
||
| use mxlink::matrix_sdk::Room; | ||
|
|
@@ -316,6 +317,18 @@ impl Bot { | |
| } | ||
| } | ||
|
|
||
| let logo_bytes: Option<Vec<u8>> = if self.inner.config.user.avatar.is_none() { | ||
| Some(LOGO_BYTES.to_vec()) | ||
| } else { | ||
| let avatar_path = self.inner.config.user.avatar.as_ref().unwrap(); | ||
| match fs::read(avatar_path) { | ||
| Ok(bytes) => Some(bytes), | ||
| Err(_e) => { | ||
| Some(LOGO_BYTES.to_vec()) | ||
| } | ||
| } | ||
| }; | ||
|
Comment on lines
+320
to
+330
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As it is right now, all If it's just That said, if we introduce additional avatar options (as discussed in my other comment in this review), then this may change. |
||
|
|
||
| let should_update_avatar = match ¤t_avatar_url { | ||
| Some(avatar_url) => { | ||
| let request = MediaRequestParameters { | ||
|
|
@@ -328,7 +341,7 @@ impl Bot { | |
| .await | ||
| .map_err(|e| anyhow::anyhow!("Failed fetching existing avatar: {:?}", e))?; | ||
|
|
||
| content.as_slice() != LOGO_BYTES | ||
| Some(content.as_slice()) != logo_bytes.as_deref() | ||
| } | ||
| None => true, | ||
| }; | ||
|
|
@@ -340,10 +353,12 @@ impl Bot { | |
| .parse() | ||
| .expect("Failed parsing mime type for logo"); | ||
|
|
||
| account | ||
| .upload_avatar(&mime_type, LOGO_BYTES.to_vec()) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("Failed uploading avatar: {:?}", e))?; | ||
| if let Some(bytes) = logo_bytes { | ||
| account | ||
| .upload_avatar(&mime_type, bytes) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("Failed uploading avatar: {:?}", e))?; | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
|
|
||
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.
A few things:
I suppose other image resolutions are also OK. It doesn't need to be 768x768. It's just what the default avatar image is like, but anything else is OK. Still, having a somewhat high-res image is probably sensible
I suppose PNG is a requirement right now, because of the
LOGO_MIME_TYPEconstant. With some extra work, we may be able to get rid of itavatar_pathmay be a better name for this setting, if this will deal with filesystem paths onlyif we'll be adding other avatar-related options, it may be better to expand this section and have sub-keys.
Example other options:
avatar.manage = true|false(allows people to the bot to not touch the currently-configured avatar)avatar.path = ...(file path)avatar.source = ...(a more generic name thanpath, so it would also allow handling of both file paths and MXC URIs). One could potentially specify the avatar using something likemxc://example.com/...in which case we won't be doingupload_avatar(), but ratherset_avatar_url()avatar.mime_type = ...(potentially allowing for non-PNG images to be used). This may be unnecessary if we can do some-detectionI suppose this complicates things a lot, but also covers a bunch of additional use-cases and makes this feature complete.