Re: [PATCHv6 8/8] gitweb: add avatar in signoff lines
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Jun 27, 2009, 09:26 UTC
- Message-ID
- <200906271126.38757.jnareb@gmail.com>
- In-Reply-To
- <1245926587-25074-9-git-send-email-giuseppe.bilotta@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.
Show 6 quoted lines
> --- > > 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).
Show 21 quoted lines
>
> 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