You should strictly only discard the following two arguments or stuff like
SGR 38;5;100;1 won't work.
This is why some people prefer the : form but the ship has rather sailed on
that.
On Fri, 6 Jan 2023, 11:52 Crystal Kolipe, <kolip...@exoticsilicon.com>
wrote:
> This version of the patchset updates a few things since the one posted
> early
> on Wednesday:
>
> NEW - Fix two off-by-ones that caused spurious trailing null characters
> when
> requesting the terminal IDs. (Noticed by Paul running tmux.)
>
> NEW - Discard any parameters passed to a CSI 38 m or CSI 48 m sequence.
> See below for more discussion on this.
>
> NEW - Tweaked dim attribute to work correctly on byte-swapped framebuffers.
>
> * Control sequences for dim text, invisible text, strike through, italic,
> and
> double underline attributes are now recognised and set flags in wscons.
> * Rendering of dim, invisible, struck, and double underlined text is now
> supported in rasops32.c, so users of 32bpp displays will see these text
> effects on the console.
> * The default number of scrollback screens has been increased from five to
> twenty.
> * F1 - F4 and F13 - F24 now send different sequences.
> * With numlock OFF, keypad 5 is now 'begin' rather than unused.
> * Home, End, keypad home, and keypad end now send different sequences.
> * A new keysym has been added - KS_Backtab.
> * Shift-TAB is now defined as KS_Backtab and sends ESC [ Z.
> * Shift-F1 - Shift-F12 now send F13 - F24.
> * Five new escape sequences added for cursor movement.
>
> Off by ones:
>
> These can actually be triggered with the existing console code, without
> even
> applying any of my patches, and stem from using sizeof with a string
> constant,
> which returns the length of the string _including_ the terminating null.
>
> CSI 38 m and CSI 48 m sequences:
>
> The 30-37 codes set one of eight fixed foreground colours, the 40-47 codes
> set
> the background in the same way. Codes 39 and 49 reset to the default
> colours.
>
> Codes 38 and 48 have become used as an escape to a larger colour space,
> typically 256 colours or 24-bit colour.
>
> A typical invocation to select colour 100 from a 256 colour palette might
> be:
>
> CSI 38;5;100m
>
> Such sequences are not defined in the terminfo entry for xterm. However,
> some
> programs use them, assuming that they are supported.
>
> The current wscons code ignores the unimplemented 38, but then proceeds to
> interpret 5 and 100 as further SGR codes.
>
> In many cases, this doesn't much of a problem, because although 5 enables
> blink, none of the rasops code supports blink anyway. However, the last
> value
> can potentially set any SGR attribute. So, for example:
>
> CSI 38;5;4m
>
> ...will not select colour 4 as the program is expecting, but instead turn
> on
> underlining.
>
> To avoid this, the lastest version of my patch simply discards all values
> after a 38 or 48, until the trailing 'm'.
>
> This works to avoid setting unwanted attributes, until such time as 256
> colour
> support is added, (if this is even desired).
>
> --- dev/wscons/wsemul_vt100_keys.c.dist Sat Mar 14 00:38:50 2015
> +++ dev/wscons/wsemul_vt100_keys.c Mon Jan 2 16:01:42 2023
> @@ -37,11 +37,9 @@
> #include <dev/wscons/wsemulvar.h>
> #include <dev/wscons/wsemul_vt100var.h>
>
> +#define vt100_fkeys_len(x) (5+(x>=8)+(x>=12))
> +
> static const u_char *vt100_fkeys[] = {
> - "\033[11~", /* F1 */
> - "\033[12~",
> - "\033[13~", /* F1-F5 normally don't send codes */
> - "\033[14~",
> "\033[15~", /* F5 */
> "\033[17~", /* F6 */
> "\033[18~",
> @@ -50,18 +48,18 @@
> "\033[21~",
> "\033[23~", /* VT100: ESC */
> "\033[24~", /* VT100: BS */
> - "\033[25~", /* VT100: LF */
> - "\033[26~",
> - "\033[28~", /* help */
> - "\033[29~", /* do */
> - "\033[31~",
> - "\033[32~",
> - "\033[33~",
> - "\033[34~", /* F20 */
> - "\033[35~",
> - "\033[36~",
> - "\033[37~",
> - "\033[38~"
> + "\033[1;2P", /* VT100: LF */
> + "\033[1;2Q",
> + "\033[1;2R", /* help */
> + "\033[1;2S", /* do */
> + "\033[15;2~",
> + "\033[17;2~",
> + "\033[18;2~",
> + "\033[19;2~", /* F20 */
> + "\033[20;2~",
> + "\033[21;2~",
> + "\033[23;2~",
> + "\033[24;2~"
> };
>
> static const u_char *vt100_pfkeys[] = {
> @@ -96,14 +94,22 @@
> edp->translatebuf, edp->flags & VTFL_UTF8));
> }
>
> - if (in >= KS_f1 && in <= KS_f24) {
> - *out = vt100_fkeys[in - KS_f1];
> - return (5);
> + if (in >= KS_f1 && in <= KS_f4) {
> + *out = vt100_pfkeys[in - KS_f1];
> + return (3);
> }
> - if (in >= KS_F1 && in <= KS_F24) {
> - *out = vt100_fkeys[in - KS_F1];
> - return (5);
> + if (in >= KS_F1 && in <= KS_F4) {
> + *out = vt100_pfkeys[in - KS_F1];
> + return (3);
> }
> + if (in >= KS_f5 && in <= KS_f24) {
> + *out = vt100_fkeys[in - KS_f5];
> + return vt100_fkeys_len(in - KS_f5);
> + }
> + if (in >= KS_F5 && in <= KS_F24) {
> + *out = vt100_fkeys[in - KS_F5];
> + return vt100_fkeys_len(in - KS_F5);
> + }
> if (in >= KS_KP_F1 && in <= KS_KP_F4) {
> *out = vt100_pfkeys[in - KS_KP_F1];
> return (3);
> @@ -148,12 +154,12 @@
> }
> switch (in) {
> case KS_Help:
> - *out = vt100_fkeys[15 - 1];
> + *out = vt100_fkeys[15 - 1 + 4]; /* vt100_fkeys starts at
> F5 */
> return (5);
> case KS_Execute: /* "Do" */
> - *out = vt100_fkeys[16 - 1];
> + *out = vt100_fkeys[16 - 1 + 4]; /* vt100_fkeys starts at
> F5 */
> return (5);
> - case KS_Find:
> + case KS_Find: /* Not defined in xterm
> terminfo */
> *out = "\033[1~";
> return (4);
> case KS_Insert:
> @@ -163,7 +169,7 @@
> case KS_KP_Delete:
> *out = "\033[3~";
> return (4);
> - case KS_Select:
> + case KS_Select: /* Not defined in xterm
> terminfo */
> *out = "\033[4~";
> return (4);
> case KS_Prior:
> @@ -174,14 +180,27 @@
> case KS_KP_Next:
> *out = "\033[6~";
> return (4);
> + case KS_Backtab:
> + *out = "\033[Z";
> + return (3);
> + /*
> + * Unlike insert, delete, page up, and page down, we purposely
> don't
> + * send the same sequence of \033OE for the non-keypad 'begin'
> key.
> + *
> + * This is because the terminfo xterm entry is mapping this to
> kb2,
> + * which is defined as 'centre of keypad'.
> + */
> + case KS_KP_Begin:
> + *out = "\033OE";
> + return (3);
> case KS_Home:
> case KS_KP_Home:
> - *out = "\033[7~";
> - return (4);
> + *out = "\033OH";
> + return (3);
> case KS_End:
> case KS_KP_End:
> - *out = "\033[8~";
> - return (4);
> + *out = "\033OF";
> + return (3);
> case KS_Up:
> case KS_KP_Up:
> if (edp->flags & VTFL_APPLCURSOR)
> --- dev/wscons/wsemul_vt100_subr.c.dist Mon May 25 06:55:49 2020
> +++ dev/wscons/wsemul_vt100_subr.c Thu Jan 5 12:29:31 2023
> @@ -231,7 +231,7 @@
> switch (A3(edp->modif1, edp->modif2, c)) {
> case A3('>', '\0', 'c'): /* DA secondary */
> wsdisplay_emulinput(edp->cbcookie, WSEMUL_VT_ID2,
> - sizeof(WSEMUL_VT_ID2));
> + sizeof(WSEMUL_VT_ID2)-1);
> break;
>
> case A3('\0', '\0', 'J'): /* ED selective erase in display */
> @@ -461,6 +461,9 @@
> edp->ccol -= min(DEF1_ARG(0), edp->ccol);
> edp->flags &= ~VTFL_LASTCHAR;
> break;
> + case 'G': /* HPA */
> + edp->ccol = min(DEF1_ARG(0)-1, edp->ncols-1);
> + break;
> case 'H': /* CUP */
> case 'f': /* HVP */
> if (edp->flags & VTFL_DECOM)
> @@ -502,16 +505,37 @@
> WSEMULOP(rc, edp, &edp->abortstate, erasecols,
> ERASECOLS(NCOLS - n, n, edp->bkgdattr));
> break;
> + case 'S': /* INDN */
> + wsemul_vt100_scrollup (edp,DEF1_ARG(0));
> + break;
> + case 'T': /* RIN */
> + wsemul_vt100_scrolldown (edp,DEF1_ARG(0));
> + break;
> case 'X': /* ECH erase character */
> n = min(DEF1_ARG(0), COLS_LEFT + 1);
> WSEMULOP(rc, edp, &edp->abortstate, erasecols,
> ERASECOLS(edp->ccol, n, edp->bkgdattr));
> break;
> + case 'Z': /* KCBT */
> + if (!edp->ccol)
> + break;
> + if (edp->tabs) {
> + for (n=edp->ccol-1 ; n>0; n--)
> + if (edp->tabs[n])
> + break;
> + } else {
> + n=((edp->ccol - 1) & 8);
> + }
> + edp->ccol=n;
> + break;
> case 'c': /* DA primary */
> if (ARG(0) == 0)
> wsdisplay_emulinput(edp->cbcookie, WSEMUL_VT_ID1,
> - sizeof(WSEMUL_VT_ID1));
> + sizeof(WSEMUL_VT_ID1)-1);
> break;
> + case 'd': /* VPA */
> + edp->crow = min(DEF1_ARG(0)-1, edp->nrows-1);
> + break;
> case 'g': /* TBC */
> if (edp->tabs != NULL)
> switch (ARG(0)) {
> @@ -549,6 +573,12 @@
> case 1: /* bold */
> flags |= WSATTR_HILIT;
> break;
> + case 2: /* dim */
> + flags |= WSATTR_DIM;
> + break;
> + case 3: /* italic */
> + flags |= WSATTR_ITALIC;
> + break;
> case 4: /* underline */
> flags |= WSATTR_UNDERLINE;
> break;
> @@ -558,11 +588,31 @@
> case 7: /* reverse */
> flags |= WSATTR_REVERSE;
> break;
> - case 22: /* ~bold VT300 only */
> + /*
> + * Invisible text only makes the _glyph_ invisible.
> + *
> + * Other active attributes such as underlining and
> + * strikeout are still displayed in the character
> cell.
> + */
> + case 8: /* invisible */
> + flags |= WSATTR_INVISIBLE;
> + break;
> + case 9: /* strike */
> + flags |= WSATTR_STRIKE;
> + break;
> + case 21: /* double underline */
> + flags |= WSATTR_DOUBLE_UNDERLINE;
> + break;
> + case 22: /* ~bold ~dim VT300 only */
> flags &= ~WSATTR_HILIT;
> + flags &= ~WSATTR_DIM;
> break;
> + case 23: /* ~italic */
> + flags &= WSATTR_ITALIC;
> + break;
> case 24: /* ~underline VT300 only */
> flags &= ~WSATTR_UNDERLINE;
> + flags &= ~WSATTR_DOUBLE_UNDERLINE;
> break;
> case 25: /* ~blink VT300 only */
> flags &= ~WSATTR_BLINK;
> @@ -570,12 +620,36 @@
> case 27: /* ~reverse VT300 only */
> flags &= ~WSATTR_REVERSE;
> break;
> + case 28: /* ~invisible */
> + flags &= ~WSATTR_INVISIBLE;
> + break;
> + case 29: /* ~strike */
> + flags &= ~WSATTR_STRIKE;
> + break;
> case 30: case 31: case 32: case 33:
> case 34: case 35: case 36: case 37:
> /* fg color */
> flags |= WSATTR_WSCOLORS;
> fgcol = ARG(n) - 30;
> break;
> + /*
> + * Some software naively assumes that sequences
> such
> + * as CSI 38;5;Xm are available without really
> + * checking the terminal capabilities.
> + *
> + * If we don't swallow the parameters following the
> + * 38, they get incorrectly interpreted as 'CSI m'
> + * sequences on their own.
> + *
> + * So an attempt to set colour 1 ends up setting
> bold
> + * and blink, because wscons treats CSI 38;5;1m as
> + * CSI 5;1m.
> + *
> + * A similar problem exists for CSI 48m sequences.
> + */
> + case 38:
> + n=edp->nargs;
> + break;
> case 39:
> /* reset fg color */
> fgcol = WSCOL_WHITE;
> @@ -587,6 +661,9 @@
> /* bg color */
> flags |= WSATTR_WSCOLORS;
> bgcol = ARG(n) - 40;
> + break;
> + case 48:
> + n=edp->nargs;
> break;
> case 49:
> /* reset bg color */
> --- dev/wscons/wsksymdef.h.dist Mon Sep 20 14:32:39 2021
> +++ dev/wscons/wsksymdef.h Mon Jan 2 15:48:18 2023
> @@ -626,6 +626,7 @@
> #define KS_Open 0xf393
> #define KS_Paste 0xf394
> #define KS_Cut 0xf395
> +#define KS_Backtab 0xf396
>
> #define KS_Menu 0xf3c0
> #define KS_Pause 0xf3c1
> --- dev/wscons/wsdisplayvar.h.dist Sun Sep 13 07:05:46 2020
> +++ dev/wscons/wsdisplayvar.h Wed Jan 4 09:24:12 2023
> @@ -94,11 +94,16 @@
> #define WSCOL_CYAN 6
> #define WSCOL_WHITE 7
> /* flag values: */
> -#define WSATTR_REVERSE 1
> -#define WSATTR_HILIT 2
> -#define WSATTR_BLINK 4
> -#define WSATTR_UNDERLINE 8
> -#define WSATTR_WSCOLORS 16
> +#define WSATTR_REVERSE 1
> +#define WSATTR_HILIT 2
> +#define WSATTR_BLINK 4
> +#define WSATTR_UNDERLINE 8
> +#define WSATTR_WSCOLORS 16
> +#define WSATTR_DIM 32
> +#define WSATTR_STRIKE 64
> +#define WSATTR_DOUBLE_UNDERLINE 128
> +#define WSATTR_INVISIBLE 256
> +#define WSATTR_ITALIC 512
> };
>
> #define WSSCREEN_NAME_SIZE 16
> --- dev/pckbc/wskbdmap_mfii.c.dist Sat May 1 13:11:16 2021
> +++ dev/pckbc/wskbdmap_mfii.c Mon Jan 2 13:51:12 2023
> @@ -59,7 +59,7 @@
> KC(12), KS_minus, KS_underscore,
> KC(13), KS_equal, KS_plus,
> KC(14), KS_Cmd_ResetEmul, KS_Delete,
> - KC(15), KS_Tab,
> + KC(15), KS_Tab, KS_Backtab,
> KC(16), KS_q,
> KC(17), KS_w,
> KC(18), KS_e,
> @@ -103,16 +103,16 @@
> KC(56), KS_Cmd2, KS_Alt_L,
> KC(57), KS_space,
> KC(58), KS_Caps_Lock,
> - KC(59), KS_Cmd_Screen0, KS_f1,
> - KC(60), KS_Cmd_Screen1, KS_f2,
> - KC(61), KS_Cmd_Screen2, KS_f3,
> - KC(62), KS_Cmd_Screen3, KS_f4,
> - KC(63), KS_Cmd_Screen4, KS_f5,
> - KC(64), KS_Cmd_Screen5, KS_f6,
> - KC(65), KS_Cmd_Screen6, KS_f7,
> - KC(66), KS_Cmd_Screen7, KS_f8,
> - KC(67), KS_Cmd_Screen8, KS_f9,
> - KC(68), KS_Cmd_Screen9, KS_f10,
> + KC(59), KS_Cmd_Screen0, KS_f1, KS_f13,
> + KC(60), KS_Cmd_Screen1, KS_f2, KS_f14,
> + KC(61), KS_Cmd_Screen2, KS_f3, KS_f15,
> + KC(62), KS_Cmd_Screen3, KS_f4, KS_f16,
> + KC(63), KS_Cmd_Screen4, KS_f5, KS_f17,
> + KC(64), KS_Cmd_Screen5, KS_f6, KS_f18,
> + KC(65), KS_Cmd_Screen6, KS_f7, KS_f19,
> + KC(66), KS_Cmd_Screen7, KS_f8, KS_f20,
> + KC(67), KS_Cmd_Screen8, KS_f9, KS_f21,
> + KC(68), KS_Cmd_Screen9, KS_f10, KS_f22,
> KC(69), KS_Num_Lock,
> KC(70), KS_Hold_Screen,
> KC(71), KS_KP_Home, KS_KP_7,
> @@ -128,8 +128,8 @@
> KC(81), KS_KP_Next, KS_KP_3,
> KC(82), KS_KP_Insert, KS_KP_0,
> KC(83), KS_KP_Delete, KS_KP_Decimal,
> - KC(87), KS_Cmd_Screen10, KS_f11,
> - KC(88), KS_Cmd_Screen11, KS_f12,
> + KC(87), KS_Cmd_Screen10, KS_f11, KS_f23,
> + KC(88), KS_Cmd_Screen11, KS_f12, KS_f24,
> KC(91), KS_f13,
> KC(92), KS_f14,
> KC(93), KS_f15,
> --- dev/rasops/rasops.c.dist Thu Jul 23 06:17:03 2020
> +++ dev/rasops/rasops.c Thu Jan 5 19:19:50 2023
> @@ -141,7 +141,7 @@
> uint32_t rs_defattr;
>
> int rs_sbscreens;
> -#define RS_SCROLLBACK_SCREENS 5
> +#define RS_SCROLLBACK_SCREENS 20
> int rs_dispoffset; /* rs_bs index, start of our actual screen
> */
> int rs_visibleoffset; /* rs_bs index, current scrollback screen
> */
> };
> @@ -568,7 +568,19 @@
> if ((flg & WSATTR_HILIT) != 0)
> fg += 8;
>
> - flg = ((flg & WSATTR_UNDERLINE) ? 1 : 0);
> + /*
> + * The rapops code expects a different layout of the bitfields in
> flg:
> + *
> + * WSATTR_UNDERLINE is moved to bit 0
> + * Bits 1 and 2 are re-purposed to indicate whether the foreground
> and
> + * background are grey or not, (I.E. red==green==blue).
> + *
> + * This was probably a convenient hack when we only had to deal
> with
> + * underlining, but further attributes should maintain their
> original
> + * bit positions for consistency between the wscons and rasops
> code.
> + */
> +
> + flg = ((flg & 0xfff8) | (flg & WSATTR_UNDERLINE ? 1 : 0));
>
> if (rasops_isgray[fg])
> flg |= 2;
> --- dev/rasops/rasops32.c.dist Mon Jul 20 09:40:45 2020
> +++ dev/rasops/rasops32.c Fri Jan 6 08:08:09 2023
> @@ -93,11 +93,44 @@
>
> b = ri->ri_devcmap[(attr >> 16) & 0xf];
> f = ri->ri_devcmap[(attr >> 24) & 0xf];
> +
> + /*
> + * Implement dim by shifting each of the red, green and blue
> values by
> + * one bit.
> + *
> + * Since we are shifting each channel equally, this works for both
> RGB
> + * and byte-swapped BGR formats with exactly 8 bits per channel,
> + * I.E. 0xXXRRGGBB, and 0xBBGGRRXX.
> + *
> + * The unused byte is always set to 0x00 for 32bpp in
> + * rasops_init_devcmap, so will be unaffected by the shift.
> + *
> + * XXX If we ever support 32-bit devices that are not 8 bits per
> channel
> + * then this code will need to change.
> + */
> +
> + if ((attr & WSATTR_DIM)!=0) {
> + f=(f>>1) & 0x7F7F7F7F;
> + }
> +
> u.d[0][0] = b; u.d[0][1] = b;
> u.d[1][0] = b; u.d[1][1] = f;
> u.d[2][0] = f; u.d[2][1] = b;
> u.d[3][0] = f; u.d[3][1] = f;
>
> + /*
> + * Implement 'invisible' attribute by changing the character to a
> space.
> + *
> + * We need to do this rather than just ignoring the character or
> filling
> + * the bits with 0x00 and returning as a special case, because
> effects
> + * such as underline and strikethrough are still rendered on
> invisible
> + * characters, (at least they are in xterm).
> + */
> +
> + if ((attr & WSATTR_INVISIBLE)!=0) {
> + uc=' ';
> + }
> +
> if (uc == ' ') {
> while (height--) {
> /* the general, pixel-at-a-time case is fast
> enough */
> @@ -202,12 +235,44 @@
> }
> }
>
> + /* Double underline relies on normal underlining being done as
> well. */
> +
> /* Do underline a pixel at a time */
> - if ((attr & 1) != 0) {
> + if ((attr & (WSATTR_DOUBLE_UNDERLINE | 1)) != 0) {
> rp -= step;
> for (cnt = 0; cnt < width; cnt++)
> ((int *)rp)[cnt] = f;
> }
> +
> + /*
> + * Double underline now just needs to paint the second underline
> two
> + * rows up. */
> +
> + if ((attr & WSATTR_DOUBLE_UNDERLINE)!=0) {
> + rp-=(step << 1);
> + for (cnt=0; cnt< width; cnt++)
> + ((int *)rp)[cnt]=f;
> + /*
> + * Reset row pointer to ensure that strikethough appears
> at a
> + * consistent height if combined with double underlining.
> + */
> +
> + rp+=(step << 1);
> + }
> +
> + /*
> + * Reset pointer to ensure that strikethough appears at a
> consistent
> + * height if combined with single underlining.
> + */
> +
> + if ((attr & WSATTR_STRIKE)!=0) {
> + if ((attr & 1)==1) {
> + rp+=step ;
> + }
> + rp -= (1+ri->ri_font->fontheight/2)*step;
> + for (cnt=0; cnt< width; cnt++)
> + ((int *)rp)[cnt]=f;
> + }
>
> return 0;
> }
>
>
