From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 2LqyOtciAmc/EQMAWB0awg (envelope-from ) for ; Sun, 06 Oct 2024 01:40:39 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=gnu.org header.i=@gnu.org header.a=rsa-sha256 header.s=fencepost-gnu-org header.b=cJd5RJKG; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id D46801E355; Sun, 6 Oct 2024 01:40:39 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-7.8 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_BLOCKED,RCVD_IN_VALIDITY_CERTIFIED,RCVD_IN_VALIDITY_RPBL, RCVD_IN_VALIDITY_SAFE,URIBL_BLOCKED,URIBL_DBL_BLOCKED_OPENDNS autolearn=ham autolearn_force=no version=4.0.0 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 1F9641E353 for ; Sun, 6 Oct 2024 01:40:38 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id ACB0E3860769 for ; Sun, 6 Oct 2024 05:40:37 +0000 (GMT) Received: from eggs.gnu.org (eggs.gnu.org [IPv6:2001:470:142:3::10]) by sourceware.org (Postfix) with ESMTPS id 917343860740 for ; Sun, 6 Oct 2024 05:40:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 917343860740 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gnu.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gnu.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 917343860740 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2001:470:142:3::10 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728193213; cv=none; b=d4qsvxdLdeyLY7DOPY2nMkVQ2P4t02XDsW4nJQF2exvis8/qqzmWRymUly2tP2kquMp+j44blQFkNxuMyoeVxE8vexp6ahkJrW51+S38B3j2cRG4s8MQne7C+OEOtbbNMIs7zb8VYcymYpg+GRK8e9NIcSZ5BdnUUtTxkqUNlbk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728193213; c=relaxed/simple; bh=YtJS75mhJitnphHzxmZ/IJGvc7jtfyGwXKt8CyoN5ZA=; h=DKIM-Signature:Date:Message-Id:From:To:Subject; b=CUpqj4JukVjBnybfSVe1VgbEb9AGbJetMztXeksg4BicrAn3SYjfDcUC70lT6dyTpk2Dqy/xwWvI5I/YvJqjOaxYpzxUbkjH/cJxrIp2IQ76Wjk4aMDCAHIJZAQcYqpznU+f6kRmOF6t8pZ6W5yvW4vRmlZqIFHndVpie0d8CUk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1sxK01-0006lm-Mk; Sun, 06 Oct 2024 01:40:09 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=aFJmGzxQeRrn65jw/VKH6TNWKuDjuDK5eLzpRse5ArI=; b=cJd5RJKGJVWJ 48oaiJ7lQDOMCGIP2s8TiXWun+UqMWevVs7ralApjULmf1wRw5DyPdsR76HXNzkQHU4vjTzRs8rxI xXOqlvQ1Y7D5csVveaTokx3qs5cc3leAb90qY0WpEdO/f7IYq7A/JI6D2Ao2s54qkoZtlWcHdArah DqQrvcYEsLd1u0rdE4ss/MyS9SFQCO1XHk2OiO187aFsi7bsCgFSYBA1x7x6QY6287insApc5kmSi z51QHEt25NNqtU6Bocd2KdEhv65r0eLlcJSrAcHk+7W1EguuugKlTFBUr+p4STec4KdcqcYHk5S2S L1BU/V2qApSikA/fDYGaeQ==; Date: Sun, 06 Oct 2024 08:40:05 +0300 Message-Id: <86r08tvlfu.fsf@gnu.org> From: Eli Zaretskii To: Andrei Pikas Cc: tom@tromey.com, gdb-patches@sourceware.org, gdb@mail.api.win In-Reply-To: <20241005192744.976782-1-gdb@mail.api.win> (message from Andrei Pikas on Sat, 5 Oct 2024 22:27:44 +0300) Subject: Re: [PATCH v8] Add an option with a color type. References: <87r08vhhx8.fsf@tromey.com> <20241005192744.976782-1-gdb@mail.api.win> X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org > From: Andrei Pikas > Cc: gdb-patches@sourceware.org, > Andrei Pikas > Date: Sat, 5 Oct 2024 22:27:44 +0300 > > gdb/Makefile.in | 2 + > gdb/NEWS | 31 ++ > gdb/cli/cli-cmds.c | 6 + > gdb/cli/cli-decode.c | 174 +++++++++ > gdb/cli/cli-decode.h | 21 ++ > gdb/cli/cli-option.c | 44 +++ > gdb/cli/cli-option.h | 21 ++ > gdb/cli/cli-setshow.c | 21 ++ > gdb/cli/cli-style.c | 49 +-- > gdb/cli/cli-style.h | 4 +- > gdb/command.h | 26 +- > gdb/doc/gdb.texinfo | 46 ++- > gdb/doc/guile.texi | 104 ++++++ > gdb/doc/python.texi | 98 +++++ > gdb/guile/guile-internal.h | 9 + > gdb/guile/guile.c | 1 + > gdb/guile/scm-color.c | 427 ++++++++++++++++++++++ > gdb/guile/scm-param.c | 33 +- > gdb/python/py-color.c | 336 +++++++++++++++++ > gdb/python/py-color.h | 35 ++ > gdb/python/py-param.c | 40 +- > gdb/python/python.c | 7 + > gdb/testsuite/gdb.base/default.exp | 8 +- > gdb/testsuite/gdb.base/style.exp | 197 ++++++++++ > gdb/testsuite/gdb.guile/scm-color.exp | 110 ++++++ > gdb/testsuite/gdb.guile/scm-parameter.exp | 47 +++ > gdb/testsuite/gdb.python/py-color.exp | 100 +++++ > gdb/testsuite/gdb.python/py-parameter.exp | 53 +++ > gdb/testsuite/lib/gdb-utils.exp | 24 +- > gdb/testsuite/lib/gdb.exp | 3 +- > gdb/top.c | 14 + > gdb/ui-style.c | 353 ++++++++++++++---- > gdb/ui-style.h | 157 +++++++- > gdb/unittests/style-selftests.c | 6 +- > 34 files changed, 2436 insertions(+), 171 deletions(-) > create mode 100644 gdb/guile/scm-color.c > create mode 100644 gdb/python/py-color.c > create mode 100644 gdb/python/py-color.h > create mode 100644 gdb/testsuite/gdb.guile/scm-color.exp > create mode 100644 gdb/testsuite/gdb.python/py-color.exp Thanks, some comments for the documentation parts below. > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -49,11 +49,42 @@ > history has been reached. It also specifies that the forward execution can > continue, and the recording will also continue. > > +* "set style" commands now supports numeric format for basic colors > + from 0 to 255 and #RRGGBB format for TrueColor. > + > +* New built-in convenience variable $_colorsupport provides comma-separated > + list of color space names supported by terminal. Each color space name is one > + of monochrome, ansi_8color, aixterm_16color, xterm_256color or rgb_24bit. > + It is handy for conditionally using styling colors based on terminal features. > + For example: > + > + (gdb) if $_regex ($_colorsupport, ".*(^|,)rgb_24bit($|,).*") > + >set style filename background #FACADE > + >else > + >if $_regex ($_colorsupport, ".*(^|,)xterm_256color($|,).*") > + >set style filename background 224 > + >else > + >set style filename background red > + >end > + >end > + > * Python API > > ** Added gdb.record.clear. Clears the trace data of the current recording. > This forces re-decoding of the trace for successive commands. > > + ** New class gdb.Color for dealing with colors. > + > + ** New constant gdb.PARAM_COLOR represents color type of a > + gdb.Parameter.value. Parameter's value is gdb.Color instance. > + > +* Guile API > + > + ** New type for dealing with colors. > + > + ** New constant PARAM_COLOR represents color type of a value > + of a object. Parameter's value is instance. > + > * Debugger Adapter Protocol changes The NEWS part is okay. > +@item $_colorsupport > +@vindex $_colorsupport@r{, convenience variable} > +@cindex color space > +Comma-separated list of @dfn{color space} names supported by terminal. Names Please always put all the index commands _before_ @item, not after it. This is so following the index entry in an Info reader places you at the line corresponding to @item and not on the line after that. > @table @code > @item set style filename background @var{color} > -Set the background to @var{color}. Valid colors are @samp{none} > -(meaning the terminal's default color), @samp{black}, @samp{red}, > -@samp{green}, @samp{yellow}, @samp{blue}, @samp{magenta}, @samp{cyan}, > -and@samp{white}. > +Set the background to @var{color}. @var{color} can be @samp{none} > +(meaning the terminal's default color), a name of one of the eight standard > +colors of ISO/IEC 6429, index from 0 to 255 into terminal's color > +palette or a hexadecimal RGB triplet in @samp{#RRGGBB} format for > +24-bit TrueColor. > + > +Valid color names are @samp{black}, @samp{red}, @samp{green}, > +@samp{yellow}, @samp{blue}, @samp{magenta}, @samp{cyan}, and > +@samp{white}. To make sure the reader understands that these 8 names are what is mentioned earlier as "the eight standard colors", I suggest to start this paragraph with "Valid names of the eight standard colors are..." > +Integers 0 to 7 are the synonyms for the standard colors. Integers 8-15 are > +used for the so-called bright colors from the aixterm extended 16-color > +palette. As followup to my previous questions: can these bright colors also be used on the Windows terminal? Also, shouldn't we document in the manual the names of those bright colors? > Integers 16-255 are the indexes into xterm extended 256-color palette > +(usually 6x6x6 cube plus gray ramp). In general, 256-color palette is terminal > +dependent and sometimes can be changed with OSC 4 sequences, e.g. > +"\033]4;1;rgb:00/FF/00\033\\". A hexadecimal 24-bit TrueColor is specified in ^^ ^^ What is that double backslash there? are those two literal backslashes? Also, please leave two spaces between sentences. > +@item PARAM_COLOR > +The value is either a string or an unsigned integer. Integer from 0 to 255 > +means index into terminal's color palette. String can be a hex RGB triplet in > +@samp{#RRGGBB} format or one of the following color names: > +@samp{none} (meaning the terminal's default color), @samp{black}, @samp{red}, > +@samp{green}, @samp{yellow}, @samp{blue}, @samp{magenta}, @samp{cyan}, > +or @samp{white}. > @end vtable This text repeats what you say below in "Colors in Guile". It is better to replace it with a cross-reference to that mode, saying here just that the value can be either an integer or a string. Btw, since this is describing a programmatic interface, should we also say what happens if the value is an integer or a string that the terminal does not support? > +@deffn {Scheme Procedure} make-color @w{@r{[}value} @ > + @w{@r{[}#:color-space color-space@r{]}@r{]}} > + > +@var{value} is an integer index of a color in palette, tuple with color > +components or a string. ^^^^^^^^^^^^^^^^ ^^^^^^^^^^ This is the first and the only time you mention tuple of components in the Guile section. If tuples are indeed allowed, they should be mentioned in the other places and also documented in more detail. > +format or one of the following color names: > +@samp{none} (meaning the terminal's default color), @samp{black}, @samp{red}, > +@samp{green}, @samp{yellow}, @samp{blue}, @samp{magenta}, @samp{cyan}, > +or @samp{white}. > + > +@var{color-space} should be one of the @samp{COLORSPACE_} constants. This > +argument tells @value{GDBN} which color space @var{value} belongs. COLOR-SPACE is an optional argument, right? If so, this should document what happens if it is omitted. More generally, I'm a bit confused by the notion of "palette" or "color space" (which are the same, right?). If I have a color whose index is N < 8, doesn't that refer to the same color regardless of the palette? And if I specify a color with #RRGGBB whose value exactly corresponds to one of the 8 standard colors, don't I get the same color regardless of the palette? If so, it sounds like the palette only makes a difference when the color is specified by a value (index or RGB string) that does NOT belong to the palette, right? And if so, why do we at all need this notion of the palette in GDB? > +@deffn {Scheme Procedure} color-none? color > +Return @code{#t} if @var{color} is terminal's default. Is it a good idea to use 'color-none?' as the name? why not color-default? Also, AFAIU a terminal has two default colors: one for the foreground, the other for the background. How does this procedure know which of these two is the caller asking about? > +@deffn {Scheme Procedure} color-indexed? color > +Return @code{#t} if @var{color} is indexed, i.e. belongs to some palette. Please use "i.e.@:" or "i.e.,". Otherwise, TeX could decide that the period ends the sentence. > +Otherwise return @code{#f}. > +@end deffn > + > +@deffn {Scheme Procedure} color-direct? color > +Return @code{#t} if @var{color} is direct in the sense of ISO/IEC 8613-6. > +Otherwise return @code{#f}. > +@end deffn I'm a bit confused about these predicates: don't at least some, perhaps even all, of the colors satisfy both indexed? and direct? predicates? If so, when will these return #f? And why have these predicates in the first place? when will they be useful? > +@deffn {Scheme Procedure} color-string color > +Return the textual representation of a @code{} object. > +@end deffn This should probably explain what is the returned string. I'm guessing it's the name for the 8 standard colors (or maybe for the 16 standard colors of a 16-color terminal), and #RRGGBB for the rest, but the manual should tell that explicitly. > +@deffn {Scheme Procedure} color-index color > +Return index of the color of a @code{} object in a palette. > +@end deffn What does this return for TrueColor colors? > +@deffn {Scheme Procedure} color-components color > +Return components of the direct @code{} object. > +@end deffn Please document the form of the return value, i.e. how the components are returned. > +@deffn {Scheme Procedure} color-escape-sequence color is_foreground > +Return string to change terminal's color to this. ^^^^^^ "the string" > +When color is initialized, its color space must be specified. What do you mean by "color is initialized"? This is the first time a color's initialization is mentioned, and it must be explained. > +@vtable @code > +@item COLORSPACE_MONOCHROME > +Palette with only terminal's default color. Two colors, right? One for foreground, the other for background. Or does this refer only to the foreground color? > +@item COLORSPACE_AIXTERM_16COLOR > +Palette with 16 colors. First eight are standard colors of ISO/IEC 6429 > +"black", "red", "green", etc. Next eight are their bright version. ^^^^^^^ "versions". > +@defvar Color.is_none > +This atribute is boolean. If its value is @code{True} then color is terminal's > +default. > +@end defvar > + > +@defvar Color.is_indexed > +This atribute is boolean. If its value is @code{True} then color is indexed, > +i.e. belongs to some palette. > +@end defvar > + > +@defvar Color.is_direct > +This atribute is boolean. If its value is @code{True} then this object > +describes direct colour in the sense of ISO/IEC 8613-6. > +@end defvar Same questions/comments here as for the equivalent Guile APIs. > + > +@defvar Color.index > +This attribute exist if @code{is_indexed} is @code{True}. Its integer value is > +index of a color in a palette. > +@end defvar > + > +@defvar Color.components > +This attribute exist if @code{is_direct} is @code{True}. Its value is tuple > +with integer components of a color. > +@end defvar The corresponding Guile APIs didn't include the conditions under which the values are significant. Why not? > +When color is initialized, its color space must be specified. The Does "initialized" refer to Color.__init__? If so, I think this should be said explicitly, perhaps where that function is described. Finally, I see from the code that we sometimes approximate the requested color (which is reasonable), but I see nothing about that in the documentation. I think we should say something about that in the manual.