On Mon, Feb 21, 2011 at 1:02 PM, Jakob Gruber <jakob.gruber@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@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.
+ + /* 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
input: a list of rows, each with a list of cols (cells)? output: a list of formats for each col? 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