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