Re: [PATCHv6 8/8] gitweb: add avatar in signoff lines

[Jakub Narebski <jnareb@gmail.com>]

On Thu, 25 June 2009, Giuseppe Bilotta wrote:

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

BTW. if it is an RFC, it should be marked as RFC in subject
("[RFC PATCHv6 8/8] gitweb: add avatar in signoff lines").

And I guess this issue should be left for later.

> ---
> 
> I can't say I'm really satisfied with the layout given by this patch.
> A significant improvement could be obtained by turning the signoff
> line block into a table with three/four columns (signoff, name,
> email/avatar). 

First, I think adding (gr)avatars to signoff lines might be not worth
it.  Neither GitHub nor Gitorious show gravatars for signoff lines
(note that they use larger images, and therefore easier to view).

Second, it is not the only possible layout.  Let's use one of existing
commits (e1d3793) in git repository as example:

  completion: add --thread=deep/shallow to format-patch

  [1] Signed-off-by: Stephen Boyd <bebarino@gmail.com> [2]          [3]            [4]|
  [1] Trivially-Acked-By: Shawn O. Pearce <spearce@spearce.org> [2] [3]            [4]|
  [1] Signed-off-by: Junio C Hamano <gitster@pobox.com> [2]         [3]            [4]|

Even without changing layout of signoff lines (so they look exactly
like they look in git-show or git-log output, modulo highlighting
and (gr)avatars), there are more possibilities:

 1. On the left side of signoff lines
 2. Current version: on the right side of signoff lines, just after
 3. On the right hand side, aligned; would probably need table
 4. On the right hand side, flushed (floated) right

There is also more complicated solution of having (gr)avatars appear
only on mouseover, either all avatars on hover over signoff block,
or single (and perhaps larger size) avatar on hover over signoff line.
This can be done using pure CSS, without JavaScript[1]

[1] http://meyerweb.com/eric/css/edge/popups/demo2.html

> But we cannot guarantee that signoff lines come all
> together in a block, so this would be more complex to implement.

Actually I think we can assume that signoff lines come all together
in single block at the very end of commit message; we should take
into account possibility of spurious empty lines between signoff
lines, though (it did happen, see e.g. 8dfb17e1).

> 
>  gitweb/gitweb.perl |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7ca115f..d385f55 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3407,7 +3407,10 @@ sub git_print_log {
>  			$signoff = 1;
>  			$empty = 0;
>  			if (! $opts{'-remove_signoff'}) {
> -				print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
> +				my ($email) = $line =~ /<(\S+@\S+)>/;
> +				print "<span class=\"signoff\">" . esc_html($line) . "</span>";
> +				print git_get_avatar($email, 'pad_before' => 1) if $email;
> +				print "<br/>\n";
>  				next;
>  			} else {
>  				# remove signoff lines
> -- 

And here is version with (gr)avatar on the left side of signoff lines
(take a look if it is not better layout):

diff --git c/gitweb/gitweb.perl w/gitweb/gitweb.perl
index 301bdd8..7701bac 100755
--- c/gitweb/gitweb.perl
+++ w/gitweb/gitweb.perl
@@ -3407,6 +3407,8 @@ sub git_print_log {
 			$signoff = 1;
 			$empty = 0;
 			if (! $opts{'-remove_signoff'}) {
+				my ($email) = $line =~ /<(\S+@\S+)>/;
+				print git_get_avatar($email, 'pad_after' => 1) if $email;
 				print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
 				next;
 			} else {


-- 
Jakub Narebski
Poland