<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/129281/">https://git.reviewboard.kde.org/r/129281/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 6th, 2016, 1:35 p.m. UTC, <b>Martin Tobias Holmedahl Sandsmark</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/129281/diff/1/?file=483169#file483169line859" style="color: black; font-weight: bold; text-decoration: underline;">src/TerminalDisplay.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">859</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// the drawText(rect,flags,string) overload is used here with null flags</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">859</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">painter</span><span class="p">.</span><span class="n">drawText</span><span class="p">(</span><span class="n">rect</span><span class="p">.</span><span class="n">x</span><span class="p">(),</span> <span class="n">rect</span><span class="p">.</span><span class="n">y</span><span class="p">()</span> <span class="o">+</span> <span class="n">QFontMetrics</span><span class="p">(</span><span class="n">font</span><span class="p">).</span><span class="n">ascent</span><span class="p">(),</span> <span class="n">_bidiEnabled</span> <span class="o">?</span> <span class="nl">text</span> <span class="p">:</span> <span class="n">LTR_OVERRIDE_CHAR</span> <span class="o">+</span> <span class="n">text</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<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;">Minor nitpick: is there any reason not to unconditionally add the LTR_OVERRIDE_CHAR?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't like the ? : stuff, and the line gets overly long.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">otherwise it looks good, so +1 from me.</p></pre>
<br />
<p>- Martin Tobias Holmedahl</p>
<br />
<p>On October 29th, 2016, 12:27 a.m. UTC, Christoph Feck 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 KDE Frameworks and Konsole.</div>
<div>By Christoph Feck.</div>
<p style="color: grey;"><i>Updated Oct. 29, 2016, 12:27 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=371687">371687</a>
</div>
<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;">When Konsole draws a line of text, it first computes the rectangle of the line that the text covers (taking into account cells width and height), then passes this rectangle to the drawText(QRect, flags, text) call.
Qt detects if the selected font does not offer all characters in the text, and substitutes individual characters with a different font. Due to designer choices, the same font point size does not lead to same pixel height (or ascent size) in all fonts, so the substituted characters might be larger than the characters from the primary font.
Using a rectangle causes Qt to position glyphs relative to the bounding box of the text, instead of anchored to the baseline.
This patch uses a pixel position instead of a rectangle to draw the text, taking into account only the baseline of the primary font.
I have added all "frameworks" developers to increase possible test coverage.</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;">On my system, lines with substituted Unicode characters are no longer shifted away from the baseline, and therefore do not appear cropped.
Further testing is needed, as there are many (equivalent, similar, or different) bug reports about font rendering on different systems, see https://bugs.kde.org/buglist.cgi?bug_status=UNCONFIRMED&bug_status=CONFIRMED&component=font&product=konsole</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">(39a8b84)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/129281/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>