From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Fri, 19 Jun 2009 22:43:46 GMT
Subject: Re: [PATCHv2] gitweb: gravatar support
Message-ID: <cb7bb73a0906191543l75722b35xa1af532f4d8e1b76@mail.gmail.com>
In-Reply-To: <7v63esklxh.fsf@alter.siamese.dyndns.org>

On Fri, Jun 19, 2009 at 10:28 PM, Junio C Hamano<gitster@pobox.com> wrote:
>
> 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?

I was having the same thoughts while writing v2 of the patch, indeed.
However, I wasn't sure if it was appropriate to the refactoring in the
same patch. A couple of places could be changed to use the existing
git_print_authorship, others would need their own function, as you
point out.

> 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 had to google for picons but yes, it makes sense.

> 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";

I had the impression that in this case it made more aesthetical sense
to have the icon on the other side. I'm not exactly a good designer
though, so we might want to prefer consistency and keep it all the
same way. This section would probably use git_print_authorship or at
least share some code with that.

-- 
Giuseppe "Oblomov" Bilotta