Re: [PATCHv4 2/2] gitweb: gravatar support

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

On Wed, Jun 24, 2009 at 3:44 PM, Nanako Shiraishi<nanako3@lavabit.com> wrote:
> Quoting Giuseppe Bilotta <giuseppe.bilotta@gmail.com>:
>
>> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
>> index 68b22ff..ddf982b 100644
>> ........
>> +img.avatar {
>> +     vertical-align:middle;
>> +}
>> +
>> .........
>> +# Pixel sizes for avatars. If the default font sizes or lineheights
>> +# are changed, it may be appropriate to change these values too via
>> +# $GITWEB_CONFIG.
>> +our %avatar_size = (
>> +     'default' => 16,
>> +     'double'  => 32
>> +) ;
>> ........
>
> Early parts of the patch talk about "avatars"; compared with "icons" Junio
> suggested, I think that is a better generic word to use for this purpose.

Well, I would argue that 'avatar' is pretty commonly understood to be
the name of the picture identifying an individual online. OTOH, I've
been thinking that there are other kind of icons that might be used
across gitweb in the future, and they are likely to use the same
sizes, so I'll go for icon_size here.

>> @@ -1474,7 +1501,7 @@ sub format_author_html {
>>       my $tag = shift;
>>       my $co = shift;
>>       my $author = chop_and_escape_str($co->{'author_name'}, @_);
>> -     return "<$tag class=\"author\">" . $author . "</$tag>\n";
>> +     return "<$tag class=\"author\">" . git_get_gravatar($co->{'author_email'}, 'space_after' => 1) . $author . "</$tag>\n";
>>  }
>
> But the function that returns a string suitable for embedding in the HTML
> page, given an e-mail address, is called get_gravatar(), not get_avatar()?
>
> I expected from an earlier review message by Junio that get_avatar() will
> look like this:
>
> sub git_get_avatar {
>        my $url = undef;
>        if ($git_gravatar_enabled) {
>                my $md5 = .......;
>                $url = "http://www.gravatar.com/avatar.php?gravatar_id=$md5";
>        } else if ($git_picons_enabled) {
>                my $userpath = .......;
>                $url = "http://www.cs.indiana.edu/picons/db/users/$userpath/face.xpm";
>        }
>        return "" unless (defined $url);
>        return $pre_white . "<img ... src=\"$url\" size=$size />" . $post_white;
> }
>
> (without "picons" part, of course).

Right. I was wondering if it would make sense to factor each avatar
block separately (i.e. have a git_get_avatar that calls
git_get_whatever) but it probably doesn't make sense.

I'll wait for Jakub's (and whoever else wants to chip in) comments,
and then I'll send an update to this patch following your
recommendations.

-- 
Giuseppe "Oblomov" Bilotta