On Fri, Jun 19, 2009 at 10:28 PM, Junio C Hamano<gitster@xxxxxxxxx> 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> " .
>> + 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
--
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