Re: [PATCHv6 4/8] gitweb: (gr)avatar support

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

2009/6/26 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 25 June 2009, Giuseppe Bilotta wrote:
>
>> Introduce avatar support: the featuer adds the appropriate img tag next
>> to author and committer in commit(diff), history, shortlog and log view.
>
> You forgot about 'tag' view (but I guess it would be done in next
> version of this patch series).

Indeed, it's automatically addressed because patch view follows the refactoring.

> There is also 'feed' action (Atom and RSS formats), but that is certainly
> separate issue, for a separate patch.

I'm not entirely sure we want avatars there.

> You would _probably_ want to squash the following, just in case:

[snip]

> But even if you don't squash it, please run t9500 with this patch
> applied, to catch Perl errors and warnings.

I'll squash it in.

> Sidenote: Gravatar API description[1] mentions 'identicon', 'monsterid',
> 'wavatar'.  There are 'picons' (personal icons)[2].  Also avatars doesn't
> need to be global: they can be some local static image somewhere in web
> server which serves gitweb script, or they can be stored somewhere in
> repository following some convention.
>
> Current implementation is flexible enough to leave place for extending
> this feature, but also doesn't try to plan too far in advance.  YAGNI
> (You Ain't Gonna Need It).
>
> [1] http://www.gravatar.com/site/implement/url
> [2] http://www.cs.indiana.edu/picons/ftp/faq.html

The forthcoming series has picons provider and gravatar fallback;
however, we might want to have some way to make the gravatar fallback
configurable.

>> +     # To enable system wide have in $GITWEB_CONFIG
>> +     # $feature{'avatar'}{'default'} = ['gravatar'];
>> +     # To have project specific config enable override in $GITWEB_CONFIG
>> +     # $feature{'avatar'}{'override'} = 1;
>> +     # and in project config gitweb.avatar = gravatar;
>> +     'avatar' => {
>> +             'override' => 0,
>> +             'default' => ['']},
>
> Note that to disable feature with non-boolean 'default' we use empty
> list [] (which means 'undef' when parsing, which is false); see
> description of features 'snapshot', 'actions'; 'ctags' what is strange
> uses [0] here...  Using [''] is a bit strange; and does not protect
> you, I think.

Using an empty string (or 0 like ctags do) is nice because it spares
the undef check you mention later on, and since empty strings and 0
evaluate to false in Perl, it's a good way to handle it. Moreover, any
string which is not an actual provider would result in no avatars.
More about this later.

>> +# check if avatars are enabled and dependencies are satisfied
>> +our ($git_avatar) = gitweb_get_feature('avatar');
>
> IMPORTANT!!!
>
> Because you now allow possibility that there can be other avatars
> than those provided by Gravatar, you should explain in comment
> what this check below does (e.g. something like "load Digest::MD5,
> required for gravatar support, and disable it if it does not exist"),
> so people adding (in the future) support for other kind of avatars
> would know that they should put similar test there, if needed.

I'll make the check and subsequent switch a little cleaner.

>> +if (($git_avatar eq 'gravatar') &&
>> +   !(eval { require Digest::MD5; 1; })) {
>> +     $git_avatar = '';
>> +}
>
> Here you would have to protect against $git_avatar being undefined...
> but you should do it anyway, as gitweb_get_feature() can return
> undef / empty list.

Using '' as defalt instead of [] shields me from this problem, and
works properly for boolean checks.

> This might be good enough starting point, but I wonder if it wouldn't
> be a better solution to provide additional column with avatar image
> when avatar support is enabled.  You would get a better layout in
> a very rare case[3] when 'Author' column is too narrow and author is
> info is wrapped:
>
>  [#] Jonathan
>  H. Random
>
> versus in separate columns case:
>
>  [#] | Jonathan
>      | H. Random
>
> But this is a very minor problem, which can be left for separate patch.
>
> [3] unless you use netbook or phone to browse...

I had considered going this way, but it made the code somewhat more
complex so I went for the simpler solution. I'll look into putting it
in separate cells further on.


-- 
Giuseppe "Oblomov" Bilotta