Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Display default avatar for avatars from enterprise hosts. #1873

Merged
merged 3 commits into from
Aug 23, 2018
Merged

Display default avatar for avatars from enterprise hosts. #1873

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
067d66c
Display default avatar for avatars from enterprise hosts.
grokys Aug 22, 2018
1b94314
Merge branch 'master' into fixes/1870-enterprise-avatars
Aug 22, 2018
b1cdf10
Log errors.
grokys Aug 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 31 additions & 4 deletions src/GitHub.App/ViewModels/ActorViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
using System;
using System.Windows.Media.Imaging;
using GitHub.Logging;
using GitHub.Models;
using GitHub.Primitives;
using GitHub.Services;
using Serilog;

namespace GitHub.ViewModels
{
public class ActorViewModel : ViewModelBase, IActorViewModel
{
const string DefaultAvatar = "pack://application:,,,/GitHub.App;component/Images/default_user_avatar.png";
static readonly ILogger log = LogManager.ForContext<ActorViewModel>();

public ActorViewModel()
{
Expand All @@ -16,10 +20,33 @@ public ActorViewModel()
public ActorViewModel(ActorModel model)
{
Login = model?.Login ?? "[unknown]";
Avatar = model?.AvatarUrl != null ?
new BitmapImage(new Uri(model.AvatarUrl)) :
AvatarProvider.CreateBitmapImage(DefaultAvatar);
AvatarUrl = model?.AvatarUrl ?? DefaultAvatar;

if (model?.AvatarUrl != null)
Copy link
Collaborator

@jcansdale jcansdale Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I find the new if (model?.AvatarUrl is string avatarUrl) syntax more readable.

Copy link
Contributor Author

@grokys grokys Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator

@jcansdale jcansdale Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@grokys grokys Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@jcansdale jcansdale Aug 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think if (model?.AvatarUrl is var avatarUrl) is any better because it's more obvious that is var isn't a cast?

{
try
{
var uri = new Uri(model.AvatarUrl);

// Image requests to enterprise hosts over https always fail currently,
// so just display the default avatar. See #1547.
if (uri.Scheme != "https" ||
uri.Host.EndsWith("githubusercontent.com", StringComparison.OrdinalIgnoreCase))
{
AvatarUrl = model.AvatarUrl;
Avatar = new BitmapImage(uri);
}
}
catch (Exception ex)
{
log.Error(ex, "Invalid avatar URL");
}
}

if (AvatarUrl == null)
{
Avatar = AvatarProvider.CreateBitmapImage(DefaultAvatar);
AvatarUrl = DefaultAvatar;
}
}

public BitmapSource Avatar { get; set; }
Expand Down