<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/115759/">https://git.reviewboard.kde.org/r/115759/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 15th, 2014, 3:13 p.m. UTC, <b>Albert Astals Cid</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/115759/diff/1/?file=244161#file244161line790" style="color: black; font-weight: bold; text-decoration: underline;">core/textpage.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int stringLengthAdaptedWithHyphen(const QString &str, const TextList::ConstIterator &it, const TextList::ConstIterator &textListEnd, PagePrivate *page)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int stringLengthAdaptedWithHyphen(const QString &str, const TextList::ConstIterator &it, const TextList::ConstIterator &textListEnd)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">790</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="kt">int</span> <span class="n">pageHeight</span> <span class="o">=</span> <span class="n"><span class="hl">page</span></span><span class="o"><span class="hl">-></span></span><span class="n"><span class="hl">m_page</span></span><span class="o"><span class="hl">-></span></span><span class="n"><span class="hl">height</span></span><span class="p"><span class="hl">()</span>;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">790</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="kt">int</span> <span class="n">pageHeight</span> <span class="o">=</span> <span class="mi"><span class="hl">100000</span></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;">why this change?</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;">As I wrote, the reason is that I think it is not good that layout analysis depends on the user's display DPI (m_page->height() is the height in pixels at 100% zoom, which depends on the DPI), since it makes no sense but would make testing more difficult (then testing layout detection would need to be done for several display DPI values separately and bug reports should include the DPI to be reproducible).
Actually here it would be most accurate to compute the y-overlap directly from the NormalizedRect's without scaling and rounding them to QRect's at all (and similarly rounding could be avoided in many other places in textpage.cpp), but I was too lazy to refactor the code and instead used a large constant for pageHeight to minimize rounding errors.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 15th, 2014, 3:13 p.m. UTC, <b>Albert Astals Cid</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/115759/diff/1/?file=244161#file244161line1873" style="color: black; font-weight: bold; text-decoration: underline;">core/textpage.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">WordsWithCharacters addNecessarySpace(RegionTextList tree, int pageWidth, int pageHeight)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1873</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="kt">double</span> <span class="n">scalingFactor</span> <span class="o">=</span> <span class="mf">2E3</span> <span class="o">/</span> <span class="p">(</span><span class="n">m_page</span><span class="o">-></span><span class="n">m_page</span><span class="o">-></span><span class="n">width</span><span class="p">()</span> <span class="o">+</span> <span class="n">m_page</span><span class="o">-></span><span class="n">m_page</span><span class="o">-></span><span class="n">height</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;">Can you explain this rationale?
Also any reason you decided to write 2E3 instead 2000?</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;">Again I got rid of the DPI dependency. I chose the value 2000 because A4 paper at 72 dpi (the DPI used by Okular before the libkscreen patch) is 597.6 x 842.4 pixels, and I rounded 597.6 + 842.4 = 1440.0 up to 2000. Higher pageWidth/pageHeight values increase CPU usage (linearly in pageWidth+pageHeight) but I guess increasing the values generally doesn't hurt layout analysis quality; too low pageWidth/pageHeight values definitely cause bad layout analysis quality. And here the width/height ratio is important too, unlike the y-overlap case above that only used y coordinates.
I wrote 2E3 because it is shorter than 2000.0. I thought that 2E3 or 2000.0 would be better than 2000 — then it is obvious from first glance that this is not integer division and also the expression would work even if Page::width() and Page::height() were refactored to return int. But perhaps 2000.0 would be clearer indeed.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 15th, 2014, 3:13 p.m. UTC, <b>Albert Astals Cid</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/115759/diff/1/?file=244162#file244162line64" style="color: black; font-weight: bold; text-decoration: underline;">tests/searchtest.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class SearchTest : public QObject</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">64</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">std</span><span class="o">::</span><span class="n">cerr</span> <span class="o"><<</span> <span class="s">"ERROR: text.size() != rect.size()"</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;">please use kDebug and remove the iostream include</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;">Actually I wanted something like QCOMPARE or an exception, so that the test were halted and error message shown. But QCOMPARE unfortunately doesn't work inside functions that are not tests themselves and when I tried exception throwing I got a compiler error that exceptions are disabled (and I don't know how the Qt test framework would handle them even if they were enabled). Then I chose stderr instead of kDebug since kDebug output can be disabled in kdebugdialog but I thought the test's reason of failure should always be shown (like the QCOMPARE error messages). But this is an unimportant error message anyway (it can only appear if the tests in searchtest.cpp themselves are incorrectly modified), so I don't oppose kDebug either. Is it a good style to never use std::cout and std::cerr, even in tests?</pre>
<br />
<p>- Jaan</p>
<br />
<p>On February 15th, 2014, 1:08 p.m. UTC, Jaan Vajakas wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Okular.</div>
<div>By Jaan Vajakas.</div>
<p style="color: grey;"><i>Updated Feb. 15, 2014, 1:08 p.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=326207">326207</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
okular
</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;">Here is a patch to fix Bug 326207. It was a simple bug in the XY Cut layout recognition code that made it too eager to see columns everywhere. I also removed the dependence of the layout analysis algorithms on the display DPI (introduced by the recently added feature of using KScreen) to make their behavior more predictable and reproducible. I added tests for this bug and also refactored SearchTest a little to use QVectors instead of bare arrays.</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;">added automatic tests and tested manually on some PDF files</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>core/textpage.cpp <span style="color: grey">(a95f7c6)</span></li>
<li>tests/searchtest.cpp <span style="color: grey">(caceb6f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115759/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>