2009/6/26 Jakub Narebski <jnareb@xxxxxxxxx>: > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html