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

Add functions to retrieve avatars #234

Merged
merged 4 commits into from
Feb 24, 2024
Merged

Conversation

Schmiddiii
Copy link
Contributor

@Schmiddiii Schmiddiii commented Feb 15, 2024

A few TODOs are left which I am unsure about:

  • When fetching the profile avatar, I am unsure about using the identified or unidentified push service. Note: Unidentified seems to work. I thus chose this.
  • Is there some way of knowing when a profile or profile avatar (or group avatar) gets outdated, to remove that from the cache.
  • This may require a migration to remove the already cached profiles, otherwise no avatars for profiles will ever be fetched.
  • Is there maybe some better format to use for avatars instead of Vec<u8>, e.g. Bytes?

@gferon
Copy link
Collaborator

gferon commented Feb 21, 2024

Thanks a lot! We could definitely introduce a convenience wrapper type for avatars if you think it's worth it.

@Schmiddiii
Copy link
Contributor Author

Schmiddiii commented Feb 21, 2024

I have now updated this PR. A few notes:

  • Using unidentified instead of identified seems to work for profile fetching, I therefore used unidentified (still don't know what is correct though).
  • Added the migration mentioned.
  • Added type AvatarBytes = Vec<u8>.
  • Prevented a panic in libsignal-service-rs when fetching a group avatar with empty path (see also Fix panic for fetching group avatar libsignal-service-rs#291).

I am still unsure if there exists some message that notifies when a profile/avatar gets out-of-date.

@Schmiddiii
Copy link
Contributor Author

Schmiddiii commented Feb 21, 2024

Note from WF chat: Profile updates seem to be done by WF and SA on a schedule. While presage does not yet have such a schedule, we could certainly do something similar. But I think this can also be done in another MR, I would therefore call this mergeable (if you don't have any comments on it).

See also:

@gferon
Copy link
Collaborator

gferon commented Feb 24, 2024

Thanks a lot for your work! I fixed some cosmetic things and some clippy warnings, and will merge it now.

@gferon gferon merged commit 6c49f6f into whisperfish:main Feb 24, 2024
4 checks passed
@gferon gferon added this to the 0.6.0 milestone Feb 25, 2024
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.

2 participants