Re: .SH + .SS spacing, 2nd try

[Ingo Schwarze <schwarze@usta.de>]

Hi Joerg,

Joerg Sonnenberger wrote on Sat, May 22, 2010 at 11:26:27PM +0200:

> this is the second try of getting the .SH and .SS vertical space
> correct.  Basically three things need to be done:
> - handle trailing .SS without body as last node
> - handle sections / header that only only .AT, .UC or empty paragraphs
> - handle empty paragraphs following paragraphs

I regard this as three independent diffs folded into one and will
comment separately.


> - handle empty paragraphs following paragraphs
>
> Index: man_term.c
> ===================================================================
> RCS file: /home/joerg/cvsroot/mdocml/man_term.c,v
> retrieving revision 1.71
> diff -u -p -r1.71 man_term.c
> --- man_term.c	17 May 2010 22:11:42 -0000	1.71
> +++ man_term.c	22 May 2010 21:11:47 -0000
> @@ -227,6 +227,9 @@ print_bvspace(struct termp *p, const str
>  		return;
>  	if (MAN_SH == n->prev->tok)
>  		return;
> +	if (MAN_PP == n->tok && MAN_PP == n->prev->tok &&
> +	    NULL == n->body->child)
> +		return;
>  
>  	term_vspace(p);
>  }

This is only a special case, it doesn't handle enough cases,
and generalizing it in this style to the general case will
quickly become tedious.  Consider, e.g.

.PP
.IP head
empty PP between IP and PP:
.PP
.PP
the end

It still has two blank lines between the two lines of text,
which it shouldn't.

I guess we need a (third) companion function of term_flushln()
and term_newln() and we need to call that at the right places.

 * term_flushln() emits one line break regardless.
 * term_newln() emits one line break unless the current line is empty.
 * term_blankline() emits two line breaks unless the current line
   is empty - that is, it emits either two or no line breaks.

The code should look very similar to term_newln(), except that
it will contain term_flushln() twice.  Coding that is easy enough,
using it properly will require some work to check all the cases
where term_flushln() and term_newln() are used now.

Equivalently, we could give term_newln() an unsigned int
argument to specifiy the number of term_flushln()s to emit.


> - handle trailing .SS without body as last node
>
> Index: man_term.c
> ===================================================================
> RCS file: /home/joerg/cvsroot/mdocml/man_term.c,v
> retrieving revision 1.71
> diff -u -p -r1.71 man_term.c
> --- man_term.c	17 May 2010 22:11:42 -0000	1.71
> +++ man_term.c	22 May 2010 21:11:47 -0000
> @@ -78,7 +78,7 @@ static	void		  print_man_head(struct ter
>  static	void		  print_man_nodelist(DECL_ARGS);
>  static	void		  print_man_node(DECL_ARGS);
>  static	void		  print_man_foot(struct termp *, 
> -				const struct man_meta *);
> +				const struct man_meta *, const struct man *);
>  static	void		  print_bvspace(struct termp *, 
>  				const struct man_node *);
>  
> @@ -183,7 +183,7 @@ terminal_man(void *arg, const struct man
>  
>  	if (n->child)
>  		print_man_nodelist(p, &mt, n->child, m);
> -	print_man_foot(p, m);
> +	print_man_foot(p, m, man);
>  }
>  
>  
> @@ -858,17 +899,33 @@ print_man_nodelist(DECL_ARGS)
>  
>  
>  static void
> -print_man_foot(struct termp *p, const struct man_meta *meta)
> +print_man_foot(struct termp *p, const struct man_meta *meta,
> +    const struct man *m)
>  {
>  	char		buf[DATESIZ];
> +	const struct man_node	*n;
>  
>  	term_fontrepl(p, TERMFONT_NONE);
>  
>  	time2a(meta->date, buf, DATESIZ);
>  
> -	term_vspace(p);
> -	term_vspace(p);
> -	term_vspace(p);
> +	/*
> +	 * Check if the last node is a .SS without body.
> +	 * In that case, no vspace is asserted.
> +	 */
> +	n = man_node(m);
> +	for (;;) {
> +		while (n->next)
> +			n = n->next;
> +		if (NULL == n->child)
> +			break;
> +		n = n->child;
> +	}
> +	if (MAN_SS != n->tok || MAN_BODY != n->type) {
> +		term_vspace(p);
> +		term_vspace(p);
> +		term_vspace(p);
> +	}
>  
>  	p->flags |= TERMP_NOSPACE | TERMP_NOBREAK;
>  	p->rmargin = p->maxrmargin - strlen(buf);

In general, i'm a fan of mimicking groff (for now).
But this cannot be intentional.
It makes no sense whatsoever, it must be a bug in groff.

By the way, it also happens when the trailing element
is .SH, not .SS, and even when it contains an empty .PP
or even an empty .IP.


> - handle sections / header that only only .AT, .UC or empty paragraphs
>
> Index: man_term.c
> ===================================================================
> RCS file: /home/joerg/cvsroot/mdocml/man_term.c,v
> retrieving revision 1.71
> diff -u -p -r1.71 man_term.c
> --- man_term.c	17 May 2010 22:11:42 -0000	1.71
> +++ man_term.c	22 May 2010 21:11:47 -0000
> @@ -641,6 +644,57 @@ post_TP(DECL_ARGS)
>  }
>  
>  
> +static void
> +print_section_vspace(struct termp *p, const struct man_node *n)
> +{
> +	const struct man_node *nn;
> +
> +	nn = n->prev;
> +	/* If there is no previous siblings, there is no need for vspace. */
> +	if (n->prev == NULL)
> +		return;
> +
> +	/*
> +	 * The previous silbing is normally .SH or .SS.  It is possible
> +	 * to be another block-level macro at the beginning of the file.
> +	 *
> +	 * No vspace is needed if the previous sibling of the same type
> +	 * (.SH or .SS) without content.  If that sibling is not a .SH or
> +	 * .SS, the vspace is not needed if all previous nodes are without
> +	 * content.
> +	 * 
> +	 * .AT, .UC and .PP without children don't count as content.
> +	 */
> +	if (MAN_SH == nn->tok || MAN_SS == nn->tok) {
> +		if (n->tok != nn->tok) {
> +			term_vspace(p);
> +			return;
> +		}
> +		if (NULL == nn->body->child)
> +			return;
> +		nn = nn->body->child;
> +	} else {
> +		nn = nn->parent->child;
> +	}
> +
> +	for (; nn && n != nn; nn = nn->next) {
> +		switch (nn->tok) {
> +		case MAN_AT:
> +			continue;
> +		case MAN_UC:
> +			continue;
> +		case MAN_PP:
> +			if (NULL == nn->body->child)
> +				continue;
> +			/* FALLTHRU */
> +		default:
> +			term_vspace(p);
> +			return;
> +		}

This appears to be insufficient, consider for example

.SH SYNOPSIS
.IP
.SH DESCRIPTION
text

which also has no blank line between the section headers.

Thus, we should not look for particular macros, but check
whether any output was generated in the section.
If so, we need to break, if not, we don't.

> +	}
> +}
> +
> +
>  /* ARGSUSED */
>  static int
>  pre_SS(DECL_ARGS)
> @@ -650,13 +704,7 @@ pre_SS(DECL_ARGS)
>  	case (MAN_BLOCK):
>  		mt->lmargin = INDENT;
>  		mt->offset = INDENT;
> -		/* If following a prior empty `SS', no vspace. */
> -		if (n->prev && MAN_SS == n->prev->tok)
> -			if (NULL == n->prev->body->child)
> -				break;
> -		if (NULL == n->prev)
> -			break;
> -		term_vspace(p);
> +		print_section_vspace(p, n);
>  		break;
>  	case (MAN_HEAD):
>  		term_fontrepl(p, TERMFONT_BOLD);
> @@ -700,14 +748,7 @@ pre_SH(DECL_ARGS)
>  	case (MAN_BLOCK):
>  		mt->lmargin = INDENT;
>  		mt->offset = INDENT;
> -		/* If following a prior empty `SH', no vspace. */
> -		if (n->prev && MAN_SH == n->prev->tok)
> -			if (NULL == n->prev->body->child)
> -				break;
> -		/* If the first macro, no vspae. */
> -		if (NULL == n->prev)
> -			break;
> -		term_vspace(p);
> +		print_section_vspace(p, n);
>  		break;
>  	case (MAN_HEAD):
>  		term_fontrepl(p, TERMFONT_BOLD);

Yes, handling the two cases with common code looks like a fine idea.

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