<table><tr><td style="">mhoogendoorn created this revision.<br />mhoogendoorn added a reviewer: Konsole.<br />mhoogendoorn added a project: Konsole.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D8464" rel="noreferrer">View Revision</a></tr></table><br /><div><strong>REVISION SUMMARY</strong><div><p>This is an attempt to fix bug <a href="https://bugs.kde.org/show_bug.cgi?id=347323" class="remarkup-link" target="_blank" rel="noreferrer">347323</a>.<br />
BUG : 347323</p>

<p>It adds support for the DECSCUSR escape code (<tt style="background: #ebebeb; font-size: 13px;">ESC [ <number> <space> q</tt>),<br />
which allows changing of the cursor blinking and shape. This is already<br />
possible by sending an escape control sequence to change profile settings, but<br />
this has some downsides:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">It changes the profile to an unnamed one, so "rightclick ; edit current profile" will not edit the profile you expect.</li>
<li class="remarkup-list-item">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.</li>
</ul>

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

<h2 class="remarkup-header">A summary of the changes:</h2>

<ul class="remarkup-list">
<li class="remarkup-list-item">Added a new construct called <tt style="background: #ebebeb; font-size: 13px;">TY_CSI_DBL</tt> to switch on CSI control sequences with two command characters in <tt style="background: #ebebeb; font-size: 13px;">processToken</tt>.</li>
<li class="remarkup-list-item">Added a character class <tt style="background: #ebebeb; font-size: 13px;">CSIDBLF</tt> for characters that can come first in a 2 character command. Found these from a cursory glance through <a href="https://www.xfree86.org/4.8.0/ctlseqs.html" class="remarkup-link" target="_blank" rel="noreferrer">the list of Xterm Control Sequences</a>.</li>
<li class="remarkup-list-item">in <tt style="background: #ebebeb; font-size: 13px;">receiveChar</tt> we check if we might be looking at a CSI command with 2 characters, if so we keep collecting characters.
<br /><br />
If we did find a 2 char CSI sequence we send it to <tt style="background: #ebebeb; font-size: 13px;">processToken()</tt> 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. <tt style="background: #ebebeb; font-size: 13px;">CSI .. $ r</tt>). It is placed before the <tt style="background: #ebebeb; font-size: 13px;">CSI m</tt> check so that an incorrect sequence like <tt style="background: #ebebeb; font-size: 13px;">ESC [ 1 <space> m</tt> is not interpreted as a <tt style="background: #ebebeb; font-size: 13px;">CSI m</tt> sequence.</li>
<li class="remarkup-list-item">In <tt style="background: #ebebeb; font-size: 13px;">processToken()</tt> we add a case for DECSCUSR and set blinking and shape accordingly.</li>
<li class="remarkup-list-item">added a call to <tt style="background: #ebebeb; font-size: 13px;">TerminalDisplay::update()</tt> in <tt style="background: #ebebeb; font-size: 13px;">TerminalDisplay::setKeyboardCursorShape()</tt>. This fixes an issue with the display of the cursor not updating (see below). Other <tt style="background: #ebebeb; font-size: 13px;">setXXX</tt> functions do updates as well so this seemed a more appropriate place than <tt style="background: #ebebeb; font-size: 13px;">Vt102Emulation::processToken()</tt>.</li>
</ul>

<h2 class="remarkup-header">Some problems/questions/remarks:</h2>

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

<h2 class="remarkup-header">The cursor redraw problem:</h2>

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

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">printf "hello"; sleep 1; printf "\e]50;CursorShape=1\x7"; sleep 2; printf "\e]50;CursorShape=0\x7\n"</pre></div>

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

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">printf "hello"; sleep 1; printf "\e[6 q"; sleep 2; printf "\e[2 q\n"</pre></div>

<p>However, without an <tt style="background: #ebebeb; font-size: 13px;">update()</tt> call in<br />
<tt style="background: #ebebeb; font-size: 13px;">TerminalDisplay::setKeyboardCursorShape()</tt> what we see is "hello" followed by<br />
a block cursor, which does not turn into an IBeam cursor after 1 second. If we<br />
make the first sleep short enough (on my pc 0.009s) it seems the cursor change<br />
gets done together with the printing of "hello" and we do see an IBeam cursor.</p>

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

<p>Initially I had an <tt style="background: #ebebeb; font-size: 13px;">updateCursor()</tt>, and that works for the testcase above, but<br />
it fails in vim when <tt style="background: #ebebeb; font-size: 13px;">showmode</tt> is set. As one of my use cases for this feature<br />
is vim, I've made it <tt style="background: #ebebeb; font-size: 13px;">update()</tt>. On my pc I've not noticed a difference in<br />
regular use. Not sure how representative of a test it is but using <tt style="background: #ebebeb; font-size: 13px;">update()</tt>:</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">time for i in $(seq 1 10000); do printf '\e[6 q'; printf '\e[2 q'; done</pre></div>

<p>Takes ~0.135s (empty screen) to ~0.350s (screen filled with 'asdf') here<br />
(fullscreen on 1920x1080, 274x61 terminal size, truetype font, full hinting).</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R319 Konsole</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D8464" rel="noreferrer">https://phabricator.kde.org/D8464</a></div></div><br /><div><strong>AFFECTED FILES</strong><div><div>src/TerminalDisplay.cpp<br />
src/Vt102Emulation.cpp</div></div></div><br /><div><strong>To: </strong>mhoogendoorn, Konsole<br /><strong>Cc: </strong>hindenburg<br /></div>