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.
>
> The feature is disabled by default, and depends on Digest::MD5.
Digest::MD5 is core Perl module and (in, I guess, most cases)
installed together with Perl.
You might want to mention it in the commit message.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx>
> ---
> gitweb/gitweb.perl | 41 ++++++++++++++++++++++++++++++++++++-----
> 1 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1e7e2d8..9f40070 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -365,6 +365,18 @@ 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];
> + # Project specific override is not supported.
> + 'gravatar' => {
> + 'override' => 0,
> + 'default' => [0]},
Yet another global feature without project specific override. Hmmm...
I wonder if project specific and global (non-overridable) features
should be separated. But it is question for a separate commit.
Question: why it is not overridable (why project specific override
is not supported for this feature)? Some projects may use Gravatars,
some might not, although I guess that usually it is deployment specific
feature.
> );
>
> sub gitweb_get_feature {
> @@ -814,6 +826,9 @@ $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 { use Digest::MD5 qw(md5_hex); 1; });
I would use line break here:
+our $git_gravatar_enabled = gitweb_check_feature('gravatar') &&
+ (eval { use Digest::MD5 qw(md5_hex); 1; });
or
+our $git_gravatar_enabled = gitweb_check_feature('gravatar')
+ && (eval { use Digest::MD5 qw(md5_hex); 1; });
I really like the fact that we do not include Digest::MD5 if it is not
required (if feature is not tuned on).
> +
> # dispatch
> if (!defined $action) {
> if (defined $hash) {
> @@ -4123,6 +4138,19 @@ sub git_project_list_body {
> print "</table>\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, $size) = @_;
> + $size = 32 if $size <= 0;
You would probably want to protect against $size being undefined:
+ $size = 32 if (!defined($size) || $size <= 0);
Because currently when you are not passing size parameter to use
default size you would get the following warning:
Use of uninitialized value in numeric le (<=) ...
Did you run the t9500, adding test enabling gravatars?
Hmmm... perhaps we should add test with all possible features turned
on (unless they need extra configuration... oh, so it isn't as easy
as I initially thought to add this...).
> + return "<img src=\"http://www.gravatar.com/avatar.php?gravatar_id="; .
> + md5_hex($email) . "&size=$size\" style=\"vertical-align:middle\" />";
Hmmm... 'style="vertical-align:middle"' or 'class="gravatar"' (plus
addition to CSS)?
> + } else {
> + return "";
> + }
> +}
> +
[...]
> @@ -4145,7 +4173,7 @@ sub git_shortlog_body {
> + "<td>" . git_get_gravatar($co{'author_email'}, 16) . " <i>" . $author . "</i></td>\n" .
> @@ -5095,8 +5123,9 @@ sub git_log {
> + "<i>" . esc_html($co{'author_name'}) . " [$ad{'rfc2822'}]</i> " .
> + git_get_gravatar($co{'author_email'}, 16) .
> + "<br/>\n</div>\n";
> @@ -5183,7 +5212,8 @@ sub git_commit {
> + print "<tr><td>author</td><td>" . esc_html($co{'author'}) . "</td>".
> + "<td rowspan=\"2\">" .git_get_gravatar(lc $co{'author_email'}) . "</td></tr>\n" .
> + print "<tr><td>committer</td><td>" . esc_html($co{'committer'}) . "</td>".
> + "<td rowspan=\"2\">" .git_get_gravatar(lc $co{'committer_email'}) . "</td></tr>\n";
Hmmm... why use 'lc' here and not in other places? Also gravatars are
not shown in 'history' view, but I guess that could wait for proper
refactoring of all log-like views/actions to use common infrastructure.
Those all look nice with the *default* font sizes. But as the size of
gravatar is used when constructing gravatar URL, to pass to gravatar.com
I don't see how this problem can be resolved... Beside making it
configurable, I guess...
--
Jakub Narebski
Poland
--
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