Review Request 118310: support for graphic properties of Geogebra objects in KIG

[David Narváez]

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



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

    This should be renamed to holderFromCalcerAndDrawer, and take a pair of these, see below...



geogebra/geogebrasection.h
<https://git.reviewboard.kde.org/r/118310/#comment49395>

    If there will be exactly one drawer in m_drawers as there will be output objects in m_outputObjects, make m_outputObjects a vector of pair of ObjectCalcer* and ObjectDrawer*



geogebra/geogebratransformer.h
<https://git.reviewboard.kde.org/r/118310/#comment49398>

    The code compiles without this include, right?



geogebra/geogebratransformer.cpp
<https://git.reviewboard.kde.org/r/118310/#comment49399>

    There's bad formatting in many places, like the double parenthesis at the end here. Use the Reformat Source script or astyle directly to get it all right



geogebra/geogebratransformer.cpp
<https://git.reviewboard.kde.org/r/118310/#comment49405>

    This whole case list needs proper formatting.



geogebra/geogebratransformer.cpp
<https://git.reviewboard.kde.org/r/118310/#comment49400>

    No magic numbers in the code (at least no more magic numbers, I should say...), make these proper enums in this class.
    
    https://en.wikipedia.org/wiki/Magic_number_(programming)#Unnamed_numerical_constants



geogebra/geogebratransformer.cpp
<https://git.reviewboard.kde.org/r/118310/#comment49401>

    Use int ObjectDrawer::pointStyleFromString( const QString& style ), which is evil but not worse than magic constants.
    
    I'm thinking this pointType thing should be an Enum and we could use the Q_ENUM facility instead of these conversions, but this is out of the scope of this patch.



geogebra/geogebratransformer.cpp
<https://git.reviewboard.kde.org/r/118310/#comment49403>

    Set it to RoundEmpty and then either figure out why is it not drawing, or add a TODO comment here



geogebra/geogebratransformer.cpp
<https://git.reviewboard.kde.org/r/118310/#comment49404>

    Set it to RectangularEmpty and then either figure out why is it not drawing, or add a TODO comment here



geogebra/geogebratransformer.cpp
<https://git.reviewboard.kde.org/r/118310/#comment49397>

    This is not in the right place, is it? From what I see, this ObjectDrawer will leak if this was not going to be added to the m_drawers vector, so it should be created once we are sure it will be added.


- David Narváez


On Nov. 15, 2014, 4:21 p.m., Aniket Anvit wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118310/
> -----------------------------------------------------------
> 
> (Updated Nov. 15, 2014, 4:21 p.m.)
> 
> 
> Review request for KDE Edu and David Narváez.
> 
> 
> Repository: kig
> 
> 
> Description
> -------
> 
> This patch enables KIG to read the graphic properties of Geogebra objects from Geogebra worksheets.
> Here is some explanation about my implementation:
>   
> the graphic properties ( lineStyle, pointStyle, lineWidth, color ) are read for each geogebra object. I add an attribute for each property to each of the intermediate representation of each of the objects created using XSLT. Some variables are added to the KigFilterGeogebra class to hold the values of the properties for each of the objects. In the attribute() callback method of the KigFilterGeogebra class , the values of the properties are read ( and suitably mapped to KIG's ) and when the endElement() callback is called a new ObjectDrawer with the last read properties is allocated for this object.
> 
> 
> Diffs
> -----
> 
>   geogebra/geogebra.xsl c1e4749 
>   geogebra/geogebrasection.h aed7dcb 
>   geogebra/geogebrasection.cpp e9778ed 
>   geogebra/geogebratransformer.h 5f36827 
>   geogebra/geogebratransformer.cpp aee8669 
>   filters/geogebra-filter.cpp cff342d 
> 
> Diff: https://git.reviewboard.kde.org/r/118310/diff/
> 
> 
> Testing
> -------
> 
> Tested! some trouble in the mapping of PointStyles.
> 
> 
> File Attachments
> ----------------
> 
> Test file
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/11/15/9799362b-472e-4814-9d78-da2bf07d612c__gui.ggb
> 
> 
> Thanks,
> 
> Aniket Anvit
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20141118/9674ca49/attachment-0001.html>