tbl: incorrect width calculation in cs[s..]

tbl: incorrect width calculation in cs[s..]

[Simon Branch]

Hello!  mandoc has some slightly odd behavior when formatting a table like this,
where the author wants a centered title:

   .TS
   tab(#);
   csss
   l1 l3 l1 l.
   heading
   00#NUL#01#SOH
   .TE

Groff outputs this:

       heading
   00 NUL   01 SOH

mandoc outputs this:

         heading
   00 NUL   01 SOH

There are three problems here:

   - The space after the first column isn't counted.
   - The imaginary spaces after the last column are counted, even though
     they aren't output.
   - Mandoc assumes that each column has three trailing spaces.

In other words, the control flow works like this:

   - Start with the first column's width.
   - If there are any continuation columns, add their widths and trailing
     spaces.

But I think it should work like this:

   - Start with the first column's width.
   - If there are any continuation columns, add the widths of the rest of
     the columns, and all the trailing spaces *except* for the ones after
     the last column.

This might still be incorrect (?) if there are more columns after
"cs...", but I think a partial fix is better than no fix at all.

Index: tbl_term.c
===================================================================
RCS file: /cvs/mandoc/tbl_term.c,v
retrieving revision 1.75
diff -u -r1.75 tbl_term.c
--- tbl_term.c  10 Aug 2021 12:55:04 -0000      1.75
+++ tbl_term.c  3 Apr 2022 04:39:18 -0000
@@ -820,8 +820,11 @@
        width = col->width;
        ic = dp->layout->col;
        hspans = dp->hspans;
-       while (hspans--)
-               width += tp->tbl.cols[++ic].width + 3;
+       while (hspans--) {
+               width += tp->tbl.cols[ic].spacing;
+               ic++;
+               width += tp->tbl.cols[ic].width;
+       }

        padr = width > len ? width - len : 0;
        padl = 0;
--
 To unsubscribe send an email to discuss+unsubscribe@mandoc.bsd.lv

Re: tbl: incorrect width calculation in cs[s..]

[Ingo Schwarze]

Hello Simon,

Simon Branch wrote on Sun, Apr 03, 2022 at 01:14:27PM -0700:

> Hello!  mandoc has some slightly odd behavior when formatting a table
> like this, where the author wants a centered title:
> 
>    .TS
>    tab(#);
>    csss
>    l1 l3 l1 l.
>    heading
>    00#NUL#01#SOH
>    .TE
> 
> Groff outputs this:
> 
>        heading
>    00 NUL   01 SOH

Which makes sense: 4 spaces before and after the heading.

> mandoc outputs this:
> 
>          heading
>    00 NUL   01 SOH

Which is clearly not right.

> There are three problems here:
> 
>    - The space after the first column isn't counted.
>    - The imaginary spaces after the last column are counted, even though
>      they aren't output.

Well, as long as we assume the spacing is always 3n (as the old code
did), we can't really say whether the magic constant "3" in the code
refers to the spacing before or after the column, so these two
perfectly cancel each other and do not represent additional bugs.

>    - Mandoc assumes that each column has three trailing spaces.

Yes.  That is indeed the issue here.

> In other words, the control flow works like this:
> 
>    - Start with the first column's width.
>    - If there are any continuation columns, add their widths and trailing
>      spaces.
>
> But I think it should work like this:
> 
>    - Start with the first column's width.
>    - If there are any continuation columns, add the widths of the rest of
>      the columns, and all the trailing spaces *except* for the ones after
>      the last column.

Yes.

Thank you for the good analysis and the perfect patch.  I committed
it to openbsd.org and bsd.lv.

> This might still be incorrect (?) if there are more columns after
> "cs...", but I think a partial fix is better than no fix at all.

Actually, my impression is it works just fine even in that case, too.
Besides, it survives the complete regression test suite, which contains
many tbl(7) tests, though admittedly few that explicitly set the
spacing between columns.


I had to apply your patch by hand because you converted tabs to
spaces.  When sending patches, please use "cvs diff -up" and make
sure neither you nor your mail program mangle the patch.

I'm including the committed patch below; please compare it to the
version you sent to see how they differ.

Yours,
  Ingo


Log Message:
-----------
When calculating the with of spanned columns, which for example matters
for centering text spanning multiple tbl(7) columns, correctly account
for the spacing between columns instead of wrongly assuming the default 
spacing of 3n.

Patch from Simon Branch <simonmbranch at gmail dot com>.

Modified Files:
--------------
    mandoc:
        tbl_term.c

Revision Data
-------------
Index: tbl_term.c
===================================================================
RCS file: /home/cvs/mandoc/mandoc/tbl_term.c,v
retrieving revision 1.75
retrieving revision 1.76
diff -Ltbl_term.c -Ltbl_term.c -u -p -r1.75 -r1.76
--- tbl_term.c
+++ tbl_term.c
@@ -820,8 +820,11 @@ tbl_literal(struct termp *tp, const stru
 	width = col->width;
 	ic = dp->layout->col;
 	hspans = dp->hspans;
-	while (hspans--)
-		width += tp->tbl.cols[++ic].width + 3;
+	while (hspans--) {
+		width += tp->tbl.cols[ic].spacing;
+		ic++;
+		width += tp->tbl.cols[ic].width;
+	}
 
 	padr = width > len ? width - len : 0;
 	padl = 0;
--
 To unsubscribe send an email to discuss+unsubscribe@mandoc.bsd.lv

Re: tbl: incorrect width calculation in cs[s..]

[Simon Branch]

On Fri, Apr 08, 2022 at 07:02:29PM +0200, Ingo Schwarze wrote:
> I had to apply your patch by hand because you converted tabs to
> spaces.  When sending patches, please use "cvs diff -up" and make
> sure neither you nor your mail program mangle the patch.

Whoops, my naive copy-paste from my original email (discarded because
I hadn't been let into the mailing list yet) was the culprit.  I'm
still getting used to the mailing list workflow ... Thank you for all
your work on mandoc and OpenBSD!

- Simon
--
 To unsubscribe send an email to discuss+unsubscribe@mandoc.bsd.lv

Re: tbl: incorrect width calculation in cs[s..]

[Ingo Schwarze]

Hi Simon,

Simon Branch wrote on Sat, Apr 09, 2022 at 10:35:26AM -0700:
> On Fri, Apr 08, 2022 at 07:02:29PM +0200, Ingo Schwarze wrote:

>> I had to apply your patch by hand because you converted tabs to
>> spaces.  When sending patches, please use "cvs diff -up" and make
>> sure neither you nor your mail program mangle the patch.

> Whoops, my naive copy-paste from my original email

Right, mouse-based copy and paste is one of the typical ways
how people mangle patches without even noticing it.

> (discarded because
> I hadn't been let into the mailing list yet) was the culprit.

No real problem caused in this case.  Let's be happy happy
you gained this experience with a small patch rather than
with a large one.  :-)

> I'm still getting used to the mailing list workflow ...

Of course, every workflow needs getting used to, but in absolute
terms, i think that mailing list worflows are significantly
simpler and *much* less error prone than web-based workflows.

Even though i avoid web-based workflows whereever possible, the
number of instances where i had to write text twice because
information had to be put into some web form which then crashed
in the middle of assembling the information are too numerous
to count.

> Thank you for all your work on mandoc and OpenBSD!

You are welcome, thank you very much for reporting problems
and even more for sending in a very good patch.

Yours,
  Ingo
--
 To unsubscribe send an email to discuss+unsubscribe@mandoc.bsd.lv