> 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
>
>
_______________________________________________
calligra-devel mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/calligra-devel
