On Fri, 19 June 2009, Junio C Hamano wrote:
> Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx> writes:
> 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.
[...]
> 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
--
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