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.
You have added gravatar support also to 'history' view, which you
should mention here. By the way, I have noticed that there is no
gravatar support in 'commitdiff' view... which probably means that,
as Junio said, this would require some refactoring and factoring out
common code.
>
> The feature is disabled by default, and depends on Digest::MD5, which
> is available in most Perl installations.
I wonder if there should be mentioned that if Digest::MD5 is not
installed, the feature will not be used...
>
> 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.
There are a few other 'code' changes...
>
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index a01eac8..ca716e6 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -28,6 +28,10 @@ img.logo {
> border-width: 0px;
> }
>
> +img.gravatar {
> + vertical-align:middle;
> +}
> +
That is good enough CSS class name for now, but in the future we might
want to use more generic name: avatar, personal_avatar, personal_icon;
or something like that.
> div.page_header {
> height: 25px;
> padding: 8px;
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1e7e2d8..c06356b 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -365,6 +365,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]},
Nice. I think it can be on per-project basis decision whether to
use avatars or not. For example if project uses fake emails, etc.
[...]
> +# 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, $whitespace) = @_;
> + $whitespace = 0 unless defined($whitespace);
> + $size = 32 if (!defined($size) || $size <= 0);
> + return "<img class=\"gravatar\" src=\"http://www.gravatar.com/avatar.php?gravatar_id="; .
> + md5_hex(lc $email) . "&size=$size\" />" . ($whitespace ? " " : "");
> + } else {
> + return "";
> + }
> +}
I don't like this change. I don't like it at all.
First, there is an issue of having two _independently_ optional
parameters. Using 'undef' / not passing parameter to use default value
is IMVHO an anti-pattern; it is a bit unwieldy / ugly to call it with
$whitespace set while using default value for $size.
Second, I think it is better to avoid such boolean parameters like
$whitespace, because you don't know from call site what it means
to pass 1 there (in C we would use constant instead of 1 here).
Both of above are solved in current gitweb code (at least in some
places) by passing hash of (named) optional parameters. So the call
(if not for last note for this issue) would look like the following:
git_get_gravatar($co->{'author_email'}, -size => 16, -whitespace => 1);
Third, rather than formatting with ' ' (ugh, just like formatting
with tab and enter in word processors) I'd rather use (if possible)
CSS style for that; something like margin-right, or padding-right...
if it would work, of course. But see also note below.
Last but not least, but after thinking about it a bit more I think now
that those optional parameters are entirely too low level, too close to
implementation. What matters is not the size (which you would then need
to change in every and each callsite) and adding after image.
What matters is whether to use double size, and whether gravatar image
is on the left side or on the right side of text it is referring to.
You would be then able to easily chose small/large size from hash of
possible sizes (mentioned %avatar_size), and just add appropriate class
differentiator for left/right hand side gravatar.
So the call would look like the following:
git_get_gravatar($co->{'author_email'}, -large => 0, -position => 'left');
or
git_get_gravatar($co->{'author_email'}, -small => 1, -padding => 'right');
or something like that[1] (-large, -small, -double_size, -inline, ...;
-position, -padding, -justify, -pad, -space, -separate, ...).
[1] Naming is unfortunately hard. Fortunately we can change it later.
> +
> sub git_print_authorship {
> my $co = shift;
>
> my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
> print "<div class=\"author_date\">" .
> + git_get_gravatar($co->{'author_email'}, 16, 1) .
> esc_html($co->{'author_name'}) .
> " [$ad{'rfc2822'}";
> if ($ad{'hour_local'} < 6) {
That is a very good idea to add it to git_print_authorship. This should
take care of many other places, and many views, I think. But you should
check if it wouldn't make gravatar appear in unwanted places.
> @@ -4145,7 +4179,7 @@ sub git_shortlog_body {
> @@ -4196,7 +4230,7 @@ sub git_history_body {
> @@ -4352,7 +4386,7 @@ sub git_search_grep_body {
> @@ -5095,8 +5129,9 @@ sub git_log {
This would really, really be easier with log-like view refactoring. This
is on my TODO list for gitweb for a quite long time...
> @@ -5183,7 +5218,8 @@ sub git_commit {
--
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