Re: [PATCHv2] gitweb: gravatar support

From: Jakub Narebski <jnareb@gmail.com>
Date: Jun 20, 2009, 19:24 UTC
Message-ID: <200906202124.33278.jnareb@gmail.com>
In-Reply-To: <7v63esklxh.fsf@alter.siamese.dyndns.org>

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

> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

Show 37 quoted lines

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

[...]

Show 5 quoted lines

> 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