Re: [PATCH 2/7] Add column layout

[Jonathan Nieder <jrnieder@gmail.com>]

Nguyễn Thái Ngọc Duy wrote:

> +++ b/column.c
> @@ -0,0 +1,177 @@
[...]
> +static int item_length(const struct column_layout *c, const char *s)
> +{
> +	int a_len = 0;
> +
> +	if (!(c->mode & COL_ANSI))
> +		return strlen(s);
> +
> +	while ((s = strstr(s, "\033[")) != NULL) {
> +		int len = strspn(s+2, "0123456789;");
> +		s += len+3; /* \033[<len><func char> */
> +		a_len += len+3;
> +	}
> +	return a_len;
> +}

I think you mean "return strlen(orig_s) - a_len".

Something like the following could be more obvious, though
it is unfortunately verbose.

	int len = 0;
	while (*s) {
		const char *next;

		/* \033[<len><func char> */
		if (!prefixcmp(s, "\033[")) {
			s += strlen("\033[");
			s += strspn(s, "0123456789;");
			if (!*s)
				... handle somehow ...
			s++;
		}

		next = strchrnul(s, '\033');
		len += next - s;
		s = next;
	}

Both miscompute the width of "Dépôt".  Something like this can do ok
if the string is modifiable and we know it is UTF-8:

		char save;
		...
		next = strchrnul(s, '\033');

		/*
		 * NEEDSWORK: a utf8_strwidth variant that
		 * accepts a memory area not terminated by \0
		 * would avoid this ugliness.
		 */
		save = *next;
		*next = '\0';
		len += utf8_strwidth(s);
		*next = save;

POSIX does not provide a strwidth function, so if we want to handle
encodings like SHIFT-JIS then something uglier[1] might come to life.

> +static void layout(const struct column_layout *c,
> +		   int total_width, int padding,
> +		   int *width,
> +		   int *rows, int *cols)
> +{
[...]
> +	*rows = c->items.nr / *cols;
> +	if (c->items.nr > *rows * *cols)
> +		(*rows)++;

Maybe
	*rows = DIV_ROUND_UP(c->items.nr, *cols);
?

> +static int squeeze_columns(const struct column_layout *c,
> +			   int *width, int padding,
> +			   int rows, int cols)

What does this function do?

Ah, it is for computing smaller, unequal column widths.

> +{
> +	int x, y, i, len, item_len, spare = 0;
> +	const char *s;
> +
> +	for (x = 0; x < cols; x++) {

		/* Find minimum width for column. */
		int len = 0;

> +		for (y = len = 0; y < rows; y++) {
[...]
> +		len += padding;
> +		if (len < width[x]) {
> +			spare += width[x] - len;
> +			width[x] = len;
> +		}

This "if" is unnecessary, I hope.  A simple

		assert(len <= width[x]);

would check that.

[...]
> +static void relayout(const struct column_layout *c,
> +		     int padding, int spare,
> +		     int *initial_width, int **width,
> +		     int *rows, int *cols)
> +{
> +	int new_rows, new_cols, new_initial_width;
> +	int i, *new_width, new_spare, total_width;
> +
> +	/*
> +	 * Assume all columns have same width, we would need
> +	 * initial_width*cols. But then after squeezing, we have
> +	 * "spare" more chars. Assume a new total_width with
> +	 * additional chars, then re-squeeze to see if it fits
> +	 * c->width.
> +	 */

Might be easier to debug if this part were deferred to a separate
patch. :)

> +	total_width = (*initial_width)*(*cols) + spare;

An odd heuristic.  Does it work well in practice?

> +static void display_columns_first(const struct column_layout *c,
> +				  int padding, const char *indent)
> +{
[...]
> +	for (y = 0; y < rows; y++) {
> +		for (x = 0; x < cols; x++) {
[...]
> +			const char *s;
> +			int len;
> +
> +			i = x * rows + y;
> +			if (i >= c->items.nr)
> +				break;
> +			s = c->items.items[i].string;
> +			len = item_length(c, s);
> +			if (width[x] < initial_width)
> +				len += initial_width - width[x];

The "if" is unnecessary, I hope.  (With that hope being checkable
by assert(width[x] <= initial_width).)

> +
> +			printf("%s%s%s",
> +			       x == 0 ? indent : "",
> +			       c->items.items[i].string,
> +			       /* If the next column at same row is
> +				  out of range, end the line. Else pad
> +				  some space.  */
> +			       i + rows >= c->items.nr ? "\n" : empty_cell + len);

Nice.  The loop body could be a separate display_cell() function for
easier contemplation.

[...]
> +++ b/column.h
> @@ -0,0 +1,20 @@
[...]
> +#define COL_DENSE         (1 << 5) /* "Ragged-right" mode, relayout if enough space */

I might be confused, but would it not be clearer to call it "unequal
column width" mode?  In other words, is it about columns not all
having the same width as one another or about having a ragged right
margin?

Thanks for a pleasant read.
Jonathan

[1] Based on tuklib_mbstr_width:

	size_t remaining = strlen(s);
	size_t len = 0;
	mbstate_t state;

	memset(&state, 0, sizeof(state));
	while (remaining) {
		wchar_t ch;
		size_t nbytes;
		int width;

		if (!prefixcmp(s, "\033["))
			...
		nbytes = mbrtowc(&ch, s, remaining, &state);
		if (!nbytes || nbytes > remaining)
			... handle error ...
		s += nbytes;
		remaining -= nbytes;
		width = wcwidth(ch);
		if (width < 0)
			... handle nonprintable character ...
		len += width;
	}