On Tue, 23 June 2009, Giuseppe Bilotta wrote:
> The refactoring allows easier customization of the output by means of
> CSS, and improves extensibility on the CGI side too.
How? The above description is slightly lacking in details. Does it
mean that we replaced remains of presentational HTML (<i> and <b> tags)
by CSS styling using classes? What about support for text browsers
such as lynx, links/elinks, w3m? (I'm not saying here that this should
block moving from presentational HTML to CSS, just that it should be
considered).
How it improves extensibility on the CGI side? Does it mean that it
would be easier to add new features or otherwise extend gitweb, because
common parts are factored out?
>
> Layout is preserved for all views except for 'commitdiff', which now
> uses the same format as 'commit'.
Currently (i.e. without this patch) gitweb uses the following forms of
showing authorship, from shortest to longest:
* "Giuseppe Bilotta"
This is form used in 'shortlog' and 'history' views.
* "Giuseppe Bilotta [Mon, 22 Jun 2009 22:49:58 +0000]"
This is form used by 'log' view.
* "Giuseppe Bilotta [Mon, 22 Jun 2009 22:49:58 +0000 (00:49 +0200)]"
This is form used by 'commitdiff' view.
* "author Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx>
Mon, 22 Jun 2009 22:49:58 +0000 (00:49 +0200)
committer Jakub Narebski <jnareb@xxxxxxxxx>
Tue, 23 Jun 2009 18:02:21 +0000 (20:02 +0200)"
This is form used by 'commit' view.
I can understand factoring out code dealing with authorship in 'commit'
view, because it is a bit different from other headers; on the other
hand it is slightly similar to dealing other headers.
I think that changing 'commitdiff' view to use more detailed author
information might be a good idea. But I do not think that having
it bundled together with this refactoring patch is a good idea. It
really should be a separate patch, and refactoring shouldn't change
output, if possible (and I think here is very much possible, see below).
If you wanted to reduce number of places where you need to add later
gravatar support, it would be better to factor out formatting authorship
information in 'log' and 'commitdiff' view, either by having 'log' view
use 'commitdiff' authorship subroutine (and not 'commitdiff' use new
'commit' authorship subroutine), or make this subroutine produce two
slightly different outputs.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx>
> ---
> gitweb/gitweb.css | 5 ++-
> gitweb/gitweb.perl | 79 +++++++++++++++++++++++++++++++---------------------
> 2 files changed, 51 insertions(+), 33 deletions(-)
>
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index a01eac8..68b22ff 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -132,11 +132,14 @@ div.list_head {
> font-style: italic;
> }
>
> +.author_date, .author {
> + font-style: italic;
> +}
> +
> div.author_date {
> padding: 8px;
> border: solid #d9d8d1;
> border-width: 0px 0px 1px 0px;
> - font-style: italic;
> }
Good.
>
> a.list {
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1e7e2d8..223648f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1469,6 +1469,14 @@ sub format_subject_html {
> }
> }
>
> +# format the author name with the given tag and optionally shortening it
I would like to see calling convention for this subroutine described,
because it is not entirely obvious; see passing of (misnamed)
chop_and_escape_str subroutine parameters.
> +sub format_author_html {
> + my $tag = shift;
> + my $co = shift;
> + my $author = chop_and_escape_str($co->{'author_name'}, @_);
> + return "<$tag class=\"author\">" . $author . "</$tag>\n";
> +}
> +
> # format git diff header line, i.e. "diff --(git|combined|cc) ..."
> sub format_git_diff_header_line {
> my $line = shift;
> @@ -3214,11 +3222,13 @@ sub git_print_header_div {
> "\n</div>\n";
> }
>
> +# Outputs the author name and date in long form
> sub git_print_authorship {
> my $co = shift;
> + my $tag = shift || 'div';
>
> my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
> - print "<div class=\"author_date\">" .
> + print "<$tag class=\"author_date\">" .
> esc_html($co->{'author_name'}) .
> " [$ad{'rfc2822'}";
> if ($ad{'hour_local'} < 6) {
> @@ -3228,7 +3238,30 @@ sub git_print_authorship {
> printf(" (%02d:%02d %s)",
> $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
> }
> - print "]</div>\n";
> + print "]</$tag>\n";
> +}
Good. You could have add option to print or not print localtime
(or use version with localtime in 'log' view).
> +
> +# Outputs the author and commiter name and date in long form
> +sub git_print_who {
Hmmm... perhaps git_print_authorship_long? I am not sure if git_print_who
is a good name to have. (Yes, I know, naming is hard).
> + my $co = shift;
> + my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
> + my %cd = parse_date($co->{'committer_epoch'}, $co->{'committer_tz'});
> + print "<tr><td>author</td><td>" . esc_html($co->{'author'}) . "</td></tr>\n".
> + "<tr>" .
> + "<td></td><td> $ad{'rfc2822'}";
> + if ($ad{'hour_local'} < 6) {
> + printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
> + $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
> + } else {
> + printf(" (%02d:%02d %s)",
> + $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
> + }
> + print "</td>" .
> + "</tr>\n";
> + print "<tr><td>committer</td><td>" . esc_html($co->{'committer'}) . "</td></tr>\n";
> + print "<tr><td></td><td> $cd{'rfc2822'}" .
> + sprintf(" (%02d:%02d %s)", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .
> + "</td></tr>\n";
> }
While it might be a good idea of factoring out this part into separate
subroutine, I don't think that 'commitdiff' should use it... well, at
least not in this commit.
>
> sub git_print_page_path {
> @@ -4142,11 +4175,9 @@ sub git_shortlog_body {
> print "<tr class=\"light\">\n";
> }
> $alternate ^= 1;
> - 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>";
> + format_author_html('td', \%co, 10) . "<td>";
+ format_author_html('td', \%co, 10) . "<td>";
Tabs are for indent, spaces are for align, at least for gitweb.perl
(this unfortunately requires either handcrafting, or using smart-tabs,
but it has the advantage of presenting the same layour regardless of
the tab size).
> print format_subject_html($co{'title'}, $co{'title_short'},
> href(action=>"commit", hash=>$commit), $ref);
> print "</td>\n" .
Very good.
> @@ -4193,11 +4224,9 @@ sub git_history_body {
> print "<tr class=\"light\">\n";
> }
> $alternate ^= 1;
> - # shortlog uses chop_str($co{'author_name'}, 10)
> - my $author = chop_and_escape_str($co{'author_name'}, 15, 3);
> print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
> - "<td><i>" . $author . "</i></td>\n" .
> - "<td>";
> + # shortlog uses format_author_html('td', \%co, 10)
> + format_author_html('td', \%co, 15, 3) . "<td>";
Tabs are for indent, spaces are for align.
And you don't need to try to align in the comment now.
> # originally git_history used chop_str($co{'title'}, 50)
> print format_subject_html($co{'title'}, $co{'title_short'},
> href(action=>"commit", hash=>$commit), $ref);
> @@ -4350,9 +4379,8 @@ sub git_search_grep_body {
> print "<tr class=\"light\">\n";
> }
> $alternate ^= 1;
> - my $author = chop_and_escape_str($co{'author_name'}, 15, 5);
> print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
> - "<td><i>" . $author . "</i></td>\n" .
> + format_author_html('td', \%co, 15, 5) .
> "<td>" .
> $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}),
> -class => "list subject"},
Good.
This one has correct whitespace mixture (tabs for indent, spaces for
align).
> @@ -5094,9 +5122,9 @@ sub git_log {
> " | " .
> $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree") .
> "<br/>\n" .
> - "</div>\n" .
> - "<i>" . esc_html($co{'author_name'}) . " [$ad{'rfc2822'}]</i><br/>\n" .
> "</div>\n";
> + git_print_authorship(\%co);
> + print "<br/>\n</div>\n";
>
> print "<div class=\"log_body\">\n";
> git_print_log($co{'comment'}, -final_empty_line=> 1);
Good.
> @@ -5115,8 +5143,6 @@ sub git_commit {
> $hash ||= $hash_base || "HEAD";
> my %co = parse_commit($hash)
> or die_error(404, "Unknown commit object");
> - my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
> - my %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
>
> my $parent = $co{'parent'};
> my $parents = $co{'parents'}; # listref
> @@ -5183,22 +5209,7 @@ sub git_commit {
> }
> print "<div class=\"title_text\">\n" .
> "<table class=\"object_header\">\n";
> - print "<tr><td>author</td><td>" . esc_html($co{'author'}) . "</td></tr>\n".
> - "<tr>" .
> - "<td></td><td> $ad{'rfc2822'}";
> - if ($ad{'hour_local'} < 6) {
> - printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
> - $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
> - } else {
> - printf(" (%02d:%02d %s)",
> - $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
> - }
> - print "</td>" .
> - "</tr>\n";
> - print "<tr><td>committer</td><td>" . esc_html($co{'committer'}) . "</td></tr>\n";
> - print "<tr><td></td><td> $cd{'rfc2822'}" .
> - sprintf(" (%02d:%02d %s)", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .
> - "</td></tr>\n";
> + git_print_who(\%co);
> print "<tr><td>commit</td><td class=\"sha1\">$co{'id'}</td></tr>\n";
> print "<tr>" .
> "<td>tree</td>" .
This makes git_commit subroutine less complex, so it might be good
idea anyway.
> @@ -5579,7 +5590,11 @@ sub git_commitdiff {
> git_header_html(undef, $expires);
> git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
> git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
> - git_print_authorship(\%co);
> + print "<div class=\"title_text\">\n" .
> + "<table class=\"object_header\">\n";
> + git_print_who(\%co);
> + print "</table>".
> + "</div>\n";
> print "<div class=\"page_body\">\n";
> if (@{$co{'comment'}} > 1) {
> print "<div class=\"log\">\n";
But here we can use git_print_authorship('div', \%co, undef, undef, -localtime=>1)
or something like that; no need to change 'commitdiff' view.
> --
> 1.6.3.rc1.192.gdbfcb
>
>
--
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