Review Request: GSoC: Add support of Anim:Formula tag in Stage

[Thorsten Zachmann]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105030/#review14505
-----------------------------------------------------------


Looks already quite good. Most of the things I commented is either a small fix or a style issue.


stage/part/animations/strategy/KPrFormulaParser.h
<http://git.reviewboard.kde.org/r/105030/#comment11466>

    Can you please remove the F Prefix as it is no longer needed and just don't add anything.



stage/part/animations/strategy/KPrFormulaParser.h
<http://git.reviewboard.kde.org/r/105030/#comment11465>

    Wouldn't it be better to have different functions for different number of parameters instead of needing to call it with a -999 value?



stage/part/animations/strategy/KPrSmilValues.h
<http://git.reviewboard.kde.org/r/105030/#comment11456>

    Please remove the comment.



stage/part/animations/strategy/KPrSmilValues.h
<http://git.reviewboard.kde.org/r/105030/#comment11457>

    Please remove the comment



stage/part/animations/strategy/KPrSmilValues.cpp
<http://git.reviewboard.kde.org/r/105030/#comment11464>

    Please move this to the initialization list of the constructor
    
    KPrSmilValues::...
    : m_formulaParser(0)
    
    as then it will already be initialized during compiling and not during runtime



stage/part/animations/strategy/KPrSmilValues.cpp
<http://git.reviewboard.kde.org/r/105030/#comment11458>

    Please fix indention and remove empty line.



stage/part/animations/strategy/KPrSmilValues.cpp
<http://git.reviewboard.kde.org/r/105030/#comment11459>

    Please move the else to a new line.



stage/part/animations/strategy/KPrSmilValues.cpp
<http://git.reviewboard.kde.org/r/105030/#comment11460>

    Please remove the comment.



stage/part/animations/strategy/KPrSmilValues.cpp
<http://git.reviewboard.kde.org/r/105030/#comment11461>

    this can be simplified to be 
    return retval;



stage/part/animations/strategy/KPrSmilValues.cpp
<http://git.reviewboard.kde.org/r/105030/#comment11462>

    Please remove the comment.



stage/part/animations/strategy/KPrSmilValues.cpp
<http://git.reviewboard.kde.org/r/105030/#comment11463>

    This can be simplified to be 
    
    QString formula = m_formulaParser->formula();


- Thorsten Zachmann


On May 27, 2012, 3:04 p.m., Paul Mendez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105030/
> -----------------------------------------------------------
> 
> (Updated May 27, 2012, 3:04 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> Add support for Anim:Formula tag in Stage. (That key is part of ODF animations specification).
> 
> Note: animations tested don't run as smooth as in LibreOffice because KeySplines tag is not implemented.
> 
> 
> Diffs
> -----
> 
>   stage/part/CMakeLists.txt 3c7916ef7496af21e65d9a5441d5cb924829c347 
>   stage/part/animations/KPrAnimate.cpp dddd1fa401d596e7e23688f950428cd0ea76b639 
>   stage/part/animations/strategy/KPrFormulaParser.h PRE-CREATION 
>   stage/part/animations/strategy/KPrFormulaParser.cpp PRE-CREATION 
>   stage/part/animations/strategy/KPrSmilValues.h 163d78b830a151ce150192000890a395f9e273dd 
>   stage/part/animations/strategy/KPrSmilValues.cpp 3faafc4eb1c8783224f9f32c38106407cc219096 
>   stage/part/animations/strategy/KPrValueParser.h 8f3c6ebcdf7ae9f5d938d97d518f21977b572940 
>   stage/part/animations/strategy/KPrValueParser.cpp ca7fac767142e64b9711ccf835efcb9a4c242e36 
> 
> Diff: http://git.reviewboard.kde.org/r/105030/diff/
> 
> 
> Testing
> -------
> 
> Test some animations of documents created in Libre Office (One test document is also uploaded)
> 
> 
> Thanks,
> 
> Paul Mendez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20120608/3256539e/attachment.htm>