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 <[email protected]>
> ---
> 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
