<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="http://git.reviewboard.kde.org/r/103044/">http://git.reviewboard.kde.org/r/103044/</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 4th, 2011, 9:41 a.m., <b>Jarosław Staniek</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="http://git.reviewboard.kde.org/r/103044/diff/1/?file=40230#file40230line118" style="color: black; font-weight: bold; text-decoration: underline;">filters/tables/excel/import/ODrawClient.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; ">QRectF ODrawClient::getGlobalRect(const MSO::OfficeArtClientAnchor &clientAnchor)</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">118</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">rgbUid</span><span class="p">.</span><span class="n">isNull</span><span class="p">())</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;">maybe isEmpty() could be better?</pre>
</blockquote>
<p>On November 7th, 2011, 9:44 a.m., <b>Lukáš Tvrdý</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;">Why?</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;">isEmpty() better explains what you mean in the condition</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 4th, 2011, 9:41 a.m., <b>Jarosław Staniek</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="http://git.reviewboard.kde.org/r/103044/diff/1/?file=40232#file40232line124" style="color: black; font-weight: bold; text-decoration: underline;">filters/tables/excel/sidewinder/cell.h</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; ">public:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">124</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// Defines a list of pictures anchored to this cell.</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;">BTW, someone will have to transform these comments to Doxygen comments...</pre>
</blockquote>
<p>On November 7th, 2011, 9:44 a.m., <b>Lukáš Tvrdý</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;">I removed that comment, why to Doxygen?</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;">that's by the way, I notices many comments not going to be published in api docs</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 4th, 2011, 9:41 a.m., <b>Jarosław Staniek</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="http://git.reviewboard.kde.org/r/103044/diff/1/?file=40234#file40234line864" style="color: black; font-weight: bold; text-decoration: underline;">filters/tables/excel/sidewinder/excel.h</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; ">public:</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">857</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="n">QMap</span><span class="o"><</span><span class="n">QByteArray</span><span class="p">,</span><span class="n">QString</span><span class="o">>&</span> <span class="n">pictureNames</span><span class="p">()</span> <span class="k">const</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;">let's return values not references; difference in performance is minimal (or none in new compilers?) while it's far safer</pre>
</blockquote>
<p>On November 7th, 2011, 9:44 a.m., <b>Lukáš Tvrdý</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;">Why is the performance minimal? Implicit sharing?
Why it is safer? I'm not returning reference to local object.</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;">Yes, implicit sharing and return-value optimization.
You're returning reference to object which has lifetime tied to the class. This may lead to external code crashing:
const QMap<QByteArray,QString>& myref = foo->pictureNames();
delete foo;
myref.value(...); // crash!
</pre>
<br />
<p>- Jarosław</p>
<br />
<p>On November 4th, 2011, 9:16 a.m., Lukáš Tvrdý wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Calligra, Marijn Kruisselbrink and Sebastian Sauer.</div>
<div>By Lukáš Tvrdý.</div>
<p style="color: grey;"><i>Updated Nov. 4, 2011, 9:16 a.m.</i></p>
<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;">This patch uses more of the libmso in excel filter.
So far the pictures were handled as PictureObject class. One problem was that
there was no support for groups for this object. So when you put two images into groups, they
are not displayed because the anchoring information is saved in different data-structure - in the
group structure.
Now I let the records to be handled by libmso, which provides this functionality.
It supports groups and even graphical styles like like borders etc.</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;">I did regression testing against master. My testing set consists of 976 xls files.
I found only positive changes.
Missing frames around pictures are displayed now, missing images are displayed now.
You can test the document.</pre>
</td>
</tr>
</table>
<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=262865">262865</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>filters/libmso/pictures.cpp <span style="color: grey">(f8bfa7c)</span></li>
<li>filters/tables/excel/import/ExcelImport.cpp <span style="color: grey">(1bb7f17)</span></li>
<li>filters/tables/excel/import/ODrawClient.cpp <span style="color: grey">(5a6507b)</span></li>
<li>filters/tables/excel/import/excelimporttoods.cc <span style="color: grey">(287aef7)</span></li>
<li>filters/tables/excel/sidewinder/cell.h <span style="color: grey">(bc080f9)</span></li>
<li>filters/tables/excel/sidewinder/cell.cpp <span style="color: grey">(8b8c0e1)</span></li>
<li>filters/tables/excel/sidewinder/excel.h <span style="color: grey">(2be1f5a)</span></li>
<li>filters/tables/excel/sidewinder/excel.cpp <span style="color: grey">(b475e5a)</span></li>
<li>filters/tables/excel/sidewinder/globalssubstreamhandler.h <span style="color: grey">(77c6e3a)</span></li>
<li>filters/tables/excel/sidewinder/globalssubstreamhandler.cpp <span style="color: grey">(f9c03b1)</span></li>
<li>filters/tables/excel/sidewinder/objects.h <span style="color: grey">(2412f07)</span></li>
<li>filters/tables/excel/sidewinder/sheet.h <span style="color: grey">(9ff346d)</span></li>
<li>filters/tables/excel/sidewinder/sheet.cpp <span style="color: grey">(440f44b)</span></li>
<li>filters/tables/excel/sidewinder/workbook.h <span style="color: grey">(2e3c2fa)</span></li>
<li>filters/tables/excel/sidewinder/workbook.cpp <span style="color: grey">(9ad61bc)</span></li>
<li>filters/tables/excel/sidewinder/worksheetsubstreamhandler.cpp <span style="color: grey">(879f76f)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103044/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>