Date: Fri, 13 Feb 2004 17:50:28 -0600 From: Christopher Wolf <wolf@ti.com> To: freebsd-cvsweb@FreeBSD.org Subject: Issues/patches for 2.9.2 Message-ID: <402D62C4.3050300@ti.com>
next in thread | raw e-mail | index | archive | help
I installed the Beta cvsweb.cgi a few days ago and found a few issues
I'd like
to suggest improvements to. Below are some context diffs and comments on
each issue.
The issues are:
- side-by-side needs more than 164 characters in some environments
- tarball/zip downloads don't seem to work in a nested module
- shortLogLen set to large values causes hard to read table wrapping
- diff/branch links are turned off when viewing a specific revision
- enscript call uses -x x option, which is not Mozilla compatible
- hard to see diffs in color side-by-side view
- side-by-side diffs often color unmodified lines
Patch and commentary below.
-W
------------------------------------------------------------------------------
*With GNU diffutils version 2.7 and an 80 character wide development
environment(!), 164 characters is not wide enough for side-by-side
diffs. A change to 168 shows all 80 columns with room for the diff
indicators:*
***************
*** 267,273 ****
},
{
'descr' => 'side by side',
! 'opts' => ['--side-by-side', '--width=164'],
'colored' => 0,
},
);
--- 268,274 ----
},
{
'descr' => 'side by side',
! 'opts' => ['--side-by-side', '--width=168'],
'colored' => 0,
},
);
***************
*** 570,576 ****
},
{
'descr' => 'side by side, colored',
! 'opts' => ['--side-by-side', '--width=164'],
'colored' => 0,
},
);
--- 571,577 ----
},
{
'descr' => 'side by side, colored',
! 'opts' => ['--side-by-side', '--width=168'],
'colored' => 0,
},
);
------------------------------------------------------------------------------
*When trying to download a tarball/zipfile, the cvs command checks the
files out into one directory, and the zip/tar command tries to read
another: the cvs command checks out the module tree path
(i.e. master/rts/shared/) while the zip/tar command refers to the base
module (i.e. shared/ in this example). This change modifies the cvs
checkout command to use the same name as the tar/zip, although a more
complex fix is probably needed:
*
***************
*** 54,59 ****
--- 54,60 ----
require 5.006;
use strict;
+ use File::Basename;
use warnings;
use filetest qw(access);
***************
*** 747,753 ****
$tag = 'HEAD' if ($tag eq 'MAIN');
my @cmd =
! ($CMD{cvs}, @cvs_options, '-Qd', $cvsroot, 'export', '-r', $tag,
$module);
my $export_err;
my ($errcode, $err) = runproc(\@cmd, '2>', \$export_err);
if ($errcode) {
--- 748,755 ----
$tag = 'HEAD' if ($tag eq 'MAIN');
my @cmd =
! ($CMD{cvs}, @cvs_options, '-Qd', $cvsroot, 'export', '-d',
basename($module), '-r', $tag, $module);
!
my $export_err;
my ($errcode, $err) = runproc(\@cmd, '2>', \$export_err);
if ($errcode) {
------------------------------------------------------------------------------
*
When the configuration variable $shortLogLen is set to something
longer (such as 500), the file and age columns in the directory view
will wrap, which is hard to read. This change makes those columns
nowrap, so that the log message is what wraps. This could be added to
the userid columns too, but those seldom contain spaces.*
***************
*** 1004,1010 ****
next if ($file eq updir() && $where eq '/');
my ($rev, $date, $log, $author, $filename, $keywordsubst) =
@{$fileinfo{$file}} if (defined($fileinfo{$file}));
! printf "<tr class=\"%s\">\n<td colspan=\"2\">",
($dirrow % 2) ? 'even' : 'odd';
if ($file eq updir()) {
--- 1006,1012 ----
next if ($file eq updir() && $where eq '/');
my ($rev, $date, $log, $author, $filename, $keywordsubst) =
@{$fileinfo{$file}} if (defined($fileinfo{$file}));
! printf "<tr class=\"%s\">\n<td nowrap colspan=\"2\">",
($dirrow % 2) ? 'even' : 'odd';
if ($file eq updir()) {
***************
*** 1035,1041 ****
# Show last change in dir
if ($filename) {
! print "</td>\n<td> </td>\n<td class=\"age\">";
print readableTime(time() - $date, 0) if $date;
print "</td>\n<td class=\"author\">", htmlquote($author)
if $show_author;
--- 1037,1043 ----
# Show last change in dir
if ($filename) {
! print "</td>\n<td> </td>\n<td nowrap class=\"age\">";
print readableTime(time() - $date, 0) if $date;
print "</td>\n<td class=\"author\">", htmlquote($author)
if $show_author;
***************
*** 1084,1090 ****
$filesfound++;
printf "<tr class=\"%s\">\n", ($dirrow % 2) ? 'even' : 'odd';
! printf '<td%s>', $allow_cvsgraph ? '' : ' colspan="2"';
my $icon = $isbinary ? $binfileicon : $fileicon;
if ($nofilelinks) {
--- 1086,1092 ----
$filesfound++;
printf "<tr class=\"%s\">\n", ($dirrow % 2) ? 'even' : 'odd';
! printf '<td%s nowrap>', $allow_cvsgraph ? '' : ' colspan="2"';
my $icon = $isbinary ? $binfileicon : $fileicon;
if ($nofilelinks) {
***************
*** 1095,1101 ****
print ' ', &link(htmlquote($file), $url), $attic;
print '</td><td class="graph">', graph_link($fileurl) if
$allow_cvsgraph;
print "</td>\n<td width=\"30\">", display_link($fileurl, $rev);
! print "</td>\n<td class=\"age\">";
print readableTime(time() - $date, 0) if $date;
print "</td>\n<td class=\"author\">", htmlquote($author) if
$show_author;
print "</td>\n<td class=\"log\">";
--- 1097,1103 ----
print ' ', &link(htmlquote($file), $url), $attic;
print '</td><td class="graph">', graph_link($fileurl) if
$allow_cvsgraph;
print "</td>\n<td width=\"30\">", display_link($fileurl, $rev);
! print "</td>\n<td class=\"age\" nowrap>";
print readableTime(time() - $date, 0) if $date;
print "</td>\n<td class=\"author\">", htmlquote($author) if
$show_author;
print "</td>\n<td class=\"log\">";
------------------------------------------------------------------------------
*
When a particular revision is chosen from a revision tree graph, most
of the log links are turned off! I could not see a reason why, and
the developers here wanted to use the tree to find a particular
revision based on tag names, and then see the differences that made
that revision. This change adds a config file variable that allows
turning on those links:*
[cvsweb.cgi:]
***************
*** 2141,2147 ****
if ($show_log_in_markup) {
readLog($fullname) unless $logged; #,$rev);
! printLog($rev, $mimetype, $isbin);
} else {
print "Version: <b>$rev</b><br />\n";
print "Tag: <b>", $input{only_with_tag}, "</b><br />\n"
--- 2143,2149 ----
if ($show_log_in_markup) {
readLog($fullname) unless $logged; #,$rev);
! printLog($rev, $mimetype, $isbin,
$DEFAULTVALUE{'showlinksinmarkupview'});
} else {
print "Version: <b>$rev</b><br />\n";
print "Tag: <b>", $input{only_with_tag}, "</b><br />\n"
[cvsweb.conf:]
***************
*** 134,145 ****
# hidenonreadable: Don't show entries which cannot be read
# 1 Hide non-readable entries
# 0 Show non-readable entries
! "hidenonreadable" => "1",
# ln: Show line numbers in HTMLized views
# 1 Show line numbers
# 0 Don't show line numbers
! "ln" => "0",
);
#
--- 136,156 ----
# hidenonreadable: Don't show entries which cannot be read
# 1 Hide non-readable entries
# 0 Show non-readable entries
! "hidenonreadable" => "0",
!
! # showlinksinmarkupview: whether to show links or text in the markup
! # views. Might be desired if viewers use the
! # graph view to find a version and desire to
! # see changes from previous view without
! # having to click back to the log view.
! # 1 Show links for Branches, CVS Tags, Diffs, etc.
! # 0 Do not show these links
! "showlinksinmarkupview" => 1,
# ln: Show line numbers in HTMLized views
# 1 Show line numbers
# 0 Don't show line numbers
! "ln" => "1",
);
#
------------------------------------------------------------------------------
*
When enscript's -x option is set to 'x', the resulting image map code
is not compatible with the newer mozilla browsers, which are fairly
particular about using 'name' vs 'id' for identifying the image map.
This change calls out to use both name and id and the result is
compatible with IE and Mozilla:*
***************
*** 3110,3116 ****
'-r', $cvsroot,
'-i',
'-M', $mapname,
! '-x', 'x',
"-Omap_branch_href=\"href=\\\"./?only_with_tag=%(%t%)$qquery\\\"\"",
"-Omap_rev_href=\"href=\\\"?rev=%(%R%)$qquery\\\"\"",
"-Omap_diff_href=\"href=\\\"%(%F%).diff" .
--- 3112,3118 ----
'-r', $cvsroot,
'-i',
'-M', $mapname,
! '-x', '4',
"-Omap_branch_href=\"href=\\\"./?only_with_tag=%(%t%)$qquery\\\"\"",
"-Omap_rev_href=\"href=\\\"?rev=%(%R%)$qquery\\\"\"",
"-Omap_diff_href=\"href=\\\"%(%F%).diff" .
------------------------------------------------------------------------------
*
The colors used to display diffs are hard to read, so I made the
following change to force changes in diffs to appear with a grey
background, making them much easier to pick out when scrolling large
files:*
[enscript/lang_cvsweb.st:]
***************
*** 44,50 ****
print ("<i>");
if (face[fg_color])
! print ("<span style=\"color: ", face[fg_color], ";
background-color: lightgrey\">");
if (face[bg_color])
print ("<span style=\"background-color: ", face[bg_color], "\">");
}
--- 44,50 ----
print ("<i>");
if (face[fg_color])
! print ("<span style=\"color: ", face[fg_color], "\">");
if (face[bg_color])
print ("<span style=\"background-color: ", face[bg_color], "\">");
}
------------------------------------------------------------------------------
*
I also found that enscript's regexps in diffs.st would often, when
displaying side-by-side colors diffs, get confused by lines of code
ending in | (think OR) or > (think #include <xxx>) if it had a trailing
tab.
I modified the regexps in enscript's hl/diffs.st to be as follows, which
works with the change of --side-by-side's --width from 164 to 168
(diff above) and adding --expand-tabs to @rcsdiff_options in the
cvsweg,conf file. Now only real diffs appear in color!!*
/^..................................................................................
> / {
/^..................................................................................
\| / {
/^..................................................................................
<$/ {
------------------------------------------------------------------------------
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?402D62C4.3050300>