GNOME Bugzilla – Bug 685759
xterm colour setting sequence format
Last modified: 2017-12-14 14:43:02 UTC
xterm 282 adds this (xterm/charproc.c); we may want to update our code for xterm parity? /* * Some background - * * Todd Larason provided the initial changes to support 256-colors in July 1999. * I pointed out that the description of SGR 38/48 in ECMA-48 was vague, and * was unsure if there would be some standard using those codes. His response * was that this was documented (it turns out, in equally vague terms) in ITU * T.416 * * Discussing this with Todd Larason in mid-1999, my point was that given the * high cost of obtaining ITU T.416 (ISO-8613-6), the standard was not going * to be effective (more than $100 then, more than $200 in 2012) * * We overlooked the detail about ":" as a subparameter delimiter (documented * in 5.4.t2 in ECMA-48). Some discussion in KDE in mid-2006 led Lars Doelle * to discuss the issue with me. Lars' initial concern dealt with the fact * that a sequence such as * CSI 38 ; 5 ; 1 m * violated the principle that SGR parameters could be used in any order. * Further discussion (see KDE #107487) resolved that the standard expected * that the sequence would look like * CSI 38 ; 5 : 1 m * which still violates that principle, since the "5:1" parameter has to * follow the "38" to be useful. * * This function accepts either format (per request by Paul Leonerd Evans). * It also accepts * CSI 38 : 5 : 1 m * according to Lars' original assumption. * * By the way - all of the parameters are decimal integers. */
Created attachment 265807 [details] [review] The wrong approach - don't use it :) Just for fun: This simple patch makes vte accept ':' as the separator everywhere in addition to ';'. At first glimpse, this might seem to be a good solution. The problem is, this approach would totally defeat the purpose of intruducing ':', we could just as well happily live with ';'. The goal should be that the new separator is _only_ accepted within those 38:2:... and 38:5:... sequences, not anywhere else.
Created attachment 265889 [details] [review] Fix v1 Here's the first candidate. Todo: - The heavy bits of parsing is factored out to a helper method. Still, there's some lightweight boring chrome (~15 lines) that's duplicated for ':' or ';' being the first separator, because they just lie on totally different code paths. (';' followed ':' separators, or ';' all along are handled at the same place.) Figure out whether this duplication can be avoided, or if this is something we're okay to live with. - Understand whether the g_value_array_new()'d subarray needs to be freed in _vte_matcher_free_params_array(), or if it's done automatically.
Trying to free causes crash ("free(): invalid pointer"), I guess this answers the last question. That being said, ChPe/Behdad could you please verify the patch, especially the G_TYPE_VALUE_ARRAY and g_value_get/set_boxed() stuff? I've never done this kind of glib programming before, I'm not sure I'm doing the right thing. I know GValueArray is deprecated, but I didn't feel like refactoring the code so I went with it. vte_sequence_handler_character_attributes() receives an argument of this type, it used to contain longs only (the semicolon-separated numbers), but now it might also contain a nested sub-array of the same type (for the colon-separated numbers). vte_sequence_parse_sgr_38_48_parameters() also expects such a data type, it receives the "outer" GValueArray in case of semicolon-separated list, and the "inner" one in case of colon-separated, it just doesn't care which one it gets. That's why it's important to have the same data type for nesting.
(I'm planning to submit tomorrow, so that it makes it into 0.35.1 and gets broader testing. Unless I get a negative review, of course :) Thanks.)
Hmm. Changing _vte_table_extract_numbers — which is used by other sequences as well — like this looks rather risky to me. It could have unintended effects on those other sequences... The GValueArray handling itself looks alright.
I'm trying to understand the situation better... _vte_table_extract_numbers() previously created a list that always consisted of longs only. Non-digit characters (other than semicolon) in the input were simply ignored. Now it may contain GValueArrays too. Also, the change in _vte_table_is_numeric_list() probably influences when we even hit the extract_numbers method (I'm not sure). In vteseq.c processing these params seems to be very protective, it always does type checking (i.e. G_VALUE_HOLDS_LONG()) before calling g_value_get_long, so apparently it tolerates in a safe way if it encounters a GValueArray, it won't crash. So the only issue I can see is that sequences containg colons that were not supported so far (resulted in something unspecified) now do something slightly different unspecified. Apparently the danger zone is where caps.c defines one of %d, %2, %3, %m or %M in the format, that's where _vte_table_extract_numbers() is invoked. I was trying a couple of escape sequences: \e[7;8H (move the cursor absolutely) -> works, just like before \e[7;8:9H, \e[7:9H and alike -> prints the raw escape, just like before \e[7A (move upwards by 7 rows) -> works, just like before \e[7;8A, \e[7;8:9A, \e[7:9A and alike -> prints the raw escape, just like before \e[?1000;1049h (enable mouse and alternate screen) -> works, just like before \e[?1000;1049:0h -> previously it printed the raw escape, now it processes 1000 (enables mouse) and drops 1049:0. I think we can live with such a change. I don't understand the code enough to say for 100% sure that everything's okay, but I'm confident enough to give it a try in an unstable tarball :)
Created attachment 266063 [details] [review] Fix v2 Changed: Bail out if there are additional colon-separated parameters (e.g. 38:2:10:20:30:40m is rejected).
Fixed in vte-0-36, keeping the bug open for vte-next.
FYI: This is now being continued/revised in bug 791456.