Re: tbl issues with 1.11.1

Re: tbl issues with 1.11.1

[Ingo Schwarze]

Hi,

Ingo Schwarze wrote on Sat, May 14, 2011 at 06:12:28AM +0200:

> Getting this right in both cases - with and without vertical rulers -
> requires rather large changes.  The logic, as far as i understand it
> so far, goes like this:
> 
>  - box produces "|"
>  - no spacing at this point
>  - first cell, width is max(lengths)
>  - one space
>  - vertical ruler, if none, another space
>  - another space, unless there was a double ruler (!!)
>  - second cell
>  - and so on.
> 
> That is vastly different from what we have now, both regarding the
> width calculations - which apparently must not include padding - and
> regarding the output itself.
> 
> The work-in-progress patch below comes closer.  However, it still
> has at least two deficiencies, so it is not ready for commit:
>  - There still is a bogus space after double rulers, see (!!).
>  - The handling of explicit spacing options got sweeped away
>    by the reorg i was forced to do.  Probably, i need to put
>    it back in at the right places.  It was in the calc routine
>    but probably belongs in the output routines themselves.

I now have an updated diff that i consider ripe for commit,
which is improved in four respects:
  - do not use spanned cells for width calculations
  - draw horizontal rulers of the right width
  - correctly draw horizontal frames
  - guard against integer underflow in tbl_literal

As explicit spacing options seem to be used very rarely, if at all,
i have left them out for now to not make the diff yet larger.

OK?
  Ingo


Index: out.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/out.c,v
retrieving revision 1.15
diff -u -p -r1.15 out.c
--- out.c	18 Sep 2011 15:54:48 -0000	1.15
+++ out.c	20 Sep 2011 17:17:01 -0000
@@ -140,6 +140,7 @@ tblcalc(struct rofftbl *tbl, const struc
 	const struct tbl_dat	*dp;
 	const struct tbl_head	*hp;
 	struct roffcol		*col;
+	int			 spans;
 
 	/*
 	 * Allocate the master column specifiers.  These will hold the
@@ -156,11 +157,18 @@ tblcalc(struct rofftbl *tbl, const struc
 	for ( ; sp; sp = sp->next) {
 		if (TBL_SPAN_DATA != sp->pos)
 			continue;
+		spans = 1;
 		/*
 		 * Account for the data cells in the layout, matching it
 		 * to data cells in the data section.
 		 */
 		for (dp = sp->first; dp; dp = dp->next) {
+			/* Do not used spanned cells in the calculation. */
+			if (0 < --spans)
+				continue;
+			spans = dp->spans;
+			if (1 < spans)
+				continue;
 			assert(dp->layout);
 			col = &tbl->cols[dp->layout->head->ident];
 			tblcalc_data(tbl, col, sp->tbl, dp);
@@ -227,39 +235,12 @@ static void
 tblcalc_literal(struct rofftbl *tbl, struct roffcol *col,
 		const struct tbl_dat *dp)
 {
-	size_t		 sz, bufsz, spsz;
+	size_t		 sz;
 	const char	*str;
 
-	/* 
-	 * Calculate our width and use the spacing, with a minimum
-	 * spacing dictated by position (centre, e.g,. gets a space on
-	 * either side, while right/left get a single adjacent space).
-	 */
-
-	bufsz = spsz = 0;
 	str = dp->string ? dp->string : "";
 	sz = (*tbl->slen)(str, tbl->arg);
 
-	/* FIXME: TBL_DATA_HORIZ et al.? */
-
-	assert(dp->layout);
-	switch (dp->layout->pos) {
-	case (TBL_CELL_LONG):
-		/* FALLTHROUGH */
-	case (TBL_CELL_CENTRE):
-		bufsz = (*tbl->len)(1, tbl->arg);
-		break;
-	default:
-		bufsz = (*tbl->len)(1, tbl->arg);
-		break;
-	}
-
-	if (dp->layout->spacing) {
-		spsz = (*tbl->len)(dp->layout->spacing, tbl->arg);
-		bufsz = bufsz > spsz ? bufsz : spsz;
-	}
-
-	sz += bufsz;
 	if (col->width < sz)
 		col->width = sz;
 }
@@ -276,7 +257,7 @@ tblcalc_number(struct rofftbl *tbl, stru
 
 	/*
 	 * First calculate number width and decimal place (last + 1 for
-	 * no-decimal numbers).  If the stored decimal is subsequent
+	 * non-decimal numbers).  If the stored decimal is subsequent to
 	 * ours, make our size longer by that difference
 	 * (right-"shifting"); similarly, if ours is subsequent the
 	 * stored, then extend the stored size by the difference.
@@ -303,11 +284,6 @@ tblcalc_number(struct rofftbl *tbl, stru
 	} else
 		d = sz + psz;
 
-	/* Padding. */
-
-	sz += (*tbl->len)(2, tbl->arg);
-	d += (*tbl->len)(1, tbl->arg);
-
 	/* Adjust the settings for this column. */
 
 	if (col->decimal > d) {
@@ -320,11 +296,4 @@ tblcalc_number(struct rofftbl *tbl, stru
 		col->width = sz;
 	if (d > col->decimal)
 		col->decimal = d;
-
-	/* Adjust for stipulated width. */
-
-	if (col->width < dp->layout->spacing)
-		col->width = dp->layout->spacing;
 }
-
-
Index: tbl_term.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/tbl_term.c,v
retrieving revision 1.9
diff -u -p -r1.9 tbl_term.c
--- tbl_term.c	18 Sep 2011 15:54:48 -0000	1.9
+++ tbl_term.c	20 Sep 2011 17:17:01 -0000
@@ -30,14 +30,14 @@ static	void	tbl_char(struct termp *, cha
 static	void	tbl_data(struct termp *, const struct tbl *,
 			const struct tbl_dat *, 
 			const struct roffcol *);
-static	void	tbl_hframe(struct termp *, const struct tbl_span *);
+static	size_t	tbl_rulewidth(struct termp *, const struct tbl_head *);
+static	void	tbl_hframe(struct termp *, const struct tbl_span *, int);
 static	void	tbl_literal(struct termp *, const struct tbl_dat *, 
 			const struct roffcol *);
 static	void	tbl_number(struct termp *, const struct tbl *, 
 			const struct tbl_dat *, 
 			const struct roffcol *);
 static	void	tbl_hrule(struct termp *, const struct tbl_span *);
-static	void	tbl_vframe(struct termp *, const struct tbl *);
 static	void	tbl_vrule(struct termp *, const struct tbl_head *);
 
 
@@ -91,12 +91,19 @@ term_tbl(struct termp *tp, const struct 
 
 	/* Horizontal frame at the start of boxed tables. */
 
-	if (TBL_SPAN_FIRST & sp->flags)
-		tbl_hframe(tp, sp);
+	if (TBL_SPAN_FIRST & sp->flags) {
+		if (TBL_OPT_DBOX & sp->tbl->opts)
+			tbl_hframe(tp, sp, 1);
+		if (TBL_OPT_DBOX & sp->tbl->opts ||
+		    TBL_OPT_BOX  & sp->tbl->opts)
+			tbl_hframe(tp, sp, 0);
+	}
 
 	/* Vertical frame at the start of each row. */
 
-	tbl_vframe(tp, sp->tbl);
+	if (TBL_OPT_BOX & sp->tbl->opts || TBL_OPT_DBOX & sp->tbl->opts)
+		term_word(tp, TBL_SPAN_HORIZ == sp->pos ||
+			TBL_SPAN_DHORIZ == sp->pos ? "+" : "|");
 
 	/*
 	 * Now print the actual data itself depending on the span type.
@@ -134,9 +141,31 @@ term_tbl(struct termp *tp, const struct 
 			if (--spans >= 0)
 				continue;
 
+			/*
+			 * All cells get a leading blank, except the
+			 * first one and those after double rulers.
+			 */
+
+			if (hp->prev && TBL_HEAD_DVERT != hp->prev->pos)
+				tbl_char(tp, ASCII_NBRSP, 1);
+
 			col = &tp->tbl.cols[hp->ident];
 			tbl_data(tp, sp->tbl, dp, col);
 
+			/* No trailing blanks. */
+
+			if (NULL == hp->next)
+				break;
+
+			/*
+			 * Add another blank between cells,
+			 * or two when there is no vertical ruler.
+			 */
+
+			tbl_char(tp, ASCII_NBRSP,
+			    TBL_HEAD_VERT  == hp->next->pos ||
+			    TBL_HEAD_DVERT == hp->next->pos ? 1 : 2);
+
 			/* 
 			 * Go to the next data cell and assign the
 			 * number of subsequent spans, if applicable.
@@ -150,7 +179,11 @@ term_tbl(struct termp *tp, const struct 
 		break;
 	}
 
-	tbl_vframe(tp, sp->tbl);
+	/* Vertical frame at the end of each row. */
+
+	if (TBL_OPT_BOX & sp->tbl->opts || TBL_OPT_DBOX & sp->tbl->opts)
+		term_word(tp, TBL_SPAN_HORIZ == sp->pos ||
+			TBL_SPAN_DHORIZ == sp->pos ? "+" : " |");
 	term_flushln(tp);
 
 	/*
@@ -159,7 +192,11 @@ term_tbl(struct termp *tp, const struct 
 	 */
 
 	if (TBL_SPAN_LAST & sp->flags) {
-		tbl_hframe(tp, sp);
+		if (TBL_OPT_DBOX & sp->tbl->opts ||
+		    TBL_OPT_BOX  & sp->tbl->opts)
+			tbl_hframe(tp, sp, 0);
+		if (TBL_OPT_DBOX & sp->tbl->opts)
+			tbl_hframe(tp, sp, 1);
 		assert(tp->tbl.cols);
 		free(tp->tbl.cols);
 		tp->tbl.cols = NULL;
@@ -171,86 +208,66 @@ term_tbl(struct termp *tp, const struct 
 
 }
 
+/*
+ * Horizontal rules extend across the entire table.
+ * Calculate the width by iterating over columns.
+ */
+static size_t
+tbl_rulewidth(struct termp *tp, const struct tbl_head *hp)
+{
+	size_t		 width;
+
+	width = tp->tbl.cols[hp->ident].width;
+	if (TBL_HEAD_DATA == hp->pos) {
+		/* Account for leading blanks. */
+		if (hp->prev && TBL_HEAD_DVERT != hp->prev->pos)
+			width++;
+		/* Account for trailing blanks. */
+		width++;
+		if (hp->next &&
+		    TBL_HEAD_VERT  != hp->next->pos &&
+		    TBL_HEAD_DVERT != hp->next->pos)
+			width++;
+	}
+	return(width);
+}
+
+/*
+ * Rules inside the table can be single or double
+ * and have crossings with vertical rules marked with pluses.
+ */
 static void
 tbl_hrule(struct termp *tp, const struct tbl_span *sp)
 {
 	const struct tbl_head *hp;
 	char		 c;
-	size_t		 width;
-
-	/*
-	 * An hrule extends across the entire table and is demarked by a
-	 * standalone `_' or whatnot in lieu of a table row.  Spanning
-	 * headers are marked by a `+', as are table boundaries.
-	 */
 
 	c = '-';
 	if (TBL_SPAN_DHORIZ == sp->pos)
 		c = '=';
 
-	/* FIXME: don't use `+' between data and a spanner! */
-
-	for (hp = sp->head; hp; hp = hp->next) {
-		width = tp->tbl.cols[hp->ident].width;
-		switch (hp->pos) {
-		case (TBL_HEAD_DATA):
-			if (hp->next)
-				width += 2;
-			tbl_char(tp, c, width);
-			break;
-		case (TBL_HEAD_DVERT):
-			tbl_char(tp, '+', width);
-			/* FALLTHROUGH */
-		case (TBL_HEAD_VERT):
-			tbl_char(tp, '+', width);
-			break;
-		default:
-			abort();
-			/* NOTREACHED */
-		}
-	}
+	for (hp = sp->head; hp; hp = hp->next)
+		tbl_char(tp,
+		    TBL_HEAD_DATA == hp->pos ? c : '+',
+		    tbl_rulewidth(tp, hp));
 }
 
+/*
+ * Rules above and below the table are always single
+ * and have an additional plus at the beginning and end.
+ * For double frames, this function is called twice,
+ * and the outer one does not have crossings.
+ */
 static void
-tbl_hframe(struct termp *tp, const struct tbl_span *sp)
+tbl_hframe(struct termp *tp, const struct tbl_span *sp, int outer)
 {
 	const struct tbl_head *hp;
-	size_t		 width;
-
-	if ( ! (TBL_OPT_BOX & sp->tbl->opts || 
-			TBL_OPT_DBOX & sp->tbl->opts))
-		return;
-
-	/* 
-	 * Print out the horizontal part of a frame or double frame.  A
-	 * double frame has an unbroken `-' outer line the width of the
-	 * table, bordered by `+'.  The frame (or inner frame, in the
-	 * case of the double frame) is a `-' bordered by `+' and broken
-	 * by `+' whenever a span is encountered.
-	 */
-
-	if (TBL_OPT_DBOX & sp->tbl->opts) {
-		term_word(tp, "+");
-		for (hp = sp->head; hp; hp = hp->next) {
-			width = tp->tbl.cols[hp->ident].width;
-			tbl_char(tp, '-', width);
-		}
-		term_word(tp, "+");
-		term_flushln(tp);
-	}
 
 	term_word(tp, "+");
-	for (hp = sp->head; hp; hp = hp->next) {
-		width = tp->tbl.cols[hp->ident].width;
-		switch (hp->pos) {
-		case (TBL_HEAD_DATA):
-			tbl_char(tp, '-', width);
-			break;
-		default:
-			tbl_char(tp, '+', width);
-			break;
-		}
-	}
+	for (hp = sp->head; hp; hp = hp->next)
+		tbl_char(tp,
+		    outer || TBL_HEAD_DATA == hp->pos ? '-' : '+',
+		    tbl_rulewidth(tp, hp));
 	term_word(tp, "+");
 	term_flushln(tp);
 }
@@ -330,14 +347,6 @@ tbl_vrule(struct termp *tp, const struct
 }
 
 static void
-tbl_vframe(struct termp *tp, const struct tbl *tbl)
-{
-
-	if (TBL_OPT_BOX & tbl->opts || TBL_OPT_DBOX & tbl->opts)
-		term_word(tp, "|");
-}
-
-static void
 tbl_char(struct termp *tp, char c, size_t len)
 {
 	size_t		i, sz;
@@ -356,37 +365,35 @@ static void
 tbl_literal(struct termp *tp, const struct tbl_dat *dp, 
 		const struct roffcol *col)
 {
-	size_t		 padl, padr, ssz;
-
-	padl = padr = 0;
+	size_t		 len, padl, padr;
 
 	assert(dp->string);
-
-	ssz = term_len(tp, 1);
+	len = term_strlen(tp, dp->string);
+	padr = col->width > len ? col->width - len : 0;
+	padl = 0;
 
 	switch (dp->layout->pos) {
 	case (TBL_CELL_LONG):
-		padl = ssz;
-		padr = col->width - term_strlen(tp, dp->string) - ssz;
+		padl = term_len(tp, 1);
+		padr = padr > padl ? padr - padl : 0;
 		break;
 	case (TBL_CELL_CENTRE):
-		padr = col->width - term_strlen(tp, dp->string);
-		if (3 > padr)
+		if (2 > padr)
 			break;
-		padl = (padr - 1) / 2;
+		padl = padr / 2;
 		padr -= padl;
 		break;
 	case (TBL_CELL_RIGHT):
-		padl = col->width - term_strlen(tp, dp->string);
+		padl = padr;
+		padr = 0;
 		break;
 	default:
-		padr = col->width - term_strlen(tp, dp->string);
 		break;
 	}
 
 	tbl_char(tp, ASCII_NBRSP, padl);
 	term_word(tp, dp->string);
-	tbl_char(tp, ASCII_NBRSP, padr + 2);
+	tbl_char(tp, ASCII_NBRSP, padr);
 }
 
 static void
@@ -423,13 +430,11 @@ tbl_number(struct termp *tp, const struc
 	} else
 		d = sz + psz;
 
-	sz += term_len(tp, 2);
-	d += term_len(tp, 1);
-
 	padl = col->decimal - d;
 
 	tbl_char(tp, ASCII_NBRSP, padl);
 	term_word(tp, dp->string);
-	tbl_char(tp, ASCII_NBRSP, col->width - sz - padl);
+	if (col->width > sz + padl)
+		tbl_char(tp, ASCII_NBRSP, col->width - sz - padl);
 }
 


--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: tbl issues with 1.11.1

[Kristaps Dzonsons]

On 20/09/2011 19:32, Ingo Schwarze wrote:
> Hi,
>
> Ingo Schwarze wrote on Sat, May 14, 2011 at 06:12:28AM +0200:
>
>> Getting this right in both cases - with and without vertical rulers -
>> requires rather large changes.  The logic, as far as i understand it
>> so far, goes like this:
>>
>>   - box produces "|"
>>   - no spacing at this point
>>   - first cell, width is max(lengths)
>>   - one space
>>   - vertical ruler, if none, another space
>>   - another space, unless there was a double ruler (!!)
>>   - second cell
>>   - and so on.
>>
>> That is vastly different from what we have now, both regarding the
>> width calculations - which apparently must not include padding - and
>> regarding the output itself.
>>
>> The work-in-progress patch below comes closer.  However, it still
>> has at least two deficiencies, so it is not ready for commit:
>>   - There still is a bogus space after double rulers, see (!!).
>>   - The handling of explicit spacing options got sweeped away
>>     by the reorg i was forced to do.  Probably, i need to put
>>     it back in at the right places.  It was in the calc routine
>>     but probably belongs in the output routines themselves.
>
> I now have an updated diff that i consider ripe for commit,
> which is improved in four respects:
>    - do not use spanned cells for width calculations
>    - draw horizontal rulers of the right width
>    - correctly draw horizontal frames
>    - guard against integer underflow in tbl_literal
>
> As explicit spacing options seem to be used very rarely, if at all,
> i have left them out for now to not make the diff yet larger.
>
> OK?
>    Ingo

Ingo,

This looks ok---the table output formatting has always been spotty. 
Please check this in (I like the in-line documentation you have, but the 
more the better, as this area's tricksy).

Thanks!

Kristaps
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv