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

[Jakub Narebski <jnareb@gmail.com>]

On Sat, 27 Jun 2009, Giuseppe Bilotta wrote:
> 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.

[...]
>> 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.

I think you are right. I have thought that Atom/RSS have place for icons
for each entry in feed, but it is not the case. Also feed reader can
use gravatars by itself. Sorry for the noise, then.

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

Do I understand this correctly that there is additional patch planned
in new release of this series providing support for gitweb.avatar = picon?

>>> +     # 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, NOTE, NOTE!

One thing I forgot about (and would be discovered when running t9500
with provided patch... among other errors) is that you need to provide
'sub' subroutine for feature to be overridable.

...And that subroutine would be responsible for returning '' (empty
string) when feature is overridable.  See other feature_* subroutines.

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

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

Well, I can understand that.  I was wrong: it is up to (currently not
defined) 'sub' to ensure that gitweb_get_feature would return ''
('default').


Still,

  +our ($git_avatar) = gitweb_get_feature('avatar') || '';

is quite simple... and can be in future subtly wrong (unless it would
be gitweb_check_feature, which always return scalar / single element).


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

Well, by "left for later" here I thought about later as in after this
patch series about gravatars get accepted :-)

-- 
Jakub Narebski
Poland