Re: [PATCH 2/7] Add column layout

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 9, 2011, 07:36 UTC
Message-ID: <20110209073655.GA2135@elie>
In-Reply-To: <1297178541-31124-3-git-send-email-pclouds@gmail.com>

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

> +++ b/column.c
> @@ -0,0 +1,177 @@

[...]

Show 14 quoted lines

> +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.

Show 5 quoted lines

> +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.

Show 5 quoted lines

> +{
> +	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++) {

[...]

Show 5 quoted lines

> +		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.

[...]

Show 15 quoted lines

> +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++) {

[...]

Show 10 quoted lines

> +			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).)

Show 8 quoted lines

> +
> +			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;
	}