[pacman-dev] [PATCH 5/6] Table formatted output functions

[Dan McGee]

On Mon, Feb 21, 2011 at 1:02 PM, Jakob Gruber <jakob.gruber at gmail.com> wrote:
> table_display takes a list of lists of strings (representing the table
> cells) and displays them formatted as a table.
> The exact format depends on the longest string in each column.
>
> Signed-off-by: Jakob Gruber <jakob.gruber at gmail.com>
> ---
>  src/pacman/util.c |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/pacman/util.h |    1 +
>  2 files changed, 109 insertions(+), 0 deletions(-)

Noticing it now, but please read the coding style document. We always
use {}, even on one line conditionals. We also format all return
statements as return(value). Yes, this one is suspect, but consistency
is more important than anything.

>
> diff --git a/src/pacman/util.c b/src/pacman/util.c
> index 85d2d8c..8f7b5e5 100644
> --- a/src/pacman/util.c
> +++ b/src/pacman/util.c
> @@ -435,6 +435,114 @@ void string_display(const char *title, const char *string)
>        printf("\n");
>  }
>
> +static void table_print_line(const alpm_list_t *line,
> +                                                                                                                       const alpm_list_t *formats)
> +{
> +       const alpm_list_t *curformat = formats;
> +       const alpm_list_t *curcell = line;
> +
> +       while (curcell && curformat) {
> +               printf(alpm_list_getdata(curformat), alpm_list_getdata(curcell));
> +               curcell = alpm_list_next(curcell);
> +               curformat = alpm_list_next(curformat);
> +       }
> +
> +       printf("\n");
> +}
> +
> +/* creates a format string by checking max cell lengths in rows */
> +static alpm_list_t *table_create_format(const alpm_list_t *rows)
> +{
> +       alpm_list_t *format, *curformat;
> +       const alpm_list_t *currow, *curcell;
> +       char *str, *formatstr;
> +       const int padding = 2;
> +       int cols, neededcols = 0;
> +
> +       /* initial format list element */
> +       format = alpm_list_add(NULL, NULL);
What's going on here? We always have a NULL first item?

> +
> +       /* go through each cell and store the longest string for each
> +        * column in the format list */
> +       currow = rows;
> +       while(currow != NULL) {
You've wrote a for() loop without the for here and the last line of the block.
> +
> +               /* starting on new row, reset format pointer to first column */
> +               curformat = format;
> +
> +               /* loop through all row cells and update format pointer if necessary */
Which isn't really a format pointer at all, but a pointer to actual data?

> +               curcell = alpm_list_getdata(currow);
> +               while(curcell != NULL) {
> +                       if(curformat == NULL) {
> +                               curformat = alpm_list_last(alpm_list_add(format, NULL));
This makes assumptions. I doubt we'll ever break it, but alpm_list_add
always needs to be reassigned back to format, in this case, before
doing anything else on it.

> +                       }
> +
> +                       str = alpm_list_getdata(curcell);
> +                       if(curformat->data == NULL ||
> +                                       strlen(str) > strlen(alpm_list_getdata(curformat)))
> +                               curformat->data = str;
> +
> +                       curformat = alpm_list_next(curformat);
> +                       curcell = alpm_list_next(curcell);
> +               }
> +
> +               currow = alpm_list_next(currow);
> +       }
> +
> +       /* now use the column width info to generate format strings */
> +       curformat = format;
> +       while(curformat != NULL) {
> +               cols = strlen(curformat->data) + padding;
> +               neededcols += cols;
> +
> +               str = (alpm_list_next(curformat) != NULL) ? "%%-%ds" : "%%%ds";
Magic here- everything else seems generic, but why is this last column special?

> +               pm_asprintf(&formatstr, str, cols);
> +
> +               curformat->data = formatstr;
> +               curformat = alpm_list_next(curformat);
> +       }
I had a real hard time following all this, but that might just be the
morning brain not working well. Your reuse of curformat->data as first
a immutable string holder and later a alloced format string holder is
particularly hard to wrap the brain around until you see it. I'd
highly recommend not mixing the two for clarity.

input: a list of rows, each with a list of cols (cells)?
output: a list of formats for each col?

> +
> +       /* return NULL if terminal is not wide enough */
> +       if(neededcols > getcols()) {
> +               FREELIST(format);
> +               return NULL;
> +       }
Hmm. Is this likely to happen? Seems odd to then not have anything
printed at all. I think we should truncate strings as necessary, just
like we do in the callback functions.

> +
> +       return format;
> +}
> +
> +/** Displays the list in table format
> + *
> + * @param title the tables title
> + * @param rows  the rows to display as a list of lists of strings. the outer
> + *                                                     list represents the rows, the inner list the cells (= columns)
> + *
> + * @return -1 if not enough terminal cols available, else 0
> + */
> +int table_display(const char *title, const alpm_list_t *rows)
> +{
> +       alpm_list_t *i, *formats;
> +
> +       if(rows == NULL)
> +               return 0;
> +
> +       formats = table_create_format(rows);
> +       if(formats == NULL) {
> +               return -1;
> +       }
> +
> +       if(title != NULL) {
> +               printf("%s\n\n", title);
> +       }
> +
> +       for(i = alpm_list_first(rows); i; i = alpm_list_next(i)) {
> +               table_print_line(alpm_list_getdata(i), formats);
> +       }
> +
> +       FREELIST(formats);
> +       return 0;
> +}
> +
>  void list_display(const char *title, const alpm_list_t *list)
>  {
>        const alpm_list_t *i;
> diff --git a/src/pacman/util.h b/src/pacman/util.h
> index ed03c13..494becf 100644
> --- a/src/pacman/util.h
> +++ b/src/pacman/util.h
> @@ -56,6 +56,7 @@ 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);
> +int table_display(const char *title, const alpm_list_t *rows);
>  void list_display(const char *title, const alpm_list_t *list);
>  void list_display_linebreak(const char *title, const alpm_list_t *list);
>  void display_targets(const alpm_list_t *pkgs, int install);
> --
> 1.7.4.1
>
>
>