Re: [PATCH] [grotty]: Use terminfo.

[G. Branden Robinson]

Hi Lennart,

Thanks a lot for taking a stab at this!

In lieu of a proper code review, I'll just riff on this to get the
issues out before an audience.

At 2023-08-19T20:08:06+0000, Lennart Jablonka wrote:
> This has nothing at all to do with making it easier to customize the
> look of man pages.

I sort of agree, if you refer very broadly to all questions of formatted
appearance.  groff man(7) has several tunable parameters for that.

If you mean by applying a color scheme, it makes one approach easier, in
principle.  It opens an avenue for people to do it by redefining
terminfo capabilities to produce whatever terminal escape sequences they
want, and then letting grotty observe the desired terminal type via the
$TERM environment variable (actually it'd be terminfo doing that, which
was the whole point of Savannah #63583).

There remains the avenue of redefining man(7) macros in .../man.local,
but historically that has not been a popular approach, and one has to
understand a moderate amount of groff formatting language to do that.

> Use of hardcoded SGR escape sequences persists as a fallback but
> should probably be dropped.

I lean that way.  Any system without terminfo is, I predict, going to be
content (or even happy) with overstruck output anyway.

> If it stays, I’m unsure of the interface to grotty.  The -T disabling
> the fallback seems hackish.

I'm unsure that we need a `-T` option if we getenv("TERM") (or if
terminfo(3) does).  No other part of groff cares about the terminal type
in any way, so sticking TERM=xterm+my-pimped-out-colors-for-man-pages in
the environment of a groff command isn't going to mess anything else up.

Okay, it might mess up the pager they pipe groff/grotty's output
through, but that's Not Our Problem.

(I observe that ncurses doesn't actually _provide_ a terminfo(3) page,
which is silly.  Or rather, the page is there, but it doesn't bother to
mention its own name in its NAME section so that makewhatis/mandb can
find it.  Guess I'll be sending a patch.)

Until it does, read "terminfo(3)" as "putp(3)".  It's brings up the
relevant page and is quicker to type anyway.

> Hyperlinks are still marked up using OSC 8, hoping that whatever
> terminal sits in front of the user interprets or ignores those.

It's a moderately safe assumption that a modern terminal emulator at
least gestures at ANSI X.34/ISO 6429/ECMA-48 escape sequence support.

Given the quantity of bugs in this area over the years, admittedly that
gesture sometimes looks more like a one-finger salute.

> Terminfo naturally doesn’t have a capability for hyperlinks.

It could get one.  Thomas Dickey has had no compunction about adding
other ones.  I understand, but am disappointed that, his new terminfo
capabilities seem always to be only 2 characters long (for termcap
compatibility).  I feel that the termcap API has been mollycoddled for
far too long.

> Autoconf is not informed about the depence on Curses; we simply link
> with -lcurses, as that is what X/Open Curses specifies.

Yeah, we'll need to Do The Right Thing here.  Not a problem.  I can take
care of it.

> The suggested option to less is changed to -r, which enables passing
> through single control characters, because some terminal descriptions
> use those;

Last time I mentioned less(1)'s `-r` option, I alarmed Ingo Schwarze.
It might be worth elaborating this point.

> I noticed that xterm’s sgr0 contains ^O.

Hmm...I see that for "xterm-xf86-v32" (meaning the xterm distributed
with XFree86 3.2, which is 25+ years old), "xtermm" (Solaris
monochrome), "color_xterm" (a long-dead fork from ~1996).  Other xterm
terminfo descriptions--and there are many--seem to use "ESC [ m"
(xterm-r5, xterm-r6); "ESC ( B ESC [ m" (xterm-basic); or 8-bit controls
"CSI m" (xterm-8bit).

I do also see it in

It's tempting to just punt this issue to the users of less(1) with
obsolete terminal emulators, those that are indifferently maintained, or
simply don't see a problem here: the Linux console driver, "qansi-g"
("QNX ANSI"), "arm100" ("Arm(RiscPC)"), "vt100+4bsd", "nsterm+acs",
"nsterm+mac", "iterm.App", "putty", "teraterm2.3", "gnome-rh72",
"konsole-base", "rxvt-basic", "rxvt", "Eterm", "hpterm",
"hpterm-color2", "emu220", "mvterm", "mterm-ansi", "decansi",
"screen-base", "dvtm", "dtterm", "newhp", ... I'm running out of energy
to type these.  Look at the terminfo database.

https://github.com/ThomasDickey/ncurses-snapshots/blob/master/misc/terminfo.src

All of the users of these terminal emulators would already be facing
this problem with other curses programs anyway.  Except they probably
don't page them.  Not sure if there's anything for us to do here.
Except keep GROFF_NO_SGR around, even if it becomes a bit of a misnomer.

> I’m too lazy to find another name for lines_.

I'm thinking "term_lines".  I'll figure something out.

> All this NEWS and ChangeLog and Autotools GNU stuff is confusing.
> Do as you wish with it.

Can do.

> I didn’t test this a whole lot, much less wrote automated tests.
> Have fun.

Oh, boy, the very best part.  :-|  But the one that helps me sleep at
night instead of awakening in a sweat over what the users may discover.

> PS: There doesn’t happen to be a convention on whether to use 
> non-ASCII characters in commit messages, right?  Seeing as you, 
> Branden, yourself aren’t always quite consistent with that.

I don't have a firm position on this.  My impression is that Git users
are almost invariably UTF-8 partisans.  I may have applied patches from
people who were using ISO Latin-something, though.  And also, Git front
ends like cgit are sometimes baffled when a commit message is in UTF-8
but the diff part is to an ISO Latin-1 file.

So the inconsistency might not _always_ be just me.  But sometimes,
sure.

> PPS: Mandoc underlines italic \~s.
> 
>       .RI [ file\~ .\|.\|.]
> 
> is rendered as
> 
>       [_f_i_l_e_ ...]
> 
> by mandoc.  Just noticed that.

Yeah.  I think that's ugly.  I'm glad we don't.

Okay, here's my "code review".  This is a full annotation for the sake
of complete coverage.  When I mention a point multiple times, it is not
to hector you, but as a guide and reminder to myself in the future.

>  INSTALL.extra                   |   5 +-
>  NEWS                            |   9 +
>  src/devices/grotty/grotty.1.man |  43 ++++-
>  src/devices/grotty/grotty.am    |   3 +-
>  src/devices/grotty/tty.cpp      | 293 +++++++++++++++-----------------
>  5 files changed, 188 insertions(+), 165 deletions(-)
> 
> diff --git a/INSTALL.extra b/INSTALL.extra
> index 78d4139af..3527b1929 100644
> --- a/INSTALL.extra
> +++ b/INSTALL.extra
> @@ -122,12 +122,15 @@ Several programs distributed with GNU roff are written 
> in the Perl
>  language.  Version 5.6.1 (1 April 2001) or later is required.
>  
>  The 'uchardet' library is an optional dependency of the 'preconv'
> -program: if this library is found by 'configure', it will be
> +program: If this library is found by 'configure', it will be

Nack.  What comes after the colon may be an independent clause, but it
is not a sentence.  ;-)

> +The 'grotty' program depends on a Curses library, which is specified
> +by X/Open Curses.

Yeah, I can elaborate this.  After some research.

> +Output drivers
> +--------------
> +
> +grotty
> +------
> +
> +o The terminfo library is now used for applying text attributes (like
> +  italic, bold, and colors).

We will want to say more about this.  What might break?

> diff --git a/src/devices/grotty/grotty.1.man b/src/devices/grotty/grotty.1.man
> index 41eabbcfa..ebdfd1639 100644
> --- a/src/devices/grotty/grotty.1.man
> +++ b/src/devices/grotty/grotty.1.man
> @@ -56,6 +56,8 @@ output driver for typewriter-like (terminal) devices
>  .RB [ \-i \||\| \-r ]
>  .RB [ \-F\~\c
>  .IR font-directory ]
> +.RB [ \-T\~\c
> +.IR term ]
[...]
> +.RB [ \-T\~\c
> +.IR term ]

As noted above, I want to see if we can go without this and just get
away with terminfo(3) doing a getenv("TERM") for us.

> @@ -136,9 +140,8 @@ Output is written to the standard output stream.
>  .P
>  By default,
>  .I grotty
> -emits SGR escape sequences
> -(from ISO\~6429,
> -popularly called \[lq]ANSI escapes\[rq])
> +uses
> +.MR terminfo 5

I would refer to the library page in section 3 here.  That's what we
use.  The section 5 page is a likely destination for the colorized man
page community, though, so I reckon we could throw them a bone later in
the page, and link to it.

> @@ -148,6 +151,15 @@ reverse video
>  [\[lq]negative image\[rq]]
>  and colors).
>  .
> +If no
> +.I TERM
> +environment variable is set
> +or terminfo encounters a different error,

Recast maybe.

If
.I terminfo
encounters an error
(perhaps because
.I TERM
is unset or its value unrecognized),

> +.I grotty
> +emits hardcoded SGR escape sequences
> +(from ISO\~6429,
> +popularly called \[lq]ANSI escapes\[rq]).

> @@ -188,10 +200,10 @@ When paging
>  .IR grotty 's
>  output with
>  .MR less 1 ,
> -the latter program must be instructed to pass SGR and OSC sequences
> +the latter program must be instructed to pass control characters
>  through to the device;
>  its
> -.B \-R
> +.B \-r
>  option is one way to achieve this
>  .RI ( less
>  version 566 or later is required for OSC\~8 support).

I guess I need to understand more about the purported hazards of `-r`.

> +.BI \-T\~ term
> +Read description for terminal type
> +.I term
> +and fail if
> +.I terminfo
> +encounters and error.
"an error"
+.
> +By default,
> +.I grotty
> +takes the terminal type from the environment variable
> +.I TERM
> +and falls back to SGR escape sequences
> +if that is not specified or there is an error.

This is another manifestation of "the -T question".

> +.TP
> +.I TERM
> +The terminal type for
> +.I terminfo.

We don't interpret it ourselves, but a library we use does.  I think I'd
throw to the library's man page for it.

.TP
.I TERM
declares the terminal type interpreted by the
.MR terminfo 3
library.

I see I could recast the description of GROFF_NO_SGR to be similarly
"stemmy".

>  grotty_LDADD = $(LIBM) \
>    libdriver.a \
>    libgroff.a \
> -  lib/libgnu.a
> +  lib/libgnu.a \
> +  -lcurses

Need to see what the Right Way is here.  The main issue may just be
deciding on the name for the library, which would involve Autoconf.
I'd also like to use terminfo directly (-ltinfo) rather than curses, if
possible.  I don't think curses proper has anything we need.

I _don't_ want to refactor grotty(1), as a single program, to support
being built with or without terminfo.  That way lies a rope bridge over
a chasm belching flames hundreds of feet into the air, and its name is
dlopen().  (Portability problems are what scare me.)

If we need to keep a terminfo-free output driver around, then I'd rather
keep grotty more or less as-is and do this work in a new
terminfo-enhanced TTY driver.  "grotitty" or something.

Shared routines could of course be moved into a common.cpp file.

> diff --git a/src/devices/grotty/tty.cpp b/src/devices/grotty/tty.cpp
> index 1b92ad8c0..9df35d8cd 100644
> --- a/src/devices/grotty/tty.cpp
> +++ b/src/devices/grotty/tty.cpp
> @@ -21,6 +21,8 @@ along with this program.  If not, see 
> <http://www.gnu.org/licenses/>. */
>  #include "device.h"
>  #include "ptable.h"
>  
> +#include <term.h>
> +
>  typedef signed char schar;
>  
>  declare_ptable(schar)
> @@ -28,8 +30,6 @@ implement_ptable(schar)
>  
>  extern "C" const char *Version_string;
>  
> -#define putstring(s) fputs(s, stdout)
> -

Oh, I see, we no longer need this--we migrate to terminfo's putp().

>  #ifndef SHRT_MIN
>  #define SHRT_MIN (-32768)
>  #endif
> @@ -38,7 +38,7 @@ extern "C" const char *Version_string;
>  #define SHRT_MAX 32767
>  #endif
>  
> -#define TAB_WIDTH 8
> +static int tab_width = 8;

Strictly, this is a style fix that might be committed first.  Symbol
visibility in the debugger is a big +1.

>  // A character of the output device fits in a 32-bit word.
>  typedef unsigned int output_character;
> @@ -56,10 +56,16 @@ static bool do_sgr_italics;
>  static bool want_reverse_video_for_italics = false;
>  static bool do_reverse_video;
>  static bool use_overstriking_drawing_scheme = false;
> +static bool use_terminfo;
> +static char *terminal_name;
>  
>  static void update_options();
>  static void usage(FILE *stream);
>  
> +// TODO(humm): We can get proper line drawing characters from terminfo
> +// using acsc, smacs, rmacs; we could do that for -Tascii + terminfo.
> +// There are other characters there we could theoreticlly use as well,
> +// like degree and pi.

I fear this promises to be a bit of a headache, not that I object to the
idea.  I'd like to kick this sort of thing past
<https://savannah.gnu.org/bugs/?62471>, which I perceive as more
important.  What do you think?

Also I need to understand the boundaries of the DEC Alternate Character
Set feature.  I guess if the terminal supports it, we have an overriding
set of glyph mappings we'd use instead of what's in the font description
files.  That would then make the font description files not necessarily
the Source of Truth for the output driver's behavior.  Headache
intensifies.  :(

> +// bold and reverse can be unset separately using ISO 6429, but not
> +// with terminfo

That sounds like something we should complain about to Thomas Dickey.

>  // many terminals can't handle 'CSI 39 m' and 'CSI 49 m' to reset
>  // the foreground and background color, respectively; we thus use
>  // 'CSI 0 m' exclusively
> -#define SGR_DEFAULT CSI "0m"
> +static const char *sgr_exit_attributes = CSI "0m";

Mmmm.  I don't like hiding other people's bugs.  With respect to
terminal emulators in particular, though, doing so seems to have been a
popular thing, and it may be one of the reasons terminal emulators have
tended to suck in some respects.  They're often brilliant at stuff like
alpha-blended color gradients and URL tooltip pop-ups, but completely
defecate themselves when it comes to basic matters of competency like
VT100 emulation.

Kinda tempted to revert this 0m hack (in a separate commit) and see what
happens.

Your change per se is fine.

>  class tty_printer : public printer {
> -  tty_glyph **lines;
> +  tty_glyph **lines_; // clashes with Curses lines

I hear the bike shed crying out for paint...

> -  void make_underline(int);
> -  void make_bold(output_character, int);
> +  void update_attributes(bool, bool, schar, schar, output_character, int);

Functions with gigantic argument lists make me a sad panda.  Can we use
a struct instead?

>    schar color_to_idx(color *);
>    void add_char(output_character, int, int, int, color *, color *,
>               unsigned char);

This one was making me sad already.

> @@ -201,10 +204,8 @@ public:

(No comments on this hunk.)

> @@ -271,54 +272,91 @@ tty_printer::tty_printer() : cached_v(0)
[...]
> -void tty_printer::make_underline(int w)
> +/* Update the current text attributes, doing little more than necessary
> +work.  The c and w parameters are not used unless
> +use_overstriking_drawing_scheme and at least one of underline and bold
> +is set. */

Should we bust this function into two?  One for overstriking output, and
one for everything else?

> +void tty_printer::update_attributes(bool underline, bool bold,
> +                                 schar fore_idx, schar back_idx,
> +                                 output_character c, int w)
>  {
>    if (use_overstriking_drawing_scheme) {
> -    if (!w)
> -      warning("can't underline zero-width character");
> -    else {
> -      putchar('_');
> -      putchar('\b');
> +    if (underline) {
> +      if (!w)
> +     warning("can't underline zero-width character");
> +      else {
> +     putchar('_');
> +     putchar('\b');
> +      }
> +    }
> +    if (bold) {
> +      if (!w)
> +     warning("can't print zero-width character in bold");
> +      else {
> +     put_char(c);
> +     putchar('\b');
> +      }
>      }
>    }
>    else {
> -    if (!is_underlining) {
> +    if (is_underlining && !underline
> +     || is_boldfacing && !bold
> +     || curr_fore_idx != DEFAULT_COLOR_IDX && fore_idx == DEFAULT_COLOR_IDX
> +     || curr_back_idx != DEFAULT_COLOR_IDX && back_idx == DEFAULT_COLOR_IDX) 
> {

<wince>

[fairly big snip]

> @@ -458,6 +496,8 @@ void tty_printer::special(char *arg, const environment 
> *env, char type)
>  // repeated arbitrarily and are separated by colons.  Omission of the
>  // URI ends the hyperlink that was begun by specifying it.  See
>  // <https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda>.
> +//
> +// TODO(humm): What should be done with OSC 8 when using terminfo?

Assume an ISO 6429-conformant terminal and blast it out.  If anyone
complains, sweatily point at Thomas Dickey and protest that the mean old
terminfo maintainer wouldn't give us a capability for it.

That's, like, page 1 of "How to Get Ahead in Software Engineering".

> -void tty_printer::change_color(const environment * const env)
> -{
> -  add_char(0, 0, env->hpos, env->vpos, env->col, env->fill, COLOR_CHANGE);
> -}
> -

I have lost track of why we no longer need this member function.
Coalesced into update_attributes() with the huge argument list?

>  void tty_printer::change_fill_color(const environment * const env)
>  {
>    add_char(0, 0, env->hpos, env->vpos, env->col, env->fill, COLOR_CHANGE);
> @@ -683,42 +718,12 @@ void tty_printer::put_char(output_character wc)
>      do *++p = (unsigned char)(((wc >> (6 * --count)) & 0x3f) | 0x80);
>        while (count > 0);
>      *++p = '\0';
> -    putstring(buf);
> +    fputs(buf, stdout);

A lingering use of fputs() rather than putp().  Your changed version
looks correct, and I'm not deeply enamored of using C preprocessor
macros the way this file was with `putstring`.

> -void tty_printer::put_color(schar color_index, int back)
> -{
> -  if (color_index == DEFAULT_COLOR_IDX) {
> -    putstring(SGR_DEFAULT);
> -    // set bold and underline again
> -    if (is_boldfacing)
> -      putstring(SGR_BOLD);
> -    if (is_underlining) {
> -      if (do_sgr_italics)
> -     putstring(SGR_ITALIC);
> -      else if (do_reverse_video)
> -     putstring(SGR_REVERSE);
> -      else
> -     putstring(SGR_UNDERLINE);
> -    }
> -    // set other color again
> -    back = !back;
> -    color_index = back ? curr_back_idx : curr_fore_idx;
> -  }
> -  if (color_index != DEFAULT_COLOR_IDX) {
> -    putstring(CSI);
> -    if (back)
> -      putchar('4');
> -    else
> -      putchar('3');
> -    putchar(color_index + '0');
> -    putchar('m');
> -  }
> -}
> -

Obsoleted by `tty_printer::update_attributes()`?

> @@ -741,7 +746,7 @@ void tty_printer::end_page(int page_length)
> @@ -757,8 +762,8 @@ void tty_printer::end_page(int page_length)

Chunks skipped (`lines_` rename).

> @@ -805,89 +810,42 @@ void tty_printer::end_page(int page_length)
[...]
> -         int next_tab_pos = ((hpos + TAB_WIDTH) / TAB_WIDTH) * TAB_WIDTH;
> +         int next_tab_pos = ((hpos + tab_width) / tab_width) * tab_width;

Static-constification of preprocessor macro.

[...]
> -         if (is_continuously_underlining)
> -           make_underline(p->w);
> -         else if (!use_overstriking_drawing_scheme
> -                  && is_underlining) {
> -           if (do_sgr_italics)
> -             putstring(SGR_NO_ITALIC);
> -           else if (do_reverse_video)
> -             putstring(SGR_NO_REVERSE);
> -           else
> -             putstring(SGR_NO_UNDERLINE);
> -           is_underlining = false;
> -         }

Why is this going away?

> +         // TODO(humm): Should we not take the width of the actual
> +         // tab?  The current status works, albeit not with
> +         // typewriters: _\b\t is rendered as multiple underlined
> +         // cells by less.  We could pass update_attributes the width
> +         // the actual tab will have and let it emit multiple
> +         // underlines.

If we support tab stop settings other than at every 8 character cells,
we might be the first Unix application that ever has.  termcap and
terminfo took a long time to show up; habits and attitudes, if not code,
had fossilized by then.  ncurses's tabs(1) man page is informative.

> +         update_attributes(is_continuously_underlining, is_boldfacing,
> +                           curr_fore_idx, curr_back_idx, '\t', p->w);

I marvel again at the argument list.

>           putchar('\t');
>           hpos = next_tab_pos;
>         }
>       }
>       for (; hpos < p->hpos; hpos++) {
> -       if (is_continuously_underlining)
> -         make_underline(p->w);
> -       else if (!use_overstriking_drawing_scheme && is_underlining) {
> -         if (do_sgr_italics)
> -           putstring(SGR_NO_ITALIC);
> -         else if (do_reverse_video)
> -           putstring(SGR_NO_REVERSE);
> -         else
> -           putstring(SGR_NO_UNDERLINE);
> -         is_underlining = false;
> -       }
> +       update_attributes(is_continuously_underlining, is_boldfacing,
> +                         curr_fore_idx, curr_back_idx, ' ', p->w);

I see how you're buying some code economy with this approach, but I
think we could get even more with a struct.

> -     if (!use_overstriking_drawing_scheme) {
> -       if (p->fore_color_idx != curr_fore_idx) {
> -         put_color(p->fore_color_idx, 0);
> -         curr_fore_idx = p->fore_color_idx;
> -       }
> -       if (p->back_color_idx != curr_back_idx) {
> -         put_color(p->back_color_idx, 1);
> -         curr_back_idx = p->back_color_idx;
> -       }
> -     }
> +     // test because we don't have any (c,w) to pass and won't do
> +     // colors if those would be used
> +     if (!use_overstriking_drawing_scheme)
> +       update_attributes(is_underlining, is_boldfacing,
> +                         p->fore_color_idx, p->back_color_idx, 0, 0);

Ditto.

> -      if (p->mode & UNDERLINE_MODE)
> -     make_underline(p->w);
> -      else if (!use_overstriking_drawing_scheme && is_underlining) {
> -     if (do_sgr_italics)
> -       putstring(SGR_NO_ITALIC);
> -     else if (do_reverse_video)
> -       putstring(SGR_NO_REVERSE);
> -     else
> -       putstring(SGR_NO_UNDERLINE);
> -     is_underlining = false;
> -      }
> -      if (p->mode & BOLD_MODE)
> -     make_bold(p->code, p->w);
> -      else if (!use_overstriking_drawing_scheme && is_boldfacing) {
> -     putstring(SGR_NO_BOLD);
> -     is_boldfacing = false;
> -      }
> -      if (!use_overstriking_drawing_scheme) {
> -     if (p->fore_color_idx != curr_fore_idx) {
> -       put_color(p->fore_color_idx, 0);
> -       curr_fore_idx = p->fore_color_idx;
> -     }
> -     if (p->back_color_idx != curr_back_idx) {
> -       put_color(p->back_color_idx, 1);
> -       curr_back_idx = p->back_color_idx;
> -     }
> -      }
> +      update_attributes(p->mode & UNDERLINE_MODE, p->mode & BOLD_MODE,
> +                     p->fore_color_idx, p->back_color_idx, p->code, p->w);

Ditto.

>        put_char(p->code);
>        hpos += p->w / font::hor;
>      }
> -    if (!use_overstriking_drawing_scheme
> -     && (is_boldfacing || is_underlining
> -         || curr_fore_idx != DEFAULT_COLOR_IDX
> -         || curr_back_idx != DEFAULT_COLOR_IDX))
> -      putstring(SGR_DEFAULT);
> +    update_attributes(false, false, DEFAULT_COLOR_IDX, DEFAULT_COLOR_IDX, 0, 
> 0);
>      putchar('\n');

<wince revisited>

[...]
> +    // TODO(humm): Do better error handling.
> +    int err;
> +    setupterm(terminal_name, 1, terminal_name != NULL ? NULL : &err);

I don't get why the ternary expression here.  setupterm _does_ have a
return value.  It returns the integer-valued macros `OK` or `ERR`.  (I
said curses was a late development in Unix, but it was early enough to
pull off this astounding feat of name space pollution.)

Essentially a lot of terminfo/curses functions return Booleans.  But
(originally) this was K&R C--everything's an int and range checks are
for [censored].  If you want Booleans, put them into bitfields so your
machine's shift and rotate instructions get some exercise.

ncurses's terminfo(3) page says that `err` can be populated with the
values 1, 0, or -1, _all of which indicate errors_.

So we want something like

+    int tistatus = setupterm(terminal_name, 1, &err);
+    if (tistatus != OK) {
+      switch(err) {
+      case -1:
+        // Can't find terminal database, fall back to something.
+        break;
+      case 0:
+        // Terminal has useless (generic) capability set, fall back to
+        // something.
+        break;
+      case 1:
+        // Terminal is "hardcopy"--NO PROBLEM!  Feed the overstrikers
+        // their delicious gruel!
+        break;
+      default:
+        // Fatal error: we don't understand what the library is trying
+        // to tell us.
+        break;
+      }

This puts another chit on the "keep only one terminal output driver"
square of our architectural bingo sheet.

> +    if (err == 1) {
> +      // TODO(humm): Do error handling for the capabilities.  Can they
> +      // be null pointers?  Can they be (char*)-1?  Should we fall back
> +      // to underline if italic is not available and the like?  Should
> +      // we ignore attributes we can't fulfill, or should we abort?

Terminfo capabilities come 3 types: Booleans ("flags"), integers
("numbers"), and strings.

At this point I'll just quote the man page.

tput(3):
   Terminal Capability Functions
     The tigetflag, tigetnum and tigetstr routines return the value of
     the capability corresponding to the terminfo capname passed to
     them, such as xenl.  The capname for each capability is given in
     the table column entitled capname code in the capabilities section
     of terminfo(5).

     These routines return special values to denote errors.

     The tigetflag routine returns

     -1   if capname is not a boolean capability, or

     0    if it is canceled or absent from the terminal description.

     The tigetnum routine returns

     -2   if capname is not a numeric capability, or

     -1   if it is canceled or absent from the terminal description.

     The tigetstr routine returns

     (char *)-1
          if capname is not a string capability, or

     0    if it is canceled or absent from the terminal description.

(This is old-school C.  We never inquire about the type of '0'.  In
other words, I don't get why this last item isn't spelled "NULL", or
'(char *)0') when its predecessor was documented with a cast.  :-|

So, re-quoting your questions:

> +      // TODO(humm): Do error handling for the capabilities.  Can they
> +      // be null pointers?

Yes, for tigetstr().

>        // Can they be (char*)-1?

Oh yes, and it's super bad news if it is.  It means someone's terminal
description is incomprehensible.  That's a fatal().

>        // Should we fall back
> +      // to underline if italic is not available and the like?

I could go either way on this.  I lean toward "no, you get only what you
ask for".  But we can throw a warning.

>        // Should
> +      // we ignore attributes we can't fulfill, or should we abort?

Same.  I wouldn't abort for this.

> +      use_terminfo = true;
> +      sgr_bold = enter_bold_mode;
> +      sgr_italic = enter_italics_mode;
> +      sgr_underline = enter_underline_mode;
> +      sgr_reverse = enter_reverse_mode;
> +      sgr_exit_attributes = exit_attribute_mode;
> +      tab_width = init_tabs;
> +    }

I guess these variables need renaming, since, conceivably, they might
not have anything to do with ANSI X3.4/ISO 6429/ECMA-48's "SGR" at all.
Nor do we care, if terminfo can get us valid and effectual information.

> -  while ((c = getopt_long(argc, argv, "bBcdfF:hiI:oruUv", long_options, 
> NULL))
> +  while ((c = getopt_long(argc, argv, "bBcdfF:hiI:orT:uUv", long_options, 
> NULL))
[...]
> +    case 'T':
> +      terminal_name = optarg;
> +      break;
[...]
> -"usage: %s [-bBcdfhioruU] [-F font-directory] [file ...]\n"
> +"usage: %s [-bBcdfhioruU] [-F font-directory] [-T term] [file ...]\n"

Not needed if we don't add `-T` at all.

Whew!  Plenty of food for thought here.  Thanks for undertaking this!

Regards,
Branden

signature.asc
Description: PGP signature