Re: [PATCHv4 2/2] gitweb: gravatar support

[Jakub Narebski <jnareb@gmail.com>]

On Tue, 23 June 2009, Giuseppe Bilotta wrote:

> Introduce gravatar support by adding the appropriate img tag next to
> author and committer in commit(diff), history, shortlog and log view.

That reminds me that I have refactoring all log-like views in my TODO
file for gitweb for quite long time.

> 
> The feature is disabled by default, and depends on Digest::MD5, which
> is a core package since Perl 5.8. If Digest::MD5 cannot be found,
> enabling this feature results in a no-op.

Good description.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.css  |    4 +++
>  gitweb/gitweb.perl |   56 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 54 insertions(+), 6 deletions(-)
> 
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index 68b22ff..ddf982b 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -28,6 +28,10 @@ img.logo {
>  	border-width: 0px;
>  }
>  
> +img.avatar {
> +	vertical-align:middle;
> +}
> +

Nitpick: "vertical-align: middle;" (with space separating key from
value).

>  div.page_header {
>  	height: 25px;
>  	padding: 8px;
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 223648f..531d589 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -195,6 +195,14 @@ our %known_snapshot_format_aliases = (
>  	'x-zip' => undef, '' => undef,
>  );
>  
> +# Pixel sizes for avatars. If the default font sizes or lineheights
> +# are changed, it may be appropriate to change these values too via
> +# $GITWEB_CONFIG.
> +our %avatar_size = (
> +	'default' => 16,
> +	'double'  => 32
> +) ;

Good idea, good description.  I wonder if it would be worth adding
to gitweb_conf.perl description in gitweb/README and gitweb/INSTALL...

Nitpick: ");"

> +
>  # You define site-wide feature defaults here; override them with
>  # $GITWEB_CONFIG as necessary.
>  our %feature = (
> @@ -365,6 +373,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]},
>  );

Good.

>  
>  sub gitweb_get_feature {
> @@ -814,6 +837,10 @@ $git_dir = "$projectroot/$project" if $project;
>  our @snapshot_fmts = gitweb_get_feature('snapshot');
>  @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
>  
> +# check if gravatars are enabled and dependencies are satisfied
> +our $git_gravatar_enabled = gitweb_check_feature('gravatar') &&
> +	(eval { require Digest::MD5; 1; });
> +

I think this is correct.

>  # dispatch
>  if (!defined $action) {
>  	if (defined $hash) {
> @@ -1474,7 +1501,7 @@ sub format_author_html {
>  	my $tag = shift;
>  	my $co = shift;
>  	my $author = chop_and_escape_str($co->{'author_name'}, @_);
> -	return "<$tag class=\"author\">" . $author . "</$tag>\n";
> +	return "<$tag class=\"author\">" . git_get_gravatar($co->{'author_email'}, 'space_after' => 1) . $author . "</$tag>\n";
>  }

Line too long, please break it.

>  
>  # format git diff header line, i.e. "diff --(git|combined|cc) ..."
> @@ -3222,6 +3249,21 @@ sub git_print_header_div {
>  	      "\n</div>\n";
>  }
>  
> +# 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, %params) = @_;
> +		my $pre_white = ($params{'space_before'} ? "&nbsp;" : "");
> +		my $post_white = ($params{'space_after'} ? "&nbsp;" : "");

This name of parameter is a bit too low level for my taste.  It doesn't
matter whether we add '&nbsp;' nonbreakable space before or after img,
but whethere gravatar image has _padding_ on the left/on the right hand
side of gravatar image.  So 'pad_left' and 'pad_right', or 'pad_before'
and 'pad_after' rather than 'space_before' and 'space_after'.

But this is a matter of taste...

> +		my $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};

Nice trick.

> +		return $pre_white . "<img class=\"avatar\" src=\"http://www.gravatar.com/avatar.php?gravatar_id=" .
> +		       Digest::MD5::md5_hex(lc $email) . "&amp;size=$size\" />" . $post_white;
> +	} else {
> +		return "";
> +	}
> +}
> +
>  # Outputs the author name and date in long form
>  sub git_print_authorship {
>  	my $co = shift;
> @@ -3238,7 +3280,8 @@ sub git_print_authorship {
>  		printf(" (%02d:%02d %s)",
>  		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
>  	}
> -	print "]</$tag>\n";
> +	print "]" . git_get_gravatar($co->{'author_email'}, 'space_before' => 1)
> +		  . "</$tag>\n";
>  }
>  

Good.

>  # Outputs the author and commiter name and date in long form
> @@ -3246,9 +3289,9 @@ sub git_print_who {
>  	my $co = shift;
>  	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
>  	my %cd = parse_date($co->{'committer_epoch'}, $co->{'committer_tz'});
> -	print "<tr><td>author</td><td>" . esc_html($co->{'author'}) . "</td></tr>\n".
> -	      "<tr>" .
> -	      "<td></td><td> $ad{'rfc2822'}";
> +	print "<tr><td>author</td><td>" . esc_html($co->{'author'}) . "</td>".
> +	      "<td rowspan=\"2\">" .git_get_gravatar($co->{'author_email'}, 'size' => 'double') . "</td></tr>\n" .

                                    . git_get_gravatar($co->{'author_email'}, 'size' => 'double') .

> +	      "<tr><td></td><td> $ad{'rfc2822'}";

If you put <tr> on separate line, as was before, it would be more 
obvious in this diff that you are only adding single line.

>  	if ($ad{'hour_local'} < 6) {
>  		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
>  		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
> @@ -3258,7 +3301,8 @@ sub git_print_who {
>  	}
>  	print "</td>" .
>  	      "</tr>\n";
> -	print "<tr><td>committer</td><td>" . esc_html($co->{'committer'}) . "</td></tr>\n";
> +	print "<tr><td>committer</td><td>" . esc_html($co->{'committer'}) . "</td>".
> +	      "<td rowspan=\"2\">" .git_get_gravatar($co->{'committer_email'}, 'size' => 'double') . "</td></tr>\n";
>  	print "<tr><td></td><td> $cd{'rfc2822'}" .
>  	      sprintf(" (%02d:%02d %s)", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .
>  	      "</td></tr>\n";

Sidenote: you add here additional column.  Which is present only in 
fragment of this table.  Doesn't it screw layout of the rest of headers?

> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

-- 
Jakub Narebski
Poland