Re: [PATCHv7 7/9] gitweb: picon avatar provider

[Jakub Narebski <jnareb@gmail.com>]

On Sat, 27 June 2009, Giuseppe Bilotta wrote:

> Simple implementation of picon that only relies on the indiana.edu
> database.

I'd like to see where you got information about picons.  But it is
not necessary to have in commit message.

... Ah, I'm sorry, it is stated in comment.  Nevermind then.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.perl |   26 ++++++++++++++++++++++----
>  1 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index de4cc63..ae73d45 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -378,14 +378,17 @@ our %feature = (
>  	# shortlog or commit will display an avatar associated with
>  	# the email of the committer(s) and/or author(s).
>  
> -	# Currently only the gravatar provider is available, and it
> -	# depends on Digest::MD5.
> +	# Currently available providers are gravatar and picon.
> +
> +	# Gravatar depends on Digest::MD5.
> +	# Picon currently relies on the indiana.edu database.
>  
>  	# To enable system wide have in $GITWEB_CONFIG
> -	# $feature{'avatar'}{'default'} = ['gravatar'];
> +	# $feature{'avatar'}{'default'} = ['provider'];
> +	# where provider is either gravatar or picon.

I wonder if it wouldn't be better to use "<provider>" (or "'<provider>'")
in place of "'provider'", as it is not about literal 'provider', but 
about one of 'gravatar' or 'picon'.

>  	# To have project specific config enable override in $GITWEB_CONFIG
>  	# $feature{'avatar'}{'override'} = 1;
> -	# and in project config gitweb.avatar = gravatar;
> +	# and in project config gitweb.avatar = provider;

Same as above.

>  	'avatar' => {
>  		'sub' => \&feature_avatar,
>  		'override' => 0,
> @@ -856,6 +859,8 @@ our @snapshot_fmts = gitweb_get_feature('snapshot');
>  our ($git_avatar) = gitweb_get_feature('avatar');
>  if ($git_avatar eq 'gravatar') {
>  	$git_avatar = '' unless (eval { require Digest::MD5; 1; });
> +} elsif ($git_avatar eq 'picon') {
> +	# no dependencies
>  } else {
>  	$git_avatar = '';
>  }

Nice.

> @@ -1523,6 +1528,17 @@ sub format_subject_html {
>  # given page, there's no risk for cache conflicts.
>  our %avatar_cache = ();
>  
> +# Compute the picon url for a given email, by using the picon search service over at
> +# http://www.cs.indiana.edu/picons/search.html
> +sub picon_url {
> +	my $email = lc shift;
> +	if (!$avatar_cache{$email}) {
> +		my ($user, $domain) = split('@', $email);
> +		$avatar_cache{$email} = "http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/single";

Hmmm... perhaps it would be better to break this very long URL (link)
in lines...

> +	}
> +	return $avatar_cache{$email};
> +}

I wonder if it is worth caching picons, at least in this form.  It isn't
as if splitting on '@' is expensive.  But perhaps to not break pattern
(that avatar URLs are cached) it is a good thing.

Thoughts for the possible future enhancements: find final URL of an image
via http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/off/1/order
by scrapping (parsing) it for .gif link, and store this URL in cache.
But that most probably isn't worth it.  Just feel like mentioning it.

> +
>  # Compute the gravatar url for a given email, if it's not in the cache already.
>  # Gravatar stores only the part of the URL before the size, since that's the
>  # one computationally more expensive. This also allows reuse of the cache for
> @@ -1546,6 +1562,8 @@ sub git_get_avatar {
>  	my $url = "";
>  	if ($git_avatar eq 'gravatar') {
>  		$url = gravatar_url($email, $size);
> +	} elsif ($git_avatar eq 'picon') {
> +		$url = picon_url($email);
>  	}
>  	# Currently only gravatars are supported, but other forms such as
>  	# picons can be added by putting an else up here and defining $url

Very nice pattern, nice use of infrastructure.

I think that adding 'width' attribute to img tag of avatar should
perhaps be put here rather than in previous patch, which is about
adding gravatar URL cache.  This chunk:

         # Currently only gravatars are supported, but other forms such as
         # picons can be added by putting an else up here and defining $url
         # as needed. If no variant puts something in $url, we assume avatars
         # are completely disabled/unavailable.
         if ($url) {
 -               return $pre_white . "<img width=\"$size\" class=\"avatar\" src=\"$url\" />" . $post_white;
 +               return $pre_white .
 +                      "<img width=\"$size\" " .
 +                           "class=\"avatar\" " .
 +                           "src=\"$url\" " .
 +                      "/>" . $post_white;
         } else {
                 return "";
         }

should be, I think, in this patch and not in previous one (or at least
in some other patch than the previous one).

BTW. you should have updated the comment here.


Should it be stated that <img width="$size" ...> is here because not 
all kinds of avatars (not all avatar providers) support selecting size
of avatar, somewhere in this comment?


Very nice, well thought infrastructure, which makes adding new avatar
providers easy.  Good work!

-- 
Jakub Narebski
Poland