<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/122153/">https://git.reviewboard.kde.org/r/122153/</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 14th, 2015, 5:32 p.m. UTC, <b>Friedrich W. H. Kossebau</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/122153/diff/1/?file=343184#file343184line293" style="color: black; font-weight: bold; text-decoration: underline;">3rdparty/kdchart/src/KDChartStockDiagram_p.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; ">void StockDiagram::Private::drawOHLCBar( int dataset, const CartesianDiagramDataCompressor::DataPoint &open,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">293</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span> <span class="p">(</span> <span class="n">angle</span> <span class="o">>=</span> <span class="mi">90</span> <span class="o">&&</span> <span class="n">angle</span> <span class="o"><</span> <span class="mi">180</span> <span class="p">)</span> <span class="o">||</span> <span class="p">(</span> <span class="n">angle</span> <span class="o">>=</span> <span class="mi">270</span> <span class="o">&&</span> <span class="n">angle</span> <span class="o"><</span><span class="hl"> </span><span class="mi">0</span> <span class="p">)</span> <span class="p">)</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">293</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span> <span class="p">(</span> <span class="n">angle</span> <span class="o">>=</span> <span class="mi">90</span> <span class="o">&&</span> <span class="n">angle</span> <span class="o"><</span> <span class="mi">180</span> <span class="p">)</span> <span class="o">||</span> <span class="p">(</span> <span class="n">angle</span> <span class="o">>=</span> <span class="mi">270</span> <span class="o">&&</span> <span class="n">angle</span> <span class="o"><<span class="hl">=</span></span><span class="hl"> </span><span class="mi"><span class="hl">36</span>0</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;"><= 360</code> or <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">< 360</code>? The KDChart 2.5.1 dump used for KDiagram has <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">< 360</code>.</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;">Friedrich asks “<= 360 or < 360?” I think it is a close call, and either one is okay. Here is Calligra's current master code:
285 bool reversedOrder = false;
286 // If 3D mode is enabled, we have to make sure the z-order is right
287 if ( threeDAttr.isEnabled() ) {
288 const int angle = threeDAttr.angle();
289 // Z-order is from right to left
290 if ( ( angle >= 0 && angle < 90 ) || ( angle >= 180 && angle < 270 ) )
291 reversedOrder = true;
292 // Z-order is from left to right
293 if ( ( angle >= 90 && angle < 180 ) || ( angle >= 270 && angle < 0 ) )
294 reversedOrder = false;
295 }
Line 293 is clearly wrong. The 0 should be replaced with 360. The error reduces the code clarity. Also, it clutters a build report with a compiler warning. But the error does not affect the processing, because the if statement is effectively a comment. When the condition evaluates to true, reversedOrder is set to false. However, the declaration of reversedOrder on line 285 initialized it to false. There is another if statement in between (line 290) that may set reversedOrder to true, but the two if statements will not both have true conditions. The two if statements could have been connected with an “else”.
Also, 0 and 360 degrees are not processed the same, both in Calligra's file and in the latest version (2.5.1) at KDAB.com. Using “angle <= 360” on the line 293 if statement would make this clear. Perhaps a future patch will treat 0 and 360 degrees the same. This would probably be done by adding a test for angle == 360 to line 290, and using “angle < 360” (instead of “angle <= 360”) on line 293.
There is another case of different processing for 0 and 360 degrees in the same file that might also be revised. Here is a small part of that code:
196 // Only top and right side is visible
197 if ( angle >= 0.0 && angle < 90.0 ) {
.....
208 // Only bottom and right side is visible
209 } else if ( angle >= 270.0 && angle <= 360.0 ) {
I doubt that I have KDE Git commit rights. So please push it for me. If you wish for me to revise the diff, let me know. Thank you for reviewing my submission.</pre>
<br />
<p>- Stephen</p>
<br />
<p>On January 19th, 2015, 5:51 p.m. UTC, Stephen Leibowitz 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 Calligra, Boudewijn Rempt and Jarosław Staniek.</div>
<div>By Stephen Leibowitz.</div>
<p style="color: grey;"><i>Updated Jan. 19, 2015, 5:51 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
calligra
</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;">These patches are also being made at kdab.com. Those who have a KDAB account can see the discussion in “Suggested Changes to KD Chart” at
https://quality.kdab.com/browse/KDCH-1020</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Calligra’s kdchart and kdgantt files are out-of-date, even with the patches from the above paragraph. For example, “Compiler warnings” at
http://mail.kde.org/pipermail/calligra-devel/2015-January/012762.html
mentioned an error in KDChartPieDiagram.cpp. But the error is in a private function that was removed from the latest version (2.5.1) of KD Chart. KDAB will not patch previous versions. See “PieDiagram::drawPieSurface” at
https://quality.kdab.com/browse/KDCH-1023</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KDAB makes available source code for the latest and earlier versions of its KD Chart and other GPL licensed software at http://docs.kdab.com/</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>3rdparty/kdchart/src/KDChartLayoutItems.cpp <span style="color: grey">(095d2cd)</span></li>
<li>3rdparty/kdchart/src/KDChartStockDiagram_p.cpp <span style="color: grey">(d8636d7)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122153/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>