Date:   Wed, 26 Jan 2022 15:04:36 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     nick black <dankamongmen@...il.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        linux-kernel@...r.kernel.org,
        Thomas Dickey <dickey@...isible-island.net>
Subject: Re: [PATCH v2] console: answer OSC 10 and 11

On Mon, Jan 24, 2022 at 12:49:26AM -0500, nick black wrote:
> XTerm and many other terminal emulators implement
> Operating System Commands 10 and 11, allowing the
> default foreground/background to be set and queried.
> Extend the VT control sequence parser to recognize
> and support these queries.

Nit, can you use the full 72 columns here?

> The VT already implements two OSCs, for changing
> and resetting the palette. In doing so (many years
> ago), it broke from the ANSI standard, and did not
> require an ST terminator. Read all about it in
> xterm(1) (see "brokenLinuxOSC"). I have followed this
> grand tradition, and similarly not required ST.
> Note that ST can still be safely sent by a client
> program, as the VT consumes it. Indeed, my Notcurses
> library does exactly this.
> 
> "Don't VTs always have black backgrounds?" Nope, you
> can change the default background color with any
> number of ANSI sequences, and then use the VT's
> Private CSI "ESC [ 8 ]" to make the current color pair
> the default attributes. Try it at home with, say:
> 
>   printf %b '\e[46m' '\e[8]' '\e[H\e[J'
> 
> The response follows XTerm's effective lead, using
> %02x/%02x/%02x to format the RGB value, rather than
> the %02x%02x%02x preferred by the preexisting
> P (set palette) OSC. This was done to simplify
> client libraries. Note that the spirit of the law,
> that the reply is a "control sequence of the same
> form which can be used to set", cannot be easily
> honored given the semantics of the Linux private CSI
> sequence. So it goes.
> 
> As a result, notcurses-info now properly detects the
> default colors dynamically. Where it used to say:
> 
>  no known default fg no known default bg
> 
> It now says on boot:
> 
>  notcurses 3.0.4 on Linux 5.16.0nlb VT
>  ...
>  default fg 0xaaaaaa default bg 0x000000
> 
> and after a change like that above:
> 
>  notcurses 3.0.4 on Linux 5.16.0nlb VT
>  ...
>  default fg 0xaaaaaa default bg 0xaa5500
> 
> This is necessary to produce readable multicolor text
> while respecting the user's background choice.
> 
> Signed-off-by: nick black <dankamongmen@...il.com>
> ---
> Changes in v2:
>  - Reverse in-kernel BGR to RGB
> 
>  drivers/tty/vt/vt.c            | 67 ++++++++++++++++++++++++++++------
>  include/linux/console_struct.h |  1 +
>  2 files changed, 58 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index f8c87c4d7399..a10629bcaaa1 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -1878,6 +1878,31 @@ int mouse_reporting(void)
>  	return vc_cons[fg_console].d->vc_report_mouse;
>  }
>  
> +/* handle the OSC query specified by vc->vc_oscmd. we currently handle only 10
> + * and 11 (default foreground and default background, respectively).
> + */
> +static void handle_osc_query(struct tty_struct *tty, const struct vc_data *vc)
> +{
> +	char buf[20];
> +	int len, idx;
> +	/* response payload conforms to XTerm: %02x/%02x/%02x for R, G, and B. */

Nit, new line needed after variable list.



> +	switch (vc->vc_oscmd) {
> +	case 10: /* get default foreground */
> +		idx = vc->vc_def_color & 0x0f;
> +		break;
> +	case 11: /* get default background */
> +		idx = (vc->vc_def_color & 0xf0) >> 4;
> +		break;

Does 10 and 11 have a #define or enum anywhere?

> +	default:
> +		return;

No need to return an error?  Why not?

> +	}
> +	/* transpose internal BGR to RGB on output */
> +	len = snprintf(buf, sizeof(buf), "\x1b]%d;rgb:%02x/%02x/%02x\x1b\\",
> +		vc->vc_oscmd, vc->vc_palette[idx * 3 + 2],
> +		vc->vc_palette[idx * 3 + 1], vc->vc_palette[idx * 3]);
> +	respond_string(buf, len, tty->port);
> +}
> +
>  /* console_lock is held */
>  static void set_mode(struct vc_data *vc, int on_off)
>  {
> @@ -2075,8 +2100,8 @@ static void restore_cur(struct vc_data *vc)
>  }
>  
>  enum { ESnormal, ESesc, ESsquare, ESgetpars, ESfunckey,
> -	EShash, ESsetG0, ESsetG1, ESpercent, EScsiignore, ESnonstd,
> -	ESpalette, ESosc, ESapc, ESpm, ESdcs };
> +	EShash, ESsetG0, ESsetG1, ESpercent, EScsiignore,
> +	ESpalette, ESosc, ESoscmd, ESoscparam, ESapc, ESpm, ESdcs };
>  
>  /* console_lock is held (except via vc_init()) */
>  static void reset_terminal(struct vc_data *vc, int do_clear)
> @@ -2230,7 +2255,7 @@ static void do_con_trol(struct tty_struct *tty, struct vc_data *vc, int c)
>  			vc->vc_state = ESsquare;
>  			return;
>  		case ']':
> -			vc->vc_state = ESnonstd;
> +			vc->vc_state = ESosc;
>  			return;
>  		case '_':
>  			vc->vc_state = ESapc;
> @@ -2287,7 +2312,10 @@ static void do_con_trol(struct tty_struct *tty, struct vc_data *vc, int c)
>  			return;
>  		}
>  		return;
> -	case ESnonstd:
> +	case ESosc:
> +		/* Operating System Commands are traditionally terminated with an ST
> +		 * or a BEL, but Linux historically eschews said terminators.
> +		 */
>  		if (c=='P') {   /* palette escape sequence */
>  			for (vc->vc_npar = 0; vc->vc_npar < NPAR; vc->vc_npar++)
>  				vc->vc_par[vc->vc_npar] = 0;
> @@ -2297,9 +2325,10 @@ static void do_con_trol(struct tty_struct *tty, struct vc_data *vc, int c)
>  		} else if (c=='R') {   /* reset palette */
>  			reset_palette(vc);
>  			vc->vc_state = ESnormal;
> -		} else if (c>='0' && c<='9')
> -			vc->vc_state = ESosc;
> -		else
> +		} else if (isdigit(c)) {
> +			vc->vc_oscmd = c - '0';
> +			vc->vc_state = ESoscmd;

We used to jump to ESosc here, what happened to that if the command is
invalid?

> +		} else
>  			vc->vc_state = ESnormal;
>  		return;
>  	case ESpalette:
> @@ -2348,7 +2377,7 @@ static void do_con_trol(struct tty_struct *tty, struct vc_data *vc, int c)
>  		if (c == ';' && vc->vc_npar < NPAR - 1) {
>  			vc->vc_npar++;
>  			return;
> -		} else if (c>='0' && c<='9') {
> +		} else if (isdigit(c)) {

Different change, you can put this in a different patch.

thanks,

greg k-h

Powered by blists - more mailing lists