At 2023-08-30T20:54:09+0000, Lennart Jablonka wrote: > > Subject: [PATCH v2] [grotty]: Use terminfo. > > Branden: ENQ
Indeed! 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. > The suggested option to less is changed to -r, which enables passing > through single control characters, because some terminal descriptions > use those; I noticed that tmux’s sgr0 contains ^O. > > I’m too lazy to find another name for lines_. > --- > v1 → v2: > > - hardcoded escape sequences are eliminated, except for OSC 8 I asked Thomas Dickey if he meant that "Hls" as used by tmux(1) is an example of user_caps(5), but it seems I'm expected to use my own fishing pole here.[1] We can deal with that later, in any event. > - variables are renamed appropriately > - update_attributes is split in two; the new update_attributes’s > parameter list is not gigantic, I hope > - errors to do with setupterm and the capabilities are handled > properly > > and more as seen in the interdiff against v1: > > diff --git a/INSTALL.extra b/INSTALL.extra > index 3527b1929..5ef903c77 100644 > --- a/INSTALL.extra > +++ b/INSTALL.extra > @@ -122,7 +122,7 @@ 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 Thanks. > 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". > diff --git a/src/devices/grotty/grotty.1.man > b/src/devices/grotty/grotty.1.man > index 43d9d7ffd..6fbd70612 100644 > --- a/src/devices/grotty/grotty.1.man > +++ b/src/devices/grotty/grotty.1.man > @@ -56,8 +56,6 @@ output driver for typewriter-like (terminal) devices > .RB [ \-i \||\| \-r ] > .RB [ \-F\~\c > .IR font-directory ] > -.RB [ \-T\~\c > -.IR term ] > .RI [ file\~ .\|.\|.] > .YS > . > @@ -66,8 +64,6 @@ output driver for typewriter-like (terminal) devices > .RB [ \-bBdfhouU ] > .RB [ \-F\~\c > .IR font-directory ] > -.RB [ \-T\~\c > -.IR term ] > .RI [ file\~ .\|.\|.] > .YS Ah, simplicity. > @@ -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. :-| > -If no > -.I TERM > -environment variable is set > -or terminfo encounters a different error, > -.I grotty > -emits hardcoded SGR escape sequences > -(from ISO\~6429, > -popularly called \[lq]ANSI escapes\[rq]). > -. [...] > . > +If the terminal description provides no way of changing the text > +attributes but does declare that overstriking characters is possible, > +.I grotty > +will use the legacy output format. Good deal. > .\" ==================================================================== > -.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". > +.\" TODO: using less -r means putting untrusted input on the terminal; > +.\" ponder this I'll think about what advice I can offer here. Some terminal types will require "less -r", others--the better ones--will do fine with "less -R". > -SGR escape sequences are not emitted; > +Terminfo capability strings are not emitted; [...] > -SGR and OSC escape sequences are not emitted. > +Terminfo capability strings and OSC escape sequences are not emitted. Feels good to climb Mount Abstraction. > -Tabs are assumed to be set every 8 columns. > +Unless the terminal description says otherwise, > +tabs are assumed to be set every 8 columns. Good catch. > -with the SGR attribute for italic text > +with the terminfo capability for italic text [...] > @@ -521,6 +516,8 @@ Ignored if > .B \-c > is also specified. > . > +Disables the overstriking fallback for hardcopy terminals. > +. > . [...] > -with the SGR attribute for reverse video text > +with the terminfo capability for reverse video text > rather than underlined text. These are all good. > -.BI \-T\~ term > -Read description for terminal type > -.I term > -and fail if > -.I terminfo > -encounters and 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. > -. > -. > -.TP Resimplification, continued. > .TP > .I TERM > -The terminal type for > -.I terminfo. > +The terminal type interpreted by the > +.MR terminfo 3 > +library. Yup. Not our problem. > 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? > #include <term.h> > > typedef signed char schar; > @@ -49,15 +50,12 @@ static bool want_emboldening_by_overstriking = true; > static bool do_bold; > static bool want_italics_by_underlining = true; > static bool do_underline; > -static bool want_glyph_composition_by_overstriking = true; > +static bool accept_glyph_composition_by_overstriking = true; > +static bool do_glyph_composition_by_overstriking; > static bool allow_drawing_commands = true; > -static bool want_sgr_italics = false; > -static bool do_sgr_italics; > +static bool want_real_italics = false; > 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; Ok. > #ifndef IS_EBCDIC_HOST > -#define CSI "\033[" > #define OSC8 "\033]8" > #define ST "\033\\" > #else > -#define CSI "\047[" > #define OSC8 "\047]8" > #define ST "\047\\" > #endif > > -// SGR handling (ISO 6429) > -// bold and reverse can be unset separately using ISO 6429, but not > -// with terminfo > -static const char *sgr_bold = CSI "1m"; > -static const char *sgr_italic = CSI "3m"; > -static const char *sgr_underline = CSI "4m"; > -static const char *sgr_reverse = CSI "7m"; > -// 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 > -static const char *sgr_exit_attributes = CSI "0m"; > +static char *enter_italics_or_the_like_mode; That's an awkward name, but it's an awkward problem. > - void update_attributes(bool, bool, schar, schar, output_character, int); > + void overstrike(bool, bool, output_character, int); > + void update_attributes(bool, bool, schar, schar); Okay, my wails soften in intensity with 4 arguments versus 6. > 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. > -/* 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 > +// 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. > + > +void tty_printer::overstrike(bool underline, bool bold, > + output_character c, int w) > { > - if (use_overstriking_drawing_scheme) { > - 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'); > - } > + if (!use_overstriking_drawing_scheme) > + return; > + > + if (underline) { > + if (!w) > + warning("can't underline zero-width character"); > + else { > + putchar('_'); > + putchar('\b'); > } > } > - else { > - 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) > { > - putp(sgr_exit_attributes); > - is_underlining = is_boldfacing = false; > - curr_fore_idx = curr_back_idx = DEFAULT_COLOR_IDX; > + if (bold) { > + if (!w) > + warning("can't print zero-width character in bold"); > + else { > + put_char(c); > + putchar('\b'); > } > + } > +} > > - if (!is_underlining && underline) { > - if (do_sgr_italics) > - putp(sgr_italic); > - else if (do_reverse_video) > - putp(sgr_reverse); > - else > - putp(sgr_underline); > - is_underlining = true; > - } > +void tty_printer::update_attributes(bool underline, bool bold, > + schar fore_idx, schar back_idx) > +{ > + if (use_overstriking_drawing_scheme) > + return; > > - if (!is_boldfacing && bold) { > - putp(sgr_bold); > - is_boldfacing = true; > - } > + 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) { > + putp(exit_attribute_mode); > + is_underlining = is_boldfacing = false; > + curr_fore_idx = curr_back_idx = DEFAULT_COLOR_IDX; > + } > > - if (curr_fore_idx != fore_idx) { > - if (use_terminfo) > - putp(tparm(set_a_foreground, fore_idx, 0, 0, 0, 0, 0, 0, 0, 0)); > - else { > - fputs(CSI, stdout); > - putchar('3'); > - putchar(fore_idx + '0'); > - putchar('m'); > - } > - curr_fore_idx = fore_idx; > - } > + if (!is_underlining && underline) { > + putp(enter_italics_or_the_like_mode); > + is_underlining = true; > + } > > - if (curr_back_idx != back_idx) { > - if (use_terminfo) > - putp(tparm(set_a_background, back_idx, 0, 0, 0, 0, 0, 0, 0, 0)); > - else { > - fputs(CSI, stdout); > - putchar('4'); > - putchar(back_idx + '0'); > - putchar('m'); > - } > - curr_back_idx = back_idx; > - } > + if (!is_boldfacing && bold) { > + putp(enter_bold_mode); > + is_boldfacing = true; > + } > + > + 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. > @@ -496,8 +472,6 @@ 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? > void tty_printer::special_link(const char *arg, const environment *env) > { > static bool is_link_active = false; > @@ -562,6 +536,11 @@ void tty_printer::special_link(const char *arg, const > environment *env) > simple_add_char(*s, env); > } > > +void tty_printer::change_color(const environment * const env) > +{ > + add_char(0, 0, env->hpos, env->vpos, env->col, env->fill, COLOR_CHANGE); > +} > + All righty. > void tty_printer::change_fill_color(const environment * const env) > { > add_char(0, 0, env->hpos, env->vpos, env->col, env->fill, COLOR_CHANGE); > @@ -798,7 +777,7 @@ void tty_printer::end_page(int page_length) > nextp->code = p->code; > continue; > } > - if (!want_glyph_composition_by_overstriking) > + if (!do_glyph_composition_by_overstriking) > continue; > } > if (hpos > p->hpos) { > @@ -807,7 +786,9 @@ void tty_printer::end_page(int page_length) > hpos--; > } while (hpos > p->hpos); > } > - else { > + else if (p->hpos > hpos) { > + update_attributes(is_continuously_underlining, is_boldfacing, > + curr_fore_idx, curr_back_idx); > if (want_horizontal_tabs) { > for (;;) { > int next_tab_pos = ((hpos + tab_width) / tab_width) * tab_width; > @@ -816,36 +797,32 @@ void tty_printer::end_page(int page_length) > // 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 > + // cells by less. We could pass overstrike the width > // the actual tab will have and let it emit multiple > // underlines. > - update_attributes(is_continuously_underlining, is_boldfacing, > - curr_fore_idx, curr_back_idx, '\t', p->w); > + overstrike(is_continuously_underlining, false, '\t', p->w); > putchar('\t'); > hpos = next_tab_pos; > } > } > for (; hpos < p->hpos; hpos++) { > - update_attributes(is_continuously_underlining, is_boldfacing, > - curr_fore_idx, curr_back_idx, ' ', p->w); > + overstrike(is_continuously_underlining, false, ' ', p->w); > putchar(' '); > } > } > assert(hpos == p->hpos); > if (p->mode & COLOR_CHANGE) { > - // 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); > + update_attributes(is_underlining, is_boldfacing, > + p->fore_color_idx, p->back_color_idx); > continue; > } > + overstrike(p->mode & UNDERLINE_MODE, p->mode & BOLD_MODE, p->code, > p->w); > update_attributes(p->mode & UNDERLINE_MODE, p->mode & BOLD_MODE, > - p->fore_color_idx, p->back_color_idx, p->code, p->w); > + p->fore_color_idx, p->back_color_idx); > put_char(p->code); > hpos += p->w / font::hor; > } > - update_attributes(false, false, DEFAULT_COLOR_IDX, DEFAULT_COLOR_IDX, > 0, 0); > + update_attributes(false, false, DEFAULT_COLOR_IDX, DEFAULT_COLOR_IDX); > putchar('\n'); > } > if (want_form_feeds) { > @@ -871,34 +848,82 @@ printer *make_printer() > static void update_options() > { > if (use_overstriking_drawing_scheme) { > - do_sgr_italics = false; > - do_reverse_video = false; > bold_underline_mode = bold_underline_mode_option; > do_bold = want_emboldening_by_overstriking; > do_underline = want_italics_by_underlining; > + do_glyph_composition_by_overstriking = > + accept_glyph_composition_by_overstriking; > + return; > } > - else { > - // TODO(humm): Do better error handling. > - int err; > - setupterm(terminal_name, 1, terminal_name != NULL ? NULL : &err); > - 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? > - 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; > + > + 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. > } > - do_sgr_italics = want_sgr_italics; > - do_reverse_video = want_reverse_video_for_italics; > - bold_underline_mode = BOLD_MODE|UNDERLINE_MODE; > - do_bold = true; > - do_underline = true; > + > + if (want_real_italics) > + enter_italics_or_the_like_mode = enter_italics_mode; > + else if (want_reverse_video_for_italics) > + enter_italics_or_the_like_mode = enter_reverse_mode; > + else > + enter_italics_or_the_like_mode = enter_underline_mode; > + > + 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. > + > + do_glyph_composition_by_overstriking = > + accept_glyph_composition_by_overstriking && over_strike; > + > + static const struct { > + char *cap; > + char const *nullmsg; > + char const *negonemsg; Oh. "neg(ative) one message". This took a bit of decoding. > + } 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. > + 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(). > + if (string_caps[i].cap == NULL) { > + if (over_strike && !want_real_italics && > !want_reverse_video_for_italics) { > + use_overstriking_drawing_scheme = true; > + update_options(); > + return; > + } > + else > + fatal(string_caps[i].nullmsg); > + } > + else if (string_caps[i].cap == (char *)-1) > + fatal(string_caps[i].negonemsg); > } > } > > @@ -916,7 +941,7 @@ int main(int argc, char **argv) > { "version", no_argument, 0, 'v' }, > { NULL, 0, 0, 0 } > }; > - while ((c = getopt_long(argc, argv, "bBcdfF:hiI:orT:uUv", long_options, > NULL)) > + while ((c = getopt_long(argc, argv, "bBcdfF:hiI:oruUv", long_options, > NULL)) > != EOF) > switch(c) { > case 'v': > @@ -925,7 +950,7 @@ int main(int argc, char **argv) > break; > case 'i': > // Use italic font instead of underlining. > - want_sgr_italics = true; > + want_real_italics = true; > break; > case 'I': > // ignore include search path > @@ -944,7 +969,7 @@ int main(int argc, char **argv) > break; > case 'o': > // Do not overstrike (other than emboldening and underlining). > - want_glyph_composition_by_overstriking = false; > + accept_glyph_composition_by_overstriking = false; > break; > case 'r': > // Use reverse mode instead of underlining. > @@ -972,9 +997,6 @@ int main(int argc, char **argv) > // Ignore \D commands. > allow_drawing_commands = false; > break; > - case 'T': > - terminal_name = optarg; > - break; > case CHAR_MAX + 1: // --help > usage(stdout); > break; > @@ -998,7 +1020,7 @@ int main(int argc, char **argv) > static void usage(FILE *stream) > { > fprintf(stream, > -"usage: %s [-bBcdfhioruU] [-F font-directory] [-T term] [file ...]\n" > +"usage: %s [-bBcdfhioruU] [-F font-directory] [file ...]\n" > "usage: %s {-v | --version}\n" > "usage: %s --help\n", > program_name, program_name, program_name); > > INSTALL.extra | 3 + > NEWS | 10 + > src/devices/grotty/grotty.1.man | 40 ++-- > src/devices/grotty/grotty.am | 3 +- > src/devices/grotty/tty.cpp | 333 ++++++++++++++++---------------- > 5 files changed, 209 insertions(+), 180 deletions(-) The rest of that looks fine; a return to the status quo getopt()-wise. 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. ACK. Regards, Branden [1] https://lists.gnu.org/archive/html/bug-ncurses/2023-08/msg00020.html
signature.asc
Description: PGP signature
