Re: [PATCHv2] gitweb: gravatar support

On Fri, Jun 19, 2009 at 10:28 PM, Junio C Hamano[off-list ref] wrote:

I see these repeated patterns in your patch.

quoted

@@ -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.

quoted

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