Hi everyone,As mentioned earlier, there was a bug in the progress bar implementation that caused the tests in multi-byte and multi-column locales to fail. I've rectified the issue by re-working each part of the progress bar to ensure that they are indeed using exactly the amount of space that has been allocated.
I also tried to clean up some of the code. I tested these commits on Travis and there seem to be no issues. All tests are passing with Russian, Turkish and C locales.
Please review the patch, and if good, I'll push it to master. Since it touches the most user-centric part of Wget, I'd like to see it as well reviewed and tested as possible.
-- Thanking You, Darshit Shah
From 1b4793de27b20d00c3ad11c510002398121993c6 Mon Sep 17 00:00:00 2001
From: Darshit Shah <dar...@gmail.com>
Date: Tue, 8 Dec 2015 20:03:37 +0100
Subject: [PATCH 1/2] Fix progress bar assertion with multibyte locales
* src/progress.c (bar_create): Define size of progress buffer explicitly
(create_image): Clean up progress bar image creation. Use memset
instead of for loops to create arrays of the same byte.
---
src/progress.c | 89 ++++++++++++++++++++++------------------------------------
1 file changed, 34 insertions(+), 55 deletions(-)
diff --git a/src/progress.c b/src/progress.c
index 61b635d..861f5ce 100644
--- a/src/progress.c
+++ b/src/progress.c
@@ -594,7 +594,8 @@ bar_create (const char *f_download, wgint initial, wgint total)
bp->width = screen_width - 1;
/* + enough space for the terminating zero, and hopefully enough room
* for multibyte characters. */
- bp->buffer = xmalloc (bp->width + 100);
+#define BUF_LEN (bp->width + 100)
+ bp->buffer = xmalloc (BUF_LEN);
logputs (LOG_VERBOSE, "\n");
@@ -899,12 +900,11 @@ get_eta (int *bcd)
static void
create_image (struct bar_progress *bp, double dl_total_time, bool done)
{
+ memset (bp->buffer, '\0', BUF_LEN);
const int MAX_FILENAME_COLS = bp->width / 4;
char *p = bp->buffer;
wgint size = bp->initial_length + bp->count;
- int size_grouped_pad; /* Used to pad the field width for size_grouped. */
-
struct bar_progress_hist *hist = &bp->hist;
int orig_filename_cols = count_cols (bp->f_download);
@@ -951,10 +951,9 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done)
if (orig_filename_cols <= MAX_FILENAME_COLS)
{
int padding = MAX_FILENAME_COLS - orig_filename_cols;
- sprintf (p, "%s ", bp->f_download);
- p += orig_filename_cols + 1;
- for (;padding;padding--)
- *p++ = ' ';
+ p += sprintf (p, "%s ", bp->f_download);
+ memset (p, ' ', padding);
+ p += padding;
}
else
{
@@ -992,9 +991,8 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done)
memcpy (p, bp->f_download + offset_bytes, bytes_in_filename);
p += bytes_in_filename;
padding = MAX_FILENAME_COLS - (padding + *cols_ret);
- for (;padding;padding--)
- *p++ = ' ';
- *p++ = ' ';
+ memset (p, ' ', padding + 1);
+ p += padding + 1;
}
/* "xx% " */
@@ -1002,15 +1000,13 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done)
{
int percentage = 100.0 * size / bp->total_length;
assert (percentage <= 100);
-
- if (percentage < 100)
- sprintf (p, "%3d%%", percentage);
- else
- strcpy (p, "100%");
- p += 4;
+ p += sprintf (p, "%3d%%", percentage);
}
else
- APPEND_LITERAL (" ");
+ {
+ memset (p, ' ', PROGRESS_PERCENT_LEN);
+ p += PROGRESS_PERCENT_LEN;
+ }
/* The progress bar: "[====> ]" or "[++==> ]". */
if (progress_size && bp->total_length > 0)
@@ -1022,7 +1018,6 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done)
int dlsz = (double)size / bp->total_length * progress_size;
char *begin;
- int i;
assert (dlsz <= progress_size);
assert (insz <= dlsz);
@@ -1032,18 +1027,19 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done)
/* Print the initial portion of the download with '+' chars, the
rest with '=' and one '>'. */
- for (i = 0; i < insz; i++)
- *p++ = '+';
+ memset (p, '+', insz);
+ p += insz;
+
dlsz -= insz;
if (dlsz > 0)
{
- for (i = 0; i < dlsz - 1; i++)
- *p++ = '=';
+ memset (p, '=', dlsz-1);
+ p += dlsz - 1;
*p++ = '>';
}
- while (p - begin < progress_size)
- *p++ = ' ';
+ memset (p, ' ', (progress_size - (p - begin)));
+ p += (progress_size - (p - begin));
*p++ = ']';
}
else if (progress_size)
@@ -1071,27 +1067,14 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done)
*p++ = ']';
}
- ++bp->tick;
+ ++bp->tick;
/* " 234.56M" */
down_size = human_readable (size, 1000, 2);
- cols_diff = 7 - count_cols (down_size);
- while (cols_diff > 0)
- {
- *p++=' ';
- cols_diff--;
- }
- sprintf (p, " %s", down_size);
- move_to_end (p);
- /* Pad with spaces to 7 chars for the size_grouped field;
- * couldn't use the field width specifier in sprintf, because
- * it counts in bytes, not characters. */
- for (size_grouped_pad = PROGRESS_FILESIZE_LEN - 7;
- size_grouped_pad > 0;
- --size_grouped_pad)
- {
- *p++ = ' ';
- }
+ cols_diff = PROGRESS_FILESIZE_LEN - count_cols (down_size);
+ memset (p, ' ', cols_diff);
+ p += cols_diff;
+ p += sprintf (p, "%s", down_size);
/* " 12.52Kb/s or 12.52KB/s" */
if (hist->total_time > 0 && hist->total_bytes)
@@ -1104,9 +1087,8 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done)
wgint dlquant = hist->total_bytes + bp->recent_bytes;
double dltime = hist->total_time + (dl_total_time - bp->recent_start);
double dlspeed = calc_rate (dlquant, dltime, &units);
- sprintf (p, " %4.*f%s", dlspeed >= 99.95 ? 0 : dlspeed >= 9.995 ? 1 : 2,
+ p += sprintf (p, " %4.*f%s", dlspeed >= 99.95 ? 0 : dlspeed >= 9.995 ? 1 : 2,
dlspeed, !opt.report_bps ? short_units[units] : short_units_bits[units]);
- move_to_end (p);
}
else
APPEND_LITERAL (" --.-KB/s");
@@ -1144,14 +1126,13 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done)
bp->last_eta_time = dl_total_time;
}
- sprintf (p, get_eta(&bytes_cols_diff),
+ p += sprintf (p, get_eta(&bytes_cols_diff),
eta_to_human_short (eta, false));
- move_to_end (p);
}
else if (bp->total_length > 0)
{
skip_eta:
- APPEND_LITERAL (" ");
+ memset (p, ' ', PROGRESS_ETA_LEN);
}
}
else
@@ -1161,21 +1142,19 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done)
int ncols;
/* Note to translators: this should not take up more room than
- available here. Abbreviate if necessary. */
- strcpy (p, _(" in "));
+ available here (6 columns). Abbreviate if necessary. */
+ strcpy (p, _(" in "));
nbytes = strlen (p);
ncols = count_cols (p);
bytes_cols_diff = nbytes - ncols;
- p += nbytes;
if (dl_total_time >= 10)
- strcpy (p, eta_to_human_short ((int) (dl_total_time + 0.5), false));
+ ncols += sprintf (p + nbytes, "%s", eta_to_human_short ((int) (dl_total_time + 0.5), false));
else
- sprintf (p, "%ss", print_decimal (dl_total_time));
- move_to_end (p);
+ ncols += sprintf (p + nbytes, "%ss", print_decimal (dl_total_time));
+ p += ncols + bytes_cols_diff;
+ memset (p, ' ', PROGRESS_ETA_LEN - ncols);
}
- while (p - bp->buffer - bytes_cols_diff < bp->width)
- *p++ = ' ';
*p = '\0';
/* 2014-11-14 Darshit Shah <dar...@gmail.com>
--
2.6.3
From a309e017bfb7935ddeaa81b475186f3e6f39d7d6 Mon Sep 17 00:00:00 2001
From: Darshit Shah <dar...@gmail.com>
Date: Tue, 8 Dec 2015 14:45:01 +0100
Subject: [PATCH 2/2] Re-enable test on multibyte locale in Travis
* .travis.yml: Use Russian locale instead of Japanese since it seems to
be more complete
* contrib/travis-ci: Re-enable testing on a Russian locale
---
.travis.yml | 5 ++---
contrib/travis-ci | 9 ++-------
2 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/.travis.yml b/.travis.yml
index bb9017b..347f0b8 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -9,8 +9,7 @@ notifications:
email:
recipients:
- dar...@gmail.com
- - bug-wget@gnu.org
- on_success: change
+ on_success: always
on_failure: always
env:
@@ -42,7 +41,7 @@ addons:
- python3
- valgrind
- language-pack-tr
- - language-pack-ja
+ - language-pack-ru
script:
- make distclean || true
diff --git a/contrib/travis-ci b/contrib/travis-ci
index 6694094..90ce300 100755
--- a/contrib/travis-ci
+++ b/contrib/travis-ci
@@ -14,7 +14,7 @@ esac
# For some reason. /proc/cpuinfo reports 16 cores on Travis, while the docs
# claim that each instance has only 2 cores. We believe the docs and force a
# value of only 2 here.
-CORES=2
+CORES=3
# Define a large number of Warning flags for the compiler. Hopefully, someone
# will sit and analyze the output to clean the warnings from the codebase.
@@ -34,12 +34,7 @@ for options in "" "--disable-ipv6"; do
echo " ./configure $DISTCHECK_CONFIGURE_FLAGS CFLAGS=\"$CFLAGS\""
./configure $DISTCHECK_CONFIGURE_FLAGS CFLAGS="$CFLAGS"
- # We would like to have more languages tested here. Currently, there is a
- # problem in the translations file for Turkish which causes the progress bar
- # to assert fail. Apart from that, at least one another language with multi
- # byte and multi column characters.
- # TODO: Add Japanese and Turkish LANG for test
- for xLCALL in C; do
+ for xLCALL in "ru_RU.UTF-8" "C" "tr_TR.UTF-8"; do
export TESTS_ENVIRONMENT="LC_ALL=$xLCALL VALGRIND_TESTS=1"
echo " TESTS_ENVIRONMENT=\"$TESTS_ENVIRONMENT\" make distcheck CFLAGS=$CFLAGS -j$CORES"
make distcheck CFLAGS="$CFLAGS" -j$CORES
--
2.6.3
signature.asc
Description: PGP signature
