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

On Sun, 28 Jun 2009, Giuseppe Bilotta wrote:

2009/6/27 Jakub Narebski [off-list ref]:

quoted

quoted

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

[...]

quoted

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?

quoted

quoted

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

quoted

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.

quoted

quoted

+     my $url = "";
+     if ($git_avatar eq 'gravatar') {
+             $url = "http://www.gravatar.com/avatar.php?gravatar_id=" .
+                     Digest::MD5::md5_hex(lc $email) . "&size=$size";

Why not use the new API[1][2]?:

Updated.

This affects also the next patch in series.

-- 
Jakub Narebski
Poland