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

[Lennart Jablonka]

Quoth G. Branden Robinson:
> At 2023-08-21T21:40:18+0000, Lennart Jablonka wrote:
> > This has nothing at all to do with making it easier to customize the
> > look of man pages.
> >
> > Hyperlinks are still marked up using OSC 8, hoping that whatever
> > terminal sits in front of the user interprets or ignores those.
> > Terminfo naturally doesn’t have a capability for hyperlinks.
> >
> > Autoconf is not informed about the dependence on Curses; we simply link
> > with -lcurses, as that is what X/Open Curses specifies.
>
> I think I will want to stress here that only the terminfo bits are used.

Sure.   Though then, too, that per X/Open Curses, there is no 
library but -l curses.

> >    automatically used by 'preconv'.  Discovery of the 'uchardet' library
> >    requires the 'pkg-config' program to be installed on your system, as
> >    well as the library's C header files--on a package-based host system,
> >   diff --git a/NEWS b/NEWS
> >   index 98be590dd..87990f487 100644
> >   --- a/NEWS
> >   +++ b/NEWS
> >   @@ -94,7 +94,8 @@ grotty
> >    ------
> >
> >    o The terminfo library is now used for applying text attributes (like
> >   -  italic, bold, and colors).
> >   +  italic, bold, and colors).  You now need Curses to run grotty.  The
> >   +  legacy overstriking output format remains.
>
> I'll recast this, but yes.
>
> I feel sure we're going to need that last sentence to avoid a panic.
>
> I'm a little uneasy with the sequencing this change imposes on
> bootstrapping scenarios.  It surely was nice to not have a terminfo
> dependency.  (Yes, even though it's my idea to add one.)  On the other
> hands:
>
> * Every part of groff you need to render a man page except the macro
>  package itself is in C++, though I suppose that doesn't mitigate much
>  with modern, freely-licensed compilers whose C and C++ support are in
>  parity.
>
> * I hate to screw embedded systems developers who have enough headaches
>  as it is.  Even if they never _ship_ man pages in what they flash to
>  ROM, and few will try to render man pages inside the embedded dev
>  environment, a PDP-11 is embedded by today's standards, and Bell Labs
>  has their man pages even in that cramped situation.
>
> * You probably _should_ be bringing up a terminal abstraction library in
>  early days.  Who knows what kind of wack emulator you'll be running
>  over the serial port?  You save yourself grief by getting terminfo
>  going early.
>
> * In desperate straits, even without terminfo, you can likely render
>  enough of a man page to get by with "groff -a".

or use cat pages

> >   @@ -140,8 +136,8 @@ Output is written to the standard output stream.
> >    By default,
> >    .I grotty
> >    uses
> >   -.MR terminfo 5
> >   -to change text attributes
> >   +.MR terminfo 3
> >   +capabilities to change text attributes
>
> I managed to speak with marbles in my mouth in my previous review.  This
> terminfo(3) reference does, in fact, work on my system.  You wouldn't
> guess it from the sources--the _real_ sources, in Thomas Dickey's
> repository (equivalently, ncurses-snapshots GitHub repo).  The `TH` line
> gets rewritten from the horrendous "curs_terminfo" as part of
> build/installation.  And I was _not_ trying to suggest that we `MR` to
> "putp(3)" except when referring to that specific function.
>
> So, yeah, sometimes it is best to ignore me.  :-|

Ehh … so should I change it back to terminfo(5)?

> >    .\" ====================================================================
> >   -.SS "SGR and OSC support in pagers"
> >   +.SS "Control character support in pagers"
> >    .\" ====================================================================
>
> I think I may tune this to something like "Interaction with pagers".

ack

> >   diff --git a/src/devices/grotty/tty.cpp b/src/devices/grotty/tty.cpp
> >   index 9df35d8cd..bbca37ee3 100644
> >   --- a/src/devices/grotty/tty.cpp
> >   +++ b/src/devices/grotty/tty.cpp
> >   @@ -21,6 +21,7 @@ along with this program.  If not, see 
> > <http://www.gnu.org/licenses/>. */
> >    #include "device.h"
> >    #include "ptable.h"
> >
> >   +#include <curses.h>
>
> Are you sure?
>
> Looking at:
>
> https://pubs.opengroup.org/onlinepubs/7908799/xcurses/putp.html
> https://pubs.opengroup.org/onlinepubs/7908799/xcurses/setupterm.html
> https://pubs.opengroup.org/onlinepubs/7908799/xcurses/tparm.html
>
> I see no suggestion that we need to #include the curses header if we're
> programming to the _standard_ API.  Does your environment require this?

You are almost correct.   The OK and ERR constants are defined in 
<curses.h> only.   Don’t worry, I’m not doing anything 
non-standard I don’t think.

> >      schar color_to_idx(color *);
> >      void add_char(output_character, int, int, int, color *, color *,
> >                 unsigned char);
> >   @@ -204,6 +191,7 @@ public:
> >      void set_char(glyph *, font *, const environment *, int, const char *);
> >      void draw(int, int *, int, const environment *);
> >      void special(char *, const environment *, char);
> >   +  void change_color(const environment * const);
>
> We might call this "change_stroke_color".  I tend to dislike
> nomenclatures where an unmodified noun is coupled with a modified one;
> it's harder to infer what the scope/meaning of the unmodified noun is.

not in scope for this patch

> >   -/* 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. */
> >   -void tty_printer::update_attributes(bool underline, bool bold,
> >   -                                 schar fore_idx, schar back_idx,
> >   -                                 output_character c, int w)
> >   +// Both ovrestrike and update_attributes need be called for text
oops, “ovrestrike”
> >   +// attributes; only one of them will take effect.  Call overstrike for
> >   +// every glyph.  The update_attributes function, too, can be called for
> >   +// every glyph, as it's a noop for the same attributes.
>
> I'll need to study the code more to make full sense of this comment.

Indeed;  reading it again, it’s worded badly.

Because overstriking happens for every single glyph, call 
overstrike for every glyph you want overstruck.

Because terminfo-set text attributes stay in effect until changed, 
you need not call update_attributes for every new glyph.   If it 
makes things simpler for you, the caller, you may do so, though:  
If called with the same arguments as the last time, 
update_attributes does not print anything.

> >   +  if (curr_fore_idx != fore_idx) {
> >   +    putp(tparm(set_a_foreground, fore_idx, 0, 0, 0, 0, 0, 0, 0, 0));
> >   +    curr_fore_idx = fore_idx;
> >   +  }
> >   +
> >   +  if (curr_back_idx != back_idx) {
> >   +    putp(tparm(set_a_background, back_idx, 0, 0, 0, 0, 0, 0, 0, 0));
> >   +    curr_back_idx = back_idx;
>
> Good grief.  Terminfo sees your 6-argument monstrosity and raises you.
> What were people thinking?  It's SO MUCH CHEAPER to pass 9 longs on the
> stack than a pointer to a struct (which a good compiler will find a
> register for anyway).
>
> Terminfo _was_ an improvement on termcap, but stuff like this reminds me
> to moderate my praise.

If I understand it correctly:  The parameters were pushed right to 
left.   If you gave tparm merely four arguments, only those four 
were pushed.   tparm would then only read those arguments it 
needed (as told by the capability string), not caring about 
whether more arguments to the right of those four were passed—they 
would be lower in the stack, after all.

In that way, tparm behaves as a variadic function in K&R C.   Now, 
it doesn’t.   We have a replacement, tiparm;  alas, my OpenBSD box 
doesn’t have that yet.

        char *tiparm(const char *, ...);

You haven’t seen it (outside ncurses docs, perhaps) because it was 
introduced in X/Open Curses Issue 7, which doesn’t seem to be 
available as “onlinepub.”

> >   +  bold_underline_mode = BOLD_MODE|UNDERLINE_MODE;
> >   +  do_bold = true;
> >   +  do_underline = true;
> >   +
> >   +  int err;
> >   +  if (setupterm(NULL, 1, &err) == ERR)
> >   +    switch (err) {
> >   +      // It's a little sad that we can't use Curses's own error
> >   +      // strings, just to be able to handle hardcopy terminals,
> >   +      // only because ncurses behaves in a non-standard manner when
> >   +      // stumbling upon the hc capability.
> >   +    case -1:
> >   +      fatal("terminfo database not found");
> >   +    case 0:
> >   +      fatal("terminal description not found");
> >   +    case 1: // hardcopy terminal (non standard) / success (standard)
> >   +      // We check for over_strike anyway.  Ncurses is nice enough
> >   +      // to load the capabilities anyway.
> >   +      ;
>
> I would explicitly comment that we're doing nothing here, on purpose.

ack

> >   +  if (tab_width == -2)
> >   +    fatal("bad it (init_tabs) capability");
> >   +
> >   +  tab_width = init_tabs != -1 ? init_tabs : 8;
> >   +
> >   +  if (over_strike == -1)
> >   +    fatal("bad os (over_strike) capability");
>
> I'll cover these diagnostics with my own scent.

sure

> >   +  } string_caps[] = {
> >   +    // all the string capabilities we use in the program
> >   +    {enter_bold_mode, "can't make text bold", "bad bold capability"},
> >   +    {enter_italics_or_the_like_mode, "can't make text italic (or the like)",
> >   +      "bad italics (or the like) capability"},
> >   +    {exit_attribute_mode, "can't disable text attributes",
> >   +      "bad sgr0 (exit attributes) capability"},
> >   +    {set_a_foreground, "can't colorize text",
> >   +      "bad setaf (foreground color) capability"},
> >   +    {set_a_background, "can't colorize text",
> >   +      "bad setab (background color) capability"},
> >   +  };
>
> Similar.
>
> Similar regarding scent.  Also, FWIW, the "a" means "ANSI"; it's an
> explicit reference to ANSI X3.4, later superseded by ECMA-48 and ISO
> 6429.

Yes, setaf is described “Set foreground color to #1 using ANSI 
escape.”

> >   +  for (int i = 0; i < sizeof string_caps / sizeof *string_caps; ++i) {
>
> Sound the klaxon!  :P  I'll port this to our new friend array_length().

sure

> I'm happy with this and will start merging.  Any tweaks I make will be
> in a subsequent commit, unless merging it as-is breaks the build/tests,
> in which case I'll let you know.

Well, it seems a little more debugging is in store.