Re: [PATCHv2] gitweb: gravatar support

On Fri, 19 June 2009, Junio C Hamano wrote:

Giuseppe Bilotta [off-list ref] writes:

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?  The first three
are identical, and you can refactor them to

-		      "<td><i>" . $author . "</i></td>\n" .
+		      "<td>" . oneline_person($author) . "</td>\n" .

where oneline_person is

	sub oneline_person {
        	my ($me) = @_;
                return ($me->{'smallicon'} .
			"<i>" . $me->{'name_escaped'} . "</i>");
        }

[...]

Well, gitweb certainly needs some love^W refactoring.  That is only one
of areas.  But should we postpone introducing new features till gitweb
gets cleaned up?  Although in this case I guess we can do cleanup during
introduction of new feature (which makes stronger case for factorizing
common elements, as they just get larger, those common elements, now with
this feature).

That way, people who do not like italics, or people who want to have the
icon at the end instead of at the beginning, can touch only one place
(i.e. "sub oneline_person").

About using presentational HTML: I think one reason behind keeping it
is to have gitweb work also in text browsers like lynx, w3m or lynx, 
which can not understand CSS.

[...]

In the medium term, we may want to go one step further and do

    package person;

and make sets of methods like "oneline_person".

That would be first in gitweb.

BTW. Should we split gitweb into packages? Should we split gitweb into
separate files (one package per file)?
-- 
Jakub Narebski
Poland