On 05/22/13 at 03:41am, Simon Gomizelj wrote:

Improve VerbosePkgList by caching attributes then and cell's length with the cell's label instead of recalculating.

Right align every cell that containing a file size, not just the last collumn.

Simplify printf statements and the alignment application.

Signed-off-by: Simon Gomizelj --- src/pacman/util.c | 135 ++++++++++++++++++++++++++++++------------------------ 1 file changed, 75 insertions(+), 60 deletions(-)

diff --git a/src/pacman/util.c b/src/pacman/util.c index 862c8e8..70a76b8 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -52,6 +52,18 @@ struct table_row_t { int size; };

+struct verbose_cell_t { + char *label; + size_t len; + int mode; +}; + +enum { + CELL_NORMAL = 0, + CELL_TITLE = 1, + CELL_RIGHT_ALIGN = 1 << 2 +}; + int trans_init(alpm_transflag_t flags, int check_valid) { int ret; @@ -424,6 +436,35 @@ static size_t string_length(const char *s) return len; }

+static void add_verbose_cell(alpm_list_t **row, char *label, int mode) +{ + struct verbose_cell_t *cell = malloc(sizeof(struct verbose_cell_t)); + + cell->label = label; + cell->mode = mode; + cell->len = string_length(label); + + *row = alpm_list_add(*row, cell); +} + +static void free_verbose_cell(void *ptr) +{ + struct verbose_cell_t *cell = ptr;

This should take a cell pointer, not void and check for NULL like our other *_free functions.

+ + free(cell->label); + free(cell); +}

None of this need be specific to the verbose package list, let's just call them table cells instead of verbose cells.

+ +static void add_transaction_sizes_row(alpm_list_t **table, const char *label, int size)

You need to rebase on master. This signature has changed.

+{ + struct table_row_t *row = malloc(sizeof(struct table_row_t)); + + row->label = label; + row->size = size; + + *table = alpm_list_add(*table, row); +} + void string_display(const char *title, const char *string, unsigned short cols) { if(title) { @@ -442,43 +483,30 @@ void string_display(const char *title, const char *string, unsigned short cols) static void table_print_line(const alpm_list_t *line, short col_padding, size_t colcount, size_t *widths, int *has_data) { - size_t i, lastcol = 0; + size_t i; int need_padding = 0; const alpm_list_t *curcell;

- for(i = colcount; i > 0; i--) { - if(has_data[i - 1]) { - lastcol = i - 1; - break; - } - } - for(i = 0, curcell = line; curcell && i < colcount; i++, curcell = alpm_list_next(curcell)) { - const char *value; + const struct verbose_cell_t *cell = curcell->data; + const char *str = (cell->label ? cell->label : ""); int cell_padding;

if(!has_data[i]) { continue; }

- value = curcell->data; - if(!value) { - value = ""; - } - /* silly printf requires padding size to be an int */ - cell_padding = (int)widths[i] - (int)string_length(value); - if(cell_padding < 0) { - cell_padding = 0; - } + cell_padding = (cell->mode & CELL_RIGHT_ALIGN ? (int)widths[i] : -(int)widths[i]); + if(need_padding) { printf("%*s", col_padding, ""); } - /* left-align all but the last column */ - if(i != lastcol) { - printf("%s%*s", value, cell_padding, ""); + + if(cell->mode & CELL_TITLE) { + printf("%s%*s%s", config->colstr.title, cell_padding, str, config->colstr.nocolor); } else { - printf("%*s%s", cell_padding, "", value); + printf("%*s", cell_padding, str); } need_padding = 1; } @@ -487,7 +515,6 @@ static void table_print_line(const alpm_list_t *line, short col_padding, }

- /** * Find the max string width of each column. Also determines whether values * exist in the column and sets the value in has_data accordingly. @@ -522,7 +549,8 @@ static size_t table_calc_widths(const alpm_list_t *header, } /* header determines column count and initial values of longest_strs */ for(i = header, curcol = 0; i; i = alpm_list_next(i), curcol++) { - colwidths[curcol] = string_length(i->data); + const struct verbose_cell_t *row = i->data; + colwidths[curcol] = row->len; /* note: header does not determine whether column has data */ }

@@ -531,8 +559,8 @@ static size_t table_calc_widths(const alpm_list_t *header, /* grab first column of each row and iterate through columns */ const alpm_list_t *j = i->data; for(curcol = 0; j; j = alpm_list_next(j), curcol++) { - const char *str = j->data; - size_t str_len = string_length(str); + const struct verbose_cell_t *row = j->data; + size_t str_len = row ? row->len : 0;

if(str_len > colwidths[curcol]) { colwidths[curcol] = str_len; @@ -597,11 +625,10 @@ static int table_display(const char *title, const alpm_list_t *header, }

if(title != NULL) { - printf("%s\n\n", title); + printf("%s%s%s\n\n", config->colstr.title, title, config->colstr.nocolor); }

table_print_line(header, padding, totalcols, widths, has_data); - printf("\n");

Why did you remove this newline? It helped visually offset the header from the data rows.

for(i = rows; i; i = alpm_list_next(i)) { table_print_line(i->data, padding, totalcols, widths, has_data); @@ -761,21 +788,15 @@ void signature_display(const char *title, alpm_siglist_t *siglist, /* creates a header row for use with table_display */ static alpm_list_t *create_verbose_header(void) { - alpm_list_t *res = NULL; - char *str; + alpm_list_t *ret = NULL;

- str = _("Name"); - res = alpm_list_add(res, str); - str = _("Old Version"); - res = alpm_list_add(res, str); - str = _("New Version"); - res = alpm_list_add(res, str); - str = _("Net Change"); - res = alpm_list_add(res, str); - str = _("Download Size"); - res = alpm_list_add(res, str); - - return res; + add_verbose_cell(&ret, _("Name"), CELL_TITLE); + add_verbose_cell(&ret, _("Old Version"), CELL_TITLE); + add_verbose_cell(&ret, _("New Version"), CELL_TITLE); + add_verbose_cell(&ret, _("Net Change"), CELL_TITLE); + add_verbose_cell(&ret, _("Download Size"), CELL_TITLE); + + return ret; }

/* returns package info as list of strings */ @@ -798,23 +819,23 @@ static alpm_list_t *create_verbose_row(pm_target_t *target) } else { pm_asprintf(&str, "%s", alpm_pkg_get_name(target->remove)); } - ret = alpm_list_add(ret, str); + add_verbose_cell(&ret, str, CELL_NORMAL);

/* old and new versions */ pm_asprintf(&str, "%s", target->remove != NULL ? alpm_pkg_get_version(target->remove) : ""); - ret = alpm_list_add(ret, str); + add_verbose_cell(&ret, str, CELL_NORMAL);

pm_asprintf(&str, "%s", target->install != NULL ? alpm_pkg_get_version(target->install) : ""); - ret = alpm_list_add(ret, str); + add_verbose_cell(&ret, str, CELL_NORMAL);

/* and size */ size -= target->remove ? alpm_pkg_get_isize(target->remove) : 0; size += target->install ? alpm_pkg_get_isize(target->install) : 0; human_size = humanize_size(size, 'M', 2, &label); pm_asprintf(&str, "%.2f %s", human_size, label); - ret = alpm_list_add(ret, str); + add_verbose_cell(&ret, str, CELL_RIGHT_ALIGN);

size = target->install ? alpm_pkg_download_size(target->install) : 0; if(size != 0) { @@ -823,21 +844,11 @@ static alpm_list_t *create_verbose_row(pm_target_t *target) } else { str = NULL; } - ret = alpm_list_add(ret, str); + add_verbose_cell(&ret, str, CELL_RIGHT_ALIGN);

return ret; }

-static void add_transaction_sizes_row(alpm_list_t **table, const char *label, int size) -{ - struct table_row_t *row = malloc(sizeof(struct table_row_t)); - - row->label = label; - row->size = size; - - *table = alpm_list_add(*table, row); -} - static void display_transaction_sizes(alpm_list_t *table) { alpm_list_t *i; @@ -894,7 +905,10 @@ static void _display_targets(alpm_list_t *targets, int verbose) for(i = targets; i; i = alpm_list_next(i)) { pm_target_t *target = i->data;

- rows = alpm_list_add(rows, create_verbose_row(target)); + if(verbose) { + rows = alpm_list_add(rows, create_verbose_row(target)); + } + if(target->install) { pm_asprintf(&str, "%s-%s", alpm_pkg_get_name(target->install), alpm_pkg_get_version(target->install)); @@ -927,8 +941,9 @@ static void _display_targets(alpm_list_t *targets, int verbose)

/* rows is a list of lists of strings, free inner lists here */ for(i = rows; i; i = alpm_list_next(i)) { - alpm_list_t *lp = i->data; - FREELIST(lp); + if(i->data) { + alpm_list_free_inner(i->data, free_verbose_cell);

Memory leak. You still need to free the i->data list. alpm_list_free_inner is NULL safe, so you can skip the NULL check.

+ } } alpm_list_free(rows); FREELIST(names); -- 1.8.2.3