<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/126921/">https://git.reviewboard.kde.org/r/126921/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 29th, 2016, 2:08 a.m. UTC, <b>Kurt Hindenburg</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not sure this is a good idea. Using variable-width fonts will cause a lot of other issues w/ displaying text, colors, selecting text, etc.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Can the issue you're having be fixed another way?</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I agree that variable-width fonts in general will not display well in Konsole. However, the current method for detecting monospaced fonts using QFontInfo::fixedPitch will label monospaced fonts with programming ligature support as being variable-width (because they necessarily have to contain glyphs that span two or more characters to display ligatures), thus refusing to set them even though they were designed to be used in a monospaced environment. Since fonts detected as being variable-width are still hidden from users in the font selector when editing their profile, I don't think that this patch should cause issues. Non-monospace fonts can only be selected by editing the Konsole profile using a text editor.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For users that had manually selected a variable width font prior to d59ad5c, their Konsole window will now freeze up with garbled graphics because the TerminalDisplay::setVTFont method run on startup will return without a font being set, leaving Konsole in an undefined state. In my opinion, continuing with a warning message telling the user that using a variable-width font might give rendering problems is preferable to Konsole not being usable at all.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've been using Konsole in conjunction with the PragmataPro ligature-containing font for more than 6 months and never had any issues until updating to a newer version with the d59ad5c patch applied. A ligature font may seem like unnecessary frills but since I spend most of my day staring into terminals I've become attached to them :) In addition, Konsole is currently the only terminal emulator under linux known to support programming ligatures (see e.g. https://github.com/tonsky/FiraCode#terminals-support) so it's even a feature that can set Konsole apart from other terminal editors.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">To summarize:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Users that never touched the profile files with a text editor will not be affected by this patch</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Users that edited the profile file to choose a monospace font will not be affected by this patch</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Users that edited the profile file to choose a monospace+ligature font currently have Konsole freezing on start, and this patch fixes their issues</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Users that edited the profile file to choose a variable-width font currently have Konsole freezing on start, and this patch will make Konsole start although there will be some rendering problems</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Another solution for this problem would be to patch fixed-width font detection of QFontInfo to detect ligature fonts as still being monospaced, although I wouldn't know how to do this (yet) and the change would have even further-reaching consequences.</p></pre>
<br />
<p>- Stefan</p>
<br />
<p>On January 28th, 2016, 2:53 p.m. UTC, Stefan Seemayer wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Konsole.</div>
<div>By Stefan Seemayer.</div>
<p style="color: grey;"><i>Updated Jan. 28, 2016, 2:53 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
konsole
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For users of monospace fonts with programming ligature support such as
Hasklig, FiraCode or PragmataPro, commit d59ad5cd8ad7cdaab2ba5df59a786d475a535db5
makes Konsole freeze on startup if they had set a variable-width font
in their default profile because no font is being set on startup.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Instead of refusing to set the variable-width font necessary for
ligatures to work, emit a warning message and continue setting the font.
This will restore support for ligature fonts.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Commit causing the problems was identified using git-bisect on ArchLinux x86-64 rolling. Patch applies and compiles on git-HEAD at da573a8a90e7721bd1929a929b694db9292ad36d .</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/TerminalDisplay.cpp <span style="color: grey">(536aea6)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126921/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>