Display default avatar for avatars from enterprise hosts. #1873
Conversation
|
LGTM. Thanks for fixing this! |
| AvatarProvider.CreateBitmapImage(DefaultAvatar); | ||
| AvatarUrl = model?.AvatarUrl ?? DefaultAvatar; | ||
|
|
||
| if (model?.AvatarUrl != null) |
There was a problem hiding this comment.
Nit: I find the new if (model?.AvatarUrl is string avatarUrl) syntax more readable.
There was a problem hiding this comment.
Interesting, what do you find more readable about that? I use the new syntax a lot, but only when a cast is involved. AvatarUrl is already typed to string so there is no cast, and so it's just a comparison with null...
There was a problem hiding this comment.
I just find that it reads well in my head: "if model?.AvatarUrl is a string then call it avatarUrl and do stuff with it".
Maybe I'm a little over obsessive about putting things in variables. Previously I'd have been inclined to do:
var avatarUrl = model?.AvatarUrl;
if (avatarUrl != null)
{
...
}
I'm now happy with the conciseness and expressiveness of if (model?.AvatarUrl is string avatarUrl). I know you can also do if (model?.AvatarUrl is var avatarUrl), but is var looks a little strange to me.
There was a problem hiding this comment.
Yeah, I dunno... The problem for me with "if model?.AvatarUrl is a string then call it avatarUrl and do stuff with it" is that it's always a string. That pattern should be used for casting IMO, not just checking for null, and it surprises me when there isn't actually a cast.
There was a problem hiding this comment.
Do you think if (model?.AvatarUrl is var avatarUrl) is any better because it's more obvious that is var isn't a cast?
| Avatar = new BitmapImage(new Uri(AvatarUrl)); | ||
| } | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
Is the catch { } required with the above url checks? If I'm reading it correctly, an exception here would be unexpected and we should log it.
There was a problem hiding this comment.
Yes, the catch is in case the URL is somehow an invalid format. And yes we should probably be logging that ;)
Image requests to enterprise hosts over https always fail currently, so just display the default avatar (see #1547).
This was already fixed for the REST portion of the application in #1579 by remembering which hosts image downloads have already failed for and making sure not to try again, however the framework of
AvatarProvider/ImageCache/ImageDownloaderused there assumes a fixed image size of 32x32, causing #1603.The whole thing needs a major revamp, which is a little tricky while we're still using a combination of REST and GraphQL, so for the moment just don't try to download images from enterprise hosts that are using
https: this should be OK because this will currently never work anyway.Fixes #1870.