Re: [PATCHv7 5/9] gitweb: (gr)avatar support

[Jakub Narebski <jnareb@gmail.com>]

On Sun, 28 Jun 2009, Giuseppe Bilotta wrote:
> 2009/6/27 Jakub Narebski <jnareb@gmail.com>:

>>> +     # Avatar support. When this feature is enabled, views such as
>>> +     # 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.
[...]

>> By the way, we might want to accept 'none' instead of empty value
>> or no value to turn off avatar support for specific project (if
>> avatars are turned on globally, and project specific override is on).
>> We use this technique for 'snapshot' feature.
>>
>> But this isn't terribly important.
> 
> Well, since 'none' is not a known provider, that trick should work
> correctly anyway.

Ah, I have forgot about this.  Perhaps it should be stated more 
explicitly that unknown provider (type) = no avatar image?  Or do I
simply not see it?

>>> +our ($git_avatar) = gitweb_get_feature('avatar');
>>> +if ($git_avatar eq 'gravatar') {
>>> +     $git_avatar = '' unless (eval { require Digest::MD5; 1; });
>>> +} else {
>>> +     $git_avatar = '';
>>> +}
>>
>> Thoughts for the future: this can lead to not very pretty if-elsif
>> chain.  We have replaces such chain for selecting action (for dispatch)
>> by using %actions hash of subroutine refs (as a kind of 'switch'/'given'
>> statement).  We could do the same for avatar provider initialization
>> and validation subroutines.
>>
>> But for now it is clear enough.  Don't worry about this issue now.
> 
> I'm also not terribly sure about how to implement _this_ through hash
> calls. But I did consider the issue (how I wish Perl had an actual
> switch statement ...)

Perl 5.10, Perl 6, Switch.pm... but it isn't something that we can use
in gitweb.  Switch statement (spelled 'given / when') in Perl6 / Perl 5.10
is very, very powerfull (from what I have read).

But what I meant here is something like the following:

  our %avatar_providers = (
  	'gravatar' => sub { eval { require Digest::MD5; 1; } },
  	'picon'    => sub { 1; },
  );
  unless (defined $avatar_providers{$git_avatar} &&
          $avatar_providers{$git_avatar}->()) {
  	$git_avatar = '';
  }

Or something like that (we can use 'undef' for providers without need
for initialization).  But that can be improved later, "in tree".


On the other hand side this solution wouldn't I think be able to deal
with specifying fallbacks using e.g. 'gravatar+picon' (gravatar using
picon URL as fallback i.e. "default" argument), or two images side by
side like GMane uses (if there are two), e.g. 'gravatar picon' or 
'gravatar, picon'.

Food for thought.  Not something to act on now.

>> Running t9500-gitweb-standalone-no-errors.sh with --debug option shows
>> failing of 15 among 87 tests, with error:
>>
>>   gitweb.perl: Use of uninitialized value in hash element at gitweb/gitweb.perl line 1524.
>>
>> which is the above line.  This line should read:
>>
>>  +     my $size = exists $opts{'size'}
>>  +             ? $avatar_size{$opts{'size'}} || $avatar_size{'default'}
>>  +             : $avatar_size{'default'};
>>
>> or something like that, e.g.:
>>
>>  +     my $size = $avatar_size{ defined $opts{'size'} ? $opts{'size'} : 'default' }
>>  +             || $avatar_size{'default'};
> 
> I did $opts{-size} ||= 'default'. Or is it bad form to modify the
> passed option hash?

No, it is O.K.  We do similar thing in href(), deleting keys from hash.

>>> +     my $url = "";
>>> +     if ($git_avatar eq 'gravatar') {
>>> +             $url = "http://www.gravatar.com/avatar.php?gravatar_id=" .
>>> +                     Digest::MD5::md5_hex(lc $email) . "&amp;size=$size";
>>
>> Why not use the new API[1][2]?:
> 
> Updated.

This affects also the next patch in series.

-- 
Jakub Narebski
Poland