GNOME Bugzilla – Bug 640040
[PATCH] Add support for OSC 112 (reset cursor color)
Last modified: 2014-04-06 18:29:42 UTC
Created attachment 178814 [details] [review] Patch for OSC 112 It would be nice to have support for the escape sequences that reset various colors: OSC 104 and 110 through 117. (Xterm has a few more, but VTE doesn't appear to support setting the corresponding colors, so no need to reset them.) I believe these are from xterm patch 252. I started with an easy one, OSC 112. This is a very small and simple patch. If it is received well I may be willing to take on some of the other escape sequences as well. Thanks.
Can you tell me how to test this?
(In reply to comment #1) > Can you tell me how to test this? I suppose there are some standard ways of testing, but I typically just do this: $ echo -e '\e]12;red\007' # Cursor becomes red $ echo -e '\e]112\007' # Cursor becomes reverse-video Also, the sequences using ST should work (I'm afraid I may not have tested this one myself): $ echo -e '\e]12;green\e\\' $ echo -e '\e]112\e\\' I should also mention that there was a comment in bug 567444 that the public API should not be used to implement things like OSC 12 and 112. This patch does propagate a potentially incorrect usage. I'm not clear on what the alternative approach would look like, but I may be willing to take on the task of fixing this (and the OSC 10 patch from that bug) if it can be clarified.
Created attachment 178908 [details] [review] Add 12 and 112 to the osc script Here's an additional patch adding OSC 12 and 112 to the osc script. Well, adding them to the "usage" text, anyway. No change is actually needed to use them.
My xterm-267 doesn't reset cursor color with OSC 112.
(In reply to comment #4) > My xterm-267 doesn't reset cursor color with OSC 112. xterm's behavior is kind of weird, and possibly broken. If the cursor color has not been set in the X resources (or on the command line that started xterm), using OSC 12 seems to change the "default" color, so resetting it has no effect. Try starting xterm with -cr <color> and it should work as expected.
I agree that the xterm behavior doesn't make sense. However, we are supposed to emulate xterm as closely as possible. Humm?
xterm-271 fixed this. Time for us to implement :)
Following the change in bug 705985, this feature is surprisingly hard to implement. The code is quite complicated, I myself hardly understand what I did two months ago :D, and the cursor's case is more complex than other colors. We have two colors for the cursor, the default set via API, and the overridden set via escapes. Both colors are stored in a pair of PangoColor (red, green, blue) and an additional boolean (to denote the "unset" case, something that can't happen with regular colors). These booleans are one-off special values for the cursor, so _vte_terminal_set_color_internal() doesn't know about them and cannot use them, unless we do a terrible special casing for them. Also the colors can be unset both via API and via escapes, something again that we don't have for normal colors and make life more complicated. Either I do a terrible one-off for the cursor with a looong comment explaining everything (I'm working on this right now), or we are braver to do bit of refactoring. I'm thinking of replacing PangoColor all over the codebase with something that can hold other special values. Overloading GdkColor's "index" could work but is semantically wtf. Overloading GdkRGBA's "alpha" is probably kinda okay (denote the default cursor by a transparent color). Or have our own VteColor data structure.
Use custom VteColor if you go down that route.
Created attachment 266565 [details] [review] Refactoring Okay, I decided to go for the clean solution and refactored the code a little bit. This patch doesn't introduce OSC 112 yet, but on top of this it'll be damn easy to add that (as well as other OSCs).
Created attachment 266570 [details] [review] OSC 112 (Kevin's patches updated)
Review of attachment 266570 [details] [review]: an extra '+' sign in the osc script should be removed before committing
Sibling issues: bug 151260, bug 567444, bug 722446.
(As part of this bug, I'd like to add support for OSC 104 too, so that anything that can be set with OSC can also be reset. I'm waiting for response from Thomas whether OSC 104 is broken in xterm, or I misunderstand something in the docs.)
Review of attachment 266565 [details] [review]: ::: src/vte-private.h @@ +176,3 @@ + gboolean is_set; + } sources[2]; +} VtePaletteColor; PangoColor stores 16 bits per component, which we don't really need. So could make this a struct { guint8 red,green,blue; guint8 set: 1}[2], using 8 bytes instead of 16 bytes. The only place where 16 bits are needed is in the response to the colour query, where we can just use (c << 8 | c). ::: src/vte.c @@ +2868,3 @@ break; case VTE_DEF_HL: + unset = TRUE; I'd prefer to keep always initialising the colour. Then when querying we can just assume it exists without needing to fallback. @@ +2871,3 @@ break; case VTE_CUR_BG: + unset = TRUE; Same here. ::: src/vteseq.c @@ +1927,3 @@ gchar buf[128]; + PangoColor *c = _vte_terminal_get_color(terminal, idx); + g_assert(c != NULL); If we always intitialise the colour, this is guaranteed and need not be asserted here. @@ +3529,3 @@ + PangoColor *c = _vte_terminal_get_color(terminal, VTE_CUR_BG); + if (c == NULL) + c = _vte_terminal_get_color(terminal, VTE_DEF_FG); This can be simplified by just always initialising the colour as I commented above/below.
Comment on attachment 266570 [details] [review] OSC 112 (Kevin's patches updated) Looks ok with the change from comment 12.
Review of attachment 266565 [details] [review]: ::: src/vte-private.h @@ +176,3 @@ + gboolean is_set; + } sources[2]; +} VtePaletteColor; Most of the code uses PangoColor, so the new _vte_terminal_get_color() just returns a pointer to the value stored here. I didn't want to rewrite the rest of vte for the new structure (not sure if we actually ever need a PangoColor), nor do I want to convert every time (and struggle with malloc/free and stuff). I know we waste about 1.5kB but we already did and it's the simplest. A subsequent change, totally orthogonal to this, could be to use VteColor {guint8 r,g,b} (but no "set" boolean) instead of PangoColor all over the code. I'm not sure if we use PangoColor in any API call which would make this change complicated, or if it's really straightforward. ::: src/vte.c @@ +2868,3 @@ break; case VTE_DEF_HL: + unset = TRUE; The values here were never used, they were masked out by the False state of booleans "highlight_color_set" and "cursor_color_set". As soon as those booleans became True, the colors here got redefined too. Those booleans no longer exist, instead, they are denoted by the "unset" state of the API slot in the palette. So the current change is a no-op in behavior, while with your suggestion the highlight/cursor color would be different. (Tricky change I know, but I hope the new code is easier to understand.) @@ +2871,3 @@ break; case VTE_CUR_BG: + unset = TRUE; ditto ::: src/vteseq.c @@ +1927,3 @@ gchar buf[128]; + PangoColor *c = _vte_terminal_get_color(terminal, idx); + g_assert(c != NULL); I can remove the assertions if you wish. But, of course, theoretically the assertion always has to be true even with my corrent code, so it's just a prevention against future breakages. Currently, due to the way the initialization happens and the features we provide via API, only CUR_BG and DEF_HL can be unset. @@ +3529,3 @@ + PangoColor *c = _vte_terminal_get_color(terminal, VTE_CUR_BG); + if (c == NULL) + c = _vte_terminal_get_color(terminal, VTE_DEF_FG); ditto
Created attachment 266692 [details] [review] OSC 104 Add OSC 104 support. Thomas's response was quite clear (releasing xterm-301 with the fix :)). Without parameters it restores all the 256 colors. With parameters it restores only those (more can be separated with semicolons).
Created attachment 266820 [details] [review] OSC 104 (v2) OSC 104 updated to use %m.
Much nicer :-) ok-to-commit for the whole series.
Fixed in vte-0-36. Keeping the bug open for vte-next.