Review Request 118550: support for Geogebra's circles in KIG (GSoC 14)

[David Narváez]

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


This is an impressive patch in terms of XSLT handling :D I pointed out some smaller issues with the code in the inline comments but I'd like to rethink the if/else in the C++ code: It is a very good catch that the assumption of mine that the objects in the arguments were always labels is wrong, so lets rethink the whole if/else to be as follows:

if ( can convert to double )
 then stack a new double
else // we know now that we are dealing with an object label
  do the stuff we were doing

I think this way it is more obvious what our line of thinking is (as opposed to "I think it's a label...hm no, maybe a double? hm no, then it was a label and it's not found"). Since we agree that this is a good idea (or at least for now) you can remove that part of the comment and just leave the example XML to clarify why are we doing this.


filters/geogebra-filter.cpp
<https://git.reviewboard.kde.org/r/118550/#comment41319>

    There's whitespace (tab?) at the end of this line



filters/geogebra.xsl
<https://git.reviewboard.kde.org/r/118550/#comment41318>

    Lets use meaningful var names in these lines


- David Narváez


On June 5, 2014, 6:26 a.m., Aniket Anvit wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118550/
> -----------------------------------------------------------
> 
> (Updated June 5, 2014, 6:26 a.m.)
> 
> 
> Review request for KDE Edu and David Narváez.
> 
> 
> Repository: kig
> 
> 
> Description
> -------
> 
> This patch adds support for all the circle types that Geogebra offers in KIG.
> 
> 
> Diffs
> -----
> 
>   filters/geogebra.xsl e52fc1d 
>   filters/geogebra-filter.cpp aaa2087 
> 
> Diff: https://git.reviewboard.kde.org/r/118550/diff/
> 
> 
> Testing
> -------
> 
> tested with a worksheet containing 2 circles of each type - compass, circle-center-point, circle-3-points. 
> 
> 
> Thanks,
> 
> Aniket Anvit
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20140606/4d32dfb2/attachment.html>