Fix avatar blurriness in profile screen

[saisumith770]

Description

Updated the avatar URL used in the profile screen to request a higher-resolution image to reduce blurriness.
Fixes #301

Outcome

Before	After

[saisumith770]

@gnprice could you review this PR and let me know if any changes are required.

[gnprice]

Thanks for the contribution!

Before we can review this in detail, it will need tests, and both the new and existing tests will need to be passing.

On running the tests and writing tests, see the project README.

For this code, because it's based on existing code in zulip-mobile, it would be good to also transcribe that code's tests and make sure that this new code passes all the same tests. If there are any tests of the zulip-mobile code that for some reason don't make sense in this version, then note that with a comment.

[saisumith770]

@gnprice There are a few sections of this PR currently that might need to be discussed before implementation. I've listed them below:

Handling Avatar URL set to null

In the AvatarImage widget the resolvedUrl variable could possibly be null, this has been marked as a TODO(#255).

I've handled this edge case inside the AvatarURL class.

But this doesn't pass a few UI tests as when url is null the widget returned is expected to be SizedBox.shrink(), I propose we should make those changes in the UI tests. The current temporary patch is as below.

Setting default size for images

In the AvatarImage widget, I added a size param that allows me to recompute the image URL for better resolution.

But the AvatarImage widget is used in several places without being provided with a size param hence I have to use a default value of 30 as implemented above.
I propose that when the size is not provided instead of using the AvatarURL class to recompute the url we can just use the resolvedUrl directly as the previous version of the app had.

I would like to hear your thoughts as to how I should proceed with them.

[gnprice]

Sure, thanks for the clear questions.

When User.avatarUrl is null, we should handle that by computing a gravatar. That doesn't have to be part of this PR, though, as it's a separate issue we're tracking as #255.

There's another case where resolvedUrl is null, though: if User.avatarUrl is a string that doesn't parse as a URL. In that case we should continue to return SizedBox.shrink(), just like if the user wasn't found. I think the cleanest way to fix that will probably be to push the tryResolveUrl call inside AvatarURL.fromUserOrBotData (note there's a free-standing function tryResolveUrl, so you don't need to pass the store around in order to do that).

For the size, the AvatarImage widget hasn't previously called for a size because it hasn't previously had anything it would do with that information — it's been always fetchinig the same image regardless of what size it'll be shown at. Now that it does need to know the size, though, it should take the size as a parameter.

I don't think there's a good default that AvatarImage can use for the size — sometimes it'll be used in a place where it's going to be shown large, and sometimes where it's going to be shown small, and there's no reasonable way it can guess. So the parameter should just be required.

[gnprice]

When User.avatarUrl is null, we should handle that by computing a gravatar.

Ah, no, I've now gone and reread #255 and I see that's not the correct behavior here.

Currently there should not be any null values in User.avatarUrl. (There will be some after #254 and/or #255.) Therefore, if we see a null value there, we should just show SizedBox.shrink() as we do now.

If we were to turn on both of the features described in #254 and #255 (and not do anything else), then we'd have two different cases both show up as null there: the case where we should compute a gravatar, and the case where we should compute a fallback avatar URL. That's because one of those would be represented by "avatar_url": null in the JSON, and the other by avatar_url being absent in the JSON. Both of those conditions are, by default, rendered as avatarUrl: null by Dart's JSON deserializer. (Which is a good thing — it stems from the fact that Dart does not share JS's distinctive bad design choice of having two different values undefined and null.)

When we do go to take care of those two issues, then, we'll need to add a wrinkle in the User class to represent that distinction. That's all out of scope for this issue, so for this PR let's leave out both the gravatar case and the fallback case.

[gnprice]

Thanks @saisumith770 for the contribution! Comments below, and see also high-level comments above.

[saisumith770]

Sure, thanks for the clear questions.

When User.avatarUrl is null, we should handle that by computing a gravatar. That doesn't have to be part of this PR, though, as it's a separate issue we're tracking as #255.

There's another case where resolvedUrl is null, though: if User.avatarUrl is a string that doesn't parse as a URL. In that case we should continue to return SizedBox.shrink(), just like if the user wasn't found. I think the cleanest way to fix that will probably be to push the tryResolveUrl call inside AvatarURL.fromUserOrBotData (note there's a free-standing function tryResolveUrl, so you don't need to pass the store around in order to do that).

For the size, the AvatarImage widget hasn't previously called for a size because it hasn't previously had anything it would do with that information — it's been always fetchinig the same image regardless of what size it'll be shown at. Now that it does need to know the size, though, it should take the size as a parameter.

I don't think there's a good default that AvatarImage can use for the size — sometimes it'll be used in a place where it's going to be shown large, and sometimes where it's going to be shown small, and there's no reasonable way it can guess. So the parameter should just be required.

@gnprice You mentioned that size should be a required parameter for AvatarImage, but in the recent_dm_conversations.dart file AvatarImage is used without providing the size param.

This is in the previous version of the app. How should this be handled !

[gnprice]

At those call sites, the caller should pass the actual size the AvatarImage will have.

If you look around at the surrounding code, the caller does know the size. It just hasn't been passing it to AvatarImage because there hasn't previously been anything that AvatarImage would do with that information.

[saisumith770]

At those call sites, the caller should pass the actual size the AvatarImage will have.

If you look around at the surrounding code, the caller does know the size. It just hasn't been passing it to AvatarImage because there hasn't previously been anything that AvatarImage would do with that information.

Refer commit 533c6ca also changed the avatar function naming in 5be3f1a

[saisumith770]

@gnprice could you review the changes! Thank you for your efforts 🎉

[gnprice]

Thanks for the revision! Here's a fresh round of review.

[gnprice]

I started doing another round of review, but then I realized you hadn't actually said the PR was ready for another round. And indeed there are a number of comments you haven't yet gotten to — which is fine, please take as much time as you need for addressing the review feedback.

Just reply here to say when it's ready for another round. I'm also happy to answer specific questions in between, like at #534 (comment) above.

Anyway, here are the couple of items I spotted as I was just beginning a review.

[saisumith770]

@gnprice I've made the suggested changes, do let me know if any revisions are required.

[gnprice]

Thanks for the revision! This is getting a lot closer.

There's a variety of comments below. One other important thing we'll need before we can merge this is that the commits in the branch need to be squashed and reorganized into clear and coherent commits, following the Zulip project's Git style: https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html

For this PR, I think that will mean two commits:

• One commit adding size to AvatarImage, and making the necessary changes where that widget is used. This commit should be an NFC commit.
• Then one commit with the rest of the changes: adding AvatarUrl and subclasses, using that from AvatarImage, adding the tests.

To get there from your current structure, my suggestion would be:

• First, git rebase to get on top of current main and eliminate the merge commit. (If this hits any merge conflicts, the easiest solution might be to abort the rebase and use git merge origin/main instead. You'll still have conflicts but all at once, which in this case is probably easiest.)
• Then git reset origin/main, so that your current commit is origin/main but you have all your changes to files.
• Then git add -p followed by git commit, to make the first commit.
• Then git add and git commit to make the second commit.

But if you have another procedure you're comfortable with that gets the right result, that's good too. If you have any questions with the Git manipulations or want any help, just ask in #git help on chat.zulip.org.

[saisumith770]

@gnprice I've made the changes, if it's alright then I will start squashing the commits as requested

[gnprice]

Sure — please go ahead.

[gnprice]

Thanks @saisumith770 for the revision! This is close to merging now.

A few comments below on the code. The commit structure all looks good.

The commit messages are close but need a few changes. I can make a final pass on those before merging, but please also try to give them another round of editing yourself. Take a look again in particular at this section of the Zulip commit style guide:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-summary-part-2

For the main commit (the second one, the one making most of the changes), see also the paragraph about GitHub issues in this section:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-description
and see this section about what information goes in the description:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#the-purpose-of-the-commit-description

Finally, looking back at the screenshots in the description: please include "before" versions as well as "after". That helps make it easy to see from the screenshots what's changed in the UI.

[gnprice]

Thanks for the revision! This is very close now to merge.

Just one comment on the revised code, below.

The commit messages also look good, except that the main commit's message should say it's closing the issue. (Hmm — and the PR description, at the top of this thread, should do so too.) It could also benefit from just a few more words saying what it's doing. So here's a revised version:

avatar: Fix blurriness on profile screen by adjusting physical image size

Fixes: #301 

Please also add "before" as well as "after" screenshots as mentioned above. (Rereading, I see you didn't actually say this was ready for re-review, so perhaps you were already planning to do that.)

[saisumith770]

@gnprice I've made the suggested changes!

[gnprice]

Looks good — merging. Thanks @saisumith770 for all your work on this!