On 02/25/2011 03:51 PM, Dan McGee wrote:
On Mon, Feb 21, 2011 at 1:02 PM, Jakob Gruber<[email protected]> wrote:char *size_to_human_string(off_t bytes); char *size_to_human_string_short(off_t bytes);Scale to the first unit with which the value will be less or equal to 2048. size_to_human_string_short uses short unit labels ("K","M",...) instead of the default ("KB", "MB"), uses only one decimal digit and has no space between the value and the label. For example, size_to_human_string: "1.31 MB", short: "1.3M". char *size_to_human_string_mb(off_t bytes); char *size_to_human_string_kb(off_t bytes); Convert to MB and KB respectively and format identically to size_to_human_string. Signed-off-by: Jakob Gruber<[email protected]> --- src/pacman/callback.c | 46 ++++++------------------ src/pacman/package.c | 20 ++++++----- src/pacman/query.c | 7 ++-- src/pacman/sync.c | 7 ++-- src/pacman/util.c | 94 +++++++++++++++++++++++++++++++++++++++++-------- src/pacman/util.h | 4 ++ 6 files changed, 111 insertions(+), 67 deletions(-) diff --git a/src/pacman/callback.c b/src/pacman/callback.c index f1e71bb..8ec4c3e 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -484,17 +484,16 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) { int infolen; int filenamelen; - char *fname, *p; + char *fname, *p, *ratestr, *xferedstr; /* used for wide character width determination and printing */ int len, wclen, wcwid, padwid; wchar_t *wcfname; int totaldownload = 0; off_t xfered, total; - double rate = 0.0, timediff = 0.0, f_xfered = 0.0; + double rate = 0.0, timediff = 0.0; unsigned int eta_h = 0, eta_m = 0, eta_s = 0; int file_percent = 0, total_percent = 0; - char rate_size = 'K', xfered_size = 'K'; if(config->noprogressbar || file_total == -1) { if(file_xfered == 0) { @@ -556,7 +555,7 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) diff_sec = current_time.tv_sec - initial_time.tv_sec; diff_usec = current_time.tv_usec - initial_time.tv_usec; timediff = diff_sec + (diff_usec / 1000000.0); - rate = xfered / (timediff * 1024.0); + rate = xfered / timediff; /* round elapsed time to the nearest second */ eta_s = (int)(timediff + 0.5); @@ -568,10 +567,10 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) /* return if the calling interval was too short */ return; } - rate = (xfered - xfered_last) / (timediff * 1024.0); + rate = (xfered - xfered_last) / timediff; /* average rate to reduce jumpiness */ rate = (rate + 2 * rate_last) / 3; - eta_s = (total - xfered) / (rate * 1024.0); + eta_s = (total - xfered) / (rate * 1024.0 * 1024.0); rate_last = rate; xfered_last = xfered; } @@ -625,38 +624,15 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) } - /* Awesome formatting for progress bar. We need a mess of Kb->Mb->Gb stuff - * here. We'll use limit of 2048 for each until we get some empirical */ - /* rate_size = 'K'; was set above */ - if(rate> 2048.0) { - rate /= 1024.0; - rate_size = 'M'; - if(rate> 2048.0) { - rate /= 1024.0; - rate_size = 'G'; - /* we should not go higher than this for a few years (9999.9 Gb/s?)*/ - } - } - - f_xfered = xfered / 1024.0; /* convert to K by default */ - /* xfered_size = 'K'; was set above */ - if(f_xfered> 2048.0) { - f_xfered /= 1024.0; - xfered_size = 'M'; - if(f_xfered> 2048.0) { - f_xfered /= 1024.0; - xfered_size = 'G'; - /* I should seriously hope that archlinux packages never break - * the 9999.9GB mark... we'd have more serious problems than the progress - * bar in pacman */ - } - } + ratestr = size_to_human_string_short((off_t)rate); + xferedstr = size_to_human_string_short(xfered); /* 1 space + filenamelen + 1 space + 7 for size + 1 + 7 for rate + 2 for /s + 1 space + 8 for eta */ - printf(" %ls%-*s %6.1f%c %#6.1f%c/s %02u:%02u:%02u", wcfname, - padwid, "", f_xfered, xfered_size, - rate, rate_size, eta_h, eta_m, eta_s); + printf(" %ls%-*s %7s %7s/s %02u:%02u:%02u", wcfname, + padwid, "", xferedstr, ratestr, eta_h, eta_m, eta_s); + free(ratestr); + free(xferedstr); free(fname); free(wcfname); diff --git a/src/pacman/package.c b/src/pacman/package.c index 77a5ee7..156d257 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -50,7 +50,7 @@ void dump_pkg_full(pmpkg_t *pkg, int level) { const char *reason; time_t bdate, idate; - char bdatestr[50] = "", idatestr[50] = ""; + char bdatestr[50] = "", idatestr[50] = "", *size;I'd prefer these defined on different lines; yes they are the "same" type but it can be confusing.
Understood, done.
I tried not locking the size summaries, and as a user I think I'd prefer keeping them as they are. Compare the following:const alpm_list_t *i; alpm_list_t *requiredby = NULL, *depstrings = NULL; @@ -105,17 +105,19 @@ void dump_pkg_full(pmpkg_t *pkg, int level) } list_display(_("Conflicts With :"), alpm_pkg_get_conflicts(pkg)); list_display(_("Replaces :"), alpm_pkg_get_replaces(pkg)); + + size = size_to_human_string_kb(alpm_pkg_get_size(pkg)); if(level< 0) { - printf(_("Download Size : %6.2f K\n"), - (double)alpm_pkg_get_size(pkg) / 1024.0); - } - if(level == 0) { - printf(_("Compressed Size: %6.2f K\n"), - (double)alpm_pkg_get_size(pkg) / 1024.0); + printf(_("Download Size : %s\n"), size); + } else if(level == 0) { + printf(_("Compressed Size: %s\n"), size); } + free(size); + + size = size_to_human_string_kb(alpm_pkg_get_isize(pkg)); + printf(_("Installed Size : %s\n"), size); + free(size); - printf(_("Installed Size : %6.2f K\n"), - (double)alpm_pkg_get_isize(pkg) / 1024.0); string_display(_("Packager :"), alpm_pkg_get_packager(pkg)); string_display(_("Architecture :"), alpm_pkg_get_arch(pkg)); string_display(_("Build Date :"), bdatestr); diff --git a/src/pacman/query.c b/src/pacman/query.c index 734875b..8a66927 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -247,10 +247,9 @@ static int query_search(alpm_list_t *targets) /* print the package size with the output if ShowSize option set */ if(!config->quiet&& config->showsize) { - /* Convert byte size to MB */ - double mbsize = (double)alpm_pkg_get_size(pkg) / (1024.0 * 1024.0); - - printf(" [%.2f MB]", mbsize); + char *size = size_to_human_string_mb(alpm_pkg_get_size(pkg)); + printf(" [%s]", size); + free(size); } diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 7af1667..21fe4ba 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -349,10 +349,9 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets) /* print the package size with the output if ShowSize option set */ if(!config->quiet&& config->showsize) { - /* Convert byte size to MB */ - double mbsize = (double)alpm_pkg_get_size(pkg) / (1024.0 * 1024.0); - - printf(" [%.2f MB]", mbsize); + char *size = size_to_human_string_mb(alpm_pkg_get_size(pkg)); + printf(" [%s]", size); + free(size); } if (!config->quiet) { diff --git a/src/pacman/util.c b/src/pacman/util.c index c08ebb1..b651478 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -501,10 +501,9 @@ void list_display_linebreak(const char *title, const alpm_list_t *list) /* prepare a list of pkgs to display */ void display_targets(const alpm_list_t *pkgs, int install) { - char *str; + char *str, *size; const alpm_list_t *i; off_t isize = 0, dlsize = 0; - double mbisize = 0.0, mbdlsize = 0.0; alpm_list_t *targets = NULL; if(!pkgs) { @@ -522,10 +521,10 @@ void display_targets(const alpm_list_t *pkgs, int install) /* print the package size with the output if ShowSize option set */ if(config->showsize) { - double mbsize = (double)alpm_pkg_get_size(pkg) / (1024.0 * 1024.0); - - pm_asprintf(&str, "%s-%s [%.2f MB]", alpm_pkg_get_name(pkg), - alpm_pkg_get_version(pkg), mbsize); + size = size_to_human_string_mb(alpm_pkg_get_size(pkg)); + pm_asprintf(&str, "%s-%s [%s]", alpm_pkg_get_name(pkg), + alpm_pkg_get_version(pkg), size); + free(size); } else { pm_asprintf(&str, "%s-%s", alpm_pkg_get_name(pkg), alpm_pkg_get_version(pkg)); @@ -533,19 +532,19 @@ void display_targets(const alpm_list_t *pkgs, int install) targets = alpm_list_add(targets, str); } - /* Convert byte sizes to MB */ - mbdlsize = (double)dlsize / (1024.0 * 1024.0); - mbisize = (double)isize / (1024.0 * 1024.0); - if(install) { pm_asprintf(&str, _("Targets (%d):"), alpm_list_count(targets)); list_display(str, targets); free(str); printf("\n"); - printf(_("Total Download Size: %.2f MB\n"), mbdlsize); + size = size_to_human_string_mb(dlsize); + printf(_("Total Download Size: %s\n"), size); + free(size); if(!(config->flags& PM_TRANS_FLAG_DOWNLOADONLY)) { - printf(_("Total Installed Size: %.2f MB\n"), mbisize); + size = size_to_human_string_mb(isize); + printf(_("Total Installed Size: %s\n"), size); + free(size); } } else { pm_asprintf(&str, _("Remove (%d):"), alpm_list_count(targets)); @@ -553,7 +552,9 @@ void display_targets(const alpm_list_t *pkgs, int install) free(str); printf("\n"); - printf(_("Total Removed Size: %.2f MB\n"), mbisize); + size = size_to_human_string_mb(isize); + printf(_("Total Removed Size: %s\n"), size); + free(size);I see no reason we should lock any of these to MB units- just use the general formatter here.
Total Download Size: 884.34 KB Total Installed Size: 4.80 MB vs Total Download Size: 0.86 MB Total Installed Size: 4.80 MB In the latter, it seems clearer that "Installed" is larger than "Download".Here's what it looks like with summaries locked to MB and ShowSize using the general formatter:
Remove (1): libjpeg-8.3.0-1 [848.00 KB]
Total Removed Size: 0.83 MB
Targets (5): libjpeg-turbo-1.1.0-1 [209.01 KB]
linux-firmware-20110227-1 [8.23 MB] ncurses-5.8-1 [945.95 KB]
ppl-0.11.2-1 [2.74 MB] v4l-utils-0.8.3-1 [232.80 KB]
Total Download Size: 12.32 MB
Total Installed Size: 58.82 MB
What do you think?
} FREELIST(targets); @@ -594,6 +595,70 @@ static char *pkg_get_location(pmpkg_t *pkg) } } +/** Converts sizes in bytes into strings. + * + * @param bytes the size in bytes + * @param format the format string to use. The first argument is of type + * float and represents the size. The second argument is of type + * char* and represents the unit label. for example, "%.2f %s" + * could result in "1.23 MB" + * @param unit the target unit. can be either one of the short unit + * labels ("B", "K", ...); or NULL, in which case the first + * unit which will bring the value to below a threshold of 2048 + * will be chosen. + * @param long_units whether to use short ("K") or long ("KB") unit labels + * + * @return the size string + */ +static char *size_to_human_string_format(off_t bytes, const char *format, + const char *unit, int long_labels) +{ + char *ret; + static const char *shortlabels[] = {"B", "K", "M", "G", "T", "P"}; + static const char *longlabels[] = {"B", "KB", "MB", "GB", "TB", "PB"}; + const char **labels = long_labels ? longlabels : shortlabels; + const int unitcount = sizeof(shortlabels) / sizeof(shortlabels[0]); + int index; + float val = (float)bytes; + + /* stop conditions: + * 1) a target unit is specified and we are at target unit, + * 2) target unit is NOT specified and value is leq 2048, + * and 3) we are already at the largest unit.s + * note that if an invalid target is specified in 1), + * we convert to the largest unit */ + for(index = 0; index< unitcount - 1; index++) { + if(unit != NULL&& strcmp(shortlabels[index], unit) == 0) break;I'd make the function param a char (not a "string") to ensure people aren't doing stupid things like passing a long label in by mistake. Then you can just use == here.
Done.
+ else if(unit == NULL&& val<= 2048.0) break; + + val /= 1024.0; + } + + pm_asprintf(&ret, format, val, labels[index]); + + return ret; +} + +char *size_to_human_string(off_t bytes) +{ + return size_to_human_string_format(bytes, "%.2f %s", NULL, 1); +} + +char *size_to_human_string_mb(off_t bytes) +{ + return size_to_human_string_format(bytes, "%.2f %s", "M", 1); +} + +char *size_to_human_string_kb(off_t bytes) +{ + return size_to_human_string_format(bytes, "%.2f %s", "K", 1); +} + +char *size_to_human_string_short(off_t bytes) +{ + return size_to_human_string_format(bytes, "%.1f%s", NULL, 0); +} +I think this API has been over-engineered with all these one-liners. We aren't exposing this as a library function or anything, so people can deal with a little complexity in the call themselves. I'd have one function for all of this, and we can then even skip the malloc/free cycle: double value humanize_size(off_t bytes, char max_unit, int long_labels, const char **label) Logic stays as is in your format function, expect we don't actually print the string- the value is returned and label, if provided, gets a pointer to the correct string. Does this make sense? I'm open for feedback of course, but I see this as 1) less boilerplate methods in util.c 2) no free() necessary by caller.
Sure, makes sense. I changed the function as you described, but renamed max_unit to target_unit (because that's what it actually is).
void print_packages(const alpm_list_t *packages) { const alpm_list_t *i; @@ -638,8 +703,7 @@ void print_packages(const alpm_list_t *packages) /* %s : size */ if(strstr(temp,"%s")) { char *size; - double mbsize = (double)pkg_get_size(pkg) / (1024.0 * 1024.0); - pm_asprintf(&size, "%.2f", mbsize); + size = size_to_human_string_format(pkg_get_size(pkg), "%.2f", "M", 0); string = strreplace(temp, "%s", size); free(size); free(temp); diff --git a/src/pacman/util.h b/src/pacman/util.h index 234a631..ed03c13 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -51,6 +51,10 @@ char *strtoupper(char *str); char *strtrim(char *str); char *strreplace(const char *str, const char *needle, const char *replace); alpm_list_t *strsplit(const char *str, const char splitchar); +char *size_to_human_string(off_t bytes); +char *size_to_human_string_mb(off_t bytes); +char *size_to_human_string_kb(off_t bytes); +char *size_to_human_string_short(off_t bytes); void string_display(const char *title, const char *string); void list_display(const char *title, const alpm_list_t *list); void list_display_linebreak(const char *title, const alpm_list_t *list); -- 1.7.4.1
