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.
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.
> +
> + * * * /* 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.
> + * * * * * * * 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
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.
I made a few changes that should hopefully make this part clearer.
table_display now also takes a list of column headers,
and table_create_format uses them to determine the column count before
looking for the longest strings and creating the formats.
This gets rid of adding the NULL in display_targets (NULL was equal to
an empty line after the header) and the NULL in table_create_format
(which was an initial value which meant "assign the first string we get
to because there aren't any initial values to run strlen() on yet").
input: a list of rows, each with a list of cols (cells)?
output: a list of formats for each col?
Yeah, exactly.
+
+ /* 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.
It shouldn't happen (often) for regular usage. On an 80 column terminal,
the current upgrade targets (which all have average length names and
versions) takes up about 2/3 of the width. I added an error message at
this spot, but I'm not sure about truncating strings? Currently, the
error is passed up through table_display, display_targets will unset
config->verbosepkglists and run itself again with the default list
display as fallback. Is that behavior acceptable?
+
+ 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
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.
Table formatted output functions
