zulip / zulip-flutter

Future Zulip client using Flutter
Apache License 2.0
171 stars 149 forks source link

Compute Gravatar URLs where needed (handle `client_gravatar` true) #255

Open gnprice opened 1 year ago

gnprice commented 1 year ago

We should properly handle this optimization: https://zulip.com/api/register-queue#parameter-client_gravatar

At the moment we don't pass that client_gravatar parameter, so we get the default behavior of true, which signs us up for the server to pass null as a user's avatar_url to express that it's a Gravatar URL we can compute from the email address. But we haven't actually written down the bit of code to compute that Gravatar URL, so we end up with a missing avatar.

I think we'll shortly flip that for the moment to pass client_gravatar as false, so that we get correct behavior. But we should follow up by actually implementing the client side of that — it's not hard — and then re-enabling the optimization.

For code demonstrating the logic, see zulip-mobile's GravatarURL in src/utils/avatar.js.

Another piece we'll need as part of implementing this issue is to deserialize the User.avatarUrl field in a way that distinguishes whether the server set the avatar_url field to null or left it out entirely. The former indicates a Gravatar, but the latter comes from user_avatar_url_field_optional (#254) and carries no information about what the avatar is.

That will likely involve a class AvatarUrl, with subclasses for a provided URL string or indicating that the avatar is a computable Gravatar or that it was omitted. This resembles the AvatarURL class hierarchy in zulip-mobile — but one thing I would like to do differently here is to avoid putting redundant information on these avatar-URL objects, in order to avoid unnecessary allocation. (After all, the list of users is a fairly hot path both for startup CPU time and for memory consumption, when in a large realm.) It's fine for the AvatarUrl value to only be possible to fully interpret by using other data from the User.

gnprice commented 1 year ago

I think we'll shortly flip that for the moment to pass client_gravatar as false, so that we get correct behavior.

(This was done in caa9603784e1a7a7aee0af4682553eed6b1d0033 in #249. Implementing the optimization, which is what the present issue is about, is still open.)

gnprice commented 3 months ago

Another piece we'll need as part of implementing this issue is to deserialize the User.avatarUrl field in a way that distinguishes whether the server set the avatar_url field to null or left it out entirely.

This can be expressed using the JsonNullable class introduced in #F784. For discussion, see chat thread (from 2023-04).