Add support for DECSCUSR.
AbandonedPublic

Authored by hindenburg on Oct 25 2017, 8:56 AM.

Details

Reviewers
mhoogendoorn
Group Reviewers
Konsole
Summary

This is an attempt to fix bug 347323.
BUG : 347323

It adds support for the DECSCUSR escape code (ESC [ <number> <space> q),
which allows changing of the cursor blinking and shape. This is already
possible by sending an escape control sequence to change profile settings, but
this has some downsides:

  • It changes the profile to an unnamed one, so "rightclick ; edit current profile" will not edit the profile you expect.
  • It changes the cursor for the whole view. For example, if we have tmux pass the sequence through to konsole, then changing the cursor in the left tmux pane will also change it in the right pane.

The DECSCUSR control sequence is different in that it uses two characters to
determine the type of command (the space, and q).

A summary of the changes:

  • Added a new construct called TY_CSI_DBL to switch on CSI control sequences with two command characters in processToken.
  • Added a character class CSIDBLF for characters that can come first in a 2 character command. Found these from a cursory glance through the list of Xterm Control Sequences.
  • in receiveChar we check if we might be looking at a CSI command with 2 characters, if so we keep collecting characters.

    If we did find a 2 char CSI sequence we send it to processToken() with the argument. Note that this one-by-one passing of arguments would not work for all sequences, some get a rectangle as arguments (e.g. CSI .. $ r). It is placed before the CSI m check so that an incorrect sequence like ESC [ 1 <space> m is not interpreted as a CSI m sequence.
  • In processToken() we add a case for DECSCUSR and set blinking and shape accordingly.
  • added a call to TerminalDisplay::update() in TerminalDisplay::setKeyboardCursorShape(). This fixes an issue with the display of the cursor not updating (see below). Other setXXX functions do updates as well so this seemed a more appropriate place than Vt102Emulation::processToken().

Some problems/questions/remarks:

  1. It will accept a code with a repeated first char, for example ESC [ <space> <space> <space> 2 <space> <space> q will be accepted as if it is just ESC [ 2 <space> q. Not sure how big of a problem that would be. Perhaps much stricter checks should be used in receiveChar()?
  2. Some defines add empty parentheses, like epp( ), so I copied that style. I don't think there is a difference, but I'm not 100% sure.
  3. Earlier in processToken multiline cases were indented at the level of the ':', so I did the same.
  4. In both Vt102Emulation::processToken() and ViewManager::applyProfileToView we have an if block to turn an int into an Enum::CursorShape, should this be made into a convenience method on the Profile class?
  5. Related, the whole retrieving of the profile defaults in processToken() feels a bit messy. We jump through hoops to get to the current profile, suddenly needing to know about Session, SessionController and SessionManager. Perhaps TerminalDisplay should have a method to reset to the default cursor?

The cursor redraw problem:

When we execute the following (starting with CursorShape=0, a block, and
blinking cursor turned off):

printf "hello"; sleep 1; printf "\e]50;CursorShape=1\x7"; sleep 2; printf "\e]50;CursorShape=0\x7\n"

We expect to see "hello" followed by a block cursor, which after 1 second
changes to an IBeam cursor. After another 2 seconds the cursor shape is reset
and we are returned to the shell prompt. This works, and uses the 'change
profile' escape code. Trying it with the DECSCUSR code, we expect to see the
same thing:

printf "hello"; sleep 1; printf "\e[6 q"; sleep 2; printf "\e[2 q\n"

However, without an update() call in
TerminalDisplay::setKeyboardCursorShape() what we see is "hello" followed by
a block cursor, which does not turn into an IBeam cursor after 1 second. If we
make the first sleep short enough (on my pc 0.009s) it seems the cursor change
gets done together with the printing of "hello" and we do see an IBeam cursor.

I think the change profile sequence did not have that problem because some
other profile setting getting applied triggered an update. Note that I first
noticed this in vim. When entering insert mode with just 'i', the cursor does
not move, and so the shape did not change until something else moved the
cursor, or triggered a repaint.

Initially I had an updateCursor(), and that works for the testcase above, but
it fails in vim when showmode is set. As one of my use cases for this feature
is vim, I've made it update(). On my pc I've not noticed a difference in
regular use. Not sure how representative of a test it is but using update():

time for i in $(seq 1 10000); do printf '\e[6 q'; printf '\e[2 q'; done

Takes ~0.135s (empty screen) to ~0.350s (screen filled with 'asdf') here
(fullscreen on 1920x1080, 274x61 terminal size, truetype font, full hinting).

Diff Detail

Repository
R319 Konsole
Lint
Lint Skipped
Unit
Unit Tests Skipped
mhoogendoorn created this revision.Oct 25 2017, 8:56 AM

Not sure if it happened with 17.08.2 (I've not seen it happen), but in 17.08.3 under some circumstances the setCursorBlinkingEnabled() call will happen from a different thread, causing a crash because the _blinkCursorTimer in TerminalDisplay is start/stopped from a thread that isn't its own.

Testcase that seems to reliable trigger it:

  • Open a tmux new tmux session, and run printf '\e[6 q\e[2 q' in it, then detach.
  • Start a Konsole built with this patch and attach to the tmux session.

Upon reattaching tmux emits a control sequence to make the cursor in the outer terminal match what is set in the tmux session, causing the mentioned crash.

Not sure how that can be avoided, but as there's not much interest it's probably best to shelve this. As I don't care about blinking cursors I can just patch locally for shape changing.

hindenburg commandeered this revision.Dec 30 2018, 11:28 PM
hindenburg added a reviewer: mhoogendoorn.
hindenburg added a subscriber: hindenburg.

Thanks for the effort/code - it appears this has since been fixed.

Restricted Application added a subscriber: konsole-devel. ยท View Herald TranscriptDec 30 2018, 11:28 PM
hindenburg abandoned this revision.Dec 30 2018, 11:28 PM