Re: [PATCHv2] gitweb: gravatar support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx> writes:

> Introduce gravatar support by adding the appropriate img tag next to
> author and committer in commit, shortlog and log view.
>
> The feature is disabled by default, and depends on Digest::MD5, which
> is available in most Perl installations.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx>
> ---
>  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.

I see these repeated patterns in your patch.

> @@ -4145,7 +4179,7 @@ sub git_shortlog_body {
>  		my $author = chop_and_escape_str($co{'author_name'}, 10);
>  		# git_summary() used print "<td><i>$co{'age_string'}</i></td>\n" .
>  		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
> -		      "<td><i>" . $author . "</i></td>\n" .
> +		      "<td>" . git_get_gravatar($co{'author_email'}, 16, 1) . "<i>" . $author . "</i></td>\n" .
>...
> -		      "<td><i>" . $author . "</i></td>\n" .
> +		      "<td>" . git_get_gravatar($co{'author_email'}, 16, 1) . "<i>" . $author . "</i></td>\n" .
>...
> -		      "<td><i>" . $author . "</i></td>\n" .
> +		      "<td>" . git_get_gravatar($co{'author_email'}, 16, 1) . "<i>" . $author . "</i></td>\n" .
>...
> -	print "<tr><td>author</td><td>" . esc_html($co{'author'}) . "</td></tr>\n".
> +	print "<tr><td>author</td><td>" . esc_html($co{'author'}) . "</td>".
> +	      "<td rowspan=\"2\">" .git_get_gravatar($co{'author_email'}) . "</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'}) . "</td></tr>\n";
>...

Doesn't it strike you as needing a bit more refactoring?  The first three
are identical, and you can refactor them to

-		      "<td><i>" . $author . "</i></td>\n" .
+		      "<td>" . oneline_person($author) . "</td>\n" .

where oneline_person is

	sub oneline_person {
        	my ($me) = @_;
                return ($me->{'smallicon'} .
			"<i>" . $me->{'name_escaped'} . "</i>");
        }

And instead of doing "my $author = chop_and_escape_str()", you will
set up richer my $author upfront, like so:

	my $author = +{
		name_escaped => chop_and_escape_str($co{'author_name'}, 10),
                smallicon => git_get_gravatar($co{'author_email'}, 16, 1),
        };

and pass it to the helper function.

That way, people who do not like italics, or people who want to have the
icon at the end instead of at the beginning, can touch only one place
(i.e. "sub oneline_person").

By the way, in the above example, I named the field 'smallicon', as use of
gravatar is merely an implementation detail.  It is plausible other people
may want to use picons instead.

I do not know about the following hunk (why does it have the icon at the
end, unlike the other ones?), but I think you got the idea.

> -		      "<i>" . esc_html($co{'author_name'}) .  " [$ad{'rfc2822'}]</i><br/>\n" .
> -		      "</div>\n";
> +		      "<i>" . esc_html($co{'author_name'}) .  " [$ad{'rfc2822'}]</i>&nbsp;" .
> +		      git_get_gravatar($co{'author_email'}, 16) .
> +		      "<br/>\n</div>\n";

In the medium term, we may want to go one step further and do

    package person;

and make sets of methods like "oneline_person".
--
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

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]