cli: enhance printing to align tabular output properly and not to waste space

Until now we have used a static width defined for each column for tabular
output. Even if this worked in most cases, it was not optimal, because by
using too wide columns we wasted space, and in case of a too narrow column the
alignment broke. So, we need to know the longest string in a column to be able
to align columns in the tabular output. Thus, the printing has to be postponed
till we have all data available, and can find the widest column. This value is
then used for aligning while printing the data.

Arrays of NmcOutputField (rows) are inserted into output_data array. When all
data have been added, print_data() can be used to print the whole output_data
array with proper alignment.

A single row can be printed using print_required_fields().

Also, output flags are redone to better match the new output_data array.
The flags are needed for every row (in tabular output); they are stored in
the first field (NmcOutputField) for the whole row.

Addapted set_val_str() and set_val_arr() to set value type (char * x char **).
Added set_val_strc(), set_val_arrc() for const values that should not be freed.

output_data takes ownership of the data added to it and takes care of freeing
the memory.

See e.g.
https://bugzilla.gnome.org/show_bug.cgi?id=699503

4 files changed, 184 insertions, 62 deletions

diff --git a/cli/src/nmcli.c b/cli/src/nmcli.c
index 7b184d057a..5d203dc827 100644
--- a/cli/src/nmcli.c
+++ b/cli/src/nmcli.c
@@ -323,7 +323,7 @@ nmc_init (NmCli *nmc)
 	nmc->mode_specified = FALSE;
 	nmc->escape_values = TRUE;
 	nmc->required_fields = NULL;
-	nmc->allowed_fields = NULL;
+	nmc->output_data = g_ptr_array_new_full (20, g_free);
 	memset (&nmc->print_fields, '\0', sizeof (NmcPrintFields));
 	nmc->nocheck_ver = FALSE;
 	nmc->ask = FALSE;
@@ -340,6 +340,8 @@ nmc_cleanup (NmCli *nmc)
 	g_slist_free (nmc->system_connections);
 
 	g_free (nmc->required_fields);
+	nmc_empty_output_fields (nmc);
+	g_ptr_array_unref (nmc->output_data);
 	if (nmc->print_fields.indices)
 		g_array_free (nmc->print_fields.indices, TRUE);
 }
diff --git a/cli/src/nmcli.h b/cli/src/nmcli.h
index 52adf95d3b..1f836b68ba 100644
--- a/cli/src/nmcli.h
+++ b/cli/src/nmcli.h
@@ -66,31 +66,25 @@ typedef enum {
 
 /* === Output fields === */
 /* Flags for NmcOutputField */
-#define NMC_OF_FLAG_ARRAY              0x00000001   /* 'value' is an NULL-terminated array rather then a single string */
+#define NMC_OF_FLAG_FIELD_NAMES        0x00000001   /* Print field names instead of values */
+#define NMC_OF_FLAG_SECTION_PREFIX     0x00000002   /* Use the first value as section prefix for the other field names - just in multiline */
+#define NMC_OF_FLAG_MAIN_HEADER_ADD    0x00000004   /* Print main header in addition to values/field names */
+#define NMC_OF_FLAG_MAIN_HEADER_ONLY   0x00000008   /* Print main header only */
 
 typedef struct {
-	const char *name;       /* Field's name */
-	const char *name_l10n;  /* Field's name for translation */
-	int width;              /* Width in screen columns */
-	void *value;            /* Value of current field - char* or char** */
-	guint32 flags;          /* Flags */
+	const char *name;         /* Field's name */
+	const char *name_l10n;    /* Field's name for translation */
+	int width;                /* Width in screen columns */
+	void *value;              /* Value of current field - char* or char** (NULL-terminated array) */
+	gboolean value_is_array;  /* Whether value is char ** instead of char* */
+	gboolean free_value;      /* Whether to free the value */
+	guint32 flags;            /* Flags - whether and how to print values/field names/headers */
 } NmcOutputField;
 
-/* Flags for NmcPrintFields */
-#define NMC_PF_FLAG_MULTILINE          0x00000001   /* Multiline output instead of tabular */
-#define NMC_PF_FLAG_TERSE              0x00000002   /* Terse output mode */
-#define NMC_PF_FLAG_PRETTY             0x00000004   /* Pretty output mode */
-#define NMC_PF_FLAG_MAIN_HEADER_ADD    0x00000008   /* Print main header in addition to values/field names */
-#define NMC_PF_FLAG_MAIN_HEADER_ONLY   0x00000010   /* Print main header only */
-#define NMC_PF_FLAG_FIELD_NAMES        0x00000020   /* Print field names instead of values */
-#define NMC_PF_FLAG_ESCAPE             0x00000040   /* Escape column separator and '\' */
-#define NMC_PF_FLAG_SECTION_PREFIX     0x00000080   /* Use the first value as section prefix for the other field names - just in multiline */
-
 typedef struct {
 	GArray *indices;      /* Array of field indices to the array of allowed fields */
 	char *header_name;    /* Name of the output */
 	int indent;           /* Indent by this number of spaces */
-	guint32 flags;        /* Various flags for controlling output: see NMC_PF_FLAG_* values */
 } NmcPrintFields;
 
 /* NmCli - main structure */
@@ -114,7 +108,7 @@ typedef struct _NmCli {
 	gboolean mode_specified;                          /* Whether tabular/multiline mode was specified via '--mode' option */
 	gboolean escape_values;                           /* Whether to escape ':' and '\' in terse tabular mode */
 	char *required_fields;                            /* Required fields in output: '--fields' option */
-	NmcOutputField *allowed_fields;                   /* Array of allowed fields for particular commands */
+	GPtrArray *output_data;                           /* GPtrArray of arrays of NmcOutputField structs - accumulates data for output */
 	NmcPrintFields print_fields;                      /* Structure with field indices to print */
 	gboolean nocheck_ver;                             /* Don't check nmcli and NM versions: option '--nocheck' */
 	gboolean ask;                                     /* Ask for missing parameters: option '--ask' */
diff --git a/cli/src/utils.c b/cli/src/utils.c
index 102b5ecce0..f311f43486 100644
--- a/cli/src/utils.c
+++ b/cli/src/utils.c
@@ -462,15 +462,33 @@ nmc_string_screen_width (const char *start, const char *end)
 void
 set_val_str (NmcOutputField fields_array[], guint32 idx, char *value)
 {
-	fields_array[idx].flags = 0;
 	fields_array[idx].value = value;
+	fields_array[idx].value_is_array = FALSE;
+	fields_array[idx].free_value = TRUE;
+}
+
+void
+set_val_strc (NmcOutputField fields_array[], guint32 idx, const char *value)
+{
+	fields_array[idx].value = (char *) value;
+	fields_array[idx].value_is_array = FALSE;
+	fields_array[idx].free_value = FALSE;
 }
 
 void
 set_val_arr (NmcOutputField fields_array[], guint32 idx, char **value)
 {
-	fields_array[idx].flags = NMC_OF_FLAG_ARRAY;
 	fields_array[idx].value = value;
+	fields_array[idx].value_is_array = TRUE;
+	fields_array[idx].free_value = TRUE;
+}
+
+void
+set_val_arrc (NmcOutputField fields_array[], guint32 idx, const char **value)
+{
+	fields_array[idx].value = (char **) value;
+	fields_array[idx].value_is_array = TRUE;
+	fields_array[idx].free_value = FALSE;
 }
 
 /*
@@ -479,13 +497,17 @@ set_val_arr (NmcOutputField fields_array[], guint32 idx, char **value)
 void
 nmc_free_output_field_values (NmcOutputField fields_array[])
 {
-	int idx = 0;
-	while (fields_array && fields_array[idx].value) {
-		if (fields_array[idx].flags & NMC_OF_FLAG_ARRAY)
-			g_strfreev (fields_array[idx].value);
-		else
-			g_free (fields_array[idx].value);
-		idx++;
+	NmcOutputField *iter = fields_array;
+
+	while (iter && iter->name) {
+		if (iter->free_value) {
+			if (iter->value_is_array)
+				g_strfreev ((char **) iter->value);
+			else
+				g_free ((char *) iter->value);
+			iter->value = NULL;
+		}
+		iter++;
 	}
 }
 
@@ -551,14 +573,63 @@ nmc_terse_option_check (NMCPrintOutput print_output, const char *fields, GError
 	return TRUE;
 }
 
+NmcOutputField *
+nmc_dup_fields_array (NmcOutputField fields[], size_t size, guint32 flags)
+{
+	NmcOutputField *row;
+
+	row = g_malloc0 (size);
+	memcpy (row, fields, size);
+	row[0].flags = flags;
+
+	return row;
+}
+
+void
+nmc_empty_output_fields (NmCli *nmc)
+{
+	guint i;
+
+	/* Free values in field structure */
+	for (i = 0; i < nmc->output_data->len; i++) {
+		NmcOutputField *fld_arr = g_ptr_array_index (nmc->output_data, i);
+		nmc_free_output_field_values (fld_arr);
+	}
+
+	/* Empty output_data array */
+	if (nmc->output_data->len > 0)
+		g_ptr_array_remove_range (nmc->output_data, 0, nmc->output_data->len);
+}
+
+static char *
+get_value_to_print (NmcOutputField *fields,
+                    gboolean field_name,
+                    const char *not_set_str,
+                    gboolean *dealloc)
+{
+	gboolean is_array = fields->value_is_array;
+	char *value;
+
+	if (field_name)
+		value = _(fields->name_l10n);
+	else
+		value = fields->value ?
+		          (is_array ? g_strjoinv (" | ", (char **) fields->value) :
+		                      (char *) fields->value) :
+		          (char *) not_set_str;
+	*dealloc = fields->value && is_array && !field_name;
+	return value;
+}
+
 /*
  * Print both headers or values of 'field_values' array.
- * Entries to print and their order are specified via indices
- * in 'fields.indices' array.
- * 'fields.flags' specify various aspects influencing the output.
+ * Entries to print and their order are specified via indices in
+ * 'nmc->print_fields.indices' array.
+ * Various flags influencing the output of fields are set up in the first item
+ * of 'field_values' array.
  */
 void
-print_fields (const NmcPrintFields fields, const NmcOutputField field_values[])
+print_required_fields (NmCli *nmc, const NmcOutputField field_values[])
 {
 	GString *str;
 	int width1, width2;
@@ -567,14 +638,15 @@ print_fields (const NmcPrintFields fields, const NmcOutputField field_values[])
 	char *indent_str;
 	const char *not_set_str = "--";
 	int i;
-	gboolean multiline = fields.flags & NMC_PF_FLAG_MULTILINE;
-	gboolean terse = fields.flags & NMC_PF_FLAG_TERSE;
-	gboolean pretty = fields.flags & NMC_PF_FLAG_PRETTY;
-	gboolean main_header_add = fields.flags & NMC_PF_FLAG_MAIN_HEADER_ADD;
-	gboolean main_header_only = fields.flags & NMC_PF_FLAG_MAIN_HEADER_ONLY;
-	gboolean field_names = fields.flags & NMC_PF_FLAG_FIELD_NAMES;
-	gboolean escape = fields.flags & NMC_PF_FLAG_ESCAPE;
-	gboolean section_prefix = fields.flags & NMC_PF_FLAG_SECTION_PREFIX;
+	const NmcPrintFields fields = nmc->print_fields;
+	gboolean multiline = nmc->multiline_output;
+	gboolean terse = (nmc->print_output == NMC_PRINT_TERSE);
+	gboolean pretty = (nmc->print_output == NMC_PRINT_PRETTY);
+	gboolean escape = nmc->escape_values;
+	gboolean main_header_add = field_values[0].flags & NMC_OF_FLAG_MAIN_HEADER_ADD;
+	gboolean main_header_only = field_values[0].flags & NMC_OF_FLAG_MAIN_HEADER_ONLY;
+	gboolean field_names = field_values[0].flags & NMC_OF_FLAG_FIELD_NAMES;
+	gboolean section_prefix = field_values[0].flags & NMC_OF_FLAG_SECTION_PREFIX;
 	gboolean main_header = main_header_add || main_header_only;
 
 	/* No headers are printed in terse mode:
@@ -606,24 +678,25 @@ print_fields (const NmcPrintFields fields, const NmcOutputField field_values[])
 			for (i = 0; i < fields.indices->len; i++) {
 				char *tmp;
 				int idx = g_array_index (fields.indices, int, i);
-				guint32 value_is_array = field_values[idx].flags & NMC_OF_FLAG_ARRAY;
+				gboolean is_array = field_values[idx].value_is_array;
 
 				/* section prefix can't be an array */
-				g_assert (!value_is_array || !section_prefix || idx != 0);
+				g_assert (!is_array || !section_prefix || idx != 0);
 
 				if (section_prefix && idx == 0)  /* The first field is section prefix */
 					continue;
 
-				if (value_is_array) {
+				if (is_array) {
 					/* value is a null-terminated string array */
 					const char **p;
 					int j;
 
 					for (p = (const char **) field_values[idx].value, j = 1; p && *p; p++, j++) {
-						tmp = g_strdup_printf ("%s%s%s[%d]:", section_prefix ? (const char*) field_values[0].value : "",
-						                                      section_prefix ? "." : "",
-						                                      _(field_values[idx].name_l10n),
-						                                      j);
+						tmp = g_strdup_printf ("%s%s%s[%d]:",
+						                       section_prefix ? (const char*) field_values[0].value : "",
+						                       section_prefix ? "." : "",
+						                       _(field_values[idx].name_l10n),
+						                       j);
 						printf ("%-*s%s\n", terse ? 0 : ML_VALUE_INDENT, tmp, *p ? *p : not_set_str);
 						g_free (tmp);
 					}
@@ -632,9 +705,10 @@ print_fields (const NmcPrintFields fields, const NmcOutputField field_values[])
 					const char *hdr_name = (const char*) field_values[0].value;
 					const char *val = (const char*) field_values[idx].value;
 
-					tmp = g_strdup_printf ("%s%s%s:", section_prefix ? hdr_name : "",
-					                                  section_prefix ? "." : "",
-					                                  _(field_values[idx].name_l10n));
+					tmp = g_strdup_printf ("%s%s%s:",
+					                       section_prefix ? hdr_name : "",
+					                       section_prefix ? "." : "",
+					                       _(field_values[idx].name_l10n));
 					printf ("%-*s%s\n", terse ? 0 : ML_VALUE_INDENT, tmp, val ? val : not_set_str);
 					g_free (tmp);
 				}
@@ -653,14 +727,8 @@ print_fields (const NmcPrintFields fields, const NmcOutputField field_values[])
 
 	for (i = 0; i < fields.indices->len; i++) {
 		int idx = g_array_index (fields.indices, int, i);
-		guint32 value_is_array = field_values[idx].flags & NMC_OF_FLAG_ARRAY;
-		char *value;
-		if (field_names)
-			value = _(field_values[idx].name_l10n);
-		else
-			value = field_values[idx].value ?
-			        (value_is_array ? g_strjoinv (" | ", (char **) field_values[idx].value) : (char *) field_values[idx].value) :
-			        (char *) not_set_str;
+		gboolean dealloc;
+		char *value = get_value_to_print ((NmcOutputField *) field_values+idx, field_names, not_set_str, &dealloc);
 
 		if (terse) {
 			if (escape) {
@@ -683,7 +751,7 @@ print_fields (const NmcPrintFields fields, const NmcOutputField field_values[])
 			table_width += field_values[idx].width + width1 - width2 + 1;
 		}
 
-		if (value_is_array && field_values[idx].value && !field_values)
+		if (dealloc)
 			g_free (value);
 	}
 
@@ -725,6 +793,59 @@ print_fields (const NmcPrintFields fields, const NmcOutputField field_values[])
 }
 
 /*
+ * Print nmc->output_data
+ *
+ * It first finds out maximal string length in columns and fill the value to
+ * 'width' member of NmcOutputField, so that columns in tabular output are
+ * properly aligned. Then each object (row in tabular) is printed using
+ * print_required_fields() function.
+ */
+void
+print_data (NmCli *nmc)
+{
+	int i, j;
+	size_t len;
+	NmcOutputField *row;
+	int num_fields = 0;
+
+	if (!nmc->output_data || nmc->output_data->len < 1)
+		return;
+
+	/* How many fields? */
+	row = g_ptr_array_index (nmc->output_data, 0);
+	while (row->name) {
+		num_fields++;
+		row++;
+	}
+
+	/* Find out maximal string lengths */
+	for (i = 0; i < num_fields; i++) {
+		size_t max_width = 0;
+		for (j = 0; j < nmc->output_data->len; j++) {
+			gboolean field_names, dealloc;
+			char *value;
+			row = g_ptr_array_index (nmc->output_data, j);
+			field_names = row[0].flags & NMC_OF_FLAG_FIELD_NAMES;
+			value = get_value_to_print (row+i, field_names, "--", &dealloc);
+			len = nmc_string_screen_width (value, NULL);
+			max_width = len > max_width ? len : max_width;
+			if (dealloc)
+				g_free (value);
+		}
+		for (j = 0; j < nmc->output_data->len; j++) {
+			row = g_ptr_array_index (nmc->output_data, j);
+			row[i].width = max_width + 1;
+		}
+	}
+
+	/* Now we can print the data. */
+	for (i = 0; i < nmc->output_data->len; i++) {
+		row = g_ptr_array_index (nmc->output_data, i);
+		print_required_fields (nmc, row);
+	}
+}
+
+/*
 * Compare versions of nmcli and NM daemon.
 * Return: TRUE  - the versions match (when only major and minor match, print a warning)
 *         FALSE - versions mismatch
diff --git a/cli/src/utils.h b/cli/src/utils.h
index cc1b126aad..b4d1c1f748 100644
--- a/cli/src/utils.h
+++ b/cli/src/utils.h
@@ -71,12 +71,17 @@ char *nmc_get_user_input (const char *ask_str);
 int nmc_string_to_arg_array (const char *line, const char *delim, char ***argv, int *argc);
 const char *nmc_string_is_valid (const char *input, const char **allowed, GError **error);
 int nmc_string_screen_width (const char *start, const char *end);
-void set_val_str (NmcOutputField fields_array[], guint32 index, char *value);
-void set_val_arr (NmcOutputField fields_array[], guint32 index, char **value);
+void set_val_str  (NmcOutputField fields_array[], guint32 index, char *value);
+void set_val_strc (NmcOutputField fields_array[], guint32 index, const char *value);
+void set_val_arr  (NmcOutputField fields_array[], guint32 index, char **value);
+void set_val_arrc (NmcOutputField fields_array[], guint32 index, const char **value);
 void nmc_free_output_field_values (NmcOutputField fields_array[]);
 GArray *parse_output_fields (const char *fields_str, const NmcOutputField fields_array[], GError **error);
 gboolean nmc_terse_option_check (NMCPrintOutput print_output, const char *fields, GError **error);
-void print_fields (const NmcPrintFields fields, const NmcOutputField field_values[]);
+NmcOutputField *nmc_dup_fields_array (NmcOutputField fields[], size_t size, guint32 flags);
+void nmc_empty_output_fields (NmCli *nmc);
+void print_required_fields (NmCli *nmc, const NmcOutputField field_values[]);
+void print_data (NmCli *nmc);
 gboolean nmc_versions_match (NmCli *nmc);
 
 #endif /* NMC_UTILS_H */