GNOME Bugzilla – Bug 720821
Add support for DECSCUSR control sequence (set cursor style)
Last modified: 2015-04-23 22:01:13 UTC
Created attachment 264615 [details] [review] adds support for DECSCUSR control sequence 0) I've locally used a patch that adds support for the DECSCUSR control sequence for over a year now. I've just reapplied it on top of v0.34.9 (as shipped by Fedora 20). Time to submit it for review. 1) Note that the main thing here was not to break the ways one can currently set ones cursor (see "cursor_shape" and "cursor_blink_mode"). So I've added "cursor_style". I feel that adding cursor_style, and setting that through DECSCUSR works best. 2) Also note that this patch is actually the two local patches I carry folded into one patch. I hope I did that right. (The two steps I used to develop this weren't really relevant for review.) 3) Review appreciated. I'll gladly resubmit, with proper commit explanation, after review.
People wanting to test this on vim might want to add something like this to their .vimrc: " 18 09 2012 function! InsertEnter() if v:insertmode == "i" silent execute "!echo -en \"^[[5 q\"" else " "r" or "v" silent execute "!echo -en \"^[[1 q\"" endif endfunction autocmd VimEnter * silent execute "!echo -en \"^[[3 q\"" autocmd InsertEnter * call InsertEnter() autocmd InsertLeave * silent execute "!echo -en \"^[[3 q\"" autocmd VimLeave * silent execute "!echo -en \"^[[0 q\""
Thanks for the initial patch! I'm ok with adding support for this sequence; the patch needs some work however. As for the behaviour: we have this same problem in other places: who takes precedence when we have a) API for setting some pref, and b) escape sequences for the same pref... ie should we really blink the cursor as requested by the escape sequence if the terminal pref is VTE_CURSOR_BLINK_OFF ? +static void +vte_sequence_handler_set_cursor_style (VteTerminal *terminal, GValueArray *params) I think most of this code should go into vte.c and be named vte_terminal_...(); code in vteseq.c usually shouldn't poke at GtkSettings or widget. The code should just store the value from the sequence (int 0..6) and then do the work to update real cursor style and blinking with the existing functions. +typedef enum _VteCursorStyle { + VTE_CURSOR_STYLE_TERMINAL_DEFAULT, + VTE_CURSOR_STYLE_BLINK_BLOCK, + VTE_CURSOR_STYLE_STEADY_BLOCK, The value order in this enum is a bit odd, since xterm says default is 1 (blinking block), while 0 is blinking block (non-default): CSI Ps SP q Set cursor style (DECSCUSR, VT520). Ps = 0 -> blinking block. Ps = 1 -> blinking block (default). Ps = 2 -> steady block. Ps = 3 -> blinking underline. Ps = 4 -> steady underline. Ps = 5 -> blinking bar (xterm). Ps = 6 -> steady bar (xterm). -static void +void vte_terminal_set_cursor_blinks_internal(VteTerminal *terminal, gboolean blink) Need to prefix non-static but internal function names with an underscore here, so they don't get exported. Also I wonder if we should also implement the corresponding DCS query? DCS $ q Pt ST Request Status String (DECRQSS). The string following the "q" is one of the following: " q -> DECSCA " p -> DECSCL r -> DECSTBM m -> SGR SP q -> DECSCUSR xterm responds with DCS 1 $ r Pt ST for valid requests, replacing the Pt with the corresponding CSI string, or DCS 0 $ r Pt ST for invalid requests.
(In reply to comment #2) > As for the behaviour: we have this same problem in other places: who takes > precedence when we have a) API for setting some pref, and b) escape sequences > for the same pref... ie should we really blink the cursor as requested by the > escape sequence if the terminal pref is VTE_CURSOR_BLINK_OFF ? Your call, obviously. But my reasoning was (and is) that control sequence are tied to one terminal while the preferences are global (for all terminals that vte controls). So it seems to correct to let control sequences (temporarily) override the global preferences involved. > +static void > +vte_sequence_handler_set_cursor_style (VteTerminal *terminal, GValueArray > *params) > > I think most of this code should go into vte.c and be named vte_terminal_...(); > code in vteseq.c usually shouldn't poke at GtkSettings or widget. The code > should just store the value from the sequence (int 0..6) and then do the work > to update real cursor style and blinking with the existing functions. I'll look into that. > +typedef enum _VteCursorStyle { > + VTE_CURSOR_STYLE_TERMINAL_DEFAULT, > + VTE_CURSOR_STYLE_BLINK_BLOCK, > + VTE_CURSOR_STYLE_STEADY_BLOCK, > > The value order in this enum is a bit odd, since xterm says default is 1 > (blinking block), while 0 is blinking block (non-default): > > CSI Ps SP q > Set cursor style (DECSCUSR, VT520). > Ps = 0 -> blinking block. > Ps = 1 -> blinking block (default). > Ps = 2 -> steady block. > Ps = 3 -> blinking underline. > Ps = 4 -> steady underline. > Ps = 5 -> blinking bar (xterm). > Ps = 6 -> steady bar (xterm). I should have commented that enum. (I also forgot to mention that 5 and 6 are xterm specific.) Anyhow, my reasoning is that 1/2, 3/4, and 5/6, are three blinking/steady pairs. That leaves 0 as special. I therefor _guessed_ that 0 means something like "set to terminal default". Otherwise 0 and 1 are aliases, which is a bit odd. But perhaps someone with access to an actual VT520 might be able to determine this. Provided that these machines had another way to set a default cursor. But perhaps you could just decide whether it's OK for vte to interpret either 0 or 1 as "reset to terminal default cursor" and see if that breaks applications that use a different assumption. > -static void > +void > vte_terminal_set_cursor_blinks_internal(VteTerminal *terminal, gboolean blink) > > Need to prefix non-static but internal function names with an underscore here, > so they don't get exported. Will do. > Also I wonder if we should also implement the corresponding DCS query? > > DCS $ q Pt ST > Request Status String (DECRQSS). The string following the "q" > is one of the following: > " q -> DECSCA > " p -> DECSCL > r -> DECSTBM > m -> SGR > SP q -> DECSCUSR > xterm responds with DCS 1 $ r Pt ST for valid requests, > replacing the Pt with the corresponding CSI string, or DCS 0 $ > r Pt ST for invalid requests. I seem to remember playing with that control sequence in xterm, without being able to really understand the response. It basically looked like linenoise to me. But, anyhow, the DECSCUSR part of DECRQSS seems to make it possible to determine the current cursor style. Am I reading DECRQSS correctly?
Yes, the query returns the current setting. See http://www.vt100.net/docs/vt510-rm/DECRQSS and http://www.vt100.net/docs/vt510-rm/DECRPSS . As for the default, http://www.vt100.net/docs/vt510-rm/DECSCUSR says that 0, 1, or no param all are 'default'. xterm responds to DCS $ q SP q ST with ^ [ P 1 $ r X SP q ^ [ \ (where X is some number). Except for the substitution of ESC with ^ [, this is as expected. However it seems to be confused (tested with xterm 298): CSI SP q ST => X = 2 CSI 0 SP q ST => X = 2 CSI 1 SP q ST => X = 2 CSI 2 SP q ST => X = 1 CSI 3 SP q ST => X = 4 CSI 4 SP q ST => X = 3 CSI 5 SP q ST => X = 6 CSI 6 SP q ST => X = 5
(In reply to comment #4) > As for the default, http://www.vt100.net/docs/vt510-rm/DECSCUSR says that 0, 1, > or no param all are 'default'. So please think a bit whether my idea to use 0 to mean "reset to system default" is an acceptable, well, deviation to be used by vte. > xterm responds to DCS $ q SP q ST with ^ [ P 1 $ r X SP q ^ [ \ (where X is > some number). Except for the substitution of ESC with ^ [, this is as expected. > > However it seems to be confused (tested with xterm 298): > > CSI SP q ST => X = 2 > CSI 0 SP q ST => X = 2 > CSI 1 SP q ST => X = 2 > CSI 2 SP q ST => X = 1 > CSI 3 SP q ST => X = 4 > CSI 4 SP q ST => X = 3 > CSI 5 SP q ST => X = 6 > CSI 6 SP q ST => X = 5 Grepping my mailbox revealed that I sent Thomas Dickey this patch over a year ago: diff --git a/misc.c b/misc.c index e56a9ff..f2b30ee 100644 --- a/misc.c +++ b/misc.c @@ -3722,14 +3722,14 @@ do_dcs(XtermWidget xw, Char * dcsbuf, size_t dcslen) #endif strcat(reply, "m"); } else if (!strcmp(cp, " q")) { /* DECSCUSR */ - int code = 0; + int code = STEADY_BLOCK; if (isCursorUnderline(screen)) - code |= 2; + code = STEADY_UNDERLINE; #if OPT_BLINK_CURS if (screen->cursor_blink_esc == 0) - code |= 1; + code -= 1; #endif - sprintf(reply, "%d%s", code + 1, cp); + sprintf(reply, "%d%s", code, cp); } else okay = False; -- Has that been applied? Perhaps the patch was incorrect. I haven't bothered to check myself.
I guess we can use either 0 (or the no-number form CSI SP q ST) to mean 'reset to default'; xterm does call them DEFAULT and DEFAULT_STYLE, resp, both using the blinking block cursor. The xterm patch has been applied, in version 282. #if OPT_BLINK_CURS if (screen->cursor_blink_esc == 0) code -= 1; #endif If I read the xterm code correctly, cursor_blink_esc is True if the cursor blinks, False if it doesn't blink, so this conditional seems to be inverted.
(In reply to comment #6) > If I read the xterm code correctly, cursor_blink_esc is True if the cursor > blinks, False if it doesn't blink, so this conditional seems to be inverted. Should I dive into this or would you like to send Thomas Dickey a patch to fix this properly (if that's needed)?
There is no need to use autocmds in vim, it supports sending escape codes while entering/leaving insert mode. see: http://vim.wikia.com/wiki/Configuring_the_cursor
*** Bug 722212 has been marked as a duplicate of this bug. ***
Paul & Christian, this is great! I hope the patch is merged in soon. In ViM, this is how I configure the cursor modes for XTerm: " Mode dependent cursors for vim let &t_ti.="\e[1 q" let &t_te.="\e[0 q" let &t_SI="\e[5 q" let &t_EI="\e[1 q" Once this patch is merged, the same sequence will work for gnome-terminal!
(In reply to comment #10) > Paul & Christian, this is great! I hope the patch is merged in soon. Thank you. But let's not clutter bugzilla.gnome.org too much. Test my current patch if you'd like and feel free to prod me via email for an updated patch. I hope to get to it soonish. > In ViM, this is how I configure the cursor modes for XTerm: > " Mode dependent cursors for vim > let &t_ti.="\e[1 q" > let &t_te.="\e[0 q" > > let &t_SI="\e[5 q" > let &t_EI="\e[1 q" Off topic, but this doesn't cover REPLACE mode, does it? > Once this patch is merged, the same sequence will work for gnome-terminal!
I tested this patch out by setting t_SI to blinking ibeam and t_EI to solid block. Leaving insert mode works fine in all cases I tried, the cursor instantaneously changes to a solid block. let &t_SI="\e[5 q" let &t_EI="\e[2 q" However, entering insert mode in a non-empty line does not change the cursor from solid block to blinking beam unless i press some other key. Example: arrow keys or starting typing. Using these settings in xterm 300 works fine.
Created attachment 266620 [details] [review] adds support for DECSCUSR control sequence (src/vteseq-n.gperf addendum) (In reply to comment #0) > 2) Also note that this patch is actually the two local patches I carry folded > into one patch. I hope I did that right. I didn't actually get that right. One also needs this addendum (on the Fedora 20 system I tried to redo my work).
Created attachment 266650 [details] [review] adds support for DECSCUSR control sequence (v2) 0) This is v2 of the patch that I promised to write about a month ago. I think I addressed (most of) the review comments. The main change is that the actual handling of this control sequence is now concentrated in src/vte.c:_vte_terminal_set_cursor_style(). Also note the new, verbose, comment that explains why I think it is OK to differentiate between the "0" parameter and the "1" parameter. 1) I also implemented support for an empty parameter (ie, none). That means that echo -e ^[[ q now works as specified. But it also prints a bit of line noise on the terminal. How does vte expect one to handle optional parameters like this one? 2) I didn't bother to implement the corresponding DCS query. I prefer to first determine whether xterm currently handles it correctly and, if needed, send a patch to xterm's maintainer before trying this for vte3 too. So: deferred for now. 3) Review and testing are, as always, appreciated.
The bug I mentioned is still present in the new attachment.
(In reply to comment #15) > The bug I mentioned is still present in the new attachment. That must be this (from comment #12): > However, entering insert mode in a non-empty line does not change the cursor > from solid block to blinking beam unless i press some other key. Example: arrow > keys or starting typing. > > Using these settings in xterm 300 works fine. I cannot reproduce. I started vim with vim -u NONE to make sure my (horrific) vimrc doesn't influence my light testing. After setting :let &t_SI="\e[5 q" and :let &t_EI="\e[2 q" in vim things behave like I expect them too (ie, cursor changes between block and I-beam instantly when hopping between insert and normal mode). Perhaps there's something specific to your setup that triggers an issue only in gnome-terminal. Maybe you could try to further pinpoint what could be causing this on your side, because, as I said, I cannot trigger this issue.
The issue does not occur with an empty vimrc. It must be something in my vimrc. Thanks!
I was able to pinpoint the source of my problem to ttimeoutlen not being set. set ttimeoutlen=100 solves my problem.
It's unclear to me how some vim setting could affect how vte paints the cursor... is this setting just masking the problem, or does the problem not in fact exist?
(In reply to comment #19) > It's unclear to me how some vim setting could affect how vte paints the > cursor... is this setting just masking the problem, or does the problem not in > fact exist? I'm inclined to think there's no real problem here. I can't reproduce it and jckeerthan@gmail.com can't reproduce it when running vim without any local configuration (ie, "vim -u NONE"). vim configuration is a bit of a black art. There are roughly a bazillion options and vim scripts basically open up the entire toolbox installed on a Linux box. So, even without knowing the configuration involved, I wouldn't bother unless someone can come up with a _very_ detailed analysis of this problem. Anyhow, I don't understand ttimeoutlen, see black art, above. But my guess is that jckeerthan@gmail.com's setup somehow tells vim to not refresh its screen until (a lot of) time passes or a key is pressed. And perhaps vim hides the update of vte's cursor (temporarily). Something like that, but see _very_ detailed analysis, above.
(In reply to comment #3) > (In reply to comment #2) > > As for the behaviour: we have this same problem in other places: who takes > > precedence when we have a) API for setting some pref, and b) escape sequences > > for the same pref... ie should we really blink the cursor as requested by the > > escape sequence if the terminal pref is VTE_CURSOR_BLINK_OFF ? > > Your call, obviously. But my reasoning was (and is) that control sequence are > tied to one terminal while the preferences are global (for all terminals that > vte controls). So it seems to correct to let control sequences (temporarily) > override the global preferences involved. (I haven't read the whole thread, nor looked at the patch, but let me comment on this one.) There has recently been some fixes to the way the color palette is handled (bug 705985 [calling the API should be idempotent - dirty fix] and bug 640040 [this dirty fix refactored to a clean solution]). The conclusion was that we need to remember the value set by API and we also need to remember the value set by escape codes, and use the latter ones if it's set. If there was such an escape code used then calling the API should not have an immediate visual effect (the value set by escapes should still be used), but should change the value we revert to in case the unsetting escape code is seen. Please make sure to follow the same pattern with the cursor mode. That is, something along these lines: API sets a variable "cursor_style_api", possible values include styles like "still block", "blinking block", "still underline" etc. Escape code sets a variable "cursor_style_esc", possible values include all the above plus "unset" which is the default. Actual display style is cursor_style_esc != unset ? cursor_style_esc : cursor_style_api.
(In reply to comment #21) > Please make sure to follow the same pattern with the cursor mode. That is, > something along these lines: > > API sets a variable "cursor_style_api", possible values include styles like > "still block", "blinking block", "still underline" etc. > > Escape code sets a variable "cursor_style_esc", possible values include all the > above plus "unset" which is the default. > > Actual display style is cursor_style_esc != unset ? cursor_style_esc : > cursor_style_api. Yes, I think that's basically what I tried to do. Not sure what API means here, though. Would fiddling with settings in gnome-terminal qualify as use of vte APIs? Anyhow, please review the patch to see whether I really implemented what you'd like to see.
> Yes, I think that's basically what I tried to do. Okay then :) > Not sure what API means here, though. Would fiddling with settings in > gnome-terminal qualify as use of vte APIs? Yup. > Anyhow, please review the patch to > see whether I really implemented what you'd like to see. I'll try to keep it in mind and get back to it, please be patient.
I took a look at your patch. Basically it's nice, thanks! :) Some minor issues: - The value should be reset in vte_terminal_reset - In vteseq, you should bail out if the value is not in [0..6] - There you're doing an implicit conversion from a 0..6 number to the enum, so you do heavily rely on the order of the enum members. This should be made explicit (to prevent further changes causing breaks), such as typedef enum _VteCursorStyle { VTE_CURSOR_STYLE_TERMINAL_DEFAULT = 0, VTE_CURSOR_STYLE_BLINK_BLOCK = 1, ... and perhaps even adding a brief comment would be useful. [Alternatively (but it's ugly) I'd personally be okay with just carrying the int (no enum) and replacing the "switch"es by %2 and /2 operations :) But I guess chpe wouldn't like this... so let's stick with the enum :)]
(In reply to comment #24) > I took a look at your patch. Basically it's nice, thanks! :) Thanks for the review. > Some minor issues: > > - The value should be reset in vte_terminal_reset > > - In vteseq, you should bail out if the value is not in [0..6] > > - There you're doing an implicit conversion from a 0..6 number to the enum, so > you do heavily rely on the order of the enum members. This should be made > explicit (to prevent further changes causing breaks), such as > > typedef enum _VteCursorStyle { > VTE_CURSOR_STYLE_TERMINAL_DEFAULT = 0, > VTE_CURSOR_STYLE_BLINK_BLOCK = 1, > ... > > and perhaps even adding a brief comment would be useful. > > [Alternatively (but it's ugly) I'd personally be okay with just carrying the > int (no enum) and replacing the "switch"es by %2 and /2 operations :) But I > guess chpe wouldn't like this... so let's stick with the enum :)] My current schedule doesn't allow me to redo my patch in the next few weeks. I hope to submit an updated version in September. Please prod if things take longer. Actually, I'm fine with you, or anyone else, taking my patch and update it to reflect your comments. Really!
Created attachment 283417 [details] [review] v3 - Updated to current git - Addressed the previous comments - Made it work with no numeric parameter - Removed doc for some non-public methods (errrrr...) Tested and it works :) vte_terminal_set_cursor_blink_mode and _vte_terminal_set_cursor_style share quite a lot of code, I really don't like it.
(In reply to comment #26) > vte_terminal_set_cursor_blink_mode and _vte_terminal_set_cursor_style share > quite a lot of code, I really don't like it. Yup it's actually buggy there: an API change in the blink state immediately takes effect, ignoring the escape sequence override even if that should be in effect.
(In reply to comment #24) > But I guess chpe wouldn't like this... so let's stick with the enum :)] As long as that's commented in the code and the underlying assumption statically-asserted, that's fine with me.
Comment on attachment 283417 [details] [review] v3 Looks good. Since we now have a way to control this from escape seq, can we deprecated/remove the API ? It seems a bit redundant to have 2 ways to configure this...
(In reply to comment #29) > Since we now have a way to control this from escape seq, can we > deprecated/remove the API ? It seems a bit redundant to have 2 ways to > configure this... I really wouldn't want to. In bug 705985 we figured out how to properly handle API vs. escape sequence for colors, the latter having higher precedence. We applied that to special colors (OSC 100-ish, like cursor color) too, and now here as well. By removing the API, applications (including gnome-terminal) would have a much harder time offering this feature. Setting a value would be more cumbersome (feeding escape sequences into the widget before the child app is forked), runtime changing from gnome-terminal's UI would become impossible (if it fed the same escape to the widget, it would be subject to race condition, cutting in half another escape sequence printed by the app).
(In reply to comment #27) > Yup it's actually buggy there: an API change in the blink state immediately > takes effect, ignoring the escape sequence override even if that should be in > effect. I don't understand why I thought it was buggy. Looks okay to me now. What is buggy, however, is when blinking is altered via a property change.
Created attachment 291238 [details] [review] v4 v4: fix the aforementioned bug, plus refactor to eliminate code duplication
Fixed in master (future 0.40).
(In reply to comment #33) > Fixed in master (future 0.40). 0) Thanks! 1) I backported your v4 of my patch to (Fedora 20's) 0.34.9. It worked (and still works) great! 2) For some reason the port to 0.36.3 (as shipped by Fedora 21) was clean, but doesn't actually work. Ie, the DECSCUSR control sequences are not recognized. I'm inclined to think I botched the port. But the port was really trivial. Is there any reason to think this stuff shouldn't work in 0.36? 3) Anyhow, Egmont, thanks again for your finishing touch!
Created attachment 293633 [details] [review] patch for 0.36 > I'm inclined to think I botched the port. I think so :) Here it is, enjoy!
(In reply to comment #35) > Created an attachment (id=293633) [details] [review] > patch for 0.36 > > > I'm inclined to think I botched the port. > > I think so :) Here it is, enjoy! Thanks. But that basically matches what I came up with. Which is odd... I'll dig a bit deeper.
(In reply to Paul Bolle from comment #34) > 1) I backported your v4 of my patch to (Fedora 20's) 0.34.9. It worked (and > still works) great! Could you attach this backport as well? I'd like to try it. I'm on Ubuntu 14.04 which also ships 0.34.9. The patch for 0.36 succeeded but didn't seem to have the desired effect for me either. Thanks!
(In reply to Kilian Evang from comment #37) > (In reply to Paul Bolle from comment #34) > > 1) I backported your v4 of my patch to (Fedora 20's) 0.34.9. It worked (and > > still works) great! > > Could you attach this backport as well? I'd like to try it. Sure, no problem. > I'm on Ubuntu 14.04 which also ships 0.34.9. The patch for 0.36 succeeded > but didn't seem to have the desired effect for me either. So it's not just me. I have dragged my feet and not yet looked into that.
Created attachment 302241 [details] [review] Backport to 0.34.9 Please let me know whether this works for you, Kilian.
So far it doesn't, but probably that's just noob me not knowing how to build and use the patched library properly. Perhaps you could have a look at what I'm doing wrong? I'd be much obliged. Basically, this is what I did: cd /home/ke/Dropbox/install wget http://ftp.acc.umu.se/pub/gnome/sources/vte/0.34/vte-0.34.9.tar.xz unxz vte-0.34.9.tar.xz cd vte-0.34.9 wget https://bugzilla.gnome.org/attachment.cgi\?id\=302241 -O decscusr.patch patch -p1 < decscusr.patch ./configure make Then I made sure no gnome-terminal windows were open and ran the following command from a shell in xterm: LD_LIBRARY_PATH=/home/ke/Dropbox/install/vte-0.34.9/src/.libs gnome-terminal I verified that the gnome-terminal I started was using the right version of vte, like this: cat /proc/29974/maps | awk '{print $6}' | grep '\.so' | sort | uniq | grep vte This gave the following output: /home/ke/Dropbox/install/vte-0.34.9/src/.libs/libvte2_90.so.9.3400.9 Looks right to me... but still when I add the lines from comment 1 to my ~/.vimrc, start vim and switch between modes, I see escape sequences like ^[[3 q being printed to the terminal, rather than the cursor changing shape.