Review Request 122153: KD Chart

[Stephen Leibowitz]

> On Feb. 14, 2015, 5:32 p.m., Friedrich W. H. Kossebau wrote:
> > 3rdparty/kdchart/src/KDChartStockDiagram_p.cpp, line 293
> > <https://git.reviewboard.kde.org/r/122153/diff/1/?file=343184#file343184line293>
> >
> >     `<= 360` or `< 360`? The KDChart 2.5.1 dump used for KDiagram has `< 360`.

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.


- Stephen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122153/#review76031
-----------------------------------------------------------


On Jan. 19, 2015, 5:51 p.m., Stephen Leibowitz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122153/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2015, 5:51 p.m.)
> 
> 
> Review request for Calligra, Boudewijn Rempt and Jarosław Staniek.
> 
> 
> Repository: calligra
> 
> 
> Description
> -------
> 
> 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
> 
> 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
> 
> 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/
> 
> 
> Diffs
> -----
> 
>   3rdparty/kdchart/src/KDChartLayoutItems.cpp 095d2cd 
>   3rdparty/kdchart/src/KDChartStockDiagram_p.cpp d8636d7 
> 
> Diff: https://git.reviewboard.kde.org/r/122153/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Stephen Leibowitz
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20150225/2a14e481/attachment.htm>