Re: [PATCHv2] gitweb: gravatar support
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Jun 20, 2009, 10:57 UTC
- Message-ID
- <200906201257.44486.jnareb@gmail.com>
- In-Reply-To
- <1245435670-5688-1-git-send-email-giuseppe.bilotta@gmail.com>
On Fri, 19 June 2009, Giuseppe Bilotta wrote:
> Introduce gravatar support by adding the appropriate img tag next to > author and committer in commit, shortlog and log view.
You have added gravatar support also to 'history' view, which you should mention here. By the way, I have noticed that there is no gravatar support in 'commitdiff' view... which probably means that, as Junio said, this would require some refactoring and factoring out common code.
> > The feature is disabled by default, and depends on Digest::MD5, which > is available in most Perl installations.
I wonder if there should be mentioned that if Digest::MD5 is not installed, the feature will not be used...
Show 9 quoted lines
> > Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com> > --- > gitweb/gitweb.css | 4 ++++ > gitweb/gitweb.perl | 51 ++++++++++++++++++++++++++++++++++++++++++++------- > 2 files changed, 48 insertions(+), 7 deletions(-) > > Changes from the previous version include gravatar use in history view, > CSS use and the possibility to override the feature on a per-project basis.
There are a few other 'code' changes...
Show 13 quoted lines
>
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index a01eac8..ca716e6 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -28,6 +28,10 @@ img.logo {
> border-width: 0px;
> }
>
> +img.gravatar {
> + vertical-align:middle;
> +}
> +That is good enough CSS class name for now, but in the future we might want to use more generic name: avatar, personal_avatar, personal_icon; or something like that.
Show 26 quoted lines
> div.page_header {
> height: 25px;
> padding: 8px;
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1e7e2d8..c06356b 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -365,6 +365,21 @@ our %feature = (
> 'sub' => \&feature_patches,
> 'override' => 0,
> 'default' => [16]},
> +
> + # Gravatar support. When this feature is enabled, views such as
> + # shortlog or commit will display the gravatar associated with
> + # the email of the committer(s) and/or author(s). Please note that
> + # the feature depends on Digest::MD5.
> +
> + # To enable system wide have in $GITWEB_CONFIG
> + # $feature{'gravatar'}{'default'} = [1];
> + # To have project specific config enable override in $GITWEB_CONFIG
> + # $feature{'gravatar'}{'override'} = 1;
> + # and in project config gitweb.gravatar = 0|1;
> + 'gravatar' => {
> + 'sub' => sub { feature_bool('gravatar', @_) },
> + 'override' => 0,
> + 'default' => [0]},Nice. I think it can be on per-project basis decision whether to use avatars or not. For example if project uses fake emails, etc.
[...]
Show 13 quoted lines
> +# insert a gravatar for the given $email at the given $size if the feature
> +# is enabled
> +sub git_get_gravatar {
> + if ($git_gravatar_enabled) {
> + my ($email, $size, $whitespace) = @_;
> + $whitespace = 0 unless defined($whitespace);
> + $size = 32 if (!defined($size) || $size <= 0);
> + return "<img class=\"gravatar\" src=\"http://www.gravatar.com/avatar.php?gravatar_id=" .
> + md5_hex(lc $email) . "&size=$size\" />" . ($whitespace ? " " : "");
> + } else {
> + return "";
> + }
> +}I don't like this change. I don't like it at all.
First, there is an issue of having two _independently_ optional parameters. Using 'undef' / not passing parameter to use default value is IMVHO an anti-pattern; it is a bit unwieldy / ugly to call it with $whitespace set while using default value for $size.
Second, I think it is better to avoid such boolean parameters like $whitespace, because you don't know from call site what it means to pass 1 there (in C we would use constant instead of 1 here).
Both of above are solved in current gitweb code (at least in some places) by passing hash of (named) optional parameters. So the call (if not for last note for this issue) would look like the following:
git_get_gravatar($co->{'author_email'}, -size => 16, -whitespace => 1);Third, rather than formatting with ' ' (ugh, just like formatting with tab and enter in word processors) I'd rather use (if possible) CSS style for that; something like margin-right, or padding-right... if it would work, of course. But see also note below.
Last but not least, but after thinking about it a bit more I think now that those optional parameters are entirely too low level, too close to implementation. What matters is not the size (which you would then need to change in every and each callsite) and adding after image. What matters is whether to use double size, and whether gravatar image is on the left side or on the right side of text it is referring to.
You would be then able to easily chose small/large size from hash of possible sizes (mentioned %avatar_size), and just add appropriate class differentiator for left/right hand side gravatar.
So the call would look like the following:
git_get_gravatar($co->{'author_email'}, -large => 0, -position => 'left');or
git_get_gravatar($co->{'author_email'}, -small => 1, -padding => 'right');or something like that[1] (-large, -small, -double_size, -inline, ...; -position, -padding, -justify, -pad, -space, -separate, ...).
[1] Naming is unfortunately hard. Fortunately we can change it later.
Show 10 quoted lines
> +
> sub git_print_authorship {
> my $co = shift;
>
> my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
> print "<div class=\"author_date\">" .
> + git_get_gravatar($co->{'author_email'}, 16, 1) .
> esc_html($co->{'author_name'}) .
> " [$ad{'rfc2822'}";
> if ($ad{'hour_local'} < 6) {That is a very good idea to add it to git_print_authorship. This should take care of many other places, and many views, I think. But you should check if it wouldn't make gravatar appear in unwanted places.
> @@ -4145,7 +4179,7 @@ sub git_shortlog_body {
> @@ -4196,7 +4230,7 @@ sub git_history_body {
> @@ -4352,7 +4386,7 @@ sub git_search_grep_body {
> @@ -5095,8 +5129,9 @@ sub git_log {This would really, really be easier with log-like view refactoring. This is on my TODO list for gitweb for a quite long time...
> @@ -5183,7 +5218,8 @@ sub git_commit {-- Jakub Narebski Poland